pax(1): pax format write support
A long time ago the posix folks extended the ustar format to allow
representing arbitrarily big files, long file names, precise timestamps,
etc. We have support to read such archives but no support to write them
out. Here's a minimal proposal following discussions with Caspar and
other folks.
What the diff below does:
- provide a 'pax' format usable with pax(1) -x pax
- use extended headers to store file names and link names that don't fit
in a standard ustar header. Long file names in the ports tree were
the intial motive for this effort.
What the diff below doesn't do:
- handle all the file attributes that ought to benefit from extended
headers. Right now I haven't even written that code and I hope to
extend handled attributes gradually.
- the diff *doesn't change the default format* used by either pax(1) or
tar(1). I think changing the default at some point makes sense, since
the format is superior and support is widely available. My opinion is
that we shouldn't expose this format by default until we're confident
that the code is reasonably complete and exercised. I have no idea
whether this ought to happen before the next release.
- since the diff doesn't change the default for tar(1) and since tar(1)
doesn't have a generic option to choose the format used, this code is
unreachable using tar(1). I'd *love* to have a bikeshed discussion
about the proper way to handle that, but preferably after the code gets
reviewed, pushed in the tree and improved. :)
Two more notes:
- no size increase for distrib/special/pax
- pax 101: you can exercise the new format with:
pax -x pax -w files... > outfile.tar
Thoughts? ok?
Index: cpio.1
===
RCS file: /home/cvs/src/bin/pax/cpio.1,v
retrieving revision 1.36
diff -u -p -r1.36 cpio.1
--- cpio.1 16 Jan 2020 16:46:46 - 1.36
+++ cpio.1 4 Sep 2023 16:26:53 -
@@ -98,6 +98,8 @@ format.
Old octal character
.Nm
format.
+.It Ar pax
+POSIX pax format.
.It Ar sv4cpio
SVR4 hex
.Nm
@@ -173,6 +175,8 @@ format.
Old octal character
.Nm
format.
+.It Ar pax
+POSIX pax format.
.It Ar sv4cpio
SVR4 hex
.Nm
@@ -298,6 +302,8 @@ be used for larger files.
.It bcpio Ta "4 Gigabytes"
.It sv4cpio Ta "4 Gigabytes"
.It cpio Ta "8 Gigabytes"
+.\" XXX should be "unlimited"
+.It pax Ta "8 Gigabytes"
.It tar Ta "8 Gigabytes"
.It ustar Ta "8 Gigabytes"
.El
Index: extern.h
===
RCS file: /home/cvs/src/bin/pax/extern.h,v
retrieving revision 1.60
diff -u -p -r1.60 extern.h
--- extern.h23 Mar 2020 20:04:19 - 1.60
+++ extern.h4 Sep 2023 14:59:23 -
@@ -296,6 +296,7 @@ int tar_wr(ARCHD *);
int ustar_id(char *, int);
int ustar_rd(ARCHD *, char *);
int ustar_wr(ARCHD *);
+int pax_wr(ARCHD *);
/*
* tty_subs.c
Index: options.c
===
RCS file: /home/cvs/src/bin/pax/options.c,v
retrieving revision 1.105
diff -u -p -r1.105 options.c
--- options.c 17 Jan 2023 16:20:28 - 1.105
+++ options.c 4 Sep 2023 16:44:55 -
@@ -214,6 +214,8 @@ FSUB fsub[] = {
{ },
/* 9: gzip, to detect failure to use -z */
{ },
+/* 10: POSIX PAX */
+ { },
#else
/* 6: compress, to detect failure to use -Z */
{NULL, 0, 4, 0, 0, 0, 0, compress_id},
@@ -223,6 +225,10 @@ FSUB fsub[] = {
{NULL, 0, 4, 0, 0, 0, 0, bzip2_id},
/* 9: gzip, to detect failure to use -z */
{NULL, 0, 4, 0, 0, 0, 0, gzip_id},
+/* 10: POSIX PAX */
+ {"pax", 5120, BLKMULT, 0, 1, BLKMULT, 0, ustar_id, no_op,
+ ustar_rd, tar_endrd, no_op, pax_wr, tar_endwr, tar_trail,
+ tar_opt},
#endif
};
#defineF_OCPIO 0 /* format when called as cpio -6 */
Index: pax.1
===
RCS file: /home/cvs/src/bin/pax/pax.1,v
retrieving revision 1.76
diff -u -p -r1.76 pax.1
--- pax.1 31 Mar 2022 17:27:14 - 1.76
+++ pax.1 5 Sep 2023 13:46:15 -
@@ -868,6 +868,11 @@ standard.
The default blocksize for this format is 10240 bytes.
Filenames stored by this format must be 100 characters or less in length;
the total pathname must be 256 characters or less.
+.It Cm pax
+The pax interchange format specified in the
+.St -p1003.1-2001
+standard.
+The default blocksize for this format is 5120 bytes.
.El
.Pp
.Nm
@@ -1081,9 +1086,10 @@ utility is compliant with the
specification,
except that the
.Cm pax
-archive format and the
+archive format is only partially supported,
+and the
.Cm listopt
-keyword are unsupported.
+keyword is unsupported.
.Pp
The flags
.Op Fl 0BDEGjOPTUYZz ,
Index: tar.c
===
RCS file: /home/cvs/src/bin/pax/tar.c,v
retrieving revision 1.73
diff -u -p -r1.73 tar.c
--- tar.c 4 Sep 2023 17:05:34 - 1.73
+++ tar.c 4 Sep 2023 2
pax(1) and tar(1): fix misleading -DSMALL
Two code sets are currently guarded with #ifdef SMALL in pax(1) and
tar(1): reading 'pax' format extended headers, and identifying various
compressed formats for user-friendliness. As noted by Caspar, the SMALL
path isn't currently used on the install media. I've been confused by
this twice already...
Here's a proposal:
1. always compile in read support for the 'pax' format extended headers.
The ustar format is limited and being able to restore archives using
the pax format in any situation would be nice. Especially if we
switch to writing out pax format archives by default one day.
We're definitely not there yet.
2. actually use -DSMALL to save a bit of storage on the install media.
The behavior is still sane, tar(1) warns that it doesn't recognize
a compressed archive, seeks through it trying to look a tar header,
and eventually gives up. Here's the tiny size change on amd64:
shannon /usr/src/distrib/special/pax$ size tar.o options.o pax obj/tar.o
obj/options.o obj/pax
textdatabss dec hex
68210 40 68611acdtar.o
7195108432 83112077options.o
390495 19024 85392 494911 78d3f pax
68210 40 68611acdobj/tar.o
6878108432 79941f3aobj/options.o
390175 19024 85392 494591 78bff obj/pax
I don't expect any regression on the ramdisks but a make release is
running just in case.
ok?
Index: bin/pax/tar.c
===
RCS file: /home/cvs/src/bin/pax/tar.c,v
retrieving revision 1.72
diff -u -p -r1.72 tar.c
--- bin/pax/tar.c 19 Aug 2023 04:21:05 - 1.72
+++ bin/pax/tar.c 4 Sep 2023 12:19:39 -
@@ -59,9 +59,7 @@ static u_long tar_chksm(char *, int);
static char *name_split(char *, int);
static int ul_oct(u_long, char *, int, int);
static int ull_oct(unsigned long long, char *, int, int);
-#ifndef SMALL
static int rd_xheader(ARCHD *arcn, int, off_t);
-#endif
static uid_t uid_nobody;
static uid_t uid_warn;
@@ -721,14 +719,11 @@ ustar_rd(ARCHD *arcn, char *buf)
if (ustar_id(buf, BLKMULT) < 0)
return(-1);
-#ifndef SMALL
reset:
-#endif
memset(arcn, 0, sizeof(*arcn));
arcn->org_name = arcn->name;
arcn->sb.st_nlink = 1;
-#ifndef SMALL
/* Process Extended headers. */
if (hd->typeflag == XHDRTYPE || hd->typeflag == GHDRTYPE) {
if (rd_xheader(arcn, hd->typeflag == GHDRTYPE,
@@ -745,7 +740,6 @@ reset:
if (hd->typeflag == XHDRTYPE || hd->typeflag == GHDRTYPE)
goto reset;
}
-#endif
if (!arcn->nlen) {
/*
@@ -1190,8 +1184,6 @@ expandname(char *buf, size_t len, char *
return(nlen);
}
-#ifndef SMALL
-
/* shortest possible extended record: "5 a=\n" */
#define MINXHDRSZ 5
@@ -1331,4 +1323,3 @@ rd_xheader(ARCHD *arcn, int global, off_
return (-1);
return (ret);
}
-#endif
Index: distrib/special/pax/Makefile
===
RCS file: /home/cvs/src/distrib/special/pax/Makefile,v
retrieving revision 1.2
diff -u -p -r1.2 Makefile
--- distrib/special/pax/Makefile13 Sep 2018 16:34:33 - 1.2
+++ distrib/special/pax/Makefile3 Sep 2023 10:38:17 -
@@ -1,7 +1,7 @@
# $OpenBSD: Makefile,v 1.2 2018/09/13 16:34:33 sthen Exp $
.PATH: ${.CURDIR}/../../../bin/pax
-CFLAGS+=-DNOCPIO -I${.CURDIR}/../../../bin/pax
+CFLAGS+=-DNOCPIO -DSMALL -I${.CURDIR}/../../../bin/pax
PROG= pax
SRCS= ar_io.c ar_subs.c buf_subs.c file_subs.c ftree.c\
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: option GPROF on riscv64
On Sat, Jul 22 2023, Scott Cheloha wrote: [...] > Is there any difference if you duplicate the contents of > intr_disable/intr_restore in MCOUNT_ENTER/MCOUNT_EXIT directly? > Unlikely, but maybe the compiler is shoving an _mcount() call into > intr_disable/intr_restore, causing infinite recursion. As said in private, that didn't help. The check at lib/libkern/mcount.c:76 should already avoid recursion. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
option GPROF on riscv64
Spotted while testing a diff from cheloha@, option GPROF doesn't build
on riscv64 because MCOUNT_ENTER/MCOUNT_EXIT from
riscv64/include/profile.h haven't been adapted for riscv64.
riscv64 /sys/arch/riscv64/compile/GPROF.MP$ doas -u build make
cc -g -Werror -Wall -Wimplicit-function-declaration -Wno-pointer-sign
-Wno-constant-conversion -Wno-address-of-packed-member
-Wno-unused-but-set-variable -Wno-gnu-folding-constant
-Wframe-larger-than=2047 -Wno-deprecated-non-prototype
-Wno-unknown-warning-option -march=rv64gc -mcmodel=medany -mno-relax
-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffreestanding -fno-pie
-O2 -pipe -nostdinc -I/sys -I/sys/arch/riscv64/compile/GPROF.MP/obj
-I/sys/arch -I/sys/dev/pci/drm/include -I/sys/dev/pci/drm/include/uapi -DDDB
-DDIAGNOSTIC -DKTRACE -DACCOUNTING -DKMEMSTATS -DPTRACE -DPOOL_DEBUG -DCRYPTO
-DSYSVMSG -DSYSVSEM -DSYSVSHM -DUVM_SWAP_ENCRYPT -DFFS -DFFS2 -DFFS_SOFTUPDATES
-DUFS_DIRHASH -DQUOTA -DEXT2FS -DMFS -DNFSCLIENT -DNFSSERVER -DCD9660 -DUDF
-DMSDOSFS -DFIFO -DFUSE -DSOCKET_SPLICE -DTCP_ECN -DTCP_SIGNATURE -DINET6
-DIPSEC -DPPP_BSDCOMP -DPPP_DEFLATE -DPIPEX -DMROUTING -DMPLS -DBOOT_CONFIG
-DPCIVERBOSE -DUSER_PCICONF -DWSDISPLAY_COMPAT_USL -DWSDISPLAY_COMPAT_RAWKBD
-DWSDISPLAY_DEFAULTSCREENS="6" -DMULTIPROCESSOR -DGPROF -DMAXUSERS=80 -D_KERNEL
-D__riscv64__ -MD -MP -pg -c /sys/kern/subr_prof.c
cc -g -Werror -Wall -Wimplicit-function-declaration -Wno-pointer-sign
-Wno-constant-conversion -Wno-address-of-packed-member
-Wno-unused-but-set-variable -Wno-gnu-folding-constant
-Wframe-larger-than=2047 -Wno-deprecated-non-prototype
-Wno-unknown-warning-option -march=rv64gc -mcmodel=medany -mno-relax
-fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -ffreestanding -fno-pie
-O2 -pipe -nostdinc -I/sys -I/sys/arch/riscv64/compile/GPROF.MP/obj
-I/sys/arch -I/sys/dev/pci/drm/include -I/sys/dev/pci/drm/include/uapi -DDDB
-DDIAGNOSTIC -DKTRACE -DACCOUNTING -DKMEMSTATS -DPTRACE -DPOOL_DEBUG -DCRYPTO
-DSYSVMSG -DSYSVSEM -DSYSVSHM -DUVM_SWAP_ENCRYPT -DFFS -DFFS2 -DFFS_SOFTUPDATES
-DUFS_DIRHASH -DQUOTA -DEXT2FS -DMFS -DNFSCLIENT -DNFSSERVER -DCD9660 -DUDF
-DMSDOSFS -DFIFO -DFUSE -DSOCKET_SPLICE -DTCP_ECN -DTCP_SIGNATURE -DINET6
-DIPSEC -DPPP_BSDCOMP -DPPP_DEFLATE -DPIPEX -DMROUTING -DMPLS -DBOOT_CONFIG
-DPCIVERBOSE -DUSER_PCICONF -DWSDISPLAY_COMPAT_USL -DWSDISPLAY_COMPAT_RAWKBD
-DWSDISPLAY_DEFAULTSCREENS="6" -DMULTIPROCESSOR -DGPROF -DMAXUSERS=80 -D_KERNEL
-D__riscv64__ -MD -MP -fno-ret-protector -c /sys/lib/libkern/mcount.c
/sys/lib/libkern/mcount.c:79:2: error: invalid operand in inline asm: 'mrs
${0:x},daif; msr daifset, #0x2'
MCOUNT_ENTER;
^
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:88:10: note: expanded
from macro 'MCOUNT_ENTER'
__asm__ ("mrs %x0,daif; msr daifset, #0x2": "=r"(s));
^
/sys/lib/libkern/mcount.c:79:2: error: unknown operand
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:88:10: note: expanded
from macro 'MCOUNT_ENTER'
__asm__ ("mrs %x0,daif; msr daifset, #0x2": "=r"(s));
^
:1:6: note: instantiated into assembly here
mrs ,daif; msr daifset, #0x2
^
/sys/lib/libkern/mcount.c:79:2: error: unknown operand
MCOUNT_ENTER;
^
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:88:10: note: expanded
from macro 'MCOUNT_ENTER'
__asm__ ("mrs %x0,daif; msr daifset, #0x2": "=r"(s));
^
:1:26: note: instantiated into assembly here
mrs ,daif; msr daifset, #0x2
^
/sys/lib/libkern/mcount.c:171:2: error: invalid operand in inline asm: 'msr
daif, ${0:x}'
MCOUNT_EXIT;
^
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:90:10: note: expanded
from macro 'MCOUNT_EXIT'
__asm__ ("msr daif, %x0":: "r"(s));
^
/sys/lib/libkern/mcount.c:171:2: error: unknown operand
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:90:10: note: expanded
from macro 'MCOUNT_EXIT'
__asm__ ("msr daif, %x0":: "r"(s));
^
:1:12: note: instantiated into assembly here
msr daif,
^
/sys/lib/libkern/mcount.c:171:2: error: invalid operand in inline asm: 'msr
daif, ${0:x}'
MCOUNT_EXIT;
^
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:90:10: note: expanded
from macro 'MCOUNT_EXIT'
__asm__ ("msr daif, %x0":: "r"(s));
^
/sys/lib/libkern/mcount.c:171:2: error: unknown operand
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:90:10: note: expanded
from macro 'MCOUNT_EXIT'
__asm__ ("msr daif, %x0":: "r"(s));
^
:1:12: note: instantiated into assembly here
msr daif,
^
/sys/lib/libkern/mcount.c:171:2: error: invalid operand in inline asm: 'msr
daif, ${0:x}'
MCOUNT_EXIT;
^
/sys/arch/riscv64/compile/GPROF.MP/obj/machine/profile.h:90:10: note: expanded
from macro 'MCOUNT_EXIT'
__asm__ ("msr daif, %x0":: "r"(s));
^
/sys/lib/libkern/mcount.c:171:2: error: u
Re: [v2] statclock: move profil(2), GPROF code into other clock interrupts
On Fri, Jul 21 2023, Mike Larkin wrote: > On Fri, Jul 21, 2023 at 05:46:32PM +0200, Jeremie Courreges-Anglas wrote: >> On Thu, Jul 20 2023, Scott Cheloha wrote: >> > On Wed, Jul 19, 2023 at 05:09:04AM +, Mike Larkin wrote: >> >> On Tue, Jul 18, 2023 at 08:21:41AM -0500, Scott Cheloha wrote: >> >> > This patch moves the profil(2)- and GPROF-specific parts of >> >> > statclock() out into into separate clock interrupt routines. The >> >> > profil(2) part moves into profclock() and is enabled/disabled as >> >> > needed during mi_switch(). The GPROF part moves into gmonclock() and >> >> > is enabled/disabled as needed via sysctl(2). >> >> > >> >> > Moving those parts out of statclock() eliminates the need for an >> >> > effective statclock frequency and we can delete all the junk related >> >> > to that: psratio/psdiv/pscnt and corresponding members of >> >> > schedstate_percpu, clockintr_setstatclockrate(), a bunch of other >> >> > clockintr-internal code >> >> > >> >> > In separate commits I have addressed: >> >> > >> >> > - General GPROF instability on amd64 >> >> > - GPROF causing a crash during suspend/resume >> >> > - CTASSERT breakage on amd64 related to schedstate_percpu >> >> > changes in this patch >> >> > >> >> > This has been kicking around for over two months. Personally, I have >> >> > tested it on amd64, arm64, macppc, octeon, and sparc64. >> >> > >> >> > Compile- and boot-tests on other platforms (alpha, i386, luna88k, >> >> > riscv64, sh) would be appreciated, but the last time I asked for tests >> >> > I got zero reports back. >> >> >> >> i386 compiles and boots. >> > >> > Great! >> > >> >> as reported in separate mail, riscv64 doesn't compile. >> > >> > I think we're missing a 'struct user' definition on riscv64. Can you >> > try this? >> >> GENERIC.MP with option GPROF doesn't build on riscv64, but this diff >> doesn't introduce any new error. Runtime untested. >> >> -- >> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >> > > Yes, I should have pointed out I did a normal build and not a GPROF build > which I have no idea how to test, nor do I use. Same disclaimer applies to > i386. To test-build this, I merely added: makeoptionsPROF="-pg" option GPROF to a GENERIC.MP copy and then booted that kernel. Scott, your statclock diff doesn't seem to make the runtime behavior any worse on riscv64 (ie it's broken, but maybe my profile.h fix isn't right). [...] nvme0 at pci6 dev 0 function 0 "Samsung PM991 NVMe" rev 0x00: intx, NVMe 1.4 nvme0: Samsung SSD 980 500GB, firmware 1B4QFXO7, serial S64DNF0R618716X scsibus0 at nvme0: 2 targets, initiator 0 sd0 at scsibus0 targ 1 lun 0: sd0: 476940MB, 512 bytes/sector, 976773168 sectors ppb6 at pci2 dev 8 function 0 "ASMedia ASM2824" rev 0x01: intx pci7 at ppb6 bus 7 "hfclk" at mainbus0 not configured "rtcclk" at mainbus0 not configured "gpio-poweroff" at mainbus0 not configured Profiling kernel, textsize=7925768 [ffc0..ffc00078f008] uhub1 at uhub0 port 2 configuration 1 interface 0 "ASMedia AS2107" rev 3.00/0.01 addr 2 uhub2 at uhub0 port 4 configuration 1 interface 0 "ASMedia AS2107" rev 2.10/0.01 addr 3 vscsi0 at root scsibus1 at vscsi0: 256 targets softraid0 at root scsibus2 at softraid0: 256 targets root on sd0a (c92bac01c037a788.a) swap on sd0b dump on sd0b panic: kernel diagnostic assertion "(csr_read(sstatus) & (SSTATUS_SPP | SSTATUS _SIE)) == 0" failed: file "/sys/arch/riscv64/riscv64/trap.c", line 123 Came fro m U mode with interrupts enabled Stopped at db_enter+0x10: c.ebreak TIDPIDUID PRFLAGS PFLAGS CPU COMMAND -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: [v2] statclock: move profil(2), GPROF code into other clock interrupts
On Thu, Jul 20 2023, Scott Cheloha wrote: > On Wed, Jul 19, 2023 at 05:09:04AM +, Mike Larkin wrote: >> On Tue, Jul 18, 2023 at 08:21:41AM -0500, Scott Cheloha wrote: >> > This patch moves the profil(2)- and GPROF-specific parts of >> > statclock() out into into separate clock interrupt routines. The >> > profil(2) part moves into profclock() and is enabled/disabled as >> > needed during mi_switch(). The GPROF part moves into gmonclock() and >> > is enabled/disabled as needed via sysctl(2). >> > >> > Moving those parts out of statclock() eliminates the need for an >> > effective statclock frequency and we can delete all the junk related >> > to that: psratio/psdiv/pscnt and corresponding members of >> > schedstate_percpu, clockintr_setstatclockrate(), a bunch of other >> > clockintr-internal code >> > >> > In separate commits I have addressed: >> > >> > - General GPROF instability on amd64 >> > - GPROF causing a crash during suspend/resume >> > - CTASSERT breakage on amd64 related to schedstate_percpu >> > changes in this patch >> > >> > This has been kicking around for over two months. Personally, I have >> > tested it on amd64, arm64, macppc, octeon, and sparc64. >> > >> > Compile- and boot-tests on other platforms (alpha, i386, luna88k, >> > riscv64, sh) would be appreciated, but the last time I asked for tests >> > I got zero reports back. >> >> i386 compiles and boots. > > Great! > >> as reported in separate mail, riscv64 doesn't compile. > > I think we're missing a 'struct user' definition on riscv64. Can you > try this? GENERIC.MP with option GPROF doesn't build on riscv64, but this diff doesn't introduce any new error. Runtime untested. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: riscv64: enable usb(4) interface, add missing usb devices?
On Mon, Jan 23 2023, Mark Kettenis wrote:
>> From: Jeremie Courreges-Anglas
>> Date: Mon, 23 Jan 2023 13:43:15 +0100
>>
>> I wanted to cross-connect the serial ports of my armv7 and riscv64
>> machines. Problem on riscv64: no uftdi driver. It looks like a lot of
>> other USB drivers are missing in GENERIC, both generic drivers and
>> hw-specific drivers: umodem, ugen, ulpt, uvideo, umb, uplcom etc.
>> Feels like it would make sense to sync those devices from arm64/GENERIC
>> instead of adding only what I need. Thoughts?
>
> Yes, that makes sense.
So here's a diff that enables most of what is found in arm64/GENERIC.
I stripped onewire(4) attached at uow(4) because uow(4) looks designed
for niche devices but 1. I can be convinced otherwise 2. I didn't try to
judge the usefulness of all new entries.
>> One glitch: usbdevs(8) isn't usable since usb(4) isn't wired in conf.c.
>> With the diffbelow I can use usbdevs(8) on my hifive. ok for the
>> usb.h/cdev_usb_init addition?
>
> You probably should copy the ugen and ulpt entries from arm64 as well.
Yes, same for ujoy(4). Here's the whole diff for GENERIC, conf.c and
MAKEDEV. The GENERIC diff is better reviewed with diff -w since there
are whitespace differences between arm64 and riscv64. Of all the
additions in conf.c, only ujoy(4) was missing from MAKEDEV.
With this, ucom(4) at uftdi(4) and usbdevs(8) work fine for my use case.
ok?
diff --git a/etc/etc.riscv64/MAKEDEV b/etc/etc.riscv64/MAKEDEV
index 7adf702e7ea..bad9f6a84c1 100644
--- a/etc/etc.riscv64/MAKEDEV
+++ b/etc/etc.riscv64/MAKEDEV
@@ -4,7 +4,7 @@
# generated from:
#
# OpenBSD: etc.riscv64/MAKEDEV.md,v 1.4 2021/11/11 09:47:34 claudio Exp
-# OpenBSD: MAKEDEV.common,v 1.118 2022/11/10 09:50:00 krw Exp
+# OpenBSD: MAKEDEV.common,v 1.119 2023/01/14 12:15:12 kettenis Exp
# OpenBSD: MAKEDEV.mi,v 1.83 2016/09/11 03:06:31 deraadt Exp
# OpenBSD: MAKEDEV.sub,v 1.14 2005/02/07 06:14:18 david Exp
#
@@ -57,6 +57,7 @@
# ugen* Generic USB devices
# uhid* Generic HID devices
# fidofido/* nodes
+# ujoyujoy/* nodes
# ulpt* Printer devices
# usb*Bus control devices used by usbd for attach/detach
# Special purpose devices:
@@ -365,6 +366,12 @@ ulpt*)
M ulpt$U c 64 $U 600
;;
+ujoy)
+ RMlist[${#RMlist[*]}]=";mkdir -p ujoy;rm -f" n=0
+ while [ $n -lt 4 ];do M ujoy/$n c 100 $n 444;n=$(($n+1));done
+ MKlist[${#MKlist[*]}]=";chmod 555 ujoy"
+ ;;
+
fido)
RMlist[${#RMlist[*]}]=";mkdir -p fido;rm -f" n=0
while [ $n -lt 4 ];do M fido/$n c 98 $n 666;n=$(($n+1));done
@@ -385,8 +392,9 @@ ugen*)
uall)
R ttyU0 ttyU1 ttyU2 ttyU3 ugen0 ugen1 ugen2 ugen3 ugen4 ugen5
- R ugen6 ugen7 ulpt0 ulpt1 fido uhid0 uhid1 uhid2 uhid3 uhid4
- R uhid5 uhid6 uhid7 usb0 usb1 usb2 usb3 usb4 usb5 usb6 usb7
+ R ugen6 ugen7 ulpt0 ulpt1 ujoy fido uhid0 uhid1 uhid2 uhid3
+ R uhid4 uhid5 uhid6 uhid7 usb0 usb1 usb2 usb3 usb4 usb5 usb6
+ R usb7
;;
ttyU[0-9a-zA-Z])
diff --git a/etc/etc.riscv64/MAKEDEV.md b/etc/etc.riscv64/MAKEDEV.md
index 4aece1f961f..c31c8a5d373 100644
--- a/etc/etc.riscv64/MAKEDEV.md
+++ b/etc/etc.riscv64/MAKEDEV.md
@@ -52,6 +52,7 @@ _DEV(uall)
_DEV(ugen, 63)
_DEV(uhid, 62)
_DEV(fido, 98)
+_DEV(ujoy, 100)
_DEV(ulpt, 64)
_DEV(usb, 61)
_TITLE(spec)
diff --git a/sys/arch/riscv64/conf/GENERIC b/sys/arch/riscv64/conf/GENERIC
index cd27318b503..a213f2a90d6 100644
--- a/sys/arch/riscv64/conf/GENERIC
+++ b/sys/arch/riscv64/conf/GENERIC
@@ -136,56 +136,124 @@ xhci*at pci?
usb* at xhci?
# USB devices
-uhub* at usb?
-uhub* at uhub?
-uhidev*at uhub?
+uhub* at usb? # USB Hubs
+uhub* at uhub?# USB Hubs
+urng* at uhub?# USB Random Number Generator
+uonerng* at uhub?# Moonbase Otago OneRNG
+umodem*at uhub?# USB Modems/Serial
+ucom* at umodem?
+uvisor*at uhub?# Handspring Visor
+ucom* at uvisor?
+uvscom*at uhub?# SUNTAC Slipper U VS-10U serial
+ucom* at uvscom?
+ubsa* at uhub?# Belkin serial adapter
+ucom* at ubsa?
+uftdi* at uhub?# FTDI FT8U100AX serial adapter
+ucom* at uftdi?
+uplcom*at uhub?# I/O DATA USB-RSAQ2 serial
adapter
+ucom* at uplcom?
+umct* at uhub?# MCT USB-RS232 serial adapter
+ucom* at umct?
+uslcom*at uhub?# Silicon Laboratories CP210x
serial
+ucom* at uslcom?
+uscom* at uhub?# Simple USB serial adapters
+ucom
riscv64: enable usb(4) interface, add missing usb devices?
I wanted to cross-connect the serial ports of my armv7 and riscv64 machines. Problem on riscv64: no uftdi driver. It looks like a lot of other USB drivers are missing in GENERIC, both generic drivers and hw-specific drivers: umodem, ugen, ulpt, uvideo, umb, uplcom etc. Feels like it would make sense to sync those devices from arm64/GENERIC instead of adding only what I need. Thoughts? One glitch: usbdevs(8) isn't usable since usb(4) isn't wired in conf.c. With the diffbelow I can use usbdevs(8) on my hifive. ok for the usb.h/cdev_usb_init addition? diff --git a/sys/arch/riscv64/conf/GENERIC b/sys/arch/riscv64/conf/GENERIC index cd27318b503..225921eb04b 100644 --- a/sys/arch/riscv64/conf/GENERIC +++ b/sys/arch/riscv64/conf/GENERIC @@ -138,6 +138,8 @@ usb*at xhci? # USB devices uhub* at usb? uhub* at uhub? +uftdi* at uhub?# FTDI FT8U100AX serial adapter +ucom* at uftdi? uhidev*at uhub? uaudio*at uhub?# USB Audio audio* at uaudio? diff --git a/sys/arch/riscv64/riscv64/conf.c b/sys/arch/riscv64/riscv64/conf.c index 5256251a3a3..673fea27d85 100644 --- a/sys/arch/riscv64/riscv64/conf.c +++ b/sys/arch/riscv64/riscv64/conf.c @@ -88,10 +88,12 @@ cdev_decl(lpt); #include "midi.h" #include "ksyms.h" #include "kstat.h" +#include "usb.h" +#include "uhid.h" +#include "ucom.h" #include "radio.h" #include "drm.h" cdev_decl(drm); -#include "uhid.h" #include "fido.h" #include "wsdisplay.h" @@ -180,12 +182,12 @@ struct cdevsw cdevsw[] = cdev_notdef(), /* 59: i4b trace device */ cdev_notdef(), /* 60: i4b phone device */ /* End of reserved slots for isdn4bsd. */ - cdev_notdef(), /* 61: USB controller */ + cdev_usb_init(NUSB,usb),/* 61: USB controller */ cdev_usbdev_init(NUHID,uhid), /* 62: USB generic HID */ cdev_notdef(), /* 63: USB generic driver */ cdev_notdef(), /* 64: USB printers */ cdev_notdef(), /* 65: urio */ - cdev_notdef(), /* 66: USB tty */ + cdev_tty_init(NUCOM,ucom), /* 66: USB tty */ cdev_mouse_init(NWSKBD, wskbd), /* 67: keyboards */ cdev_mouse_init(NWSMOUSE, /* 68: mice */ wsmouse), -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: crunchgen and llvm 15
On Fri, Dec 30 2022, Miod Vallat wrote:
>> I'm not sure which way to go: either sprinkle some -fno-common between
>> crunchgen and distrib/special/Makefile.inc, or drop -dc on lld archs
>> only. What would you folks prefer?
>
> I'd rather have the same behaviour for all platforms, but I need to test
> a few more gcc platforms first.
How do your tests fare so far? We still need a way to handle the
removal of -dc in lld 15*.
Theo expressed concern over a possible size increase. My not-so-strong
understanding of the issue at hand tells me that the "ld -dc" -> "-fno-common"
switch shouldn't lead to a huge size increase (or no increase at all).
Do you have numbers on eg hppa, mips64 or sparc64?
* I'm going to keep the ${LINKER_VERSION} workaround in my next
iterations[0], unless we agree to use -fno-common.
[0] https://github.com/jcourreges/openbsd-src/tree/llvm15-6
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: ESRT support for the amd64 bootloader
On Wed, Dec 28 2022, Mark Kettenis wrote:
> Dear Sergii,
>
> Sorry for the delay, but I have finally found the time to work on the
> EFI variable and ESRT support for OpenBSD. As a first step, here is a
> diff that adds support for copying the ESRT in the bootloader and
> passing it on to the kernel.
>
> I adjusted your diff a bit. It now adds the new config_esrt member at
> the end of the bios_efiinfo struct and sets a flag to indicate that
> extra bit of information is present. That makes it possible to load
> new kernels with the old bootloader and vice versa.
>
> patrick@, mlarkin@, yasuoka@ and other devs: ok?
Tested on my x280, I used the following diff to check whether ESRT was
detected and copied:
efi0 at bios0: UEFI 2.5
efi0: Lenovo rev 0x1380, ESRT present
ok jca@
Index: arch/amd64/amd64/efi_machdep.c
===
RCS file: /home/cvs/src/sys/arch/amd64/amd64/efi_machdep.c,v
retrieving revision 1.4
diff -u -p -r1.4 efi_machdep.c
--- arch/amd64/amd64/efi_machdep.c 7 Nov 2022 01:41:57 - 1.4
+++ arch/amd64/amd64/efi_machdep.c 31 Dec 2022 03:38:20 -
@@ -135,9 +135,14 @@ efi_attach(struct device *parent, struct
printf("%s: ", sc->sc_dev.dv_xname);
for (i = 0; st->FirmwareVendor[i]; i++)
printf("%c", st->FirmwareVendor[i]);
- printf(" rev 0x%x\n", st->FirmwareRevision);
+ printf(" rev 0x%x", st->FirmwareRevision);
}
efi_leave(sc);
+
+ if (bios_efiinfo->flags & BEI_ESRT)
+ printf(", ESRT present");
+
+ printf("\n");
if (efi_enter_check(sc))
return;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: crunchgen and llvm 15
On Sat, Dec 31 2022, Theo de Raadt wrote: > It seems some changes are being pushed without actually testing > on the non-clang architectures. > > Can this get backed out until the correct procedure is followed? Well nothing has been committed regarding this crunchgen change. I hope you're not expecting us to back out other changes which are IMHO less controversial. > But I think it is also important to realize that not every > new behaviour of clang is "systems ready". Some of what they > are pushing aggressively is BS. The fact that they dropped ld -dc probably means the new behavior isn't "systems ready". OTOH I didn't know about ld -dc being used in crunchgen either... -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: crunchgen and llvm 15
On Fri, Dec 30 2022, Miod Vallat wrote:
> You need this extra chunk for your diff to work. With that, the built
> instbin binary appears to behave as expected on hppa.
Heh, nice, thank you.
I'm not sure which way to go: either sprinkle some -fno-common between
crunchgen and distrib/special/Makefile.inc, or drop -dc on lld archs
only. What would you folks prefer?
commit 548e9fc8c0cf82bccbd91ce2d0b7af7ae12a3207
Author: jca
Date: Fri Dec 30 08:01:56 2022 +0100
crunchgen: stop using ld -dc on lld archs (dropped in lld 15)
diff --git a/usr.sbin/crunchgen/crunchgen.c b/usr.sbin/crunchgen/crunchgen.c
index 4526290197e..7136c1d19c9 100644
--- a/usr.sbin/crunchgen/crunchgen.c
+++ b/usr.sbin/crunchgen/crunchgen.c
@@ -898,7 +898,11 @@ top_makefile_rules(FILE * outmk)
fprintf(outmk, "CFLAGS+=-fno-asynchronous-unwind-tables\n");
fprintf(outmk, "LDFLAGS+=$(NOPIE_LDFLAGS)\n");
fprintf(outmk, "STRIP?=strip\n");
+ fprintf(outmk, ".if ${LINKER_VERSION} == \"lld\"\n");
+ fprintf(outmk, "LINK=$(LD) -r ${LDFLAGS}\n");
+ fprintf(outmk, ".else\n");
fprintf(outmk, "LINK=$(LD) -dc -r ${LDFLAGS}\n");
+ fprintf(outmk, ".endif\n");
fprintf(outmk, "LIBS=");
for (l = libdirs; l != NULL; l = l->next)
fprintf(outmk, " -L%s", l->str);
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: clang 15 and zlib
On Wed, Dec 28 2022, Jeremie Courreges-Anglas wrote: > To build src cleanly with clang-15 you'd need the diff below. The > alternative would be to patch our three zlib copies and wait for > upstream to fix it. A more comfortable alternative is to just neuter > the warning. -Wno-unknown-warning-option prevents llvm 13 from > complaining that it doesn't know about -Wno-deprecated-non-prototype and > should ease the transition. I took care to only add this workaround on > clang archs, Committed, thanks. > and to respect the COMPILER_VERSION checks where due. I had missed the COMPILER_VERSION check in sys/arch/amd64/stand/Makefile.inc and sys/arch/i386/stand/Makefile.inc, now fixed. You'll have to clean up your tree. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
clang 15 and zlib
To build src cleanly with clang-15 you'd need the diff below. The
alternative would be to patch our three zlib copies and wait for
upstream to fix it. A more comfortable alternative is to just neuter
the warning. -Wno-unknown-warning-option prevents llvm 13 from
complaining that it doesn't know about -Wno-deprecated-non-prototype and
should ease the transition. I took care to only add this workaround on
clang archs, and to respect the COMPILER_VERSION checks where due.
ok?
commit b439995c024854e130e7211b1afee5c1e3f0b1cd
Author: jca
Date: Sat Dec 17 19:30:28 2022 +0100
sys/arch/*/stand: work around clang 15 error in zlib
Upstream issue:
https://github.com/madler/zlib/issues/633
diff --git a/sys/arch/amd64/stand/Makefile.inc
b/sys/arch/amd64/stand/Makefile.inc
index 7e45fedcc49..d35dc33cb2d 100644
--- a/sys/arch/amd64/stand/Makefile.inc
+++ b/sys/arch/amd64/stand/Makefile.inc
@@ -1,6 +1,9 @@
# $OpenBSD: Makefile.inc,v 1.18 2017/07/25 13:32:14 robert Exp $
CFLAGS=${DEBUG} ${COPTS} -Oz -Wall -Werror
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+CFLAGS+= -Wno-deprecated-non-prototype -Wno-unknown-warning-option
CFLAGS+= -ffreestanding -fno-stack-protector -DMDRANDOM
CPPFLAGS+=-I${S} -I${SADIR}/libsa -I. -I${.CURDIR}
SACFLAGS=-D_STANDALONE
diff --git a/sys/arch/arm64/stand/efiboot/Makefile
b/sys/arch/arm64/stand/efiboot/Makefile
index 6cbb987af65..93be268d3bf 100644
--- a/sys/arch/arm64/stand/efiboot/Makefile
+++ b/sys/arch/arm64/stand/efiboot/Makefile
@@ -53,6 +53,9 @@ COPTS+= -Wno-attributes -Wno-format
COPTS+=-ffreestanding -fno-stack-protector
COPTS+=-fshort-wchar -fPIC -fno-builtin
COPTS+=-Wall -Werror
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+COPTS+=-Wno-deprecated-non-prototype
-Wno-unknown-warning-option
PROG.elf= ${PROG:S/.EFI/.elf/}
CLEANFILES+= ${PROG.elf} ${PROG.elf}.tmp
diff --git a/sys/arch/armv7/stand/efiboot/Makefile
b/sys/arch/armv7/stand/efiboot/Makefile
index d6ffc41bf38..fd78e744cd3 100644
--- a/sys/arch/armv7/stand/efiboot/Makefile
+++ b/sys/arch/armv7/stand/efiboot/Makefile
@@ -51,6 +51,9 @@ COPTS+= -ffreestanding -fno-stack-protector
COPTS+=-fshort-wchar -fPIC -fno-builtin
COPTS+=-Wall -Werror
COPTS+=-mfloat-abi=soft
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+COPTS+=-Wno-deprecated-non-prototype
-Wno-unknown-warning-option
PROG.elf= ${PROG:S/.EFI/.elf/}
CLEANFILES+= ${PROG.elf} ${PROG.elf}.tmp
diff --git a/sys/arch/i386/stand/Makefile.inc b/sys/arch/i386/stand/Makefile.inc
index cd14f5b8954..ca70fed2124 100644
--- a/sys/arch/i386/stand/Makefile.inc
+++ b/sys/arch/i386/stand/Makefile.inc
@@ -2,6 +2,9 @@
CFLAGS=${DEBUG} ${COPTS} -Oz -Wall -Werror
CFLAGS+= -ffreestanding -fno-stack-protector -DMDRANDOM
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+CFLAGS+= -Wno-deprecated-non-prototype -Wno-unknown-warning-option
CPPFLAGS+=-I${S} -I${SADIR}/libsa -I. -I${.CURDIR}
SACFLAGS=-D_STANDALONE
DEBUGFLAGS=
diff --git a/sys/arch/riscv64/stand/efiboot/Makefile
b/sys/arch/riscv64/stand/efiboot/Makefile
index a9906d58a01..36f84cf29df 100644
--- a/sys/arch/riscv64/stand/efiboot/Makefile
+++ b/sys/arch/riscv64/stand/efiboot/Makefile
@@ -54,6 +54,9 @@ COPTS+= -Wno-attributes -Wno-format
COPTS+=-ffreestanding -fno-stack-protector
COPTS+=-fshort-wchar -fPIC -fno-builtin
COPTS+=-Wall -Werror
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+COPTS+=-Wno-deprecated-non-prototype
-Wno-unknown-warning-option
PROG.elf= ${PROG:S/.EFI/.elf/}
CLEANFILES+= ${PROG.elf} ${PROG.elf}.tmp
commit 57a9510544cc42e91487b43363f5f9ba429829b0
Author: jca
Date: Mon Dec 19 18:05:54 2022 +0100
sys: fix clang 15 errors with zlib
While here, add -Wno-unknown-warning-option so that it's possible to build
kernels with llvm 13 which doesn't understand -Wno-deprecated-non-prototype.
Upstream zlib issue:
https://github.com/madler/zlib/issues/633
diff --git a/sys/arch/amd64/conf/Makefile.amd64
b/sys/arch/amd64/conf/Makefile.amd64
index d2704424f34..e77c296313f 100644
--- a/sys/arch/amd64/conf/Makefile.amd64
+++ b/sys/arch/amd64/conf/Makefile.amd64
@@ -72,6 +72,9 @@ CMACHFLAGS+= -mno-retpoline
NO_INTEGR_AS= -no-integrated-as
CWARNFLAGS+= -Wno-address-of-packed-member -Wno-constant-conversion \
-Wno-unused-but-set-variable -Wno-gnu-folding-constant
+# XXX Workaround for zlib + clang 15
+# https://github.com/madler/zlib/issues/633
+CWARNFLAGS+= -Wno-deprecated-non-prototype -Wno-unknown-warning-option
.endif
DEBUG?=-g
diff --g
crunchgen and llvm 15
At first it looked like a fluke when building a release. What's this
"ld -dc" command-line? Well -dc is short for --define-common, an option
that has been dropped by upstream llvm(lld).
IIUC this ld(1) behavior is somewhat similar to -fno-common for cc(1).
base-clang already does -fno-common by default, base-gcc does not. The
diff below replaces ld -dc by cc -fno-common, but TBF I'm not sure what
we're trying to achieve (avoid?) here. A test (make build + release) on
a base-gcc arch would be welcome.
commit 6cf10f57e9d6a5c359007e7b75ed737be8954bb4
Author: jca
Date: Wed Dec 28 16:36:41 2022 +0100
crunchgen: stop using ld -dc (dropped in lld 15)
Instead, use cc -fno-common to prevent base-gcc from using commons.
base-clang already does the right thing.
diff --git a/usr.sbin/crunchgen/crunchgen.c b/usr.sbin/crunchgen/crunchgen.c
index 4526290197e..6350b61195b 100644
--- a/usr.sbin/crunchgen/crunchgen.c
+++ b/usr.sbin/crunchgen/crunchgen.c
@@ -893,12 +893,13 @@ top_makefile_rules(FILE * outmk)
fprintf(outmk, ".include \n");
fprintf(outmk, "CFLAGS+=$(NOPIE_FLAGS)\n");
fprintf(outmk, "CFLAGS+=-Oz\n");
+ fprintf(outmk, "CFLAGS+=-fno-common\n");
fprintf(outmk, "CFLAGS+=-fno-stack-protector\n");
fprintf(outmk, "CFLAGS+=-fno-unwind-tables\n");
fprintf(outmk, "CFLAGS+=-fno-asynchronous-unwind-tables\n");
fprintf(outmk, "LDFLAGS+=$(NOPIE_LDFLAGS)\n");
fprintf(outmk, "STRIP?=strip\n");
- fprintf(outmk, "LINK=$(LD) -dc -r ${LDFLAGS}\n");
+ fprintf(outmk, "LINK=$(LD) -r ${LDFLAGS}\n");
fprintf(outmk, "LIBS=");
for (l = libdirs; l != NULL; l = l->next)
fprintf(outmk, " -L%s", l->str);
diff --git a/usr.sbin/crunchgen/crunchide.c b/usr.sbin/crunchgen/crunchide.c
index 16e032370db..84539fb57be 100644
--- a/usr.sbin/crunchgen/crunchide.c
+++ b/usr.sbin/crunchgen/crunchide.c
@@ -29,7 +29,7 @@
* crunchide.c - tiptoes through an a.out symbol table, hiding all defined
* global symbols. Allows the user to supply a "keep list" of symbols
* that are not to be hidden. This program relies on the use of the
- * linker's -dc flag to actually put global bss data into the file's
+ * compiler's -fno-common flag to actually put global bss data into the
file's
* bss segment (rather than leaving it as undefined "common" data).
*
* The point of all this is to allow multiple programs to be linked
@@ -39,8 +39,8 @@
* small stub routine, called "foostub.c", eg:
* int foo_main(int argc, char **argv){ return main(argc, argv); }
* like so:
- * cc -c foo.c foostub.c
- * ld -dc -r foo.o foostub.o -o foo.combined.o
+ * cc -fno-common -c foo.c foostub.c
+ * ld -r foo.o foostub.o -o foo.combined.o
* crunchide -k _foo_main foo.combined.o
* at this point, foo.combined.o can be linked with another program
* and invoked with "foo_main(argc, argv)". foo's main() and any
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
macppc: ansify some code (fix clang 15 errors)
This should fix warnings in the macppc build (kernel + bootloader) with
clang 15 (-Wdeprecated-non-prototype). Not build tested (no macppc
machine at hand) but should be safe. ok?
Index: arch/macppc/dev/pm_direct.c
===
RCS file: /home/cvs/src/sys/arch/macppc/dev/pm_direct.c,v
retrieving revision 1.33
diff -u -p -r1.33 pm_direct.c
--- arch/macppc/dev/pm_direct.c 23 Oct 2022 08:00:10 - 1.33
+++ arch/macppc/dev/pm_direct.c 27 Dec 2022 02:14:20 -
@@ -200,11 +200,7 @@ extern voidadb_pass_up(struct adbComman
* This function dumps contents of the PMData
*/
void
-pm_printerr(ttl, rval, num, data)
- char *ttl;
- int rval;
- int num;
- char *data;
+pm_printerr(char *ttl, int rval, int num, char *data)
{
int i;
Index: arch/macppc/stand/ofdev.c
===
RCS file: /home/cvs/src/sys/arch/macppc/stand/ofdev.c,v
retrieving revision 1.28
diff -u -p -r1.28 ofdev.c
--- arch/macppc/stand/ofdev.c 12 Oct 2022 09:23:45 - 1.28
+++ arch/macppc/stand/ofdev.c 27 Dec 2022 02:15:09 -
@@ -164,8 +164,7 @@ struct fs_ops file_system[4];
int nfsys;
static u_long
-get_long(p)
- const void *p;
+get_long(const void *p)
{
const unsigned char *cp = p;
@@ -238,12 +237,8 @@ read_mac_label(struct of_dev *devp, char
* Find a valid disklabel.
*/
static int
-search_label(devp, off, buf, lp, off0)
- struct of_dev *devp;
- u_long off;
- char *buf;
- struct disklabel *lp;
- u_long off0;
+search_label(struct of_dev *devp, u_long off, char *buf, struct disklabel *lp,
+u_long off0)
{
size_t read;
struct dos_partition *p;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
libsa: ansify some pxe code
These functions trip up -Wdeprecated-non-prototype (clang 15). We'll
probably use -Wno-deprecated-non-prototype until zlib gets fixed
upstream but let's clean up the code first.
zlib2ansi doesn't find other errors in sys/arch/*clang archs* except for
macppc (diff for that next).
ok?
Index: sys/lib/libsa/netif.c
===
RCS file: /home/cvs/src/sys/lib/libsa/netif.c,v
retrieving revision 1.13
diff -u -p -r1.13 netif.c
--- sys/lib/libsa/netif.c 25 Oct 2021 15:59:46 - 1.13
+++ sys/lib/libsa/netif.c 27 Dec 2022 02:05:49 -
@@ -260,8 +260,7 @@ netif_put(struct iodesc *desc, void *pkt
}
struct iodesc *
-socktodesc(sock)
- int sock;
+socktodesc(int sock)
{
if (sock >= SOPEN_MAX) {
errno = EBADF;
Index: sys/arch/amd64/stand/libsa/pxe.c
===
RCS file: /home/cvs/src/sys/arch/amd64/stand/libsa/pxe.c,v
retrieving revision 1.7
diff -u -p -r1.7 pxe.c
--- sys/arch/amd64/stand/libsa/pxe.c6 Mar 2016 22:41:24 - 1.7
+++ sys/arch/amd64/stand/libsa/pxe.c27 Dec 2022 02:05:49 -
@@ -241,8 +241,7 @@ pxe_netif_open(void)
}
void
-pxe_netif_close(sock)
- int sock;
+pxe_netif_close(int sock)
{
t_PXENV_UDP_CLOSE *uc = (void *) pxe_command_buf;
@@ -271,8 +270,7 @@ pxe_netif_shutdown(void)
}
struct iodesc *
-pxesocktodesc(sock)
- int sock;
+pxesocktodesc(int sock)
{
#ifdef NETIF_DEBUG
Index: sys/arch/i386/stand/libsa/pxe.c
===
RCS file: /home/cvs/src/sys/arch/i386/stand/libsa/pxe.c,v
retrieving revision 1.7
diff -u -p -r1.7 pxe.c
--- sys/arch/i386/stand/libsa/pxe.c 7 Mar 2016 05:32:47 - 1.7
+++ sys/arch/i386/stand/libsa/pxe.c 27 Dec 2022 02:05:49 -
@@ -241,8 +241,7 @@ pxe_netif_open(void)
}
void
-pxe_netif_close(sock)
- int sock;
+pxe_netif_close(int sock)
{
t_PXENV_UDP_CLOSE *uc = (void *) pxe_command_buf;
@@ -271,8 +270,7 @@ pxe_netif_shutdown(void)
}
struct iodesc *
-pxesocktodesc(sock)
- int sock;
+pxesocktodesc(int sock)
{
#ifdef NETIF_DEBUG
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: perl: fix build with clang 15
On Mon, Dec 26 2022, Andrew Hewus Fresh wrote:
> On Tue, Dec 27, 2022 at 02:22:43AM +0100, Jeremie Courreges-Anglas wrote:
>>
>> MD5.xs:375:21: error: mixing declarations and code is incompatible with
>> standards before C99 [-Werror,-Wdeclaration-after-statement]
>> unsigned char *buf = (unsigned char *)(SvPV(ST(2), len));
>>
>> The build system asks for -Werror *and* -Wdeclaration-after-statement so
>> let's fix the code. ok?
>
> I just happened to be in this local patch today updating for 5.36. This
> seems OK to me and I'll make sure I update it.
Oooh, thanks for pointing this out, I did not expect a local patch in
here. I'll commit this then.
> Upstream declares variable there that we don't use.
>
> https://github.com/afresh1/OpenBSD-perl/blob/blead/patches/GOOD/use_our_MD5.patch#L373-L374
Understood.
>
>> Index: gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs
>> ===
>> RCS file: /home/cvs/src/gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs,v
>> retrieving revision 1.20
>> diff -u -p -r1.20 MD5.xs
>> --- gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs 1 Mar 2021 23:21:24 -
>> 1.20
>> +++ gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs 27 Dec 2022 01:20:54 -
>> @@ -371,8 +371,8 @@ context(ctx, ...)
>> PPCODE:
>> if (items > 2) {
>> STRLEN len;
>> -ctx->count = SvUV(ST(1)) << 3;
>> unsigned char *buf = (unsigned char *)(SvPV(ST(2), len));
>> +ctx->count = SvUV(ST(1)) << 3;
>> ctx->state[0] = buf[ 0] | (buf[ 1]<<8) | (buf[ 2]<<16) | (buf[
>> 3]<<24);
>> ctx->state[1] = buf[ 4] | (buf[ 5]<<8) | (buf[ 6]<<16) | (buf[
>> 7]<<24);
>> ctx->state[2] = buf[ 8] | (buf[ 9]<<8) | (buf[10]<<16) |
>> (buf[11]<<24);
>>
>>
>> --
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
perl: fix build with clang 15
MD5.xs:375:21: error: mixing declarations and code is incompatible with
standards before C99 [-Werror,-Wdeclaration-after-statement]
unsigned char *buf = (unsigned char *)(SvPV(ST(2), len));
The build system asks for -Werror *and* -Wdeclaration-after-statement so
let's fix the code. ok?
Index: gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs
===
RCS file: /home/cvs/src/gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs,v
retrieving revision 1.20
diff -u -p -r1.20 MD5.xs
--- gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs 1 Mar 2021 23:21:24 -
1.20
+++ gnu/usr.bin/perl/cpan/Digest-MD5/MD5.xs 27 Dec 2022 01:20:54 -
@@ -371,8 +371,8 @@ context(ctx, ...)
PPCODE:
if (items > 2) {
STRLEN len;
- ctx->count = SvUV(ST(1)) << 3;
unsigned char *buf = (unsigned char *)(SvPV(ST(2), len));
+ ctx->count = SvUV(ST(1)) << 3;
ctx->state[0] = buf[ 0] | (buf[ 1]<<8) | (buf[ 2]<<16) | (buf[
3]<<24);
ctx->state[1] = buf[ 4] | (buf[ 5]<<8) | (buf[ 6]<<16) | (buf[
7]<<24);
ctx->state[2] = buf[ 8] | (buf[ 9]<<8) | (buf[10]<<16) |
(buf[11]<<24);
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
gdb: fix build with clang 15
/usr/src/gnu/usr.bin/binutils/gdb/tui/tui-stack.c:359:16: error: incompatible
integer to pointer conversion passing 'CORE_ADDR' (aka 'unsigned long') to
parameter of type 'CORE_ADDR *' (aka 'unsigned long *') [-Wint-conversion]
&low, (CORE_ADDR) NULL) == 0)
^~~~
/usr/src/gnu/usr.bin/binutils/gdb/symtab.h:1084:21: note: passing argument to
parameter here
CORE_ADDR *);
^
The code wants a pointer to a "CORE_ADDR", thus the "(CORE_ADDR)" cast
is erroneous. Just pass NULL. ok?
Index: gnu/usr.bin/binutils/gdb/tui/tui-stack.c
===
RCS file: /home/cvs/src/gnu/usr.bin/binutils/gdb/tui/tui-stack.c,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 tui-stack.c
--- gnu/usr.bin/binutils/gdb/tui/tui-stack.c21 May 2004 19:15:20 -
1.1.1.1
+++ gnu/usr.bin/binutils/gdb/tui/tui-stack.c27 Dec 2022 01:15:51 -
@@ -356,7 +356,7 @@ tui_show_frame_info (struct frame_info *
else
{
if (find_pc_partial_function (get_frame_pc (fi), (char **) NULL,
- &low, (CORE_ADDR) NULL) == 0)
+ &low, NULL) == 0)
error ("No function contains program counter for selected
frame.\n");
else
low = tui_get_low_disassembly_address (low, get_frame_pc (fi));
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Size reduction experiments for LLVM-built sparc64 kernel
On Sun, Dec 25 2022, Koakuma wrote:
> Some weekend updates:
>
> 1. The clang-built kernels seem to be working well enough that I could
>complete building a (GCC-built) userland.
That's good to know, I think I haven't tested that after we moved to
llvm 13.
> 2. I tried a larger set of LLVM patches (D51206, D128263, D130006,
>D132465, D135515, D138532, D138741, D138887, D138922, D139535, and
>D140515) and while it does reduce the kernel binary, it did not
>do it much - the kernel only gets about 10k smaller compared to
>the previous build.
>(that is, still ~77k bigger than the GCC-built binary)
That's a nice improvement but a lot of patches. patrick@ and I started
working on an update to llvm 15. No timeline yet but maybe some/all of
the changes you mentioned would already be integrated in that update?
(Or conclicting with it...)
> text databss dec hex
> 8089416 2295436 728216 3068a9926c bsd.clang+patch+noinline
> 8066232 2304032 732528 11102792a96a48
> bsd.clang+patchv2+noinline
> 7862920 2429596 730968 11023484a8347c bsd.gcc
>
> 3. Also, additionally I also tried to build the userland with clang but
>sadly it fails with some compiler errors:
>
> With `make COMPILER_VERSION=clang CC=clang build`, I'm getting:
As mentioned privately better remove sparc64 from GCC4_ARCHS in
bsd.own.mk and let the Makefiles pick up clang/clang++.
> In file included from
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:15,
> from
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic.h:16,
> from
> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:1:
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_platform.h:25:18:
> error: missing binary operator before token "("
> In file included from
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic.h:16,
> from
> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:1:
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:412:
> error: 'constexpr' does not name a type
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h:413:
> error: 'constexpr' does not name a type
> In file included from
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang.h:20,
> from
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic.h:63,
> from
> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:1:
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:
> In function 'typename T::Type __sanitizer::atomic_load(const volatile T*,
> __sanitizer::memory_order)':
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:54:
> error: '__ATOMIC_SEQ_CST' was not declared in this scope
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:
> In function 'void __sanitizer::atomic_store(volatile T*, typename T::Type,
> __sanitizer::memory_order)':
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:79:
> error: '__ATOMIC_SEQ_CST' was not declared in this scope
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:
> In function 'typename T::Type __sanitizer::atomic_load(const volatile T*,
> __sanitizer::memory_order) [with T = __sanitizer::atomic_uint32_t]':
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic.h:76:
> instantiated from 'typename T::Type __sanitizer::atomic_load_relaxed(const
> volatile T*) [with T = __sanitizer::atomic_uint32_t]'
> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:27:
>instantiated from here
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:53:
> error: '__atomic_load' was not declared in this scope
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:
> In function 'typename T::Type __sanitizer::atomic_load(const volatile T*,
> __sanitizer::memory_order) [with T = __sanitizer::atomic_uintptr_t]':
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic.h:76:
> instantiated from 'typename T::Type __sanitizer::atomic_load_relaxed(const
> volatile T*) [with T = __sanitizer::atomic_uintptr_t]'
> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:34:
>instantiated from here
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer_common/sanitizer_atomic_clang_other.h:53:
> error: '__atomic_load' was not declared in this scope
> /usr/src/gnu/llvm/compiler-rt/lib/sanitizer
Re: LLVM 15: mismatched bound errors
On Sat, Dec 24 2022, Patrick Wildt wrote: > On Thu, Dec 22, 2022 at 01:14:57AM +0100, Patrick Wildt wrote: >> On Tue, Dec 20, 2022 at 05:48:41PM -0700, Todd C. Miller wrote: >> > On Tue, 20 Dec 2022 23:44:08 +0100, Patrick Wildt wrote: >> > >> > > clang complains when the function is declared with a fixed array size in >> > > a parameter while the prototype is unbounded, like this: >> > > >> > > /usr/src/sys/net/pf.c:4353:54: error: argument 'sns' of type 'struct >> > > pf_src_n >> > > ode *[4]' with mismatched bound [-Werror,-Warray-parameter] >> > > struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX]) >> > > ^ >> > > /usr/src/sys/net/pf.c:203:28: note: previously declared as 'struct >> > > pf_src_nod >> > > e *[]' here >> > > struct pf_src_node *[]); >> > > ^ >> > > 1 error generated. >> > > >> > > We have a few of that, and was wondering what the better solution is. >> > > clang apparently accepts using * instead of []. The alternative would >> > > be to hardcode the size in the prototype as well. Here's a diff for >> > > a three files for the first version, as example. >> > >> > Using * instead of [] is the saner approach. Hard-coding the sizes >> > into the prototype seems like a bad idea, even if clang is now smart >> > enough to complain about a mismatch. >> > >> > - todd >> >> So, here's the full diff that allows me to compile arm64 GENERIC.MP, >> with radeondrm and amdgpu disabled. > > Right, sorry for derailing the thread with a different discussion. > Here's a diff only for the array/ptr changes. That's for the kernel part. libsa diff below, ok? Index: hmac_sha1.h === RCS file: /home/cvs/src/sys/lib/libsa/hmac_sha1.h,v retrieving revision 1.1 diff -u -p -p -u -r1.1 hmac_sha1.h --- hmac_sha1.h 9 Oct 2012 12:36:50 - 1.1 +++ hmac_sha1.h 27 Dec 2022 00:45:39 - @@ -22,4 +22,4 @@ * HMAC-SHA-1 (from RFC 2202). */ void hmac_sha1(const u_int8_t *, size_t, const u_int8_t *, -size_t, u_int8_t []); +size_t, u_int8_t *); -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Thu, Dec 22 2022, Todd C. Miller wrote:
> On Thu, 22 Dec 2022 02:08:42 +0100, Jeremie Courreges-Anglas wrote:
>
>> https://github.com/jcourreges/openbsd-src/commit/4862df383ccb8a8e03d5c11b4f
>> b739b6a3a5a7c7
>>
>> Sadly making the size available in the declaration doesn't seem to be
>> clang any smarter (yet?). clang won't warn about passing the address of
>> array[10] to a function which access array[15] or so.
>>
>> I don't care much about the direction we end up using, but specifying
>> the size in the declaration isn't insane. We seldom pass a pointers to
>> a buffer without an accompanying buffer length.
>
> My objection to adding sizes to the prototype and function declaration
> is that it encourages things like:
>
> int foo(char buf[2048])
> {
> ...
> snprintf(buf, sizeof(buf), "See spot run, run spot run...");
> }
>
> But of course, sizeof(buf) is really sizeof(char *). The compiler
> will warn when you do this so perhaps it is not such a big problem.
> It still feels like a footgun to me.
-Wsizeof-pointer-memaccess should indeed help here.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Sat, Dec 24 2022, Patrick Wildt wrote: > On Thu, Dec 22, 2022 at 01:14:57AM +0100, Patrick Wildt wrote: >> On Tue, Dec 20, 2022 at 05:48:41PM -0700, Todd C. Miller wrote: >> > On Tue, 20 Dec 2022 23:44:08 +0100, Patrick Wildt wrote: >> > >> > > clang complains when the function is declared with a fixed array size in >> > > a parameter while the prototype is unbounded, like this: >> > > >> > > /usr/src/sys/net/pf.c:4353:54: error: argument 'sns' of type 'struct >> > > pf_src_n >> > > ode *[4]' with mismatched bound [-Werror,-Warray-parameter] >> > > struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX]) >> > > ^ >> > > /usr/src/sys/net/pf.c:203:28: note: previously declared as 'struct >> > > pf_src_nod >> > > e *[]' here >> > > struct pf_src_node *[]); >> > > ^ >> > > 1 error generated. >> > > >> > > We have a few of that, and was wondering what the better solution is. >> > > clang apparently accepts using * instead of []. The alternative would >> > > be to hardcode the size in the prototype as well. Here's a diff for >> > > a three files for the first version, as example. >> > >> > Using * instead of [] is the saner approach. Hard-coding the sizes >> > into the prototype seems like a bad idea, even if clang is now smart >> > enough to complain about a mismatch. >> > >> > - todd >> >> So, here's the full diff that allows me to compile arm64 GENERIC.MP, >> with radeondrm and amdgpu disabled. > > Right, sorry for derailing the thread with a different discussion. > Here's a diff only for the array/ptr changes. > > ok? A bit late to this party but ok jca@, thanks. Why do you mention building arm64 GENERIC.MP "with radeondrm and amdgpu disabled"? Don't those drivers build with llvm15? -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Wed, Dec 21 2022, "Theo de Raadt" wrote: >> But yeah, maybe we'll just flip the default option in LLVM and then >> we'll just not use that warning... at all? > > is this specific warning finding dangerous bugs? is it finding a substantial > number of dangerous bugs? No, but if we remove -Wdeprecated-non-prototype from -Wall, software built with clang -Wall on OpenBSD won't hit this warning and may later fail when built with clang -Wall on other systems. *I* don't care that much, but I suspect that this is not the additional warning that will hurt us most... -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Thu, Dec 22 2022, Patrick Wildt wrote: > Am Thu, Dec 22, 2022 at 01:57:37AM +0100 schrieb Jeremie Courreges-Anglas: >> On Thu, Dec 22 2022, Theo Buehler wrote: >> > On Thu, Dec 22, 2022 at 11:39:41AM +1100, Jonathan Gray wrote: >> >> On Thu, Dec 22, 2022 at 01:20:32AM +0100, Theo Buehler wrote: >> >> > > Any concerns regarding the changes in libz? It introduces diff to >> >> > > upstream, but the recent commits seemed to indicate we have forked >> >> > > anyway? >> >> > >> >> > I've worked hard to keep the diff to upstream minimal. Why are these >> >> > changes needed? >> >> >> >> the warnings are from -Wdeprecated-non-prototype >> >> >> >> https://github.com/madler/zlib/issues/633 >> > >> > So, can't we fix this by adding -Wno-deprecated-non-prototype to the >> > kernel build and wait until madler converts upstream? >> >> That's what I initially proposed on my own wip branch: >> >> >> https://github.com/jcourreges/openbsd-src/commit/50d8bd24dadc25aa7e985de898bccf92a60b72ee >> >> -Wno-deprecated-non-prototype + -Wno-unknown-warning-option (the latter >> option because current clang doesn't understand the former). libz built >> as part of libsa and the bootloaders also needs a similar tweak in MD >> Makefiles: >> >> >> https://github.com/jcourreges/openbsd-src/commit/2107b762420ef6ea863e349e5faea4254d44fdf9 >> >> The build of src/lib/libz doesn't use -Werror and thus doesn't error out. >> >> The two commits above are obviously incomplete, other archs need similar >> handling. Apart from the fact that I authored them, I think they're >> the sanest way to handle this libz "oddness". Not just because I care >> about tb's sanity. :) > > I had hoped that we can resolve that without adding another set of -W > flags everywhere, but I guess the libz churn isn't worth it. > > But yeah, maybe we'll just flip the default option in LLVM and then > we'll just not use that warning... at all? That's an approach we didn't use for llvm13. Right now clang-local(1) only lists -Wpointer-sign and -Waddress-of-packed-member as different from upstream. As far as base is concerned, -Wno-deprecated-non-prototype can be introduced just for libz (kernels + bootloaders) and then removed as soon as upstream makes the jump. So for base we don't really need to tweak the defaults, I'd rather see what happens in ports wrt -Wdeprecated-non-prototype, and I don't think it's the biggest offender. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Tue, Dec 20 2022, Todd C. Miller wrote: > On Tue, 20 Dec 2022 23:44:08 +0100, Patrick Wildt wrote: > >> clang complains when the function is declared with a fixed array size in >> a parameter while the prototype is unbounded, like this: >> >> /usr/src/sys/net/pf.c:4353:54: error: argument 'sns' of type 'struct pf_src_n >> ode *[4]' with mismatched bound [-Werror,-Warray-parameter] >> struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX]) >> ^ >> /usr/src/sys/net/pf.c:203:28: note: previously declared as 'struct pf_src_nod >> e *[]' here >> struct pf_src_node *[]); >> ^ >> 1 error generated. >> >> We have a few of that, and was wondering what the better solution is. >> clang apparently accepts using * instead of []. The alternative would >> be to hardcode the size in the prototype as well. Here's a diff for >> a three files for the first version, as example. > > Using * instead of [] is the saner approach. Hard-coding the sizes > into the prototype seems like a bad idea, even if clang is now smart > enough to complain about a mismatch. At first I went with this approach: https://github.com/jcourreges/openbsd-src/commit/b24dd00848e30977d1aed7404afe9455aff0c9dc but the full kernel diff to add the sizes to the declaration is short, as the size is either hardcoded in the same file or hidden behind a define in scope: https://github.com/jcourreges/openbsd-src/commit/4862df383ccb8a8e03d5c11b4fb739b6a3a5a7c7 Sadly making the size available in the declaration doesn't seem to be clang any smarter (yet?). clang won't warn about passing the address of array[10] to a function which access array[15] or so. I don't care much about the direction we end up using, but specifying the size in the declaration isn't insane. We seldom pass a pointers to a buffer without an accompanying buffer length. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: LLVM 15: mismatched bound errors
On Thu, Dec 22 2022, Theo Buehler wrote: > On Thu, Dec 22, 2022 at 11:39:41AM +1100, Jonathan Gray wrote: >> On Thu, Dec 22, 2022 at 01:20:32AM +0100, Theo Buehler wrote: >> > > Any concerns regarding the changes in libz? It introduces diff to >> > > upstream, but the recent commits seemed to indicate we have forked >> > > anyway? >> > >> > I've worked hard to keep the diff to upstream minimal. Why are these >> > changes needed? >> >> the warnings are from -Wdeprecated-non-prototype >> >> https://github.com/madler/zlib/issues/633 > > So, can't we fix this by adding -Wno-deprecated-non-prototype to the > kernel build and wait until madler converts upstream? That's what I initially proposed on my own wip branch: https://github.com/jcourreges/openbsd-src/commit/50d8bd24dadc25aa7e985de898bccf92a60b72ee -Wno-deprecated-non-prototype + -Wno-unknown-warning-option (the latter option because current clang doesn't understand the former). libz built as part of libsa and the bootloaders also needs a similar tweak in MD Makefiles: https://github.com/jcourreges/openbsd-src/commit/2107b762420ef6ea863e349e5faea4254d44fdf9 The build of src/lib/libz doesn't use -Werror and thus doesn't error out. The two commits above are obviously incomplete, other archs need similar handling. Apart from the fact that I authored them, I think they're the sanest way to handle this libz "oddness". Not just because I care about tb's sanity. :) -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: amptimer(4): switch to clockintr
On Sat, Dec 10 2022, Jeremie Courreges-Anglas wrote: > On Fri, Dec 09 2022, Scott Cheloha wrote: >> Next up for the armv7 clockintr switch: amptimer(4). >> >> - Remove amptimer-specific clock interrupt scheduling bits and >> randomized statclock bits. >> - Remove debug evcounts. The interrupt's evcount counts all clock >> interrupts from now on. >> - Remove unused USE_GTIMER_CMP pieces. >> - Wire up amptimer_intrclock. >> >> jca: You have a cubox with this device. Does this compile? >> If so, does it boot? > > Builds but the machine didn't come up after reboot. I'm not at home > right now so it'll have to wait a day or two, unless someone else can > test on their own machine. Here's an updated diff with the fix found by cheloha (use 1 instead of 0 to trigger the timer ASAP). A GENERIC kernel with this diff completed a make build (minus some parts of llvm/clang), no clock drift visible so far. A bsd.rd including this diff successfully ran an upgrade over fec(4). dmesg: --8<-- OpenBSD 7.2-current (GENERIC) #9: Wed Dec 14 23:57:14 CET 2022 j...@cubox.lan:/sys/arch/armv7/compile/GENERIC real mem = 3980587008 (3796MB) avail mem = 3900547072 (3719MB) random: good seed from bootblocks mainbus0 at root: SolidRun Cubox-i Dual/Quad cpu0 at mainbus0 mpidr 0: ARM Cortex-A9 r2p10 cpu0: 32KB 32b/line 4-way L1 VIPT I-cache, 32KB 32b/line 4-way L1 D-cache cortex0 at mainbus0 amptimer0 at cortex0: 396000 kHz armliicc0 at cortex0: rtl 7 waymask: 0x000f simplebus0 at mainbus0: "soc" ampintc0 at simplebus0 nirq 160, ncpu 4: "interrupt-controller" "dma-apbh" at simplebus0 not configured "hdmi" at simplebus0 not configured "gpu" at simplebus0 not configured "gpu" at simplebus0 not configured "timer" at simplebus0 not configured "cache-controller" at simplebus0 not configured simplebus1 at simplebus0: "bus" imxccm0 at simplebus1 imxanatop0 at simplebus1 syscon0 at simplebus1: "snvs" imxrtc0 at syscon0 "snvs-lpgpr" at syscon0 not configured imxsrc0 at simplebus1 syscon1 at simplebus1: "iomuxc-gpr" "mux-controller" at syscon1 not configured "ipu1_csi0_mux" at syscon1 not configured "ipu2_csi1_mux" at syscon1 not configured imxiomuxc0 at simplebus1 simplebus2 at simplebus1: "spba-bus" "spdif" at simplebus2 not configured imxuart0 at simplebus2: console "asrc" at simplebus2 not configured "vpu" at simplebus1 not configured "pwm" at simplebus1 not configured "timer" at simplebus1 not configured imxgpio0 at simplebus1 imxgpio1 at simplebus1 imxgpio2 at simplebus1 imxgpio3 at simplebus1 imxgpio4 at simplebus1 imxgpio5 at simplebus1 imxgpio6 at simplebus1 imxdog0 at simplebus1 "usbphy" at simplebus1 not configured "usbphy" at simplebus1 not configured imxgpc0 at simplebus1 "sdma" at simplebus1 not configured simplebus3 at simplebus0: "bus" syscon2 at simplebus3: "efuse" "crypto" at simplebus3 not configured imxehci0 at simplebus3 usb0 at imxehci0: USB revision 2.0 uhub0 at usb0 configuration 1 interface 0 "i.MX EHCI root hub" rev 2.00/1.00 addr 1 imxehci1 at simplebus3 usb1 at imxehci1: USB revision 2.0 uhub1 at usb1 configuration 1 interface 0 "i.MX EHCI root hub" rev 2.00/1.00 addr 1 "usbmisc" at simplebus3 not configured fec0 at simplebus3 fec0: address d0:63:b4:xx:xx:xx atphy0 at fec0 phy 0: AR8035 10/100/1000 PHY, rev. 2 imxesdhc0 at simplebus3 imxesdhc0: 198 MHz base clock sdmmc0 at imxesdhc0: 4-bit, sd high-speed, mmc high-speed, dma imxesdhc1 at simplebus3 imxesdhc1: 198 MHz base clock sdmmc1 at imxesdhc1: 4-bit, sd high-speed, mmc high-speed, dma imxiic0 at simplebus3 iic0 at imxiic0 imxiic1 at simplebus3 iic1 at imxiic1 pcfrtc0 at iic1 addr 0x68: battery low "memory-controller" at simplebus3 not configured "vdoa" at simplebus3 not configured imxuart1 at simplebus3 "ipu" at simplebus0 not configured "sram" at simplebus0 not configured imxahci0 at simplebus0: AHCI 1.3 scsibus0 at imxahci0: 32 targets "gpu" at simplebus0 not configured "ipu" at simplebus0 not configured scsibus1 at sdmmc1: 2 targets, initiator 0 sd0 at scsibus1 targ 1 lun 0: removable sd0: 30436MB, 512 bytes/sector, 62333952 sectors bwfm0 at sdmmc0 function 1 manufacturer 0x02d0, product 0x4330 at sdmmc0 function 2 not configured vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets bootfile: sd0a:/bsd boot device: sd0 root on sd0a (a9d38e4f8011a0ce.a) swap on sd0b dump on sd0b bwfm0: address 6c:ad:f8:xx:xx:xx -->8-- ok jca@ when you see fit. Index: sys/arch/arm/include/cpu.h =
clang on riscv64: fix LTO link with PIE/PIC objects
On riscv64 LTO fails for some ports with this error: ld: error: linking module flags 'SmallDataLimit': IDs have conflicting values in '.o' and 'ld-temp.o' This is because some objects files are built PIC and others with PIE or no PIC at all, and the final link which creates ld-temp.o fails to take this into account. The problem is specific to LTO on riscv64, regular ELF linking isn't affected. This issue has been reported upstream recently, with a fix that uses llvm::Module::Min: https://reviews.llvm.org/D131230 Our llvm version doesn't support llvm::Module::Min so instead of backporting all/some parts of: https://github.com/llvm/llvm-project/commit/b0343a38a5910e980bb031e4014655d77cd0c162 I suggest we just replace llvm::Module::Error with llvm::Module::Warning here. Except for the pointless warnings, the end result would hopefully be the same: the first (and lowest) value is used, the second value from ld-temp.o is ignored. This will help me get the same LTO support on riscv64 as on other archs. Ports affected so far: games/arx-libertatis, multimedia/svt-av1, net/rrdtool, net/wireshark. I'm sure the list will get longer. ok? Index: clang/lib/CodeGen/CodeGenModule.cpp === RCS file: /cvs/src/gnu/llvm/clang/lib/CodeGen/CodeGenModule.cpp,v retrieving revision 1.1.1.4 diff -u -p -r1.1.1.4 CodeGenModule.cpp --- clang/lib/CodeGen/CodeGenModule.cpp 17 Dec 2021 12:24:37 - 1.1.1.4 +++ clang/lib/CodeGen/CodeGenModule.cpp 15 Dec 2022 22:35:55 - @@ -846,7 +846,7 @@ void CodeGenModule::EmitBackendOptionsMe break; case llvm::Triple::riscv32: case llvm::Triple::riscv64: -getModule().addModuleFlag(llvm::Module::Error, "SmallDataLimit", +getModule().addModuleFlag(llvm::Module::Warning, "SmallDataLimit", CodeGenOpts.SmallDataLimit); break; } -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: amptimer(4): switch to clockintr
On Fri, Dec 09 2022, Scott Cheloha wrote: > Next up for the armv7 clockintr switch: amptimer(4). > > - Remove amptimer-specific clock interrupt scheduling bits and > randomized statclock bits. > - Remove debug evcounts. The interrupt's evcount counts all clock > interrupts from now on. > - Remove unused USE_GTIMER_CMP pieces. > - Wire up amptimer_intrclock. > > jca: You have a cubox with this device. Does this compile? > If so, does it boot? Builds but the machine didn't come up after reboot. I'm not at home right now so it'll have to wait a day or two, unless someone else can test on their own machine. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: agtimer(4/armv7): switch to clockintr
On Wed, Dec 07 2022, Scott Cheloha wrote:
> ARMv7 has four interrupt clocks available. I think it'll be easier to
> review/test if we do the clockintr switch driver by driver instead of
> all at once in a massive single email. When all four driver patches
> are confirmed to work, I'll commit them.
>
> Here's a patch to switch agtimer(4/armv7) to clockintr.
>
> - Remove agtimer-specific clock interrupt scheduling bits
> and randomized statclock bits.
>
> - Wire up agtimer_intrclock.
>
> I am looking for a tester to help me get it compiling,
Fails to build because of a signature mismatch for agtimer_trigger(),
updated diff below.
> and then run it
> through a kernel-release-upgrade cycle.
That's not what you're asking for, but no regression spotted on a cubox
machine - which seems to use amptimer(4) according to dmesg.
Index: sys/arch/arm/include/cpu.h
===
RCS file: /home/cvs/src/sys/arch/arm/include/cpu.h,v
retrieving revision 1.61
diff -u -p -r1.61 cpu.h
--- sys/arch/arm/include/cpu.h 6 Jul 2021 09:34:06 - 1.61
+++ sys/arch/arm/include/cpu.h 7 Dec 2022 23:09:20 -
@@ -149,6 +149,7 @@ voidarm32_vector_init(vaddr_t, int);
* Per-CPU information. For now we assume one CPU.
*/
+#include
#include
#include
#include
@@ -198,7 +199,7 @@ struct cpu_info {
#ifdef GPROF
struct gmonparam *ci_gmon;
#endif
-
+ struct clockintr_queue ci_queue;
charci_panicbuf[512];
};
Index: sys/arch/arm/include/_types.h
===
RCS file: /home/cvs/src/sys/arch/arm/include/_types.h,v
retrieving revision 1.19
diff -u -p -r1.19 _types.h
--- sys/arch/arm/include/_types.h 5 Mar 2018 01:15:25 - 1.19
+++ sys/arch/arm/include/_types.h 7 Dec 2022 23:09:20 -
@@ -35,6 +35,8 @@
#ifndef _ARM__TYPES_H_
#define _ARM__TYPES_H_
+#define__HAVE_CLOCKINTR
+
#if defined(_KERNEL)
typedef struct label_t {
long val[11];
Index: sys/arch/arm/cortex/agtimer.c
===
RCS file: /home/cvs/src/sys/arch/arm/cortex/agtimer.c,v
retrieving revision 1.15
diff -u -p -r1.15 agtimer.c
--- sys/arch/arm/cortex/agtimer.c 12 Mar 2022 14:40:41 - 1.15
+++ sys/arch/arm/cortex/agtimer.c 7 Dec 2022 23:37:14 -
@@ -17,9 +17,11 @@
*/
#include
+#include
#include
#include
#include
+#include
#include
#include
@@ -51,28 +53,12 @@ static struct timecounter agtimer_timeco
.tc_priv = NULL,
};
-struct agtimer_pcpu_softc {
- uint64_tpc_nexttickevent;
- uint64_tpc_nextstatevent;
- u_int32_t pc_ticks_err_sum;
-};
-
struct agtimer_softc {
struct device sc_dev;
int sc_node;
-
- struct agtimer_pcpu_softc sc_pstat[MAXCPUS];
-
- u_int32_t sc_ticks_err_cnt;
u_int32_t sc_ticks_per_second;
- u_int32_t sc_ticks_per_intr;
- u_int32_t sc_statvar;
- u_int32_t sc_statmin;
-
-#ifdef AMPTIMER_DEBUG
- struct evcount sc_clk_count;
- struct evcount sc_stat_count;
-#endif
+ uint64_tsc_nsec_cycle_ratio;
+ uint64_tsc_nsec_max;
};
intagtimer_match(struct device *, void *, void *);
@@ -81,9 +67,11 @@ uint64_t agtimer_readcnt64(void);
intagtimer_intr(void *);
void agtimer_cpu_initclocks(void);
void agtimer_delay(u_int);
+void agtimer_rearm(void *, uint64_t);
void agtimer_setstatclockrate(int stathz);
void agtimer_set_clockrate(int32_t new_frequency);
void agtimer_startclock(void);
+void agtimer_trigger(void *);
const struct cfattach agtimer_ca = {
sizeof (struct agtimer_softc), agtimer_match, agtimer_attach
@@ -93,6 +81,11 @@ struct cfdriver agtimer_cd = {
NULL, "agtimer", DV_DULL
};
+struct intrclock agtimer_intrclock = {
+ .ic_rearm = agtimer_rearm,
+ .ic_trigger = agtimer_trigger
+};
+
uint64_t
agtimer_readcnt64(void)
{
@@ -155,16 +148,13 @@ agtimer_attach(struct device *parent, st
agtimer_frequency =
OF_getpropint(sc->sc_node, "clock-frequency", agtimer_frequency);
sc->sc_ticks_per_second = agtimer_frequency;
-
+ sc->sc_nsec_cycle_ratio =
+ sc->sc_ticks_per_second * (1ULL << 32) / 10;
+ sc->sc_nsec_max = UINT64_MAX / sc->sc_nsec_cycle_ratio;
printf(": %d kHz\n", sc->sc_ticks_per_second / 1000);
/* XXX: disable user access */
-#ifdef AMPTIMER_DEBUG
- evcount_attach(&sc->sc_clk_count, "clock", NULL);
- evcount_attach(&sc->sc_stat_count, "stat", NULL);
-#endif
-
/*
* private timer and interrupts not enabled u
Re: Fix evcount_percpu() after evcount_init_percpu() (plus bits for mips64)
On Sun, Dec 04 2022, Visa Hankala wrote:
> Do not re-insert the event counter to evcount_list in evcount_percpu().
> Otherwise the list becomes corrupt when evcount_percpu() is called
> after evcount_init_percpu().
>
> OK?
Woops. ok jca@
> As an extra, use percpu counters with mips64 clock and ipi interrupts.
Looks good but I have no way to test it, ok jca@ if it works for you.
> Index: kern/subr_evcount.c
> ===
> RCS file: src/sys/kern/subr_evcount.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 subr_evcount.c
> --- kern/subr_evcount.c 10 Nov 2022 07:05:41 - 1.14
> +++ kern/subr_evcount.c 4 Dec 2022 14:17:59 -
> @@ -56,7 +56,6 @@ evcount_percpu(struct evcount *ec)
> TAILQ_INSERT_TAIL(&evcount_percpu_init_list, ec, next);
> } else {
> ec->ec_percpu = counters_alloc(1);
> - TAILQ_INSERT_TAIL(&evcount_list, ec, next);
> }
> }
>
> Index: arch/mips64/mips64/clock.c
> ===
> RCS file: src/sys/arch/mips64/mips64/clock.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 clock.c
> --- arch/mips64/mips64/clock.c19 Nov 2022 16:23:48 - 1.48
> +++ arch/mips64/mips64/clock.c4 Dec 2022 14:17:58 -
> @@ -37,7 +37,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -100,6 +99,7 @@ clockattach(struct device *parent, struc
>*/
> set_intr(INTPRI_CLOCK, CR_INT_5, cp0_int5);
> evcount_attach(&cp0_clock_count, "clock", &cp0_clock_irq);
> + evcount_percpu(&cp0_clock_count);
>
> /* try to avoid getting clock interrupts early */
> cp0_set_compare(cp0_get_count() - 1);
> @@ -121,7 +121,7 @@ cp0_int5(uint32_t mask, struct trapframe
> struct cpu_info *ci = curcpu();
> int s;
>
> - atomic_inc_long((unsigned long *)&cp0_clock_count.ec_count);
> + evcount_inc(&cp0_clock_count);
>
> cp0_set_compare(cp0_get_count() - 1); /* clear INT5 */
>
> Index: arch/mips64/mips64/ipifuncs.c
> ===
> RCS file: src/sys/arch/mips64/mips64/ipifuncs.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 ipifuncs.c
> --- arch/mips64/mips64/ipifuncs.c 10 Apr 2022 13:23:14 - 1.25
> +++ arch/mips64/mips64/ipifuncs.c 4 Dec 2022 14:17:58 -
> @@ -84,6 +84,7 @@ mips64_ipi_init(void)
> if (!cpuid) {
> mtx_init(&smp_rv_mtx, IPL_HIGH);
> evcount_attach(&ipi_count, "ipi", &ipi_irq);
> + evcount_percpu(&ipi_count);
> }
>
> hw_ipi_intr_clear(cpuid);
> @@ -113,8 +114,7 @@ mips64_ipi_intr(void *arg)
> for (bit = 0; bit < MIPS64_NIPIS; bit++) {
> if (pending_ipis & (1UL << bit)) {
> (*ipifuncs[bit])();
> - atomic_inc_long(
> - (unsigned long *)&ipi_count.ec_count);
> + evcount_inc(&ipi_count);
> }
> }
> }
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
riscv64: print SBI vendor/version
With the diff below we get more details about the SBI version running on
the machine. My Unmatched machine has OpenBSD version 0.9 but upstream
has released 1.1 since, it implements v0.2 of the SBI spec but 0.3 and
1.0 have been released since. I suspect this information could be
useful for new boards.
OpenBSD 7.2-current (GENERIC.MP) #11: Thu Dec 1 23:39:45 CET 2022
j...@hifive.wxcvbn.org:/usr/src/sys/arch/riscv64/compile/GENERIC.MP
real mem = 17179869184 (16384MB)
avail mem = 16416411648 (15655MB)
SBI: OpenSBI v0.9, SBI Specification Version 0.2
random: good seed from bootblocks
mainbus0 at root: SiFive HiFive Unmatched A00
cpu0 at mainbus0: SiFive U7 imp 20181004 rv64imafdc
intc0 at cpu0
[...]
I tweaked the code in sbi.c to put all the information on a single line.
Printing the information in cpu_startup() seemed the most appropriate
since this code isn't hooked up in a driver.
ok?
Index: machdep.c
===
RCS file: /cvs/src/sys/arch/riscv64/riscv64/machdep.c,v
retrieving revision 1.29
diff -u -p -r1.29 machdep.c
--- machdep.c 30 Oct 2022 17:43:40 - 1.29
+++ machdep.c 3 Dec 2022 21:41:55 -
@@ -273,6 +273,8 @@ cpu_startup(void)
printf("avail mem = %lu (%luMB)\n", ptoa(uvmexp.free),
ptoa(uvmexp.free) / 1024 / 1024);
+ sbi_print_version();
+
curpcb = &proc0.p_addr->u_pcb;
curpcb->pcb_flags = 0;
curpcb->pcb_tf = &proc0tf;
Index: sbi.c
===
RCS file: /cvs/src/sys/arch/riscv64/riscv64/sbi.c,v
retrieving revision 1.6
diff -u -p -r1.6 sbi.c
--- sbi.c 2 Jul 2021 08:44:37 - 1.6
+++ sbi.c 4 Nov 2022 21:38:54 -
@@ -76,22 +76,22 @@ sbi_print_version(void)
switch (sbi_impl_id) {
case (SBI_IMPL_ID_BBL):
- printf("SBI: Berkely Boot Loader %lu\n", sbi_impl_version);
+ printf("SBI: Berkely Boot Loader %lu", sbi_impl_version);
break;
case (SBI_IMPL_ID_OPENSBI):
major = sbi_impl_version >> OPENSBI_VERSION_MAJOR_OFFSET;
minor = sbi_impl_version & OPENSBI_VERSION_MINOR_MASK;
- printf("SBI: OpenSBI v%u.%u\n", major, minor);
+ printf("SBI: OpenSBI v%u.%u", major, minor);
break;
default:
- printf("SBI: Unrecognized Implementation: %lu\n", sbi_impl_id);
+ printf("SBI: Unrecognized Implementation: %lu", sbi_impl_id);
break;
}
major = (sbi_spec_version & SBI_SPEC_VERS_MAJOR_MASK) >>
SBI_SPEC_VERS_MAJOR_OFFSET;
minor = (sbi_spec_version & SBI_SPEC_VERS_MINOR_MASK);
- printf("SBI Specification Version: %u.%u\n", major, minor);
+ printf(", SBI Specification Version %u.%u\n", major, minor);
}
#ifdef MULTIPROCESSOR
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: lld patch needed to build ports/lang/gcc/{8,11} on riscv64
On Fri, Sep 09 2022, Jeremie Courreges-Anglas wrote:
> When building ports-gcc on riscv64 I get this kind of error:
>
> --8<--
> ld: error: relocation refers to a symbol in a discarded section: .LEHB30
>>>> defined in .libs/libcp1.o
>>>> referenced by connection.hh:72
>>>> (/usr/ports/pobj/gcc-8.4.0/gcc-8.4.0/libcc1/connection.hh:72)
>>>> .libs/libcp1.o:(.gcc_except_table+0x1A9)
>>>> referenced by connection.hh:72
>>>> (/usr/ports/pobj/gcc-8.4.0/gcc-8.4.0/libcc1/connection.hh:72)
>>>> .libs/libcp1.o:(.gcc_except_table+0x1AD)
> -->8--
>
> The checks in ld.lld try to implement the ELF specs in a strict way,
> but this causes problems on riscv[0] and other architectures (the
> comments in the diff with additional context below). The
> .gcc_except_table case has been fixed in newer clang releases[1] but
> I suspect gcc hasn't improved upstream - looking only at their bugzilla.
>
> One can work around this with -ffunction-sections so that
> .gcc_except_table is split and the parts pointing at discarded sections
> can be pruned. But this feels like a hack and would require egcc/eg++
> consumers to use it. AFAIK there is no documented knob to downgrade this
> kind of error to a warning.
>
> Hence the diff below, which happens to be the same proposal as in [0].
> I have split this case from the checks above, thinking that it would let
> me both document the workaround and lessen the risk of merge conflict.
> But I can merge the .gcc_except_table case with the checks above it if
> preferred.
>
> Thoughts? ok?
My follow-up lingered in the draft folder:
I have worked around this by using -ffunction-sections in the
lang/gcc/{8,11}. So far this issue hasn't popped up in
a lang/gcc/8,-c++ consumer port but since riscv64 uses clang as the
default compiler there aren't many of them. This diff can be ignored
for now.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Some ENTRY_NB for libc/arch/i386
On Sat, Dec 03 2022, Theo Buehler wrote: > A while ago I added ENTRY_NB to i386 to fix some warnings in libm. Turns > out that libc also has a few of them on i386, diff below fixes the > > warning: bcopy changed binding to STB_WEAK > > for bcmp, bzero, bcopy, brk, sbrk. > > http://bluhm.genua.de/regress/results/2022-12-02T15%3A17%3A04Z/setup-ot4.log > is a build log containing the warnings. [...] LGTM, ok jca@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
riscv64: use evcount_percpu(9) for clock interrupts
ok? Index: clock.c === RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v retrieving revision 1.6 diff -u -p -r1.6 clock.c --- clock.c 19 Nov 2022 16:02:37 - 1.6 +++ clock.c 30 Nov 2022 19:28:49 - @@ -100,6 +100,7 @@ cpu_initclocks(void) clock_intr, NULL, NULL); evcount_attach(&clock_count, "clock", NULL); + evcount_percpu(&clock_count); cpu_startclock(); } @@ -136,7 +137,7 @@ clock_intr(void *frame) intr_disable(); splx(s); - clock_count.ec_count++; + evcount_inc(&clock_count); return 0; } -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
riscv64: drop unused WEAK_REFERENCE macro
WEAK_REFERENCE seems to come from FreeBSD, it's not used in our tree. (WEAK_ALIAS is defined a few lines above). ok? Index: sys/arch/riscv64/include/asm.h === RCS file: /cvs/src/sys/arch/riscv64/include/asm.h,v retrieving revision 1.6 diff -u -p -r1.6 asm.h --- sys/arch/riscv64/include/asm.h 2 Dec 2022 12:27:08 - 1.6 +++ sys/arch/riscv64/include/asm.h 2 Dec 2022 23:13:38 - @@ -104,10 +104,6 @@ .weak alias;\ alias = sym -#defineWEAK_REFERENCE(sym, alias) \ - .weak alias;\ - .set alias,sym - #defineSWAP_FAULT_HANDLER(handler, tmp0, tmp1) \ ld tmp0, CI_CURPCB(tp);/* Load the pcb */ \ ld tmp1, PCB_ONFAULT(tmp0);/* Save old handler */ \ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
ENTRY_NB() on riscv64
clang emits this warning when compiling brk.S and sbrk.S on riscv64: /tmp/sbrk-06c40b.s:46:2: warning: sbrk changed binding to STB_WEAK .weak sbrk Let's avoid this using the "ENTRY_NB" approach implemented by guenther@ on other archs. Diff tailored to reduce the differences with arm64/include/asm.h (that's why I swapped .p2align and .type). ok? Index: sys/arch/riscv64/include/asm.h === RCS file: /cvs/src/sys/arch/riscv64/include/asm.h,v retrieving revision 1.6 diff -u -p -r1.6 asm.h --- sys/arch/riscv64/include/asm.h 2 Dec 2022 12:27:08 - 1.6 +++ sys/arch/riscv64/include/asm.h 2 Dec 2022 20:00:49 - @@ -53,6 +53,11 @@ # define _ALIGN_TEXT .align 0 #endif +/* NB == No Binding: use .globl or .weak as necessary */ +#define _ENTRY_NB(x) \ + .text; .p2align 1; .type x,@function; x: +#define _ENTRY(x) .globl x; _ENTRY_NB(x) + #if defined(PROF) || defined(GPROF) // XXX Profiler Support #define _PROF_PROLOGUE \ @@ -81,10 +86,9 @@ #define RETGUARD_SYMBOL(x) #endif -#define_ENTRY(x) \ - .text; .globl x; .type x,@function; .p2align 1; x: #defineENTRY(y)_ENTRY(y); _PROF_PROLOGUE #defineENTRY_NP(y) _ENTRY(y) +#defineENTRY_NB(y) _ENTRY_NB(y); _PROF_PROLOGUE #defineASENTRY(y) _ENTRY(y); _PROF_PROLOGUE #defineASENTRY_NP(y) _ENTRY(y) #defineEND(y) .size y, . - y Index: lib/libc/arch/riscv64/sys/brk.S === RCS file: /cvs/src/lib/libc/arch/riscv64/sys/brk.S,v retrieving revision 1.3 diff -u -p -r1.3 brk.S --- lib/libc/arch/riscv64/sys/brk.S 2 Dec 2022 12:27:08 - 1.3 +++ lib/libc/arch/riscv64/sys/brk.S 2 Dec 2022 16:25:32 - @@ -26,7 +26,7 @@ __minbrk: .dword _end -ENTRY(brk) +ENTRY_NB(brk) RETGUARD_SETUP(brk, t6) lla t1, __minbrk ld t5, 0(t1) Index: lib/libc/arch/riscv64/sys/sbrk.S === RCS file: /cvs/src/lib/libc/arch/riscv64/sys/sbrk.S,v retrieving revision 1.3 diff -u -p -r1.3 sbrk.S --- lib/libc/arch/riscv64/sys/sbrk.S2 Dec 2022 12:27:08 - 1.3 +++ lib/libc/arch/riscv64/sys/sbrk.S2 Dec 2022 16:25:50 - @@ -28,7 +28,7 @@ __curbrk: .dword _end END(__curbrk) -ENTRY(sbrk) +ENTRY_NB(sbrk) RETGUARD_SETUP(sbrk, t6) lla t1, __curbrk -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Get rid of UVM_VNODE_CANPERSIST
On Tue, Nov 15 2022, Martin Pieuchot wrote:
> UVM vnode objects include a reference count to keep track of the number
> of processes that have the corresponding pages mapped in their VM space.
>
> When the last process referencing a given library or executable dies,
> the reaper will munmap this object on its behalf. When this happens it
> doesn't free the associated pages to speed-up possible re-use of the
> file. Instead the pages are placed on the inactive list but stay ready
> to be pmap_enter()'d without requiring I/O as soon as a newly process
> needs to access them.
>
> The mechanism to keep pages populated, known as UVM_VNODE_CANPERSIST,
> doesn't work well with swapping [0]. For some reason when the page daemon
> wants to free pages on the inactive list it tries to flush the pages to
> disk and panic(9) because it needs a valid reference to the vnode to do
> so.
>
> This indicates that the mechanism described above, which seems to work
> fine for RO mappings, is currently buggy in more complex situations.
> Flushing the pages when the last reference of the UVM object is dropped
> also doesn't seem to be enough as bluhm@ reported [1].
>
> The diff below, which has already be committed and reverted, gets rid of
> the UVM_VNODE_CANPERSIST logic. I'd like to commit it again now that
> the arm64 caching bug has been found and fixed.
>
> Getting rid of this logic means more I/O will be generated and pages
> might have a faster reuse cycle. I'm aware this might introduce a small
> slowdown,
Numbers for my usual make -j4 in libc,
on an Unmatched riscv64 box, now:
16m32.65s real21m36.79s user30m53.45s system
16m32.37s real21m33.40s user31m17.98s system
16m32.63s real21m35.74s user31m12.01s system
16m32.13s real21m36.12s user31m06.92s system
After:
19m14.15s real21m09.39s user36m51.33s system
19m19.11s real21m02.61s user36m58.46s system
19m21.77s real21m09.23s user37m03.85s system
19m09.39s real21m08.96s user36m36.00s system
4 cores amd64 VM, before (-current plus an other diff):
1m54.31s real 2m47.36s user 4m24.70s system
1m52.64s real 2m45.68s user 4m23.46s system
1m53.47s real 2m43.59s user 4m27.60s system
After:
2m34.12s real 2m51.15s user 6m20.91s system
2m34.30s real 2m48.48s user 6m23.34s system
2m37.07s real 2m49.60s user 6m31.53s system
> however I believe we should work towards loading files from the
> buffer cache to save I/O cycles instead of having another layer of cache.
> Such work isn't trivial and making sure the vnode <-> UVM relation is
> simple and well understood is the first step in this direction.
>
> I'd appreciate if the diff below could be tested on many architectures,
> include the offending rpi4.
Mike has already tested a make build on a riscv64 Unmatched. I have
also run regress in sys, lib/libc and lib/libpthread on that arch. As
far as I can see this looks stable on my machine, but what I really care
about is the riscv64 bulk build cluster (I'm going to start another
bulk build soon).
> Comments? Oks?
The performance drop in my microbenchmark kinda worries me but it's only
a microbenchmark...
> [0] https://marc.info/?l=openbsd-bugs&m=164846737707559&w=2
> [1] https://marc.info/?l=openbsd-bugs&m=166843373415030&w=2
>
> Index: uvm/uvm_vnode.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
> retrieving revision 1.130
> diff -u -p -r1.130 uvm_vnode.c
> --- uvm/uvm_vnode.c 20 Oct 2022 13:31:52 - 1.130
> +++ uvm/uvm_vnode.c 15 Nov 2022 13:28:28 -
> @@ -161,11 +161,8 @@ uvn_attach(struct vnode *vp, vm_prot_t a
>* add it to the writeable list, and then return.
>*/
> if (uvn->u_flags & UVM_VNODE_VALID) { /* already active? */
> + KASSERT(uvn->u_obj.uo_refs > 0);
>
> - /* regain vref if we were persisting */
> - if (uvn->u_obj.uo_refs == 0) {
> - vref(vp);
> - }
> uvn->u_obj.uo_refs++; /* bump uvn ref! */
>
> /* check for new writeable uvn */
> @@ -235,14 +232,14 @@ uvn_attach(struct vnode *vp, vm_prot_t a
> KASSERT(uvn->u_obj.uo_refs == 0);
> uvn->u_obj.uo_refs++;
> oldflags = uvn->u_flags;
> - uvn->u_flags = UVM_VNODE_VALID|UVM_VNODE_CANPERSIST;
> + uvn->u_flags = UVM_VNODE_VALID;
> uvn->u_nio = 0;
> uvn->u_size = used_vnode_size;
>
> /*
>* add a reference to the vnode. this reference will stay as long
>* as there is a valid mapping of the vnode. dropped when the
> - * reference count goes to zero [and we either free or persist].
> + * reference count goes to zero.
>*/
> vref(vp);
>
> @@ -323,16 +320,6 @@ uvn_detach(struct uvm_object *uobj)
>*/
> vp->v_flag &= ~VTEXT;
>
> - /*
> - * we jus
Re: Patch for getopt_long.c
On Tue, Nov 15 2022, Mathias Bavay wrote: > Hi, > > Please find here attached a patch with minor touch up on > src/lib/libc/stdlib/getopt_long.c : > > Â Â * Added an SPDX license identifier; As Theo answered, we don't use those identifiers (except sometimes in external source code added to the base system). > Â * moved the declarations of two variables in order to reduce their scope; We sometimes do this but 1. some people don't like much the idea 2. this change isn't worth a change/commit on its own (IMHO). Also, only unified diffs please (-u). See https://www.openbsd.org/faq/faq5.html#Diff > All the best, > > Mathias Bavay > > 1d0 > < // SPDX-License-Identifier: BSD-2-Clause > 133c132 > < int cyclelen, i, j, ncycle, nnonopts, nopts; > --- >> int cstart, cyclelen, i, j, ncycle, nnonopts, nopts, pos; > 145,146c144,145 > < const int cstart = panonopt_end+i; > < int pos = cstart; > --- >> cstart = panonopt_end+i; >> pos = cstart; > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: non regression tests for some packaging scripts
On Mon, Nov 14 2022, Marc Espie wrote:
> I'd like to add some non-regression tests for some scripts, most notably
> register-plist, which is... let's say not perfect at the moment.
>
> I believe
> ${PORTSDIR}/regress is probably the simplest least disturbing way to do that.
We have /usr/ports/tests.
> Since the manpages already exist under src, I could also (somehow) put them
> in /usr/src/regress.
>
> Of course, this means requiring a checkout of the ports tree to check things.
>
> What do you people think ?
I think those belong to the ports tree.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: riscv64: switch to clockintr(9)
On Tue, Nov 08 2022, Mark Kettenis wrote: >> Date: Tue, 8 Nov 2022 13:06:51 + >> From: Scott Cheloha >> >> On Mon, Nov 07, 2022 at 09:23:26PM +0100, Jeremie Courreges-Anglas wrote: >> > On Sun, Nov 06 2022, Scott Cheloha wrote: >> > > This patch switches riscv64 to clockintr(9). >> > > >> > > jca@ has been testing it (on a SiFive board?). It has survived two >> > > parallel release builds and upgrades from the resulting bsd.rd. >> > >> > I still get the same results on my HiFive Unmatched (produced by >> > SiFive indeed). The diff LGTM. >> > >> > Looks like clock and stat evcounts get merged with this diff. Not a big >> > problem but probably not intended? >> >> Whoops, good eye, forgot to mention that. >> >> This change is intended. With this patch, we can't keep a distinct >> count of hardclock() and statclock() calls anymore because >> clockintr_dispatch(9) handles the dispatch on behalf of the riscv64 >> code. So the "stat" evcount goes away. >> >> The "clock" evcount now represents a count of clock interrupts. >> That's it. One per riscv64 clock_intr() call. ack. Still ok jca@ > And I think that is fine. Those are the actual interrupts. Fine with me too! -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: riscv64: switch to clockintr(9)
On Sun, Nov 06 2022, Scott Cheloha wrote: > This patch switches riscv64 to clockintr(9). > > jca@ has been testing it (on a SiFive board?). It has survived two > parallel release builds and upgrades from the resulting bsd.rd. I still get the same results on my HiFive Unmatched (produced by SiFive indeed). The diff LGTM. Looks like clock and stat evcounts get merged with this diff. Not a big problem but probably not intended? > It could use testing on another machine. Are there other practical > machines aside from SiFive? There aren't that many vendors/machines that we do support: https://www.openbsd.org/riscv64.html -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
riscv64 pmap: sync memory writes before remote sfence.vma
Here's the diff I've been using for the last bulk build on riscv64. It
resulted in a single kernel crash:
cpu1: pool_do_get: pted free list modified: page 0xffc22cc0a000; item addr
0xffc22cc0a6d0; offset 0x0=0xa06137b98e6f6767 != 0xa06137b98ed06767
and 5 userland clang crashes. Yes, that is an improvement over the
18, 27 and 22 clang crashes in the three previous bulk builds.
But it doesn't fix the problem(s) we're still chasing.
The rationale is basically the text from the spec, I added rather
verbose comments quoting said spec, much like what exists in
icache_flush(). But we can make it shorter if wanted. Maybe just
"SFENCE.VMA orders only the local hart's implicit references to the
memory-management data structures."?
ok?
Bonus: sifive_cip_1200_errata is a remnant, cpu_errata_sifive_cip_1200
is the actual variable used on affected CPUs. To be committed
separately.
Index: pmap.c
===
RCS file: /cvs/src/sys/arch/riscv64/riscv64/pmap.c,v
retrieving revision 1.24
diff -u -p -r1.24 pmap.c
--- pmap.c 17 Oct 2022 19:51:54 - 1.24
+++ pmap.c 31 Oct 2022 21:15:07 -
@@ -42,8 +42,6 @@ pmap_is_active(struct pmap *pm, struct c
#endif
-int sifive_cip_1200_errata;
-
void
do_tlb_flush_page(pmap_t pm, vaddr_t va)
{
@@ -59,8 +57,23 @@ do_tlb_flush_page(pmap_t pm, vaddr_t va)
hart_mask |= (1UL << ci->ci_hartid);
}
- if (hart_mask != 0)
+ /*
+* From the RISC-V privileged spec:
+*
+* SFENCE.VMA orders only the local hart's implicit references
+* to the memory-management data structures. Consequently, other
+* harts must be notified separately when the memory-management
+* data structures have been modified. One approach is to use 1)
+* a local data fence to ensure local writes are visible
+* globally, then 2) an interprocessor interrupt to the other
+* thread, then 3) a local SFENCE.VMA in the interrupt handler
+* of the remote thread, and finally 4) signal back to
+* originating thread that operation is complete.
+*/
+ if (hart_mask != 0) {
+ membar_sync();
sbi_remote_sfence_vma(&hart_mask, va, PAGE_SIZE);
+ }
#endif
sfence_vma_page(va);
@@ -81,8 +94,23 @@ do_tlb_flush(pmap_t pm)
hart_mask |= (1UL << ci->ci_hartid);
}
- if (hart_mask != 0)
+ /*
+* From the RISC-V privileged spec:
+*
+* SFENCE.VMA orders only the local hart's implicit references
+* to the memory-management data structures. Consequently, other
+* harts must be notified separately when the memory-management
+* data structures have been modified. One approach is to use 1)
+* a local data fence to ensure local writes are visible
+* globally, then 2) an interprocessor interrupt to the other
+* thread, then 3) a local SFENCE.VMA in the interrupt handler
+* of the remote thread, and finally 4) signal back to
+* originating thread that operation is complete.
+*/
+ if (hart_mask != 0) {
+ membar_sync();
sbi_remote_sfence_vma(&hart_mask, 0, -1);
+ }
#endif
sfence_vma();
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: [patch] Delete outdated warning about HiFive Unmatched onboard ethernet device from riscv64 installation notes
On Mon, Oct 31 2022, Miguel Landaeta wrote:
> Hi,
>
> I just installed 7.2 successfully on a HiFive Unmatched box and the
> onboard cad(4) device is working as expected. I think installation
> notes should be updated.
Correct, I have only ever used cad(4) on my Unmatched, with no problem
seen since Aug 2021.
Committed, thanks.
> Cheers,
>
> diff --git a/distrib/notes/riscv64/prep b/distrib/notes/riscv64/prep
> index c5443f8587f..ae63bc6cab7 100644
> --- a/distrib/notes/riscv64/prep
> +++ b/distrib/notes/riscv64/prep
> @@ -7,8 +7,7 @@ HiFive Unmatched
> Copy install{:--:}OSrev.img to a USB stick, and boot with both it and the
> vendor provided uSD card (unmodified) inserted. This should enable firmware
> and U-Boot to be loaded from uSD and OpenBSD bootloader and kernel to be
> -loaded from USB stick. At this time onboard Ethernet is not well supported,
> -so PCIe or USB Ethernet device will be required.
> +loaded from USB stick.
>
> QEMU with OpenSBI and U-Boot
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: HiFive Unmatched clean poweroff using the power button
On Thu, Aug 18 2022, Jeremie Courreges-Anglas wrote:
> Some time ago I wanted to get a clean poweroff from the power button on
> my Unmatched, so that I don't get fsck at reboot the morning after
> someone sleeps in the room where the machine lives. kettenis kindly
> provided sfgpio(4) to get interrupts working on dapmic(4) instead of my
> initial hack that used polling.
>
> One issue I struggled with for a bit is that masking irqs also masks the
> wake-up events, particularly the events we use for dapmic_reset().
> With the diff below most interrupt events are masked off during runtime,
> until we're shutting down/rebooting. Maybe this is too paranoid and
> I should let more events go through the intr handler, eg for wake-up
> events that I don't envision yet? (I would love to get wake on lan
> support in cad(4) but my attempts led nowhere so far.)
>
> Also interesting, the fault register needs to be cleared at boot, else
> the interrupt will keep triggering after eg a hard button-driven poweroff.
> We could log the faults found in FAULT_LOG at boot time to know why the
> machine has stopped. But I'm more concerned about what to do if we get
> a fault at runtime (see the XXX). When I tried to disestablish the
> interrupt handler with fdt_intr_disestablish(sc->sc_ih) from
> dapmic_reset(), I got a fault. Maybe something worth investigating.
The FAULT_LOG register handling seems appropriate after reading entry
6.2 in:
https://www.renesas.com/eu/en/document/apn/shared-irq-line-considerations-pm-059
I wish I had read this document earlier. :)
> The code below is based off da9063_datasheet_2v2.pdf. I don't know of
> other machines we run on that use this controller, the only entry in
> dmesglog is matthieu's Unmatched machine.
>
> Tests, input and oks welcome.
In the case where my hifive unmatched lives, I have wired both the
shutdown and the reset buttons. Yesterday I tried to see whether the
reset button could be handled as a clean reboot, but looking at the
hifive unmatched board schematics it seems that the PMIC has no control
over it (even if tweaking nRES_MODE). So the diff stands for scrutiny.
I would like input and maybe oks to get this in. ;)
Index: dev/fdt/dapmic.c
===
RCS file: /cvs/src/sys/dev/fdt/dapmic.c,v
retrieving revision 1.2
diff -u -p -r1.2 dapmic.c
--- dev/fdt/dapmic.c6 Apr 2022 18:59:28 - 1.2
+++ dev/fdt/dapmic.c17 Aug 2022 21:59:57 -
@@ -19,6 +19,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -28,11 +31,31 @@
#include
+#include
+
extern void (*cpuresetfn)(void);
extern void (*powerdownfn)(void);
/* Registers */
+#define FAULT_LOG 0x05
#define EVENT_A0x06
+#define EVENT_A_EVENTS_D (1 << 7)
+#define EVENT_A_EVENTS_C (1 << 6)
+#define EVENT_A_EVENTS_B (1 << 5)
+#define EVENT_A_E_nONKEY (1 << 0)
+#define EVENT_B0x07
+#define EVENT_C0x08
+#define EVENT_D0x09
+#define IRQ_MASK_A 0x0a
+#define IRQ_MASK_A_M_RESERVED ((1 << 7) | (1 << 6) | (1 << 5))
+#define IRQ_MASK_A_M_SEQ_RDY (1 << 4)
+#define IRQ_MASK_A_M_ADC_RDY (1 << 3)
+#define IRQ_MASK_A_M_TICK (1 << 2)
+#define IRQ_MASK_A_M_ALARM(1 << 1)
+#define IRQ_MASK_A_M_nONKEY (1 << 0)
+#define IRQ_MASK_B 0x0b
+#define IRQ_MASK_C 0x0c
+#define IRQ_MASK_D 0x0d
#define CONTROL_F 0x13
#define CONTROL_F_WAKE_UP (1 << 2)
#define CONTROL_F_SHUTDOWN(1 << 1)
@@ -55,11 +78,20 @@ extern void (*powerdownfn)(void);
#define ALARM_Y0x4b
#define ALARM_Y_TICK_ON (1 << 7)
+#ifdef DAPMIC_DEBUG
+# define DPRINTF(args) do { printf args; } while (0)
+#else
+# define DPRINTF(args) do {} while (0)
+#endif
+
struct dapmic_softc {
struct device sc_dev;
i2c_tag_t sc_tag;
i2c_addr_t sc_addr;
+ int (*sc_ih)(void *);
+ struct task sc_task;
+
struct todr_chip_handle sc_todr;
};
@@ -80,8 +112,11 @@ int dapmic_clock_read(struct dapmic_soft
intdapmic_clock_write(struct dapmic_softc *, struct clock_ymdhms *);
intdapmic_gettime(struct todr_chip_handle *, struct timeval *);
intdapmic_settime(struct todr_chip_handle *, struct timeval *);
+void dapmic_reset_irq_mask(struct dapmic_softc *);
void dapmic_reset(void);
void dapmic_powerdown(void);
+intdapmic_intr(void *);
+void dapmic_shutdown_task(void *);
int
dapmic_match(struct device *parent, void *match, void *aux)
@@ -96,
lld patch needed to build ports/lang/gcc/{8,11} on riscv64
When building ports-gcc on riscv64 I get this kind of error:
--8<--
ld: error: relocation refers to a symbol in a discarded section: .LEHB30
>>> defined in .libs/libcp1.o
>>> referenced by connection.hh:72
>>> (/usr/ports/pobj/gcc-8.4.0/gcc-8.4.0/libcc1/connection.hh:72)
>>> .libs/libcp1.o:(.gcc_except_table+0x1A9)
>>> referenced by connection.hh:72
>>> (/usr/ports/pobj/gcc-8.4.0/gcc-8.4.0/libcc1/connection.hh:72)
>>> .libs/libcp1.o:(.gcc_except_table+0x1AD)
-->8--
The checks in ld.lld try to implement the ELF specs in a strict way,
but this causes problems on riscv[0] and other architectures (the
comments in the diff with additional context below). The
.gcc_except_table case has been fixed in newer clang releases[1] but
I suspect gcc hasn't improved upstream - looking only at their bugzilla.
One can work around this with -ffunction-sections so that
.gcc_except_table is split and the parts pointing at discarded sections
can be pruned. But this feels like a hack and would require egcc/eg++
consumers to use it. AFAIK there is no documented knob to downgrade this
kind of error to a warning.
Hence the diff below, which happens to be the same proposal as in [0].
I have split this case from the checks above, thinking that it would let
me both document the workaround and lessen the risk of merge conflict.
But I can merge the .gcc_except_table case with the checks above it if
preferred.
Thoughts? ok?
[0] https://reviews.llvm.org/D83244
[1] https://reviews.llvm.org/D83655
Index: ELF/Relocations.cpp
===
RCS file: /cvs/src/gnu/llvm/lld/ELF/Relocations.cpp,v
retrieving revision 1.4
diff -u -p -U15 -r1.4 Relocations.cpp
--- ELF/Relocations.cpp 17 Dec 2021 14:46:47 - 1.4
+++ ELF/Relocations.cpp 6 Sep 2022 01:08:23 -
@@ -974,30 +974,36 @@ static bool maybeReportUndefined(Symbol
return false;
// clang (as of 2019-06-12) / gcc (as of 8.2.1) PPC64 may emit a .rela.toc
// which references a switch table in a discarded .rodata/.text section. The
// .toc and the .rela.toc are incorrectly not placed in the comdat. The ELF
// spec says references from outside the group to a STB_LOCAL symbol are not
// allowed. Work around the bug.
//
// PPC32 .got2 is similar but cannot be fixed. Multiple .got2 is infeasible
// because .LC0-.LTOC is not representable if the two labels are in different
// .got2
if (cast(sym).discardedSecIdx != 0 &&
(sec.name == ".got2" || sec.name == ".toc"))
return false;
+ // GCC (at least 8 and 11) can produce a ".gcc_except_table" with relocations
+ // to discarded sections on riscv64
+ if (cast(sym).discardedSecIdx != 0 &&
+ sec.name == ".gcc_except_table")
+return false;
+
bool isWarning =
(config->unresolvedSymbols == UnresolvedPolicy::Warn && canBeExternal) ||
config->noinhibitExec;
undefs.push_back({&sym, {{&sec, offset}}, isWarning});
return !isWarning;
}
// MIPS N32 ABI treats series of successive relocations with the same offset
// as a single relocation. The similar approach used by N64 ABI, but this ABI
// packs all relocations into the single relocation record. Here we emulate
// this for the N32 ABI. Iterate over relocation with the same offset and put
// theirs types into the single bit-set.
template static RelType getMipsN32RelType(RelTy *&rel, RelTy
*end) {
RelType type = 0;
uint64_t offset = rel->r_offset;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: add sendmmsg and recvmmsg systemcalls
On Tue, Aug 30 2022, Moritz Buhl wrote:
> Hi tech@,
>
> the following diff only contains recvmmsg which should be the more useful
> syscall of the two.
>
> I implemented some minor feedback regarding the man page and attaching
> an error from recvit to the socket in case some messages were
> received before.
>
> I am also looking into passing the timeout through recvit and
> soreceive in order to not block indefinetly on a blocking socket
> as the other implementations do:
>
> BUGS
>
>The timeout argument does not work as intended. The timeout is
>checked only after the receipt of each datagram, so that if up to
>vlen-1 datagrams are received before the timeout expires, but
>then no further datagrams are received, the call will block
>forever.
> https://www.man7.org/linux/man-pages/man2/recvmmsg.2.html
>
> But I would prefer doing this in another change.
As discussed with Moritz, ports/net/knot (which I co-maintain) expects
to be able to use both recvmmsg and sendmmsg. I didn't check other
ports but from the code we read, using both recvmmsg and sendmmsg looks
legit (useful).
And since Moritz insists, please find my bikeshedding below. :)
tl;dr nothing looks wrong to me but I haven't tested it yet.
[...]
> Index: sys/kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.201
> diff -u -p -r1.201 uipc_syscalls.c
> --- sys/kern/uipc_syscalls.c 14 Aug 2022 01:58:28 - 1.201
> +++ sys/kern/uipc_syscalls.c 30 Aug 2022 17:03:09 -
> @@ -805,6 +805,135 @@ done:
> }
>
> int
> +sys_recvmmsg(struct proc *p, void *v, register_t *retval)
> +{
> + struct sys_recvmmsg_args /* {
> + syscallarg(int) s;
> + syscallarg(struct mmsghdr *)mmsg;
> + syscallarg(unsigned int)vlen;
> + syscallarg(unsigned int)flags;
> + syscallarg(struct timespec *) timeout;
> + } */ *uap = v;
> + struct mmsghdr mmsg;
> + struct timespec ts, now;
> + struct iovec aiov[UIO_SMALLIOV], *uiov, *iov = aiov;
> + struct file *fp;
> + struct socket *so;
> + struct timespec *timeout;
> + unsigned int vlen, dg;
"dg" is a weird name. At first it looks like an index, I would name it
idx but if you want to keep the original name maybe put it on its own
line and add a comment like /* number of datagrams received */?
> + int error = 0, flags, s;
> +
> + timeout = SCARG(uap, timeout);
> + if (timeout != NULL) {
> + error = copyin(SCARG(uap, timeout), &ts, sizeof(ts));
^^^ could reuse the
timeout local variable
> + if (error != 0)
The local idiom is
if (error)
> + return error;
There are other occurrences below which you may or may not address if
you prefer to limit the differences with NetBSD.
> + getnanotime(&now);
> + timespecadd(&now, &ts, &ts);
> + }
> +
> + s = SCARG(uap, s);
> + if ((error = getsock(p, s, &fp)) != 0)
> + return (error);
> + so = (struct socket *)fp->f_data;
> +
> + flags = SCARG(uap, flags);
> +
> + vlen = SCARG(uap, vlen);
Maybe add a comment?
/* Arbitrarily capped at 1024 items. */
> + if (vlen > 1024)
> + vlen = 1024;
> +
> + for (dg = 0; dg < vlen;) {
> + error = copyin(SCARG(uap, mmsg) + dg, &mmsg, sizeof(mmsg));
We could have a struct mmsghdr *mmsgp local pointer and use mmsgp[idx]
instead of a mix of a macro and pointer arithmetics.
> + if (error != 0)
> + break;
> +
> + if (mmsg.msg_hdr.msg_iovlen > IOV_MAX) {
> + error = EMSGSIZE;
> + break;
> + }
> +
> + if (mmsg.msg_hdr.msg_iovlen > UIO_SMALLIOV)
> + iov = mallocarray(mmsg.msg_hdr.msg_iovlen,
> + sizeof(struct iovec), M_IOV, M_WAITOK);
> + else
> + iov = aiov;
> +
> + if (mmsg.msg_hdr.msg_iovlen > 0) {
> + error = copyin(mmsg.msg_hdr.msg_iov, iov,
> + mmsg.msg_hdr.msg_iovlen * sizeof(struct iovec));
> + if (error)
> + break;
> + }
> +
> + uiov = mmsg.msg_hdr.msg_iov;
> + mmsg.msg_hdr.msg_iov = iov;
> + mmsg.msg_hdr.msg_flags = flags;
> +
> + error = recvit(p, s, &mmsg.msg_hdr, NULL, retval);
> + if (error != 0) {
> + if (error == EAGAIN && dg > 0)
> + error = 0;
> + break;
> + }
> +
> + if (dg == 0 && flags & MSG_WAITFORONE) {
> + flags &= ~MSG_WAITFORONE;
> +
HiFive Unmatched clean poweroff using the power button
Some time ago I wanted to get a clean poweroff from the power button on
my Unmatched, so that I don't get fsck at reboot the morning after
someone sleeps in the room where the machine lives. kettenis kindly
provided sfgpio(4) to get interrupts working on dapmic(4) instead of my
initial hack that used polling.
One issue I struggled with for a bit is that masking irqs also masks the
wake-up events, particularly the events we use for dapmic_reset().
With the diff below most interrupt events are masked off during runtime,
until we're shutting down/rebooting. Maybe this is too paranoid and
I should let more events go through the intr handler, eg for wake-up
events that I don't envision yet? (I would love to get wake on lan
support in cad(4) but my attempts led nowhere so far.)
Also interesting, the fault register needs to be cleared at boot, else
the interrupt will keep triggering after eg a hard button-driven poweroff.
We could log the faults found in FAULT_LOG at boot time to know why the
machine has stopped. But I'm more concerned about what to do if we get
a fault at runtime (see the XXX). When I tried to disestablish the
interrupt handler with fdt_intr_disestablish(sc->sc_ih) from
dapmic_reset(), I got a fault. Maybe something worth investigating.
The code below is based off da9063_datasheet_2v2.pdf. I don't know of
other machines we run on that use this controller, the only entry in
dmesglog is matthieu's Unmatched machine.
Tests, input and oks welcome.
Index: dev/fdt/dapmic.c
===
RCS file: /cvs/src/sys/dev/fdt/dapmic.c,v
retrieving revision 1.2
diff -u -p -r1.2 dapmic.c
--- dev/fdt/dapmic.c6 Apr 2022 18:59:28 - 1.2
+++ dev/fdt/dapmic.c17 Aug 2022 21:59:57 -
@@ -19,6 +19,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -28,11 +31,31 @@
#include
+#include
+
extern void (*cpuresetfn)(void);
extern void (*powerdownfn)(void);
/* Registers */
+#define FAULT_LOG 0x05
#define EVENT_A0x06
+#define EVENT_A_EVENTS_D (1 << 7)
+#define EVENT_A_EVENTS_C (1 << 6)
+#define EVENT_A_EVENTS_B (1 << 5)
+#define EVENT_A_E_nONKEY (1 << 0)
+#define EVENT_B0x07
+#define EVENT_C0x08
+#define EVENT_D0x09
+#define IRQ_MASK_A 0x0a
+#define IRQ_MASK_A_M_RESERVED ((1 << 7) | (1 << 6) | (1 << 5))
+#define IRQ_MASK_A_M_SEQ_RDY (1 << 4)
+#define IRQ_MASK_A_M_ADC_RDY (1 << 3)
+#define IRQ_MASK_A_M_TICK (1 << 2)
+#define IRQ_MASK_A_M_ALARM(1 << 1)
+#define IRQ_MASK_A_M_nONKEY (1 << 0)
+#define IRQ_MASK_B 0x0b
+#define IRQ_MASK_C 0x0c
+#define IRQ_MASK_D 0x0d
#define CONTROL_F 0x13
#define CONTROL_F_WAKE_UP (1 << 2)
#define CONTROL_F_SHUTDOWN(1 << 1)
@@ -55,11 +78,20 @@ extern void (*powerdownfn)(void);
#define ALARM_Y0x4b
#define ALARM_Y_TICK_ON (1 << 7)
+#ifdef DAPMIC_DEBUG
+# define DPRINTF(args) do { printf args; } while (0)
+#else
+# define DPRINTF(args) do {} while (0)
+#endif
+
struct dapmic_softc {
struct device sc_dev;
i2c_tag_t sc_tag;
i2c_addr_t sc_addr;
+ int (*sc_ih)(void *);
+ struct task sc_task;
+
struct todr_chip_handle sc_todr;
};
@@ -80,8 +112,11 @@ int dapmic_clock_read(struct dapmic_soft
intdapmic_clock_write(struct dapmic_softc *, struct clock_ymdhms *);
intdapmic_gettime(struct todr_chip_handle *, struct timeval *);
intdapmic_settime(struct todr_chip_handle *, struct timeval *);
+void dapmic_reset_irq_mask(struct dapmic_softc *);
void dapmic_reset(void);
void dapmic_powerdown(void);
+intdapmic_intr(void *);
+void dapmic_shutdown_task(void *);
int
dapmic_match(struct device *parent, void *match, void *aux)
@@ -96,6 +131,7 @@ dapmic_attach(struct device *parent, str
{
struct dapmic_softc *sc = (struct dapmic_softc *)self;
struct i2c_attach_args *ia = aux;
+ int node = *(int *)ia->ia_cookie;
sc->sc_tag = ia->ia_tag;
sc->sc_addr = ia->ia_addr;
@@ -105,12 +141,35 @@ dapmic_attach(struct device *parent, str
sc->sc_todr.todr_settime = dapmic_settime;
todr_attach(&sc->sc_todr);
- printf("\n");
-
if (cpuresetfn == NULL)
cpuresetfn = dapmic_reset;
if (powerdownfn == NULL)
powerdownfn = dapmic_powerdown;
+
+ task_set(&sc->sc_task, dapmic_shutdown_task, sc);
+
+ /* Mask away events we don't care about */
+ dapmic_reg_write(sc, IRQ_MASK_A,
+ 0xff & ~(IRQ_MASK_A_M_RESERVED | IRQ_MASK_A_M_nONKEY));
+ dapmic_reg_write(sc, IRQ_MASK_B, 0xff);
+ dapmic_reg_write(sc, IRQ_MASK_C, 0xff);
+
Re: libsoup2/3 conflicts and gstreamer1 [Re: audio/quodlibet & devel/libsoup]
On Thu, Aug 11 2022, Stuart Henderson wrote:
> Moving from ports@.
>
> Quick intro:
>
> A library (gstreamer1) uses functions from another library (libsoup)
> which exists in two incompatible versions (libsoup-2.4.so.XX and
> libsoup-3.0.so.XX).
>
> Other software calling gstreamer might use one or other of these two
> libsoups for its own purposes, so gstreamer has been written to work
> with either version of libsoup, but it needs to know which it should
> use.
>
> the text below from my ports@ mail follows on:
>
>> gstreamer is supposed to detect a libsoup which is already loaded into the
>> address space (by using dlopen() with RTLD_NOLOAD to test this) and adapts
>> itself to use the appropriate libsoup API+ABI. It does this specifically
>> to avoid this type of conflict. Only if it can't find a copy of libsoup2
>> or libsoup3 already loaded does it use its own preference order (libsoup3
>> by default) and dlopen()s it (via g_module_open) itself.
>>
>> (see gst_soup_load_library() in ext/soup/gstsouploader.c from
>> gstreamer1/plugins-good).
>>
>> But we don't have RTLD_NOLOAD so this trick doesn't work...
>
> ... gstreamer code for this is at
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/ext/soup/gstsouploader.c#L146
>
> Brad pointed me at a diff from guenther@ adding RTLD_NOLOAD (and
> RTLD_NODELETE, written before semarie's version of RTLD_NODELETE):
> https://marc.info/?l=openbsd-tech&m=162068525021512&w=2
>
> With this applied and gstreamer1-plugins-good rebuilt, the code in
> gst_soup_load_library works as expected.
>
> I've included the rebased diff below and added a simple regress test
> that I wrote, loosely based on the rtld_nodelete test plus what
> gstreamer1 is doing.
>
> Looking around some other ports (it's difficult to do a proper search
> because it shows up in rust's translated version of dlfcn.h for various
> OS which ends up vendored into various trees) I see some optional use
> in a few ports e.g. asterisk ("Check to see if the given resource is
> loaded") and vlc (I think this is trying to load a linux v4l2 library
> only if not already loaded but allow it to fail gracefully).
>
> Is this something we want?
I think so. The code looks good, would support a valid (IMO) use case
and would get us rid of a few patches and possibly blocking problems in
ports.
ok jca@
(I'm not sure it would help in the audio/quodlibet case, even after
patching the hardcoded sonames.)
One tweak below,
[...]
> Index: regress/libexec/ld.so/noload/test1/Makefile
> ===
> RCS file: regress/libexec/ld.so/noload/test1/Makefile
> diff -N regress/libexec/ld.so/noload/test1/Makefile
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ regress/libexec/ld.so/noload/test1/Makefile 11 Aug 2022 09:38:48
> -
> @@ -0,0 +1,32 @@
> +# $OpenBSD$
> +
> +.include
> +
> +PROG = test1
> +
> +LIBADIR != if test -d ${.CURDIR}/../liba/${__objdir}; then \
> + echo "${.CURDIR}/../liba/${__objdir}"; \
> + else\
> + echo "${.CURDIR}/../liba"; \
> + fi
> +
> +LIBBDIR != if test -d ${.CURDIR}/../libb/${__objdir}; then \
> + echo "${.CURDIR}/../libb/${__objdir}"; \
> + else\
> + echo "${.CURDIR}/../libb"; \
> + fi
> +
> +LIBANAME = ${LIBADIR}/liba.so.0.0
> +LIBBNAME = ${LIBBDIR}/libb.so.0.0
> +
> +CFLAGS +=-DLIBANAME=\"${LIBANAME}\" \
> + -DLIBBNAME=\"${LIBBNAME}\"
> +
> +REGRESS_TARGETS += run-regress
> +
> +run-regress: ${PROG}
> + if ${PROG} | grep found; then echo failed; exit 1; fi
> + LD_PRELOAD=${LIBANAME} ${PROG} | grep ${LIBANAME}.found
> + LD_PRELOAD=${LIBBNAME} ${PROG} | grep ${LIBBNAME}.found
> +
> +.include
This fails if . is not in PATH, could you please use ./${PROG}?
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Sun, Aug 14 2022, Vitaliy Makkoveev wrote:
> I propose to avoid netlock relocking for SIOCGIFSFFPAGE too.
sigh, that seems to be needed indeed.
> Also, since the relock should be avoided for many commands, I prefer
> to have something like below:
>
> switch(cmd){
> case SIOCGIFMEDIA:
> case ...:
> relock = 1;
> break;
> }
>
> if (relock)
> NET_UNLOCK();
Fine with me. "netlock_held" made more sense to me as the variable
name, I hope you won't mind. ;)
Works fine on GENERIC.MP.
Index: dev/fdt/if_cad.c
===
RCS file: /cvs/src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_cad.c
--- dev/fdt/if_cad.c8 Mar 2022 16:13:08 - 1.11
+++ dev/fdt/if_cad.c14 Aug 2022 20:20:00 -
@@ -550,12 +550,22 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
{
struct cad_softc *sc = ifp->if_softc;
struct ifreq *ifr = (struct ifreq *)data;
- int error = 0;
+ int error = 0, netlock_held = 1;
int s;
- NET_UNLOCK();
+ switch (cmd) {
+ case SIOCGIFMEDIA:
+ case SIOCSIFMEDIA:
+ case SIOCGIFSFFPAGE:
+ netlock_held = 0;
+ break;
+ }
+
+ if (netlock_held)
+ NET_UNLOCK();
rw_enter_write(&sc->sc_cfg_lock);
- NET_LOCK();
+ if (netlock_held)
+ NET_LOCK();
s = splnet();
switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Sun, Aug 14 2022, Jeremie Courreges-Anglas wrote:
> On Thu, Jul 28 2022, Alexander Bluhm wrote:
>> Hi,
>>
>> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
>> Legacy drivers run with kernel lock, interface media is MP safe or
>> has kernel lock.
>>
>> ixl(4) talks about net lock but in fact has kernel lock.
>>
>> Problem is that smtpd(8) periodically checks media status. This
>> stops network if netlock was taken.
>>
>> ok?
>
> I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
> (didn't try GENERIC). RAMDISK after
>
> OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
>
> hangs some time after mounting root. On GENERIC.MP I get a panic:
>
> --8<--
> Automatic boot in progress: starting file system checks.
> /dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
> /dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
> /dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
> /dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
> /dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
> /dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
> /dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
> /dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
> /dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
> /dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
> /dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
> /dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
> /dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
> pf enabled
> kern.bufcachepercent: 20 -> 80
> sysctl: kern.allowdt: value is not available
> ddb.console: 0 -> 1
> starting network
> panic: netlock: lock not held
> Stopped at panic+0x106:addia0,zero,256TIDPIDUID
> PR
> FLAGS PFLAGS CPU COMMAND
> *464996 1081 0 0x3 00 ifconfig
> 492269 29052 0 0x14000 0x2001 zerothread
> panic() at panic+0x106
> rw_exit_write() at rw_exit_write+0x94
> cad_ioctl() at cad_ioctl+0x34
> ifioctl() at ifioctl+0xaaa
> soo_ioctl() at soo_ioctl+0x152
> sys_ioctl() at sys_ioctl+0x230
> svc_handler() at svc_handler+0x284
> do_trap_user() at do_trap_user+0x15c
> cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
> end of kernel
> end trace frame: 0x5408b2ae8, count: 6
> https://www.openbsd.org/ddb.html describes the minimum info required in bug
> reports. Insufficient info makes it difficult to find and fix bugs.
>
> -->8--
>
> The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
Done, the diff fixes bsd.rd too.
> I'm not completely sure this is the best way to solve it but I can't
> spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
> ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
Thoughts? ok?
> Index: dev/fdt/if_cad.c
> ===
> RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_cad.c
> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 - 1.11
> +++ dev/fdt/if_cad.c 14 Aug 2022 13:04:41 -
> @@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
> int error = 0;
> int s;
>
> - NET_UNLOCK();
> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
> + NET_UNLOCK();
> rw_enter_write(&sc->sc_cfg_lock);
> - NET_LOCK();
> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
> + NET_LOCK();
> s = splnet();
>
> switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Thu, Jul 28 2022, Alexander Bluhm wrote:
> Hi,
>
> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
> Legacy drivers run with kernel lock, interface media is MP safe or
> has kernel lock.
>
> ixl(4) talks about net lock but in fact has kernel lock.
>
> Problem is that smtpd(8) periodically checks media status. This
> stops network if netlock was taken.
>
> ok?
I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
(didn't try GENERIC). RAMDISK after
OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
hangs some time after mounting root. On GENERIC.MP I get a panic:
--8<--
Automatic boot in progress: starting file system checks.
/dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
/dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
/dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
/dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
/dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
/dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
/dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
/dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
/dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
/dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
/dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
/dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
/dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
pf enabled
kern.bufcachepercent: 20 -> 80
sysctl: kern.allowdt: value is not available
ddb.console: 0 -> 1
starting network
panic: netlock: lock not held
Stopped at panic+0x106:addia0,zero,256TIDPIDUID PR
FLAGS PFLAGS CPU COMMAND
*464996 1081 0 0x3 00 ifconfig
492269 29052 0 0x14000 0x2001 zerothread
panic() at panic+0x106
rw_exit_write() at rw_exit_write+0x94
cad_ioctl() at cad_ioctl+0x34
ifioctl() at ifioctl+0xaaa
soo_ioctl() at soo_ioctl+0x152
sys_ioctl() at sys_ioctl+0x230
svc_handler() at svc_handler+0x284
do_trap_user() at do_trap_user+0x15c
cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
end of kernel
end trace frame: 0x5408b2ae8, count: 6
https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.
-->8--
The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
I'm not completely sure this is the best way to solve it but I can't
spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
Index: dev/fdt/if_cad.c
===
RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_cad.c
--- dev/fdt/if_cad.c8 Mar 2022 16:13:08 - 1.11
+++ dev/fdt/if_cad.c14 Aug 2022 13:04:41 -
@@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
int error = 0;
int s;
- NET_UNLOCK();
+ if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
+ NET_UNLOCK();
rw_enter_write(&sc->sc_cfg_lock);
- NET_LOCK();
+ if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
+ NET_LOCK();
s = splnet();
switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: echo(1): check for stdio errors
On Wed, Aug 10 2022, Scott Cheloha wrote: > On Wed, Aug 10, 2022 at 02:23:08PM -0600, Theo de Raadt wrote: >> Scott Cheloha wrote: >> >> > On Wed, Aug 10, 2022 at 12:26:17PM -0600, Theo de Raadt wrote: >> > > Scott Cheloha wrote: >> > > >> > > > We're sorta-kinda circling around adding the missing (?) stdio error >> > > > checking to other utilities in bin/ and usr.bin/, no? I want to be >> > > > sure I understand how to do the next patch, because if we do that it >> > > > will probably be a bunch of programs all at once. >> > > >> > > This specific program has not checked for this condition since at least >> > > 2 AT&T UNIX. >> > > >> > > Your change does not just add a new warning. It adds a new exit code >> > > condition. >> > > >> > > Some scripts using echo, which accepted the condition because echo would >> > > exit 0 and not check for this condition, will now see this exit 1. Some >> > > scripts will abort, because they use "set -o errexit" or similar. >> > > >> > > You are changing the exit code for a command which is used a lot. >> > > >> > > POSIX does not require or specify exit 1 for this condition. If you >> > > disagree, please show where it says so. >> > >> > It's the usual thing. >0 if "an error occurred". >> >> The 40 year old code base says otherwise. >> >> > Here is my thinking: >> > >> >echo(1) has ONE job: print the arguments given. >> > >> > If it fails to print those arguments, shouldn't we signal that to the >> > program that forked echo(1)? >> >> Only if you validate all callers can handle this change in behaviour. >> >> > How is echo(1) supposed to differentiate between a write(2) that is >> > allowed to fail, e.g. a diagnostic printout from fw_update to the >> > user's stderr, and one that is not allowed to fail? >> >> Perhaps it is not supposed to validate this problem in 2022, because it >> didn't validate it for 40 years. >> >> > Consider this scenario: >> > >> > 1. A shell script uses echo(1) to write something to a file. >> > >> >/bin/echo foo.dat >> /var/workerd/data-processing-list >> > >> > 2. The bytes don't arrive on disk because the file system is full. >> > >> > 3. The shell script succeeds because echo(1) can't fail, even if >> > it fails to do what it was told to do. >> > >> > Isn't that bad? >> > >> > And it isn't necessarily true that some other thing will fail later >> > and the whole interlocking system will fail. ENOSPC is a transient >> > condition. One write(2) can fail and the next write(2) can succeed. >> >> Yet, for 40 years noone complained. >> >> Consider the situation you break and change the behaviour of 1000's of >> shell scripts, and haven'd lifted your finger once to review all the >> shell scripts that call echo. >> >> Have you even compared this behaviour to the echo built-ins in all >> the shells? > > I assume what you mean to say is, roughly: > > Gee, this seems risky. > > What do other echo implementations do? > > 1. Our ksh(1) already checks for stdout errors in the echo builtin. So do any of the scripts in our source tree use /bin/echo for whatever reason? If so, could one of these scripts be broken if /bin/echo started to report an error? Shouldn't those scripts be reviewed? /bin/echo may look unimportant but I think it deserves the same treatment as more complex tools that you may modify in a similar way. While I agree that adding more error reporting would be good, it sounds fair that it should be done while caring for the actual errors. ;) [...] -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: echo(1): check for stdio errors
On Wed, Aug 10 2022, "Theo de Raadt" wrote: > Scott Cheloha wrote: > >> We're sorta-kinda circling around adding the missing (?) stdio error >> checking to other utilities in bin/ and usr.bin/, no? I want to be >> sure I understand how to do the next patch, because if we do that it >> will probably be a bunch of programs all at once. > > > This specific program has not checked for this condition since at least > 2 AT&T UNIX. > > Your change does not just add a new warning. It adds a new exit code > condition. > > Some scripts using echo, which accepted the condition because echo would > exit 0 and not check for this condition, will now see this exit 1. Some > scripts will abort, because they use "set -o errexit" or similar. About this, and only related to echo: the builtin version in ksh(1) does report an error. I have just tried the mentioned ENOSPC case with exec 3>/tmp/o/file; echo foobar >&3; echo $? and /tmp/o being a full MFS filesystem. So shell scripts run with ksh(1) should already handle echo - the builtin - failures. I think it makes sense for the echo(1) executable to behave like the shell builtin. For the other programs, well, I don't have a strong feeling. I'll just note that adding stdio error reporting has been done in some other projects. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
move swblk_t from sys/types.h to sys/blist.h and use it in blist (was: change its type and us Re: patch: change swblk_t type and use it in blist)
On Sat, Aug 06 2022, Sebastien Marie wrote:
> On Sat, Aug 06, 2022 at 02:19:31AM +0200, Jeremie Courreges-Anglas wrote:
>> On Fri, Aug 05 2022, Sebastien Marie wrote:
>> > Hi,
>> >
>> > When initially ported blist from DragonFlyBSD, we used custom type bsblk_t
>> > and
>> > bsbmp_t instead of the one used by DragonFlyBSD (swblk_t and u_swblk_t).
>> >
>> > The reason was swblk_t is already defined on OpenBSD, and was incompatible
>> > with
>> > blist (int32_t). It is defined, but not used (outside some regress file
>> > which
>> > seems to be not affected by type change).
>> >
>> > This diff changes the __swblk_t definition in sys/_types.h to be 'unsigned
>> > long', and switch back blist to use swblk_t (and u_swblk_t, even if it
>> > isn't
>> > 'unsigned swblk_t').
>> >
>> > It makes the diff with DragonFlyBSD more thin. I added a comment with the
>> > git id
>> > used for the initial port.
>> >
>> > I tested it on i386 and amd64 (kernel and userland).
>> >
>> > By changing bitmap type from 'u_long' to 'u_swblk_t' ('u_int64_t'), it
>> > makes the
>> > regress the same on 64 and 32bits archs (and it success on both).
>> >
>> > Comments or OK ?
>>
>> This seems fair, but maybe we should just zap the type from sys/types.h and
>> define it only in sys/blist.h, as done in DragonflyBSD?
>
> Yes, it makes lot of sense.
>
> Updated diff below.
>
>> I'm building a release on amd64 with the type removed (also from
>> regress). I don't expect fallout in ports (and I can take care of it if
>> there is any).
>
> I also have a release building on i386.
>
> Thanks.
As expected, make release passes on amd64, with the diff below which
also includes the regress bits. Feel free to commit it (ok jca@) before
your blist changes (which I won't review today, EBUSY).
Index: sys/sys/_types.h
===
RCS file: /home/cvs/src/sys/sys/_types.h,v
retrieving revision 1.9
diff -u -p -r1.9 _types.h
--- sys/sys/_types.h22 Aug 2014 23:05:15 - 1.9
+++ sys/sys/_types.h6 Aug 2022 00:02:30 -
@@ -60,7 +60,6 @@ typedef __uint8_t __sa_family_t; /* sock
typedef__int32_t __segsz_t; /* segment size */
typedef__uint32_t __socklen_t;/* length type for network
syscalls */
typedeflong__suseconds_t; /* microseconds (signed) */
-typedef__int32_t __swblk_t; /* swap offset */
typedef__int64_t __time_t; /* epoch time */
typedef__int32_t __timer_t; /* POSIX timer identifiers */
typedef__uint32_t __uid_t;/* user id */
Index: sys/sys/types.h
===
RCS file: /home/cvs/src/sys/sys/types.h,v
retrieving revision 1.48
diff -u -p -r1.48 types.h
--- sys/sys/types.h 9 Feb 2019 04:54:11 - 1.48
+++ sys/sys/types.h 6 Aug 2022 00:02:41 -
@@ -144,7 +144,6 @@ typedef __mode_tmode_t; /* permissions
typedef__nlink_t nlink_t;/* link count */
typedef__rlim_trlim_t; /* resource limit */
typedef__segsz_t segsz_t;/* segment size */
-typedef__swblk_t swblk_t;/* swap offset */
typedef__uid_t uid_t; /* user id */
typedef__useconds_tuseconds_t; /* microseconds */
typedef__suseconds_t suseconds_t;/* microseconds (signed) */
Index: regress/misc/c++abi/nm1.C
===
RCS file: /home/cvs/src/regress/misc/c++abi/nm1.C,v
retrieving revision 1.2
diff -u -p -r1.2 nm1.C
--- regress/misc/c++abi/nm1.C 7 Feb 2017 12:57:12 - 1.2
+++ regress/misc/c++abi/nm1.C 5 Aug 2022 23:58:59 -
@@ -23,7 +23,6 @@ D(sa_family_t)
D(segsz_t)
D(socklen_t)
D(suseconds_t)
-D(swblk_t)
D(uid_t)
D(uint64_t)
D(uint32_t)
Index: regress/misc/c++abi/nm1.ref
===
RCS file: /home/cvs/src/regress/misc/c++abi/nm1.ref,v
retrieving revision 1.3
diff -u -p -r1.3 nm1.ref
--- regress/misc/c++abi/nm1.ref 29 Oct 2013 03:00:40 - 1.3
+++ regress/misc/c++abi/nm1.ref 5 Aug 2022 23:59:47 -
@@ -18,7 +18,6 @@ sa_family_t(unsigned char)
segsz_t(int)
socklen_t(unsigned int)
suseconds_t(long)
-swblk_t(int)
uid_t(unsigned int)
uint64_t(unsigned long long)
uint32_t(unsigned int)
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: patch: change swblk_t type and use it in blist
On Fri, Aug 05 2022, Sebastien Marie wrote:
> Hi,
>
> When initially ported blist from DragonFlyBSD, we used custom type bsblk_t
> and
> bsbmp_t instead of the one used by DragonFlyBSD (swblk_t and u_swblk_t).
>
> The reason was swblk_t is already defined on OpenBSD, and was incompatible
> with
> blist (int32_t). It is defined, but not used (outside some regress file which
> seems to be not affected by type change).
>
> This diff changes the __swblk_t definition in sys/_types.h to be 'unsigned
> long', and switch back blist to use swblk_t (and u_swblk_t, even if it isn't
> 'unsigned swblk_t').
>
> It makes the diff with DragonFlyBSD more thin. I added a comment with the git
> id
> used for the initial port.
>
> I tested it on i386 and amd64 (kernel and userland).
>
> By changing bitmap type from 'u_long' to 'u_swblk_t' ('u_int64_t'), it makes
> the
> regress the same on 64 and 32bits archs (and it success on both).
>
> Comments or OK ?
This seems fair, but maybe we should just zap the type from sys/types.h and
define it only in sys/blist.h, as done in DragonflyBSD?
https://reviews.freebsd.org/D23666
That would make your diff to DragonflyBSD even shorter. :)
Also it looks like FreeBSD removed their swblk_t definition in 2020:
https://reviews.freebsd.org/D23666
I'm building a release on amd64 with the type removed (also from
regress). I don't expect fallout in ports (and I can take care of it if
there is any).
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Thu, Aug 04 2022, Scott Cheloha wrote:
> On Thu, Aug 04, 2022 at 09:39:13AM +0200, Jeremie Courreges-Anglas wrote:
>> On Mon, Aug 01 2022, Scott Cheloha wrote:
>> > On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
>> >> On Sun, Jul 31 2022, Scott Cheloha wrote:
>> >> > Hi,
>> >> >
>> >> > I am unsure how to properly mask RISC-V timer interrupts in hardware
>> >> > at or above IPL_CLOCK. I think that would be cleaner than doing
>> >> > another timer interrupt deferral thing.
>> >> >
>> >> > But, just to get the ball rolling, here a first attempt at the timer
>> >> > interrupt deferral thing for riscv64. The motivation, as with every
>> >> > other platform, is to eventually make it unnecessary for the machine
>> >> > dependent code to know anything about the clock interrupt schedule.
>> >> >
>> >> > The thing I'm most unsure about is where to retrigger the timer in the
>> >> > PLIC code. It seems right to do it from plic_setipl() because I want
>> >> > to retrigger it before doing soft interrupt work, but I'm not sure.
>>
>> You're adding the timer reset to plic_setipl() but the latter is called
>> after softintr processing in plic_splx().
>>
>> /* Pending software intr is handled here */
>> if (ci->ci_ipending & riscv_smask[new])
>> riscv_do_pending_intr(new);
>>
>> plic_setipl(new);
>
> Yes, but plic_setipl() is also called from the softintr loop in
> riscv_do_pending_intr() (riscv64/intr.c) right *before* we dispatch
> any pending soft interrupts:
>
>594 void
>595 riscv_do_pending_intr(int pcpl)
>596 {
>597 struct cpu_info *ci = curcpu();
>598 u_long sie;
>599
>600 sie = intr_disable();
>601
>602 #define DO_SOFTINT(si, ipl) \
>603 if ((ci->ci_ipending & riscv_smask[pcpl]) & \
>604 SI_TO_IRQBIT(si)) { \
>605 ci->ci_ipending &= ~SI_TO_IRQBIT(si); \
> * 606 riscv_intr_func.setipl(ipl);\
>607 intr_restore(sie); \
>608 softintr_dispatch(si); \
>609 sie = intr_disable(); \
>610 }
>611
>612 do {
>613 DO_SOFTINT(SIR_TTY, IPL_SOFTTTY);
>614 DO_SOFTINT(SIR_NET, IPL_SOFTNET);
>615 DO_SOFTINT(SIR_CLOCK, IPL_SOFTCLOCK);
>616 DO_SOFTINT(SIR_SOFT, IPL_SOFT);
>617 } while (ci->ci_ipending & riscv_smask[pcpl]);
>
> We might be fine doing it just once in plic_splx() before we do any
> soft interrupt stuff. That's closer to what we're doing on other
> platforms.
>
> I just figured it'd be safer to do it in plic_setipl() because we're
> already disabling interrupts there. It seems I guessed correctly
> because the patch didn't hang your machine.
Ugh, I had missed that setipl call, thanks for pointing it out.
Since I don't wander into this code on a casual basis I won't object,
but this looks very unobvious to me. :)
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Mon, Aug 01 2022, Scott Cheloha wrote:
> On Mon, Aug 01, 2022 at 07:15:33PM +0200, Jeremie Courreges-Anglas wrote:
>> On Sun, Jul 31 2022, Scott Cheloha wrote:
>> > Hi,
>> >
>> > I am unsure how to properly mask RISC-V timer interrupts in hardware
>> > at or above IPL_CLOCK. I think that would be cleaner than doing
>> > another timer interrupt deferral thing.
>> >
>> > But, just to get the ball rolling, here a first attempt at the timer
>> > interrupt deferral thing for riscv64. The motivation, as with every
>> > other platform, is to eventually make it unnecessary for the machine
>> > dependent code to know anything about the clock interrupt schedule.
>> >
>> > The thing I'm most unsure about is where to retrigger the timer in the
>> > PLIC code. It seems right to do it from plic_setipl() because I want
>> > to retrigger it before doing soft interrupt work, but I'm not sure.
You're adding the timer reset to plic_setipl() but the latter is called
after softintr processing in plic_splx().
/* Pending software intr is handled here */
if (ci->ci_ipending & riscv_smask[new])
riscv_do_pending_intr(new);
plic_setipl(new);
>> > Unless I'm missing something, I don't think I need to do anything in
>> > the default interrupt handler code, i.e. riscv64_dflt_setipl(), right?
>>
>> No idea about about the items above, but...
>>
>> > I have no riscv64 machine, so this is untested. Would appreciate
>> > tests and feedback.
>>
>> There's an #include missing in plic.c,
>
> Whoops, corrected patch attached below.
>
>> with that added your diff builds and GENERIC.MP seems to behave
>> (currently running make -j4 build), but I don't know exactly which
>> problems I should look for.
>
> Thank you for trying it out.
>
> The patch changes how clock interrupt work is deferred on riscv64.
>
> If the code is wrong, the hardclock and statclock should eventually
> die on every CPU. The death of the hardclock in particular would
> manifest to the user as livelock. The scheduler would stop preempting
> userspace and it would be impossible to use the machine interactively.
>
> There isn't really a direct way to exercise this code change.
>
> The best we can do is make the machine busy. If the machine is busy
> we can expect more spl(9) calls and more deferred clock interrupt
> work, which leaves more opportunities for the bug to surface.
>
> So, a parallel `make build` is fine. It's our gold standard for
> making the machine really busy.
The diff survived three make -j4 build/release in a row, the clock seems
stable.
> Index: include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/riscv64/include/cpu.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 cpu.h
> --- include/cpu.h 10 Jun 2022 21:34:15 - 1.12
> +++ include/cpu.h 1 Aug 2022 17:36:41 -
> @@ -92,7 +92,7 @@ struct cpu_info {
> uint64_tci_lasttb;
> uint64_tci_nexttimerevent;
> uint64_tci_nextstatevent;
> - int ci_statspending;
> + volatile intci_timer_deferred;
>
> uint32_tci_cpl;
> uint32_tci_ipending;
> Index: riscv64/clock.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 clock.c
> --- riscv64/clock.c 24 Jul 2021 22:41:09 - 1.3
> +++ riscv64/clock.c 1 Aug 2022 17:36:41 -
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -106,6 +107,17 @@ clock_intr(void *frame)
> int s;
>
> /*
> + * If the clock interrupt is masked, defer all clock interrupt
> + * work until the clock interrupt is unmasked from splx(9).
> + */
> + if (ci->ci_cpl >= IPL_CLOCK) {
> + ci->ci_timer_deferred = 1;
> + sbi_set_timer(UINT64_MAX);
> + return 0;
> + }
> + ci->ci_timer_deferred = 0;
> +
> + /*
>* Based on the actual time delay since the last clock interrupt,
>* we arrange for earlier interrupt next time.
>*/
> @@ -132,30 +144,23 @@ clock_intr(void *frame)
>
> sbi_set_timer(nextevent);
>
> - if (ci->ci_cpl >= IPL_CLOCK) {
> - ci->ci_statspending += nstats;
> - } else {
> -
Re: riscv64: trigger deferred timer interrupts from splx(9)
On Sun, Jul 31 2022, Scott Cheloha wrote:
> Hi,
>
> I am unsure how to properly mask RISC-V timer interrupts in hardware
> at or above IPL_CLOCK. I think that would be cleaner than doing
> another timer interrupt deferral thing.
>
> But, just to get the ball rolling, here a first attempt at the timer
> interrupt deferral thing for riscv64. The motivation, as with every
> other platform, is to eventually make it unnecessary for the machine
> dependent code to know anything about the clock interrupt schedule.
>
> The thing I'm most unsure about is where to retrigger the timer in the
> PLIC code. It seems right to do it from plic_setipl() because I want
> to retrigger it before doing soft interrupt work, but I'm not sure.
>
> Unless I'm missing something, I don't think I need to do anything in
> the default interrupt handler code, i.e. riscv64_dflt_setipl(), right?
No idea about about the items above, but...
> I have no riscv64 machine, so this is untested. Would appreciate
> tests and feedback.
There's an #include missing in plic.c, with that added
your diff builds and GENERIC.MP seems to behave (currently running
make -j4 build), but I don't know exactly which problems I should look
for.
> -Scott
>
> Index: include/cpu.h
> ===
> RCS file: /cvs/src/sys/arch/riscv64/include/cpu.h,v
> retrieving revision 1.12
> diff -u -p -r1.12 cpu.h
> --- include/cpu.h 10 Jun 2022 21:34:15 - 1.12
> +++ include/cpu.h 1 Aug 2022 01:13:38 -
> @@ -92,7 +92,7 @@ struct cpu_info {
> uint64_tci_lasttb;
> uint64_tci_nexttimerevent;
> uint64_tci_nextstatevent;
> - int ci_statspending;
> + volatile intci_timer_deferred;
>
> uint32_tci_cpl;
> uint32_tci_ipending;
> Index: riscv64/clock.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 clock.c
> --- riscv64/clock.c 24 Jul 2021 22:41:09 - 1.3
> +++ riscv64/clock.c 1 Aug 2022 01:13:38 -
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -106,6 +107,17 @@ clock_intr(void *frame)
> int s;
>
> /*
> + * If the clock interrupt is masked, defer all clock interrupt
> + * work until the clock interrupt is unmasked from splx(9).
> + */
> + if (ci->ci_cpl >= IPL_CLOCK) {
> + ci->ci_timer_deferred = 1;
> + sbi_set_timer(UINT64_MAX);
> + return 0;
> + }
> + ci->ci_timer_deferred = 0;
> +
> + /*
>* Based on the actual time delay since the last clock interrupt,
>* we arrange for earlier interrupt next time.
>*/
> @@ -132,30 +144,23 @@ clock_intr(void *frame)
>
> sbi_set_timer(nextevent);
>
> - if (ci->ci_cpl >= IPL_CLOCK) {
> - ci->ci_statspending += nstats;
> - } else {
> - nstats += ci->ci_statspending;
> - ci->ci_statspending = 0;
> -
> - s = splclock();
> - intr_enable();
> -
> - /*
> - * Do standard timer interrupt stuff.
> - */
> - while (ci->ci_lasttb < prevtb) {
> - ci->ci_lasttb += tick_increment;
> - clock_count.ec_count++;
> - hardclock((struct clockframe *)frame);
> - }
> + s = splclock();
> + intr_enable();
>
> - while (nstats-- > 0)
> - statclock((struct clockframe *)frame);
> -
> - intr_disable();
> - splx(s);
> + /*
> + * Do standard timer interrupt stuff.
> + */
> + while (ci->ci_lasttb < prevtb) {
> + ci->ci_lasttb += tick_increment;
> + clock_count.ec_count++;
> + hardclock((struct clockframe *)frame);
> }
> +
> + while (nstats-- > 0)
> + statclock((struct clockframe *)frame);
> +
> + intr_disable();
> + splx(s);
>
> return 0;
> }
> Index: dev/plic.c
> ===
> RCS file: /cvs/src/sys/arch/riscv64/dev/plic.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 plic.c
> --- dev/plic.c6 Apr 2022 18:59:27 - 1.10
> +++ dev/plic.c1 Aug 2022 01:13:38 -
> @@ -557,6 +557,10 @@ plic_setipl(int new)
> /* higher values are higher priority */
> plic_set_threshold(ci->ci_cpuid, new);
>
> + /* trigger deferred timer interrupt if cpl is now low enough */
> + if (ci->ci_timer_deferred && new < IPL_CLOCK)
> + sbi_set_timer(0);
> +
> intr_restore(sie);
> }
>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: amuart(4) DDB
On Fri, Jul 15 2022, Mark Kettenis wrote:
> Same problem as sfuart(4); no support for detecting a BREAK. So
> leverage db_rint(9) to provide a way to enter DDB.
>
> ok?
I have no hardware to build or test this, but I guess you do and the
change looks fine. ok jca@
> Index: dev/fdt/amluart.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/amluart.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 amluart.c
> --- dev/fdt/amluart.c 24 Oct 2021 17:52:26 - 1.3
> +++ dev/fdt/amluart.c 15 Jul 2022 09:20:10 -
> @@ -276,8 +276,20 @@ amluart_softintr(void *arg)
>
> splx(s);
>
> - while (ibufp < ibufend)
> - (*linesw[tp->t_line].l_rint)(*ibufp++, tp);
> + while (ibufp < ibufend) {
> + int i = *ibufp++;
> +#ifdef DDB
> + if (tp->t_dev == cn_tab->cn_dev) {
> + int j = db_rint(i);
> +
> + if (j == 1) /* Escape received, skip */
> + continue;
> + if (j == 2) /* Second char wasn't 'D' */
> + (*linesw[tp->t_line].l_rint)(27, tp);
> + }
> +#endif
> + (*linesw[tp->t_line].l_rint)(i, tp);
> + }
> }
>
> int
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: unp_solock_peer() and READ_ONCE()
On Thu, Jul 14 2022, Vitaliy Makkoveev wrote: [...] > Also may be the problem lays in other layer, but triggered here... What is the syzkaller report that lead to this discussion? -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Don't build lldb on archs where it's not supported
We currently build most of the lldb support code on all archs, even
though we only build and install the lldb executable (and lldb-server)
on amd64 and arm64. This wastes a significant amount of CPU cycles
during all base builds for no good reason that I can see.
The diff below puts the lldb-or-not switch in bsd.own.mk and skips the
support code build accordingly.
ok?
Index: share/mk/bsd.own.mk
===
RCS file: /home/cvs/src/share/mk/bsd.own.mk,v
retrieving revision 1.212
diff -u -p -r1.212 bsd.own.mk
--- share/mk/bsd.own.mk 23 Nov 2021 10:30:08 - 1.212
+++ share/mk/bsd.own.mk 7 Jul 2022 23:09:04 -
@@ -19,6 +19,7 @@ CLANG_ARCH=aarch64 amd64 arm i386 mips64
GCC4_ARCH=alpha hppa sh sparc64
GCC3_ARCH=m88k
LLD_ARCH=aarch64 amd64 arm i386 powerpc powerpc64 riscv64
+LLDB_ARCH=aarch64 amd64
# m88k: ?
PIE_ARCH=aarch64 alpha amd64 arm hppa i386 mips64 mips64el powerpc powerpc64
riscv64 sh sparc64
@@ -55,6 +56,12 @@ AR_VERSION?=llvm
.else
LINKER_VERSION?=bfd
AR_VERSION?=binutils
+.endif
+
+.if !empty(LLDB_ARCH:M${_arch})
+BUILD_LLDB?=yes
+.else
+BUILD_LLDB?=no
.endif
.if !empty(STATICPIE_ARCH:M${_arch})
Index: gnu/usr.bin/clang/Makefile
===
RCS file: /home/cvs/src/gnu/usr.bin/clang/Makefile,v
retrieving revision 1.19
diff -u -p -r1.19 Makefile
--- gnu/usr.bin/clang/Makefile 9 Jul 2022 16:25:37 - 1.19
+++ gnu/usr.bin/clang/Makefile 10 Jul 2022 00:14:39 -
@@ -51,6 +51,7 @@ SUBDIR+=liblldELF
SUBDIR+=lld
+.if ${BUILD_LLDB:L} == "yes"
SUBDIR+=lldb-tblgen
SUBDIR+=include/lldb/Commands
SUBDIR+=include/lldb/Core
@@ -97,6 +98,7 @@ SUBDIR+=liblldbUtility
SUBDIR+=lldb
SUBDIR+=lldb-server
+.endif
SUBDIR+=include/llvm-objcopy
SUBDIR+=llvm-objcopy
Index: gnu/usr.bin/clang/lldb/Makefile
===
RCS file: /home/cvs/src/gnu/usr.bin/clang/lldb/Makefile,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile
--- gnu/usr.bin/clang/lldb/Makefile 17 Dec 2021 14:55:47 - 1.11
+++ gnu/usr.bin/clang/lldb/Makefile 18 Feb 2022 21:32:18 -
@@ -2,12 +2,7 @@
.include
-.if (${MACHINE} == "arm64") || (${MACHINE} == "amd64")
PROG= lldb
-.else
-NOPROG=lldb
-.endif
-
BINDIR=/usr/bin
LIBEXECDIR=/usr/libexec
Index: gnu/usr.bin/clang/lldb-server/Makefile
===
RCS file: /home/cvs/src/gnu/usr.bin/clang/lldb-server/Makefile,v
retrieving revision 1.6
diff -u -p -r1.6 Makefile
--- gnu/usr.bin/clang/lldb-server/Makefile 17 Dec 2021 14:55:47 -
1.6
+++ gnu/usr.bin/clang/lldb-server/Makefile 18 Feb 2022 21:32:26 -
@@ -2,12 +2,7 @@
.include
-.if (${MACHINE} == "arm64") || (${MACHINE} == "amd64")
PROG= lldb-server
-.else
-NOPROG=lldb-server
-.endif
-
BINDIR=/usr/bin
NOMAN=
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Build and install llvm-readelf (and llvm-readobj)
On riscv64 I miss having a usable readelf. We could just build
llvm-readelf instead. The actual tool is called llvm-readobj, with
GNU compat when called under the llvm-readelf or readelf names.
People will probably find this useful on arm64 and powerpc64 too.
This shouldn't have a negative impact on ports. ok?
Index: Makefile
===
RCS file: /home/cvs/src/gnu/usr.bin/clang/Makefile,v
retrieving revision 1.18
diff -u -p -r1.18 Makefile
--- Makefile17 Dec 2021 14:55:44 - 1.18
+++ Makefile9 Jul 2022 10:51:13 -
@@ -102,6 +102,8 @@ SUBDIR+=include/llvm-objcopy
SUBDIR+=llvm-objcopy
SUBDIR+=include/llvm-objdump
SUBDIR+=llvm-objdump
+SUBDIR+=include/llvm-readobj
+SUBDIR+=llvm-readobj
.if ${AR_VERSION:L} == "llvm"
SUBDIR+=libLLVMDlltoolDriver
Index: include/llvm-readobj/Makefile
===
RCS file: include/llvm-readobj/Makefile
diff -N include/llvm-readobj/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ include/llvm-readobj/Makefile 6 Jul 2022 00:55:05 -
@@ -0,0 +1,23 @@
+# $OpenBSD: Makefile,v 1.2 2021/12/17 14:55:44 patrick Exp $
+
+.include
+
+TBLGEN=${.OBJDIR}/../../llvm-tblgen/llvm-tblgen
+OBJCOPY_INC=${.CURDIR}/../../../../llvm/llvm/tools/llvm-readobj
+
+GEN= Opts.inc
+
+all: ${GEN}
+
+install:
+ @# Nothing here so far ...
+
+clean cleandir:
+ rm -f ${GEN}
+
+Opts.inc: ${OBJCOPY_INC}/Opts.td
+ ${TBLGEN} -I${.CURDIR}/../../../../llvm/llvm/include \
+ -I${.CURDIR}/../../../../llvm/llvm/tools/llvm-readobj \
+ -gen-opt-parser-defs -o ${.TARGET} ${.ALLSRC}
+
+.include
Index: llvm-readobj/Makefile
===
RCS file: llvm-readobj/Makefile
diff -N llvm-readobj/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ llvm-readobj/Makefile 9 Jul 2022 01:21:04 -
@@ -0,0 +1,30 @@
+# $OpenBSD: Makefile,v 1.2 2021/12/17 14:55:47 patrick Exp $
+
+.include
+
+PROG= llvm-readobj
+BINDIR=/usr/bin
+LINKS= ${BINDIR}/llvm-readobj ${BINDIR}/llvm-readelf
+MAN= llvm-readelf.1 llvm-readobj.1
+
+SRCS= ARMWinEHPrinter.cpp \
+ COFFDumper.cpp \
+ COFFImportDumper.cpp \
+ ELFDumper.cpp \
+ llvm-readobj.cpp \
+ MachODumper.cpp \
+ ObjDumper.cpp \
+ WasmDumper.cpp \
+ Win64EHDumper.cpp \
+ WindowsResourceDumper.cpp \
+ XCOFFDumper.cpp
+
+CPPFLAGS+= -I${.OBJDIR}/../include/llvm-readobj
+
+.PATH: ${.CURDIR}/../../../llvm/llvm/tools/llvm-readobj
+
+LLVM_LIBDEPS= LLVM
+
+LDADD+= -L ${.OBJDIR}/../libLLVM -lLLVM
+
+.include
Index: llvm-readobj/llvm-readelf.1
===
RCS file: llvm-readobj/llvm-readelf.1
diff -N llvm-readobj/llvm-readelf.1
--- /dev/null 1 Jan 1970 00:00:00 -
+++ llvm-readobj/llvm-readelf.1 9 Jul 2022 01:20:20 -
@@ -0,0 +1,269 @@
+.\" Man page generated from reStructuredText.
+.
+.
+.nr rst2man-indent-level 0
+.
+.de1 rstReportMargin
+\\$1 \\n[an-margin]
+level \\n[rst2man-indent-level]
+level margin: \\n[rst2man-indent\\n[rst2man-indent-level]]
+-
+\\n[rst2man-indent0]
+\\n[rst2man-indent1]
+\\n[rst2man-indent2]
+..
+.de1 INDENT
+.\" .rstReportMargin pre:
+. RS \\$1
+. nr rst2man-indent\\n[rst2man-indent-level] \\n[an-margin]
+. nr rst2man-indent-level +1
+.\" .rstReportMargin post:
+..
+.de UNINDENT
+. RE
+.\" indent \\n[an-margin]
+.\" old: \\n[rst2man-indent\\n[rst2man-indent-level]]
+.nr rst2man-indent-level -1
+.\" new: \\n[rst2man-indent\\n[rst2man-indent-level]]
+.in \\n[rst2man-indent\\n[rst2man-indent-level]]u
+..
+.TH "LLVM-READELF" "1" "2022-07-05" "13" "LLVM"
+.SH NAME
+llvm-readelf \- GNU-style LLVM Object Reader
+.SH SYNOPSIS
+.sp
+\fBllvm\-readelf\fP [\fIoptions\fP] [\fIinput...\fP]
+.SH DESCRIPTION
+.sp
+The \fBllvm\-readelf\fP tool displays low\-level format\-specific information
+about one or more object files.
+.sp
+If \fBinput\fP is "\fB\-\fP", \fBllvm\-readelf\fP reads from standard
+input. Otherwise, it will read from the specified \fBfilenames\fP\&.
+.SH OPTIONS
+.INDENT 0.0
+.TP
+.B \-\-all
+Equivalent to specifying all the main display options relevant to the file
+format.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-addrsig
+Display the address\-significance table.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-arch\-specific, \-A
+Display architecture\-specific information, e.g. the ARM attributes section on
ARM.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-bb\-addr\-map
+Display the contents of the basic block address map section(s), which contain
the
+address of each function, along with the relative offset of each basic block.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-demangle, \-C
+Display demangled symbol names in the output.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-dyn\-relocations
+Display the dynamic relocation entries.
+.UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-dyn\-symbols, \-\-dyn\-syms
+Disp
Re: amd64 serial console changes
On Thu, Jun 30 2022, Hrvoje Popovski wrote: > On 30.6.2022. 16:48, Hrvoje Popovski wrote: >> On 30.6.2022. 15:14, Anton Lindqvist wrote: >>> On Thu, Jun 30, 2022 at 01:07:46PM +0200, Mark Kettenis wrote: Ah right. Please commit! >>> Here's the complete diff, ok? >> >> >> Hi, >> >> with this diff : >> >> dell r620 - serial console >> com1 at acpi0 COMA addr 0x2f8/0x8 irq 3: ns16550a, 16 byte fifo >> com1: console >> com0 at acpi0 COMB addr 0x3f8/0x8 irq 4: ns16550a, 16 byte fifo >> >> works fast as before with first boot but second boot is slow... >> >> supermicro - ipmi console >> com0 at acpi0 UAR1 addr 0x3f8/0x8 irq 4: ns16550a, 16 byte fifo >> com1 at acpi0 UAR2 addr 0x2f8/0x8 irq 3: ns16550a, 16 byte fifo >> com1: console >> >> is slow as without this diff .. >> >> >> i will try on few more machines this diff ... >> > > after applying diff i did > cd /sys/arch/amd64/compile/GENERIC.MP && make -j6 obj && make config && > make clean && time make -j6 && make install && reboot > > > is this ok? You need to rebuild the boot files in /sys/arch/amd64/stand, install them and then run installboot. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Unlocking pledge(2)
On Tue, Jun 28 2022, Martin Pieuchot wrote:
> On 28/06/22(Tue) 18:17, Jeremie Courreges-Anglas wrote:
>>
>> Initially I just wandered in syscall_mi.h and found the locking scheme
>> super weird, even if technically correct. pledge_syscall() better be
>> safe to call without the kernel lock so I don't understand why we're
>> sometimes calling it with the kernel lock and sometimes not.
>>
>> ps_pledge is 64 bits so it's not possible to unset bits in an atomic
>> manner on all architectures. Even if we're only removing bits and there
>> is probably no way to see a completely garbage value, it makes sense to
>> just protect ps_pledge (and ps_execpledge) in the usual manner so that
>> we can unlock the syscall. The diff below protects the fields using
>> ps_mtx even though I initially used a dedicated ps_pledge_mtx.
>> unveil_destroy() needs to be moved after the critical section.
>> regress/sys/kern/pledge looks happy with this. The sys/syscall_mi.h
>> change can be committed in a separate step.
>>
>> Input and oks welcome.
>
> This looks nice. I doubt there's any existing program where you can
> really test this. Even firefox and chromium should do things
> correctly.
>
> Maybe you should write a regress test that tries to break the kernel.
That would need some pondering, I'm not sure what kind of breakage
you're envisioning. The obvious check to perform would be to ensure
that we're not increasing the promises but the code already does it.
Hmm.
For now, here's the latest diff based on input from the hackroom, which
basically amounts to:
- don't take the mutex to read the value. Even on 32 bits machines that
can't write an 8 byte value in a single go we don't expect garbage to
on a read crossing a write, since we only remove bits and never add
some.
- use READ_ONCE when we're storing the value in a local variable, to
prevent possible reloading from memory. In the current code it would
not be a problem but ensuring that we do not reload from memory is
just good practice for stuff that can be modified concurrently.
(This part can be committed in a second step, just like the
syscall_mi.h change.)
ok?
Index: kern/init_sysent.c
===
RCS file: /home/cvs/src/sys/kern/init_sysent.c,v
retrieving revision 1.238
diff -u -p -r1.238 init_sysent.c
--- kern/init_sysent.c 27 Jun 2022 14:26:05 - 1.238
+++ kern/init_sysent.c 28 Jun 2022 15:18:25 -
@@ -1,10 +1,10 @@
-/* $OpenBSD: init_sysent.c,v 1.238 2022/06/27 14:26:05 cheloha Exp $
*/
+/* $OpenBSD$ */
/*
* System call switch table.
*
* DO NOT EDIT-- this file is automatically generated.
- * created from; OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04
mvs Exp
+ * created from; OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05
cheloha Exp
*/
#include
@@ -248,7 +248,7 @@ const struct sysent sysent[] = {
sys_listen }, /* 106 = listen */
{ 4, s(struct sys_chflagsat_args), 0,
sys_chflagsat },/* 107 = chflagsat */
- { 2, s(struct sys_pledge_args), 0,
+ { 2, s(struct sys_pledge_args), SY_NOLOCK | 0,
sys_pledge }, /* 108 = pledge */
{ 4, s(struct sys_ppoll_args), 0,
sys_ppoll },/* 109 = ppoll */
Index: kern/kern_pledge.c
===
RCS file: /home/cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.282
diff -u -p -r1.282 kern_pledge.c
--- kern/kern_pledge.c 26 Jun 2022 06:11:49 - 1.282
+++ kern/kern_pledge.c 28 Jun 2022 17:00:51 -
@@ -21,6 +21,7 @@
#include
#include
+#include
#include
#include
#include
@@ -465,13 +466,26 @@ sys_pledge(struct proc *p, void *v, regi
struct process *pr = p->p_p;
uint64_t promises, execpromises;
int error;
+ int unveil_cleanup = 0;
+ /* Check for any error in user input */
if (SCARG(uap, promises)) {
error = parsepledges(p, "pledgereq",
SCARG(uap, promises), &promises);
if (error)
return (error);
+ }
+ if (SCARG(uap, execpromises)) {
+ error = parsepledges(p, "pledgeexecreq",
+ SCARG(uap, execpromises), &execpromises);
+ if (error)
+ return (error);
+ }
+
+ mtx_enter(&pr->ps_mtx);
+ /* Check for any error wrt current promises */
+ if (SCARG(uap, promises)) {
/* In "error" mode, ignore promise increase requests,
* but accept promis
Unlocking pledge(2)
Initially I just wandered in syscall_mi.h and found the locking scheme
super weird, even if technically correct. pledge_syscall() better be
safe to call without the kernel lock so I don't understand why we're
sometimes calling it with the kernel lock and sometimes not.
ps_pledge is 64 bits so it's not possible to unset bits in an atomic
manner on all architectures. Even if we're only removing bits and there
is probably no way to see a completely garbage value, it makes sense to
just protect ps_pledge (and ps_execpledge) in the usual manner so that
we can unlock the syscall. The diff below protects the fields using
ps_mtx even though I initially used a dedicated ps_pledge_mtx.
unveil_destroy() needs to be moved after the critical section.
regress/sys/kern/pledge looks happy with this. The sys/syscall_mi.h
change can be committed in a separate step.
Input and oks welcome.
Index: arch/amd64/amd64/vmm.c
===
RCS file: /home/cvs/src/sys/arch/amd64/amd64/vmm.c,v
retrieving revision 1.315
diff -u -p -r1.315 vmm.c
--- arch/amd64/amd64/vmm.c 27 Jun 2022 15:12:14 - 1.315
+++ arch/amd64/amd64/vmm.c 28 Jun 2022 13:54:25 -
@@ -713,7 +713,7 @@ pledge_ioctl_vmm(struct proc *p, long co
case VMM_IOC_CREATE:
case VMM_IOC_INFO:
/* The "parent" process in vmd forks and manages VMs */
- if (p->p_p->ps_pledge & PLEDGE_PROC)
+ if (pledge_get(p->p_p) & PLEDGE_PROC)
return (0);
break;
case VMM_IOC_TERM:
@@ -1312,7 +1312,7 @@ vm_find(uint32_t id, struct vm **res)
* The managing vmm parent process can lookup all
* all VMs and is indicated by PLEDGE_PROC.
*/
- if (((p->p_p->ps_pledge &
+ if (((pledge_get(p->p_p) &
(PLEDGE_VMM | PLEDGE_PROC)) == PLEDGE_VMM) &&
(vm->vm_creator_pid != p->p_p->ps_pid))
return (pledge_fail(p, EPERM, PLEDGE_VMM));
Index: kern/init_sysent.c
===
RCS file: /home/cvs/src/sys/kern/init_sysent.c,v
retrieving revision 1.238
diff -u -p -r1.238 init_sysent.c
--- kern/init_sysent.c 27 Jun 2022 14:26:05 - 1.238
+++ kern/init_sysent.c 28 Jun 2022 15:18:25 -
@@ -1,10 +1,10 @@
-/* $OpenBSD: init_sysent.c,v 1.238 2022/06/27 14:26:05 cheloha Exp $
*/
+/* $OpenBSD$ */
/*
* System call switch table.
*
* DO NOT EDIT-- this file is automatically generated.
- * created from; OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04
mvs Exp
+ * created from; OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05
cheloha Exp
*/
#include
@@ -248,7 +248,7 @@ const struct sysent sysent[] = {
sys_listen }, /* 106 = listen */
{ 4, s(struct sys_chflagsat_args), 0,
sys_chflagsat },/* 107 = chflagsat */
- { 2, s(struct sys_pledge_args), 0,
+ { 2, s(struct sys_pledge_args), SY_NOLOCK | 0,
sys_pledge }, /* 108 = pledge */
{ 4, s(struct sys_ppoll_args), 0,
sys_ppoll },/* 109 = ppoll */
Index: kern/kern_event.c
===
RCS file: /home/cvs/src/sys/kern/kern_event.c,v
retrieving revision 1.191
diff -u -p -r1.191 kern_event.c
--- kern/kern_event.c 27 Jun 2022 13:35:21 - 1.191
+++ kern/kern_event.c 28 Jun 2022 13:55:18 -
@@ -331,7 +331,7 @@ filt_procattach(struct knote *kn)
int s;
if ((curproc->p_p->ps_flags & PS_PLEDGE) &&
- (curproc->p_p->ps_pledge & PLEDGE_PROC) == 0)
+ (pledge_get(curproc->p_p) & PLEDGE_PROC) == 0)
return pledge_fail(curproc, EPERM, PLEDGE_PROC);
if (kn->kn_id > PID_MAX)
Index: kern/kern_pledge.c
===
RCS file: /home/cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.282
diff -u -p -r1.282 kern_pledge.c
--- kern/kern_pledge.c 26 Jun 2022 06:11:49 - 1.282
+++ kern/kern_pledge.c 28 Jun 2022 15:21:46 -
@@ -21,6 +21,7 @@
#include
#include
+#include
#include
#include
#include
@@ -465,13 +466,26 @@ sys_pledge(struct proc *p, void *v, regi
struct process *pr = p->p_p;
uint64_t promises, execpromises;
int error;
+ int unveil_cleanup = 0;
+ /* Check for any error in user input */
if (SCARG(uap, promises)) {
error = parsepledges(p, "pledgereq",
SCARG(uap, promises), &promises);
if (error)
return (error);
+ }
+ if (SCARG(uap, execpromises)) {
+ error = parsepledges(p, "pl
Re: proctree lock diff
On Fri, Nov 29 2019, Martin Pieuchot wrote:
> For archive, here's the diff on top of -current.
Here's a refreshed diff with the bare minimum changes needed to prevent
a WITNESS kernel from crashing when running /usr/src/regress/sys.
Index: kern/exec_elf.c
===
RCS file: /home/cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.166
diff -u -p -r1.166 exec_elf.c
--- kern/exec_elf.c 12 May 2022 16:29:58 - 1.166
+++ kern/exec_elf.c 27 Jun 2022 13:37:58 -
@@ -1185,12 +1185,14 @@ coredump_notes_elf(struct proc *p, void
cpi.cpi_sigcatch = pr->ps_sigacts->ps_sigcatch;
cpi.cpi_pid = pr->ps_pid;
+ rw_enter_read(&proctreelk);
cpi.cpi_ppid = pr->ps_ppid;
cpi.cpi_pgrp = pr->ps_pgid;
if (pr->ps_session->s_leader)
cpi.cpi_sid = pr->ps_session->s_leader->ps_pid;
else
cpi.cpi_sid = 0;
+ rw_exit_read(&proctreelk);
cpi.cpi_ruid = p->p_ucred->cr_ruid;
cpi.cpi_euid = p->p_ucred->cr_uid;
Index: kern/kern_acct.c
===
RCS file: /home/cvs/src/sys/kern/kern_acct.c,v
retrieving revision 1.46
diff -u -p -r1.46 kern_acct.c
--- kern/kern_acct.c22 Feb 2022 17:22:29 - 1.46
+++ kern/kern_acct.c27 Jun 2022 13:37:58 -
@@ -226,11 +226,13 @@ acct_process(struct proc *p)
acct.ac_gid = pr->ps_ucred->cr_rgid;
/* (7) The terminal from which the process was started */
+ rw_enter_read(&proctreelk);
if ((pr->ps_flags & PS_CONTROLT) &&
pr->ps_pgrp->pg_session->s_ttyp)
acct.ac_tty = pr->ps_pgrp->pg_session->s_ttyp->t_dev;
else
acct.ac_tty = -1;
+ rw_exit_read(&proctreelk);
/* (8) The boolean flags that tell how process terminated or
misbehaved. */
acct.ac_flag = pr->ps_acflag;
Index: kern/kern_exec.c
===
RCS file: /home/cvs/src/sys/kern/kern_exec.c,v
retrieving revision 1.230
diff -u -p -r1.230 kern_exec.c
--- kern/kern_exec.c22 Feb 2022 17:14:14 - 1.230
+++ kern/kern_exec.c27 Jun 2022 13:37:58 -
@@ -520,9 +520,11 @@ sys_execve(struct proc *p, void *v, regi
atomic_setbits_int(&pr->ps_flags, PS_EXEC);
if (pr->ps_flags & PS_PPWAIT) {
+ rw_enter_read(&proctreelk);
atomic_clearbits_int(&pr->ps_flags, PS_PPWAIT);
atomic_clearbits_int(&pr->ps_pptr->ps_flags, PS_ISPWAIT);
wakeup(pr->ps_pptr);
+ rw_exit_read(&proctreelk);
}
/*
Index: kern/kern_exit.c
===
RCS file: /home/cvs/src/sys/kern/kern_exit.c,v
retrieving revision 1.203
diff -u -p -r1.203 kern_exit.c
--- kern/kern_exit.c31 Mar 2022 01:41:22 - 1.203
+++ kern/kern_exit.c27 Jun 2022 13:37:58 -
@@ -154,6 +154,7 @@ exit1(struct proc *p, int xexit, int xsi
* is set; we wake up the parent early to avoid deadlock.
*/
if (pr->ps_flags & PS_PPWAIT) {
+ /* XXXPT `proctreelk` ? */
atomic_clearbits_int(&pr->ps_flags, PS_PPWAIT);
atomic_clearbits_int(&pr->ps_pptr->ps_flags,
PS_ISPWAIT);
@@ -222,6 +223,7 @@ exit1(struct proc *p, int xexit, int xsi
* If parent has the SAS_NOCLDWAIT flag set, we're not
* going to become a zombie.
*/
+ /* XXXPT `proctreelk` ? */
if (pr->ps_pptr->ps_sigacts->ps_sigflags & SAS_NOCLDWAIT)
atomic_setbits_int(&pr->ps_flags, PS_NOZOMBIE);
}
@@ -236,11 +238,7 @@ exit1(struct proc *p, int xexit, int xsi
* thread of a process that isn't PS_NOZOMBIE, we'll put
* the process on the zombprocess list below.
*/
- /*
-* NOTE: WE ARE NO LONGER ALLOWED TO SLEEP!
-*/
- p->p_stat = SDEAD;
-
+ rw_enter_write(&proctreelk);
LIST_REMOVE(p, p_hash);
LIST_REMOVE(p, p_list);
@@ -300,6 +298,7 @@ exit1(struct proc *p, int xexit, int xsi
process_clear_orphan(qr);
}
}
+ rw_exit_write(&proctreelk);
/* add thread's accumulated rusage into the process's total */
ruadd(rup, &p->p_ru);
@@ -325,9 +324,13 @@ exit1(struct proc *p, int xexit, int xsi
* wait4() to return ECHILD.
*/
if (pr->ps_flags & PS_NOZOMBIE) {
- struct process *ppr = pr->ps_pptr;
+ struct process *ppr;
+
+ rw_enter_write(&proctreelk);
+ ppr = pr->ps_pptr;
Re: Don't take kernel lock on pipex(4) pppoe input
On Sun, Jun 26 2022, Alexander Bluhm wrote:
> On Sun, Jun 26, 2022 at 11:26:13PM +0200, Jeremie Courreges-Anglas wrote:
>> On Mon, Jun 27 2022, Vitaliy Makkoveev wrote:
>> > On Sun, Jun 26, 2022 at 10:39:10PM +0200, Alexander Bluhm wrote:
>> >> On Sun, Jun 26, 2022 at 07:45:57PM +0300, Vitaliy Makkoveev wrote:
>> >> > This extra serialization is not required. In packet processing path we
>> >> > have shared netlock held, but we do read-only access on per session
>> >> > `flags' and `ifindex'. We always modify them from ioctl(2) path with
>> >> > exclusive netlock held. The rest of pipex(4) session is immutable or
>> >> > uses per-session locks.
>> >>
>> >> I was wondering about pipex_enable variable. It is documented as
>> >> protected by netlock.
>> >> net/pipex.c:int pipex_enable = 0; /* [N] */
>> >>
>> >> But I did not find NET_LOCK() in pipex_sysctl() or its callers.
>> >> Should we grab net lock there or in net_sysctl() in the write path?
>> >> Although only an integer is set atomically, it looks strange if
>> >> such a value is changed while we are in the middle of packet
>> >> processing.
>> >>
>> >
>> > I guess it should be declared as atomic. We use ifq_set_maxlen(...,1)
>> > hack so we don't cross netlock borders in pipex(4) output path, but it's
>> > expected we could cross them. We never check `pipex_enable' within
>> > (*if_qstart)() and we don't worry it's not serialized with the rest of
>> > stack. Also we will process already enqueued pipex(4) packets regardless
>> > on `pipex_enable' state.
>> >
>> > But this means we need to use local copy of `pipex_enable' within
>> > pppx_if_output(), otherwise we loose consistency.
>>
>> FWIW pipex_enable_local = READ_ONCE(pipex_enable) would prevent the
>> compiler from reading the global value twice.
>
> Or use atomic_load_int() to prevent over optimization.
READ_ONCE and atomic_load_int both use the volatile pointer dereference
trick to prevent reloading, so they should be equivalent for this use
case.
> I would prefer atomic_...() access for [A] variables.
... if you can live with the fact that some read accesses are done with
atomic_load_int and some are not, and write access isn't done with
atomic_store_int.
I'm not saying it's a problem in practice, just suggesting that
READ_ONCE() looks more appropriate here, you folks decide what suits you
best.
> bluhm
>
>> > Index: sys/net/if_pppx.c
>> > ===
>> > RCS file: /cvs/src/sys/net/if_pppx.c,v
>> > retrieving revision 1.116
>> > diff -u -p -r1.116 if_pppx.c
>> > --- sys/net/if_pppx.c 26 Jun 2022 15:50:21 - 1.116
>> > +++ sys/net/if_pppx.c 26 Jun 2022 21:14:02 -
>> > @@ -817,7 +817,9 @@ pppx_if_output(struct ifnet *ifp, struct
>> >struct pppx_if *pxi = (struct pppx_if *)ifp->if_softc;
>> >struct pppx_hdr *th;
>> >int error = 0;
>> > - int proto;
>> > + int pipex_enable_local, proto;
>> > +
>> > + pipex_enable_local = pipex_enable;
>> >
>> >NET_ASSERT_LOCKED();
>> >
>> > @@ -831,7 +833,7 @@ pppx_if_output(struct ifnet *ifp, struct
>> >if (ifp->if_bpf)
>> >bpf_mtap_af(ifp->if_bpf, dst->sa_family, m, BPF_DIRECTION_OUT);
>> > #endif
>> > - if (pipex_enable) {
>> > + if (pipex_enable_local) {
>> >switch (dst->sa_family) {
>> > #ifdef INET6
>> >case AF_INET6:
>> > @@ -856,7 +858,7 @@ pppx_if_output(struct ifnet *ifp, struct
>> >}
>> >*mtod(m, int *) = proto;
>> >
>> > - if (pipex_enable)
>> > + if (pipex_enable_local)
>> >error = if_enqueue(ifp, m);
>> >else {
>> >M_PREPEND(m, sizeof(struct pppx_hdr), M_DONTWAIT);
>> > Index: sys/net/pipex.c
>> > ===
>> > RCS file: /cvs/src/sys/net/pipex.c,v
>> > retrieving revision 1.138
>> > diff -u -p -r1.138 pipex.c
>> > --- sys/net/pipex.c26 Jun 2022 15:50:21 - 1.138
>> > +++ sys/net/pipex.c26 Jun 2022 21:14:02 -
>> > @@ -94,7 +94,7 @@ struct pool mppe_key_pool;
>> > * L pipex_list_mtx
>> > */
>> >
>> > -int pipex_enable = 0; /* [N] */
>> > +int pipex_enable = 0; /* [A] */
>> > struct pipex_hash_head
>> > pipex_session_list, /* [L] master session
>> > list */
>> > pipex_close_wait_list,/* [L] expired session
>> > list */
>> >
>>
>> --
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Don't take kernel lock on pipex(4) pppoe input
On Mon, Jun 27 2022, Vitaliy Makkoveev wrote:
> On Sun, Jun 26, 2022 at 10:39:10PM +0200, Alexander Bluhm wrote:
>> On Sun, Jun 26, 2022 at 07:45:57PM +0300, Vitaliy Makkoveev wrote:
>> > This extra serialization is not required. In packet processing path we
>> > have shared netlock held, but we do read-only access on per session
>> > `flags' and `ifindex'. We always modify them from ioctl(2) path with
>> > exclusive netlock held. The rest of pipex(4) session is immutable or
>> > uses per-session locks.
>>
>> I was wondering about pipex_enable variable. It is documented as
>> protected by netlock.
>> net/pipex.c:int pipex_enable = 0; /* [N] */
>>
>> But I did not find NET_LOCK() in pipex_sysctl() or its callers.
>> Should we grab net lock there or in net_sysctl() in the write path?
>> Although only an integer is set atomically, it looks strange if
>> such a value is changed while we are in the middle of packet
>> processing.
>>
>
> I guess it should be declared as atomic. We use ifq_set_maxlen(...,1)
> hack so we don't cross netlock borders in pipex(4) output path, but it's
> expected we could cross them. We never check `pipex_enable' within
> (*if_qstart)() and we don't worry it's not serialized with the rest of
> stack. Also we will process already enqueued pipex(4) packets regardless
> on `pipex_enable' state.
>
> But this means we need to use local copy of `pipex_enable' within
> pppx_if_output(), otherwise we loose consistency.
FWIW pipex_enable_local = READ_ONCE(pipex_enable) would prevent the
compiler from reading the global value twice.
> Index: sys/net/if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 if_pppx.c
> --- sys/net/if_pppx.c 26 Jun 2022 15:50:21 - 1.116
> +++ sys/net/if_pppx.c 26 Jun 2022 21:14:02 -
> @@ -817,7 +817,9 @@ pppx_if_output(struct ifnet *ifp, struct
> struct pppx_if *pxi = (struct pppx_if *)ifp->if_softc;
> struct pppx_hdr *th;
> int error = 0;
> - int proto;
> + int pipex_enable_local, proto;
> +
> + pipex_enable_local = pipex_enable;
>
> NET_ASSERT_LOCKED();
>
> @@ -831,7 +833,7 @@ pppx_if_output(struct ifnet *ifp, struct
> if (ifp->if_bpf)
> bpf_mtap_af(ifp->if_bpf, dst->sa_family, m, BPF_DIRECTION_OUT);
> #endif
> - if (pipex_enable) {
> + if (pipex_enable_local) {
> switch (dst->sa_family) {
> #ifdef INET6
> case AF_INET6:
> @@ -856,7 +858,7 @@ pppx_if_output(struct ifnet *ifp, struct
> }
> *mtod(m, int *) = proto;
>
> - if (pipex_enable)
> + if (pipex_enable_local)
> error = if_enqueue(ifp, m);
> else {
> M_PREPEND(m, sizeof(struct pppx_hdr), M_DONTWAIT);
> Index: sys/net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 pipex.c
> --- sys/net/pipex.c 26 Jun 2022 15:50:21 - 1.138
> +++ sys/net/pipex.c 26 Jun 2022 21:14:02 -
> @@ -94,7 +94,7 @@ struct pool mppe_key_pool;
> * L pipex_list_mtx
> */
>
> -int pipex_enable = 0; /* [N] */
> +int pipex_enable = 0; /* [A] */
> struct pipex_hash_head
> pipex_session_list, /* [L] master session
> list */
> pipex_close_wait_list, /* [L] expired session list */
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: M-lock __mp_lock implementation
On Sun, Jun 26 2022, Jeremie Courreges-Anglas wrote: > I noticed that our MI __mp_lock (kernel lock, sched lock) > implementation is based on a ticket lock without any backoff. > The downside of this algorithm is that it results in bus trafic increase > because all the actors are writing (lock releaser) and reading (lock > waiters) the same memory region from different cpus. Reducing > effectively that contention with a proportional backoff doesn't look > easy, since we have to target the minimum backoff time even though we > ignore how much time the lock will actually be held. > > Some algorithms (MCS, CLH, M-lock) avoid the sharing by building a queue > and letting waiters spin on a mostly private memory region. I initially > tried the MCS lock and it gave good results on amd64 and riscv64, until > it broke on arm64. Which kinda confirms what I read in various papers: > it looks simple but it's hard to get right. > > The M-lock implementation below is easy to understand and seems to give > better (or similar) results than both ticket lock and MCS on amd64, > arm64 and riscv64 machines. False sharing is partly avoided at the > expense of growing the data structure, which should be fine since the > __mp_lock is only used for the kernel and sched lock. > > I'm showing this off right now to see if there is interest. Benchmarks > results would help confirm that. I don't own big machines where it > would really shine. My test case for contention is building libc with max number of jobs: while sleep 1; do doas -u build make clean >/dev/null 2>&1; time doas -u build make -j4 >/dev/null 2>&1;done Here are some benchmarks, with help from tb@. Unfortunately I don't have the numbers for my riscv64 unmatched (currently offline, result was a slight improvement). robert@ showed some rather impressive result on a machine of his but it consisted of only one timing, before and after. Obviously such timings should be done on a mostly idle machine. 4 cores amd64 vm: cpu0: Intel(R) Xeon(R) CPU E3-1245 v5 @ 3.50GHz, 783.36 MHz, 06-5e-03 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,FSGSBASE,TSC_ADJUST,BMI1,HLE,AVX2,SMEP,BMI2,ERMS,INVPCID,RTM,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,UMIP,IBRS,IBPB,STIBP,SSBD,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES,MELTDOWN cpu0: 64KB 64b/line 2-way I-cache, 64KB 64b/line 2-way D-cache, 512KB 64b/line 16-way L2 cache cpu0: ITLB 255 4KB entries direct-mapped, 255 4MB entries direct-mapped cpu0: DTLB 255 4KB entries direct-mapped, 255 4MB entries direct-mapped cpu0: smt 0, core 0, package 0 -current: 2m01.57s real 2m41.80s user 4m27.86s system 2m00.40s real 2m41.01s user 4m27.05s system 2m00.67s real 2m41.05s user 4m27.61s system m-lock: 1m59.73s real 2m40.26s user 4m23.76s system 2m00.94s real 2m42.52s user 4m24.46s system 1m59.97s real 2m41.88s user 4m22.46s system 16 cores arm64 m1: mainbus0 at root: Apple Mac mini (M1, 2020) cpu0 at mainbus0 mpidr 0: Apple Icestorm r1p1 cpu0: 128KB 64b/line 8-way L1 VIPT I-cache, 64KB 64b/line 8-way L1 D-cache cpu0: 4096KB 128b/line 16-way L2 cache cpu0: TLBIOS+IRANGE,TS+AXFLAG,FHM,DP,SHA3,RDM,Atomic,CRC32,SHA2+SHA512,SHA1,AES+PMULL,SPECRES,SB,FRINTTS,GPI,LRCPC+LDAPUR,FCMA,JSCVT,API+PAC,DPB,SpecSEI,PAN+ATS1E1,LO,HPDS,CSV3,CSV2 -current: 1m55.93s real 2m05.46s user 8m27.08s system 1m56.90s real 2m06.11s user 8m27.11s system 1m56.17s real 2m06.42s user 8m24.11s system m-lock1.diff: 1m48.43s real 2m05.45s user 8m07.63s system 1m48.65s real 2m06.27s user 8m07.15s system 1m49.41s real 2m04.13s user 8m10.92s system 16 cores amd64 server: bios0: Dell Inc. Precision 3640 Tower cpu0: Intel(R) Core(TM) i7-10700K CPU @ 3.80GHz, 3691.40 MHz, 06-a5-05 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXS R,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,TSC_ADJUST,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,PKU,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,XSAVEC,XGETBV1,XSAVES cpu0: 256KB 64b/line 8-way L2 cache -current: 0m55.83s real 1m38.42s user 4m30.81s system 0m55.07s real 1m39.35s user 4m27.74s system 0m55.44s real 1m37.57s user 4m30.68s system m-lock1.diff: 0m53.06s real 1m39.42s user 4m12.92s system 0m52.83s real 1m37.94s user 4m12.33s
M-lock __mp_lock implementation
I noticed that our MI __mp_lock (kernel lock, sched lock)
implementation is based on a ticket lock without any backoff.
The downside of this algorithm is that it results in bus trafic increase
because all the actors are writing (lock releaser) and reading (lock
waiters) the same memory region from different cpus. Reducing
effectively that contention with a proportional backoff doesn't look
easy, since we have to target the minimum backoff time even though we
ignore how much time the lock will actually be held.
Some algorithms (MCS, CLH, M-lock) avoid the sharing by building a queue
and letting waiters spin on a mostly private memory region. I initially
tried the MCS lock and it gave good results on amd64 and riscv64, until
it broke on arm64. Which kinda confirms what I read in various papers:
it looks simple but it's hard to get right.
The M-lock implementation below is easy to understand and seems to give
better (or similar) results than both ticket lock and MCS on amd64,
arm64 and riscv64 machines. False sharing is partly avoided at the
expense of growing the data structure, which should be fine since the
__mp_lock is only used for the kernel and sched lock.
I'm showing this off right now to see if there is interest. Benchmarks
results would help confirm that. I don't own big machines where it
would really shine.
Index: kern/kern_lock.c
===
RCS file: /home/cvs/src/sys/kern/kern_lock.c,v
retrieving revision 1.72
diff -u -p -r1.72 kern_lock.c
--- kern/kern_lock.c26 Apr 2022 15:31:14 - 1.72
+++ kern/kern_lock.c26 Jun 2022 13:41:42 -
@@ -27,6 +27,7 @@
#include
+//#define MP_LOCKDEBUG
#ifdef MP_LOCKDEBUG
#ifndef DDB
#error "MP_LOCKDEBUG requires DDB"
@@ -80,16 +81,28 @@ _kernel_lock_held(void)
#ifdef __USE_MI_MPLOCK
-/* Ticket lock implementation */
+/*
+ * M-lock implementation from
+ * Design of a minimal-contention lock: M-lock Technical Report
+ * CS-PM-2018-08-19
+ *
https://homes.cs.ru.ac.za/philip/Publications/Techreports/2018/M-lock-report/M-lock-report.pdf
+ */
#include
void
___mp_lock_init(struct __mp_lock *mpl, const struct lock_type *type)
{
- memset(mpl->mpl_cpus, 0, sizeof(mpl->mpl_cpus));
- mpl->mpl_users = 0;
- mpl->mpl_ticket = 1;
+ int i;
+
+ memset(mpl, 0, sizeof(*mpl));
+ mpl->mpl_initial_lock.lockval = 0;
+ mpl->mpl_global_lock = &mpl->mpl_initial_lock;
+ for (i = 0; i < nitems(mpl->mpl_cpus); i++) {
+ mpl->mpl_cpus[i].lock_storage.lockval = 1;
+ mpl->mpl_cpus[i].mylock = &mpl->mpl_cpus[i].lock_storage;
+ mpl->mpl_cpus[i].spinon = &mpl->mpl_cpus[i].lock_storage;
+ }
#ifdef WITNESS
mpl->mpl_lock_obj.lo_name = type->lt_name;
@@ -105,7 +118,7 @@ ___mp_lock_init(struct __mp_lock *mpl, c
}
static __inline void
-__mp_lock_spin(struct __mp_lock *mpl, u_int me)
+__mp_lock_spin(struct __mp_lock *mpl, struct __mp_lock_cpu *lock)
{
struct schedstate_percpu *spc = &curcpu()->ci_schedstate;
#ifdef MP_LOCKDEBUG
@@ -113,7 +126,7 @@ __mp_lock_spin(struct __mp_lock *mpl, u_
#endif
spc->spc_spinning++;
- while (mpl->mpl_ticket != me) {
+ while (lock->spinon->lockval != 0) {
CPU_BUSY_CYCLE();
#ifdef MP_LOCKDEBUG
@@ -140,17 +153,27 @@ __mp_lock(struct __mp_lock *mpl)
#endif
s = intr_disable();
- if (cpu->mplc_depth++ == 0)
- cpu->mplc_ticket = atomic_inc_int_nv(&mpl->mpl_users);
+ if (cpu->mplc_depth++ == 0) {
+ cpu->spinon = atomic_swap_ptr(&mpl->mpl_global_lock,
+ cpu->mylock);
+ }
intr_restore(s);
- __mp_lock_spin(mpl, cpu->mplc_ticket);
+ __mp_lock_spin(mpl, cpu);
membar_enter_after_atomic();
WITNESS_LOCK(&mpl->mpl_lock_obj, LOP_EXCLUSIVE);
}
void
+__mp_lock_do_release(struct __mp_lock_cpu *lock)
+{
+ lock->mylock->lockval = 0;
+ lock->mylock = lock->spinon;
+ lock->mylock->lockval = 1; /* ready for next time */
+}
+
+void
__mp_unlock(struct __mp_lock *mpl)
{
struct __mp_lock_cpu *cpu = &mpl->mpl_cpus[cpu_number()];
@@ -168,7 +191,7 @@ __mp_unlock(struct __mp_lock *mpl)
s = intr_disable();
if (--cpu->mplc_depth == 0) {
membar_exit();
- mpl->mpl_ticket++;
+ __mp_lock_do_release(cpu);
}
intr_restore(s);
}
@@ -191,7 +214,7 @@ __mp_release_all(struct __mp_lock *mpl)
#endif
cpu->mplc_depth = 0;
membar_exit();
- mpl->mpl_ticket++;
+ __mp_lock_do_release(cpu);
intr_restore(s);
return (rv);
@@ -233,7 +256,7 @@ __mp_lock_held(struct __mp_lock *mpl, st
{
struct __mp_lock_cpu *cpu = &mpl->mpl_cpus[CPU_INFO_UNIT(ci)];
- return (cpu->mplc_ticket == mpl->mpl_ticket && cpu->mplc_depth > 0);
+ return (cpu->spinon->lockval == 0 && cpu->mplc_depth >
Re: Fix lock order reversal in nfs_inactive()
On Sun, Jun 19 2022, Visa Hankala wrote: > On Sun, Jun 19, 2022 at 11:05:38AM +0200, Jeremie Courreges-Anglas wrote: >> On Fri, Jun 17 2022, Jeremie Courreges-Anglas wrote: >> > On Thu, Jun 16 2022, Visa Hankala wrote: >> >> nfs_inactive() has a lock order reversal. When it removes the silly >> >> file, it locks the directory vnode while it already holds the lock >> >> of the argument file vnode. This clashes for example with name lookups >> >> where directory vnodes are locked before file vnodes. >> >> >> >> The reversal can cause a deadlock when an NFS client has multiple >> >> processes that create, modify and remove files in the same >> >> NFS directory. >> >> >> >> The following patch makes the silly file removal happen after >> >> nfs_inactive() has released the file vnode lock. This should be safe >> >> because the silly file removal is independent of nfs_inactive()'s >> >> argument vnode. >> >> The diff makes sense to me. Did you spot it reviewing the code, or >> using WITNESS? > > I noticed it by code review. > > WITNESS is somewhat helpless with vnode locks because they can involve > multiple levels of lock nesting. In fact, the order checking between > vnodes has been disabled by initializing the locks with RWL_IS_VNODE. > To fix this, the kernel would have to pass nesting information around > the filesystem code. > > This particular deadlock can be triggered for example by quickly > writing and removing temporary files in an NFS directory using one > process while another process lists the directory contents repeatedly. Dunno how many other problems you may have spotted, but this one looks obvious. Thanks and ok jca@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Fix lock order reversal in nfs_inactive()
On Sun, Jun 19 2022, Visa Hankala wrote: > On Sun, Jun 19, 2022 at 11:05:38AM +0200, Jeremie Courreges-Anglas wrote: >> On Fri, Jun 17 2022, Jeremie Courreges-Anglas wrote: >> > On Thu, Jun 16 2022, Visa Hankala wrote: >> >> nfs_inactive() has a lock order reversal. When it removes the silly >> >> file, it locks the directory vnode while it already holds the lock >> >> of the argument file vnode. This clashes for example with name lookups >> >> where directory vnodes are locked before file vnodes. >> >> >> >> The reversal can cause a deadlock when an NFS client has multiple >> >> processes that create, modify and remove files in the same >> >> NFS directory. >> >> >> >> The following patch makes the silly file removal happen after >> >> nfs_inactive() has released the file vnode lock. This should be safe >> >> because the silly file removal is independent of nfs_inactive()'s >> >> argument vnode. >> >> The diff makes sense to me. Did you spot it reviewing the code, or >> using WITNESS? > > I noticed it by code review. > > WITNESS is somewhat helpless with vnode locks because they can involve > multiple levels of lock nesting. In fact, the order checking between > vnodes has been disabled by initializing the locks with RWL_IS_VNODE. > To fix this, the kernel would have to pass nesting information around > the filesystem code. I see, thanks for confirming. > This particular deadlock can be triggered for example by quickly > writing and removing temporary files in an NFS directory using one > process while another process lists the directory contents repeatedly. > >> >> Could some NFS users test this? >> > >> > I'm running this diff on the riscv64 build cluster, since 1h25mn with no >> > hang. Let's see how it goes. >> >> This run did finish properly yesterday. >> >> > This cluster doesn't use NFS as much as it could (build logs stored >> > locally) but I can try that in the next build. >> >> So I have restarted a build with this diff and dpb(1) logging on an >> NFS-mounted /usr/ports/logs. I get a dpb(1) hang after 1400+ packages >> built. Any other attempt to access the NFS-mounted filesystem results >> in a hang. Let me know if I can extract more data from the system. > > No need this time. Those wait messages give some pointers. Another hang, slightly different excerpt below. >> shannon ~$ grep nfs riscv64/nfs-hang.txt >> 97293 72036 49335 0 30x91 nfs_fsync perl >> 69045 83700 64026 55 30x82 nfs_fsync c++ >> 80365 37354 15104 55 30x100082 nfs_fsync make >> 28876 139812 59322 55 30x100082 nfs_fsync make >> 6160 193238 61541 1000 30x13 nfsnode ksh >> 7535 421732 0 0 3 0x14280 nfsrcvlk nfsio >> 70437 237308 0 0 3 0x14280 nfsrcvlk nfsio >> 97073 406345 0 0 3 0x14200 nfsrcvlk nfsio >> 88487 390804 0 0 3 0x14200 nfsrcvlk nfsio >> 58945 91139 92962 0 30x80 nfsd nfsd >> 75619 357314 92962 0 30x80 nfsd nfsd >> 39027 137228 92962 0 30x80 nfsd nfsd >> 22028 406380 92962 0 30x80 nfsd nfsd >> 92962 11420 1 0 30x80 netconnfsd >> 90467 310188 0 0 3 0x14280 nfsrcvlk update shannon ~$ grep -e nfs riscv64/nfs-hang-2.txt 54340 24206 19639 1000 30x13 nfsnode ksh 43709 63222 55473 55 30x82 nfs_fsync cmake 27658 186996 47176 55 30x100082 nfs_fsync make 38397 435956 77676 55 30x82 nfsrcvlk c++ 37625 117156 96553 0 30x13 nfsnode ssh 96553 293049 2676 0 30x93 nfsrcvlk perl 72080 505789 0 0 3 0x14280 nfsrcvlk nfsio 68474 378836 0 0 3 0x14200 nfsrcvlk nfsio 98412 13475 0 0 3 0x14280 nfsrcvlk nfsio 41393 345501 0 0 3 0x14200 netio nfsio 75129 466227 20392 0 30x80 nfsd nfsd 7762 364728 20392 0 30x80 nfsd nfsd 65425 254516 20392 0 30x80 nfsd nfsd 8062 263249 20392 0 30x80 nfsd nfsd 20392 248868 1 0 30x80 netconnfsd 96499 295545 0 0 3 0x14280 nfsrcvlk update -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Fix lock order reversal in nfs_inactive()
On Fri, Jun 17 2022, Jeremie Courreges-Anglas wrote: > On Thu, Jun 16 2022, Visa Hankala wrote: >> nfs_inactive() has a lock order reversal. When it removes the silly >> file, it locks the directory vnode while it already holds the lock >> of the argument file vnode. This clashes for example with name lookups >> where directory vnodes are locked before file vnodes. >> >> The reversal can cause a deadlock when an NFS client has multiple >> processes that create, modify and remove files in the same >> NFS directory. >> >> The following patch makes the silly file removal happen after >> nfs_inactive() has released the file vnode lock. This should be safe >> because the silly file removal is independent of nfs_inactive()'s >> argument vnode. The diff makes sense to me. Did you spot it reviewing the code, or using WITNESS? >> Could some NFS users test this? > > I'm running this diff on the riscv64 build cluster, since 1h25mn with no > hang. Let's see how it goes. This run did finish properly yesterday. > This cluster doesn't use NFS as much as it could (build logs stored > locally) but I can try that in the next build. So I have restarted a build with this diff and dpb(1) logging on an NFS-mounted /usr/ports/logs. I get a dpb(1) hang after 1400+ packages built. Any other attempt to access the NFS-mounted filesystem results in a hang. Let me know if I can extract more data from the system. shannon ~$ grep nfs riscv64/nfs-hang.txt 97293 72036 49335 0 30x91 nfs_fsync perl 69045 83700 64026 55 30x82 nfs_fsync c++ 80365 37354 15104 55 30x100082 nfs_fsync make 28876 139812 59322 55 30x100082 nfs_fsync make 6160 193238 61541 1000 30x13 nfsnode ksh 7535 421732 0 0 3 0x14280 nfsrcvlk nfsio 70437 237308 0 0 3 0x14280 nfsrcvlk nfsio 97073 406345 0 0 3 0x14200 nfsrcvlk nfsio 88487 390804 0 0 3 0x14200 nfsrcvlk nfsio 58945 91139 92962 0 30x80 nfsd nfsd 75619 357314 92962 0 30x80 nfsd nfsd 39027 137228 92962 0 30x80 nfsd nfsd 22028 406380 92962 0 30x80 nfsd nfsd 92962 11420 1 0 30x80 netconnfsd 90467 310188 0 0 3 0x14280 nfsrcvlk update -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Fix lock order reversal in nfs_inactive()
On Thu, Jun 16 2022, Visa Hankala wrote: > nfs_inactive() has a lock order reversal. When it removes the silly > file, it locks the directory vnode while it already holds the lock > of the argument file vnode. This clashes for example with name lookups > where directory vnodes are locked before file vnodes. > > The reversal can cause a deadlock when an NFS client has multiple > processes that create, modify and remove files in the same > NFS directory. > > The following patch makes the silly file removal happen after > nfs_inactive() has released the file vnode lock. This should be safe > because the silly file removal is independent of nfs_inactive()'s > argument vnode. > > Could some NFS users test this? I'm running this diff on the riscv64 build cluster, since 1h25mn with no hang. Let's see how it goes. This cluster doesn't use NFS as much as it could (build logs stored locally) but I can try that in the next build. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: speaker(4): unhook driver and manpage from build
On Thu, Apr 28 2022, Scott Cheloha wrote: > speaker(4) is a whimsical thing, but I don't think we should have a > dedicated chiptune interpreter in the kernel. > This patch unhooks the driver and the manpage from the build. The > driver is built for alpha, amd64, and i386. > > A subsequent patch will move all relevant files to the attic and clean > up manpage cross references. > > Nothing in base or xenocara includes . > > I see a couple SPKRTONE and SPKRTUNE symbols in ports, but I imagine > those ports don't use the symbols if they are missing. > > ok? People seem to find this useful, for real world use cases. Is there a technical reason to delete it besides "this doesn't belong here"? -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: ddb: simplify "machine" command handling
On Mon, Apr 11 2022, Christian Weisgerber wrote: > Christian Weisgerber: > >> This will allow constifying the ddb command tables in a subsequent >> step. > > And here's that boring follow-up diff. sparc64 and riscv64 GENERIC and GENERIC.MP build successfully with this diff and the previous one, "machine" behaves as expected in ddb(4). ok jca@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: gpio: Add missing device_unref()
On Sun, Apr 10 2022, Visa Hankala wrote: > Make gpio(4) release the device reference that device_lookup() takes. > > OK? ok jca@ -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: [PATCH] [src] usr.bin/sendbug/sendbug.c - update categories
On Tue, Mar 29 2022, Theo Buehler wrote:
> On Tue, Mar 29, 2022 at 08:51:25PM +0200, Jeremie Courreges-Anglas wrote:
>> On Tue, Mar 29 2022, Raf Czlonka wrote:
>> > Hello,
>> >
>> > sparc and vax ports have been retired a while back; add riscv64
>> > while there.
>>
>> Committed, thanks.
>>
>> There are more missing entries I think. If I follow the existing
>> pattern, naming the cpu architectures and not the platforms:
>> - aarch64 is missing
>> - powerpc64 is missing
>> - mips64el is missing
>
> Perhaps we can ditch the architectures list that seems to have been out
> of sync more often than not
For now I have synced the architectures list.
> and reduce categories to something like
> these?
>
> documentation userland kernel
>
> If somebody is smart enough to narrow a bug down to something
> arch-specific, they're hopefully smart enough to mention that in their
> bug report.
I think such a move would make sense but I'm unsure about the details.
Removing "library" may not be a problem. But removing "system" means
that users have to think hard whether the problem lies in kernel or
userland, when the problem may lie between both, or generally be hard to
classify.
Thoughts?
>> and after folding the line because of line wrapping:
>>
>> ok?
>>
>>
>> Index: sendbug.c
>> ===
>> RCS file: /cvs/src/usr.bin/sendbug/sendbug.c,v
>> retrieving revision 1.79
>> diff -u -p -r1.79 sendbug.c
>> --- sendbug.c29 Mar 2022 18:44:12 - 1.79
>> +++ sendbug.c29 Mar 2022 18:49:41 -
>> @@ -44,7 +44,8 @@ void template(FILE *);
>> voidusbdevs(FILE *);
>>
>> const char *categories = "system user library documentation kernel "
>> -"alpha amd64 arm hppa i386 m88k mips64 powerpc riscv64 sh sparc64";
>> +"alpha aarch64 amd64 arm hppa i386 m88k mips64 mips64el powerpc
>> powerpc64 "
>> +"riscv64 sh sparc64";
>> const char *comment[] = {
>> "",
>> "",
>>
>> --
>> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: [diff] gzip verbose message
On Wed, Apr 06 2022, prx wrote:
> Since gzip has -k flag to keep original file, the output when used with -v
> flag is wrong.
>
> Find below a stupid diff almost shorter than this message.
>
> Regards
>
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/compress/main.c,v
> retrieving revision 1.99
> diff -u -r1.99 main.c
> --- main.c14 Mar 2022 21:52:08 - 1.99
> +++ main.c6 Apr 2022 12:49:51 -
> @@ -932,7 +932,7 @@
> return;
> }
> if (!pipin) {
> - fprintf(stderr, "\t%4.1f%% -- replaced with %s\n",
> + fprintf(stderr, "\t%4.1f%% -- compressed to %s\n",
> (uncompressed - compressed) * 100.0 / uncompressed, file);
> }
> compressed += hlen;
>
The diff below keeps the same output by default and outputs the same as
GNU gzip(1) is -k is given.
ok?
Index: main.c
===
RCS file: /home/cvs/src/usr.bin/compress/main.c,v
retrieving revision 1.99
diff -u -p -p -u -r1.99 main.c
--- main.c 14 Mar 2022 21:52:08 - 1.99
+++ main.c 10 Apr 2022 11:10:43 -
@@ -53,6 +53,7 @@
enum program_mode pmode;
int cat, decomp, pipin, force, verbose, testmode, list, recurse, storename;
+int kflag;
extern char *__progname;
const struct compressor {
@@ -167,12 +168,12 @@ main(int argc, char *argv[])
const char *optstr, *s;
char *p, *infile;
char outfile[PATH_MAX], _infile[PATH_MAX], suffix[16];
- int bits, ch, error, rc, cflag, kflag, oflag;
+ int bits, ch, error, rc, cflag, oflag;
if (pledge("stdio rpath wpath cpath fattr chown", NULL) == -1)
err(1, "pledge");
- bits = cflag = kflag = oflag = 0;
+ bits = cflag = oflag = 0;
storename = -1;
p = __progname;
if (p[0] == 'g') {
@@ -932,8 +933,9 @@ verbose_info(const char *file, off_t com
return;
}
if (!pipin) {
- fprintf(stderr, "\t%4.1f%% -- replaced with %s\n",
- (uncompressed - compressed) * 100.0 / uncompressed, file);
+ fprintf(stderr, "\t%4.1f%% -- %s %s\n",
+ (uncompressed - compressed) * 100.0 / uncompressed,
+ kflag ? "created" : "replaced with", file);
}
compressed += hlen;
fprintf(stderr, "%lld bytes in, %lld bytes out\n",
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: [PATCH] [src] usr.bin/sendbug/sendbug.c - update categories
On Tue, Mar 29 2022, Raf Czlonka wrote:
> Hello,
>
> sparc and vax ports have been retired a while back; add riscv64
> while there.
Committed, thanks.
There are more missing entries I think. If I follow the existing
pattern, naming the cpu architectures and not the platforms:
- aarch64 is missing
- powerpc64 is missing
- mips64el is missing
and after folding the line because of line wrapping:
ok?
Index: sendbug.c
===
RCS file: /cvs/src/usr.bin/sendbug/sendbug.c,v
retrieving revision 1.79
diff -u -p -r1.79 sendbug.c
--- sendbug.c 29 Mar 2022 18:44:12 - 1.79
+++ sendbug.c 29 Mar 2022 18:49:41 -
@@ -44,7 +44,8 @@ void template(FILE *);
void usbdevs(FILE *);
const char *categories = "system user library documentation kernel "
-"alpha amd64 arm hppa i386 m88k mips64 powerpc riscv64 sh sparc64";
+"alpha aarch64 amd64 arm hppa i386 m88k mips64 mips64el powerpc powerpc64 "
+"riscv64 sh sparc64";
const char *comment[] = {
"",
"",
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
riscv64: chatty SIGILL printf
Just like breakpoints, SIGILL shouldn't print anything. FWIW this seems
to only happen once in a ports bulk build.
ok?
Index: trap.c
===
RCS file: /home/cvs/src/sys/arch/riscv64/riscv64/trap.c,v
retrieving revision 1.17
diff -u -p -p -u -r1.17 trap.c
--- trap.c 3 Sep 2021 14:58:25 - 1.17
+++ trap.c 22 Mar 2022 21:42:23 -
@@ -153,8 +153,6 @@ do_trap_user(struct trapframe *frame)
fpu_load(p);
break;
}
- printf("ILL at %lx scause %lx stval %lx\n", frame->tf_sepc,
- frame->tf_scause, frame->tf_stval);
sv.sival_ptr = (void *)frame->tf_stval;
trapsignal(p, SIGILL, 0, ILL_ILLTRP, sv);
break;
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: fix multiple iwm/iwx interfaces
On Mon, Mar 14 2022, Stefan Sperling wrote:
> It is currently impossible to use more than one iwm or iwx interface
> in a system because I don't understand C.
>
> Trying to bring up an uninitialized interface anyway results in a
> kernel panic ("bogus channel pointer" from net80211), so prevent
> the device from being used in case we never managed to initialize it.
>
> ok?
I only tested iwm(4) 8265 but the change makes sense, ok jca@
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: add -k / --keep for gzip(1)
On Sat, Mar 12 2022, Solene Rapenne wrote:
> On Sat, 12 Mar 2022 15:49:40 +0100
> Solene Rapenne :
>
>> On Sat, 05 Mar 2022 19:15:02 -0700
>> "Todd C. Miller" :
>>
>> > On Sun, 06 Mar 2022 02:58:30 +0100, Jeremie Courreges-Anglas wrote:
>> >
>> > > I'm not sure what you mean here. Solene's diff added -k to both
>> > > compress(1) and gzip(1) (and their uncompressor counterparts).
>> > > Adding -k to gzip/gunzip only would indeed make the usage() slightly
>> > > more complicated.
>> > >
>> > > So do we want to add -k to compress(1) or not? (No strong opinion.)
>> > >
>> > > I like the general idea and the diff seems to work as intended.
>> >
>> > I don't really care whether or not we add -k to compress(1). However,
>> > since other versions of compress do not have a -k option it is
>> > probably best to avoiding adding it.
>> >
>> > - todd
>>
>> here is a new version that doesn't add -k to compress and uncompress
>>
>> gzip -k and gunzip -k works as expected
>> compress -k and uncompress -k will display unknown option -- k and
>> show the usage that doesn't show k flag
>
> as millert@ told me, I forgot to update the usage, new diff
Works fine. Here's an updated diff with suggestions:
- "k" was not completely removed from compress's struct compressor opt
string, and was not needed in null_method
- try to keep the *flag variables ordered
- rework the condition that decides whether we unlink and print
a warning message. I found the existing code not much readable with
a chain of flags check, a syscall and another flag check.
Also the new line was over 80 chars.
- adding "k" to usage can be made shorter since "L" is next to "k"
If you like those changes, ok jca@
Index: gzip.1
===
RCS file: /home/cvs/src/usr.bin/compress/gzip.1,v
retrieving revision 1.14
diff -u -p -r1.14 gzip.1
--- gzip.1 7 Oct 2014 21:06:30 - 1.14
+++ gzip.1 13 Mar 2022 11:47:00 -
@@ -43,13 +43,13 @@
.Nd compress and expand data (deflate mode)
.Sh SYNOPSIS
.Nm gzip
-.Op Fl 123456789cdfhLlNnOqrtVv
+.Op Fl 123456789cdfhkLlNnOqrtVv
.Op Fl b Ar bits
.Op Fl o Ar filename
.Op Fl S Ar suffix
.Op Ar
.Nm gunzip
-.Op Fl cfhLlNnqrtVv
+.Op Fl cfhkLlNnqrtVv
.Op Fl o Ar filename
.Op Ar
.Nm gzcat
@@ -183,6 +183,8 @@ behave as
.Xr cat 1 .
.It Fl h
Print a short help message.
+.It Fl k
+Keep input files after compression or decompression.
.It Fl L
A no-op which exists for compatibility only.
On GNU gzip, it displays the program's license.
Index: main.c
===
RCS file: /home/cvs/src/usr.bin/compress/main.c,v
retrieving revision 1.98
diff -u -p -r1.98 main.c
--- main.c 18 Jan 2021 00:46:58 - 1.98
+++ main.c 13 Mar 2022 12:04:05 -
@@ -75,8 +75,8 @@ const struct compressor {
"deflate",
".gz",
"\037\213",
- "123456789ab:cdfhLlNnOo:qrS:tVv",
- "cfhLlNno:qrtVv",
+ "123456789ab:cdfhkLlNnOo:qrS:tVv",
+ "cfhkLlNno:qrtVv",
"fhqr",
gz_ropen,
gz_read,
@@ -141,6 +141,7 @@ const struct option longopts[] = {
{ "uncompress", no_argument,0, 'd' },
{ "force", no_argument,0, 'f' },
{ "help", no_argument,0, 'h' },
+ { "keep", no_argument,0, 'k' },
{ "list", no_argument,0, 'l' },
{ "license",no_argument,0, 'L' },
{ "no-name",no_argument,0, 'n' },
@@ -166,12 +167,12 @@ main(int argc, char *argv[])
const char *optstr, *s;
char *p, *infile;
char outfile[PATH_MAX], _infile[PATH_MAX], suffix[16];
- int bits, ch, error, rc, cflag, oflag;
+ int bits, ch, error, rc, cflag, kflag, oflag;
if (pledge("stdio rpath wpath cpath fattr chown", NULL) == -1)
err(1, "pledge");
- bits = cflag = oflag = 0;
+ bits = cflag = kflag = oflag = 0;
storename = -1;
p = __progname;
if (p[0] == 'g') {
@@ -276,6 +277,9 @@ main(int argc, char *argv[])
strlcpy(suffix, method->suffix, sizeof(suffix));
bits = 6;
Re: riscv64: ld.lld is too picky on ABI mismatch
On Mon, Oct 25 2021, Jeremie Courreges-Anglas wrote:
> On Mon, Oct 25 2021, Patrick Wildt wrote:
>> Am Mon, Oct 25, 2021 at 11:43:55AM +0200 schrieb Mark Kettenis:
>>> > From: Jeremie Courreges-Anglas
>>> > Date: Sun, 24 Oct 2021 17:30:46 +0100
>>> >
>>> > clang(1) defaults to FP ABI but ld(1) -melf64lriscv doesn't. This is
>>> > a problem as soon as a port tries to use raw ld(1) -b binary to embed
>>> > data in a .o file.
>>> >
>>> > Downgrading this hard error to a warning seems more useful as far as the
>>> > ports tree is concerned. The diff below fixes
>>> > databases/postgresql-pllua and should also fix textproc/mupdf and
>>> > net/utox.
>>> >
>>> > There's another diff here: https://reviews.llvm.org/D106378
>>> > but it's slightly more complicated and it received seemingly negative
>>> > feedback. So I'm just using a minimal change to fit our needs.
>>> >
>>> > ok?
>>>
>>> I think we should try to avoid deviating from upstream as much as
>>> possible. And I agree with the folks who argue that the mismatching
>>> objects are created with the wrong tools.
>
> Well the only alternative they suggest is using assembly and .incbin.
> Maybe that works for the FreeBSD folks working on low-level stuff, but
> not so much as a general purpose tool to include binary data in builds.
>
>>> But if mortimer@ and
>>> patrick@ can deal with carrying this local modification I won't
>>> object.
>
> I can't speak for them but given the local changes we have in clang and
> lld land this two lines diff doesn't seem daunting. ;)
>
>> Well, I was about to send an LLVM 13 diff... so can we revisit this diff
>> when we're updated to LLVM 13?
I have added comments on https://reviews.llvm.org/D106378#3219454 but
the discussion has stalled again (I'll try to revive it).
I'm still using the diff below on the riscv64 ports build machines, and
I'd like to get rid of the burden of keeping it in my tree.
ok?
Index: ELF/Arch/RISCV.cpp
===
RCS file: /cvs/src/gnu/llvm/lld/ELF/Arch/RISCV.cpp,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 RISCV.cpp
--- ELF/Arch/RISCV.cpp 17 Dec 2021 12:25:02 - 1.1.1.3
+++ ELF/Arch/RISCV.cpp 28 Jan 2022 09:11:18 -
@@ -124,8 +124,8 @@ uint32_t RISCV::calcEFlags() const {
target |= EF_RISCV_RVC;
if ((eflags & EF_RISCV_FLOAT_ABI) != (target & EF_RISCV_FLOAT_ABI))
- error(toString(f) +
-": cannot link object files with different floating-point ABI");
+ warn(toString(f) +
+": linking object files with different floating-point ABI");
if ((eflags & EF_RISCV_RVE) != (target & EF_RISCV_RVE))
error(toString(f) +
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Add kernel stack trace saving for riscv64
On Tue, Mar 08 2022, Visa Hankala wrote:
> This patch adds kernel stack trace saving for riscv64, for the benefit
> of dt(4) and witness(4).
Nice!
> The unwinder is slow because of the symbol
> lookup, but this can be tweaked later.
A dumb approach that appears to work: add
cpu_exception_handler_supervisor_end and cpu_exception_handler_user_end
symbols, and perform a range check.
> The limit variable prevents the unwinder from using user-controllable
> register values. The limit has to reflect the kernel stack setup in
> cpu_fork(). To ensure consistency, the stack start address is stored
> in a variable in struct pcb.
>
> OK?
Works for me on the Unmatched with dt(4), thanks. ok jca@ fwiw
> Index: arch/riscv64/include/pcb.h
> ===
> RCS file: src/sys/arch/riscv64/include/pcb.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 pcb.h
> --- arch/riscv64/include/pcb.h30 Jun 2021 22:20:56 - 1.3
> +++ arch/riscv64/include/pcb.h8 Mar 2022 16:54:58 -
> @@ -39,5 +39,6 @@ struct pcb {
>
> caddr_t pcb_onfault;// On fault handler
> struct fpregpcb_fpstate;// Floating Point state */
> + register_t pcb_kstack; /* kernel stack address */
> };
> #endif /* _MACHINE_PCB_H_ */
> Index: arch/riscv64/riscv64/db_trace.c
> ===
> RCS file: src/sys/arch/riscv64/riscv64/db_trace.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 db_trace.c
> --- arch/riscv64/riscv64/db_trace.c 22 Feb 2022 07:46:04 - 1.5
> +++ arch/riscv64/riscv64/db_trace.c 8 Mar 2022 16:54:58 -
> @@ -141,3 +141,56 @@ db_stack_trace_print(db_expr_t addr, int
> }
> (*pr)("end trace frame: 0x%lx, count: %d\n", frame, count);
> }
> +
> +void
> +stacktrace_save_at(struct stacktrace *st, unsigned int skip)
> +{
> + struct callframe *frame, *lastframe, *limit;
> + struct pcb *pcb = curpcb;
> + Elf_Sym *sym;
> + db_expr_t diff;
> + vaddr_t ra, subr;
> +
> + st->st_count = 0;
> +
> + if (pcb == NULL)
> + return;
> +
> + ra = (vaddr_t)__builtin_return_address(0);
> + frame = (struct callframe *)__builtin_frame_address(0);
> + KASSERT(INKERNEL(frame));
> + limit = (struct callframe *)((struct trapframe *)pcb->pcb_kstack - 1);
> +
> + while (st->st_count < STACKTRACE_MAX) {
> + if (skip == 0)
> + st->st_pc[st->st_count++] = ra;
> + else
> + skip--;
> +
> + sym = db_search_symbol(ra, DB_STGY_PROC, &diff);
> + if (sym == NULL)
> + break;
> + subr = ra - (vaddr_t)diff;
> +
> + lastframe = frame;
> + if (subr == (vaddr_t)cpu_exception_handler_supervisor ||
> + subr == (vaddr_t)cpu_exception_handler_user) {
> + struct trapframe *tf = (struct trapframe *)frame;
> +
> + frame = (struct callframe *)tf->tf_s[0];
> + ra = tf->tf_ra;
> + } else {
> + frame = frame[-1].f_frame;
> + if (frame == NULL)
> + break;
> + ra = frame[-1].f_ra;
> + }
> +
> + if (frame <= lastframe)
> + break;
> + if (frame >= limit)
> + break;
> + if (!INKERNEL(ra))
> + break;
> + }
> +}
> Index: arch/riscv64/riscv64/vm_machdep.c
> ===
> RCS file: src/sys/arch/riscv64/riscv64/vm_machdep.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 vm_machdep.c
> --- arch/riscv64/riscv64/vm_machdep.c 24 Feb 2022 14:19:10 - 1.10
> +++ arch/riscv64/riscv64/vm_machdep.c 8 Mar 2022 16:54:58 -
> @@ -75,13 +75,12 @@ cpu_fork(struct proc *p1, struct proc *p
>
> pmap_activate(p2);
>
> - tf = (struct trapframe *)((u_long)p2->p_addr
> + pcb->pcb_kstack = STACKALIGN((u_long)p2->p_addr
> + USPACE
> - - sizeof(struct trapframe)
> - sizeof(register_t)/* for holding curcpu */
> - 0x10);
>
> - tf = (struct trapframe *)STACKALIGN(tf);
> + tf = (struct trapframe *)pcb->pcb_kstack - 1;
> pcb->pcb_tf = tf;
> *tf = *p1->p_addr->u_pcb.pcb_tf;
>
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: add -k / --keep for gzip(1)
On Thu, Mar 03 2022, Todd C. Miller wrote:
> On Thu, 03 Mar 2022 15:11:13 +, Miod Vallat wrote:
>
>> > I think this makes sense if only for better GNU gzip compatibility.
>> > OK millert@
>>
>> But does the `-k' flag needs to be added to compress(1) too?
>
> No, it just makes usage() slightly more complicated.
> But that diff was missing an update to usage() anyway.
I'm not sure what you mean here. Solene's diff added -k to both
compress(1) and gzip(1) (and their uncompressor counterparts).
Adding -k to gzip/gunzip only would indeed make the usage() slightly
more complicated.
So do we want to add -k to compress(1) or not? (No strong opinion.)
I like the general idea and the diff seems to work as intended.
> Index: compress.1
> ===
> RCS file: /home/reposync/src/usr.bin/compress/compress.1,v
> retrieving revision 1.48
> diff -u -p -r1.48 compress.1
> --- compress.117 Mar 2014 14:23:50 - 1.48
> +++ compress.13 Mar 2022 12:08:01 -
> @@ -44,13 +44,13 @@
> .Nd compress and expand data (compress mode)
> .Sh SYNOPSIS
> .Nm compress
> -.Op Fl 123456789cdfghlNnOqrtv
> +.Op Fl 123456789cdfghklNnOqrtv
> .Op Fl b Ar bits
> .Op Fl o Ar filename
> .Op Fl S Ar suffix
> .Op Ar
> .Nm uncompress
> -.Op Fl cfhlNnqrtv
> +.Op Fl cfhklNnqrtv
> .Op Fl o Ar filename
> .Op Ar
> .Nm zcat
> @@ -192,6 +192,8 @@ Use the deflate scheme, which reportedly
> mode).
> .It Fl h
> Print a short help message.
> +.It Fl k
> +Keep input files after compression or decompression.
> .It Fl l
> List information for the specified compressed files.
> The following information is listed:
> Index: gzip.1
> ===
> RCS file: /home/reposync/src/usr.bin/compress/gzip.1,v
> retrieving revision 1.14
> diff -u -p -r1.14 gzip.1
> --- gzip.17 Oct 2014 21:06:30 - 1.14
> +++ gzip.13 Mar 2022 12:08:21 -
> @@ -43,13 +43,13 @@
> .Nd compress and expand data (deflate mode)
> .Sh SYNOPSIS
> .Nm gzip
> -.Op Fl 123456789cdfhLlNnOqrtVv
> +.Op Fl 123456789cdfhkLlNnOqrtVv
> .Op Fl b Ar bits
> .Op Fl o Ar filename
> .Op Fl S Ar suffix
> .Op Ar
> .Nm gunzip
> -.Op Fl cfhLlNnqrtVv
> +.Op Fl cfhkLlNnqrtVv
> .Op Fl o Ar filename
> .Op Ar
> .Nm gzcat
> @@ -183,6 +183,8 @@ behave as
> .Xr cat 1 .
> .It Fl h
> Print a short help message.
> +.It Fl k
> +Keep input files after compression or decompression.
> .It Fl L
> A no-op which exists for compatibility only.
> On GNU gzip, it displays the program's license.
> Index: main.c
> ===
> RCS file: /home/reposync/src/usr.bin/compress/main.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 main.c
> --- main.c18 Jan 2021 00:46:58 - 1.98
> +++ main.c3 Mar 2022 12:00:28 -
> @@ -75,8 +75,8 @@ const struct compressor {
> "deflate",
> ".gz",
> "\037\213",
> - "123456789ab:cdfhLlNnOo:qrS:tVv",
> - "cfhLlNno:qrtVv",
> + "123456789ab:cdfhkLlNnOo:qrS:tVv",
> + "cfhkLlNno:qrtVv",
> "fhqr",
> gz_ropen,
> gz_read,
> @@ -92,8 +92,8 @@ const struct compressor {
> "compress",
> ".Z",
> "\037\235",
> - "123456789ab:cdfghlNnOo:qrS:tv",
> - "cfhlNno:qrtv",
> + "123456789ab:cdfghklNnOo:qrS:tv",
> + "cfhklNno:qrtv",
> "fghqr",
> z_ropen,
> zread,
> @@ -110,8 +110,8 @@ const struct compressor null_method = {
> "null",
> ".nul",
> "XX",
> - "123456789ab:cdfghlNnOo:qrS:tv",
> - "cfhlNno:qrtv",
> + "123456789ab:cdfghklNnOo:qrS:tv",
> + "cfhklNno:qrtv",
> "fghqr",
> null_ropen,
> null_read,
> @@ -141,6 +141,7 @@ const struct option longopts[] = {
> { "uncompress", no_argument,0, 'd' },
> { "force", no_argument,0, 'f' },
> { "help", no_argument,0, 'h' },
> + { "keep", no_argument,0, 'k' },
> { "list", no_argument,0, 'l' },
> { "license",no_argument,0, 'L' },
> { "no-name",no_argument,0, 'n' },
> @@ -166,12 +167,12 @@ main(int argc, char *argv[])
> const char *optstr, *s;
> char *p, *infile;
> char outfile[PATH_MAX], _infile[PATH_MAX], suffix[16];
> - int bits, ch, error, rc, cflag, oflag;
> + int bits, ch, error, rc, cflag, oflag, kflag;
>
> if (pledge("stdio rpath wpath cpath fattr chown", NULL) == -1)
> err(1, "pledge");
>
> - bits = cflag = oflag = 0;
> + bits = cflag = oflag = kflag = 0;
> storename = -1;
> p = __progname;
> if (p[0] == 'g') {
> @@ -276,6 +277,9 @@ main(int argc, char *argv[])
> st
Re: add openvpn 1194/udp/tcp port to /etc/services
On Tue, Mar 01 2022, Landry Breuil wrote: > Hi, > > while looking at other things i noticed OpenVPN wasnt in /etc/services. > > apparently its listed/registered by IANA since 2004 (i know, not a reason), > and > i have it on a debian 11 box and on a FreeBSD 12.2 server. > > i see two reasons: > - making sure another service doesnt squat the port for outgoing cnx, since > in some configs openvpn is started manually after boot > - write nicer pf rules :) > > oks ? yup (net/openvpn maintainer here) > Index: services > === > RCS file: /cvs/src/etc/services,v > retrieving revision 1.103 > diff -u -r1.103 services > --- services 2 Sep 2021 10:46:22 - 1.103 > +++ services 1 Mar 2022 10:18:08 - > @@ -173,6 +173,8 @@ > pop3s995/tcp spop3 # pop3 protocol over > TLS/SSL > socks1080/tcp# Socks > kpop 1109/tcp# Pop with Kerberos > +openvpn 1194/tcp# OpenVPN > +openvpn 1194/udp# OpenVPN > ms-sql-s 1433/tcpMicrosoft-SQL-Server > ms-sql-s 1433/udpMicrosoft-SQL-Server > ms-sql-m 1434/tcpMicrosoft-SQL-Monitor > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: lldb(1) libedit support
On Tue, Feb 15 2022, Greg Steuck wrote: > Andrei writes: > >> The lldb(1) debugger was recently added in base and as I was playing around >> with it I noticed the lack of line editing functionality. >> >> This is because currently lldb is built without support for >> libedit. It would be nice to have libedit in base such to be able to >> link lldb against it, as the lack of line editing functionality makes >> it difficult to use. > > libedit is in base and gnu/usr.bin/clang/lldb/Makefile even links > lldb against libedit: > : LDADD+= -lcurses -ledit -lpanel > > Curiously, trying to enable libedit support in lldb with the following > patch didn't seem to help (arrow keys still show ^[[A and such). > > modified gnu/usr.bin/clang/include/lldb/Host/Config.h > @@ -41,7 +41,7 @@ > > #define CURSES_HAVE_NCURSES_CURSES_H 0 > > -#define LLDB_ENABLE_LIBEDIT 0 > +#define LLDB_ENABLE_LIBEDIT 1 > > #define LLDB_ENABLE_LIBXML2 0 Here's what the devel/llvm cmake build system defines. Editing in emacs mode (default) seems to work but I have tested very lightly so far. Works for you? ok? diff --git gnu/usr.bin/clang/include/lldb/Host/Config.h gnu/usr.bin/clang/include/lldb/Host/Config.h index 184c61330a6..3a07fd60667 100644 --- gnu/usr.bin/clang/include/lldb/Host/Config.h +++ gnu/usr.bin/clang/include/lldb/Host/Config.h @@ -9,9 +9,9 @@ #ifndef LLDB_HOST_CONFIG_H #define LLDB_HOST_CONFIG_H -#define LLDB_EDITLINE_USE_WCHAR 0 +#define LLDB_EDITLINE_USE_WCHAR 1 -#define LLDB_HAVE_EL_RFUNC_T 0 +#define LLDB_HAVE_EL_RFUNC_T 1 #define HAVE_SYS_TYPES_H 1 @@ -37,11 +37,11 @@ #define LLDB_ENABLE_LZMA 0 -#define LLDB_ENABLE_CURSES 0 +#define LLDB_ENABLE_CURSES 1 #define CURSES_HAVE_NCURSES_CURSES_H 0 -#define LLDB_ENABLE_LIBEDIT 0 +#define LLDB_ENABLE_LIBEDIT 1 #define LLDB_ENABLE_LIBXML2 0 -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
ubsan_minimal documentation hint (was: Re: Ship ubsan_minimal library in base?)
On Tue, Feb 15 2022, Jeremie Courreges-Anglas wrote: [...] >>> Index: clang-local.1 >>> === >>> RCS file: /home/cvs/src/share/man/man1/clang-local.1,v >>> retrieving revision 1.22 >>> diff -u -p -p -u -r1.22 clang-local.1 >>> --- clang-local.1 7 Sep 2021 17:39:49 - 1.22 >>> +++ clang-local.1 5 Feb 2022 17:11:48 - >>> @@ -93,6 +93,13 @@ option to treat signed integer overflows >>> prevent dangerous optimizations which could remove security critical >>> overflow >>> checks. >>> .It >>> +Only ubsan_minimal support is shipped by the base system. >>> +To make use of it, pass >>> +.Nm clang >>> +the following options: >>> +.Fl fsanitize=undefined >>> +.Fl fsanitize-minimal-runtime . > > I'm not 100% happy with this wording, I'll try to rework it. Maybe something like this? The idea is to point people to ubsan_minimal instead of assuming that ubsan isn't supported at all. I can't see a better place to document this. For more context: UBSan is supposed to catch undefined behavior at runtime and to react by printing an error message and/or aborting, etc. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html Greg, still ok with this version? Thoughts? Index: clang-local.1 === RCS file: /home/cvs/src/share/man/man1/clang-local.1,v retrieving revision 1.22 diff -u -p -p -u -r1.22 clang-local.1 --- clang-local.1 7 Sep 2021 17:39:49 - 1.22 +++ clang-local.1 16 Feb 2022 22:37:05 - @@ -93,6 +93,12 @@ option to treat signed integer overflows prevent dangerous optimizations which could remove security critical overflow checks. .It +The base system ships support for the ubsan_minimal sanitizer runtime +but not for the default ubsan runtime. +See the documentation for the +.Fl fsanitize-minimal-runtime +flag. +.It The .Xr malloc 3 , .Xr calloc 3 , -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Ship ubsan_minimal library in base?
On Sat, Feb 05 2022, Greg Steuck wrote: > Sweet, let's look at the two patches below. No sets yet. > > I tested on amd64 with the UBSan sample program and also with > games/battlestar. If somebody could repeat the steps I listed in the > previous email on a non-amd64 platform, that would be useful. > > Jeremie Courreges-Anglas writes: > >> On Sat, Feb 05 2022, Jeremie Courreges-Anglas wrote: >>> On Fri, Feb 04 2022, Greg Steuck wrote: >>>> How do people feel about shipping the minimal UBSan runtime library[1] >>>> in the base system? It takes very little to build (Makefile + a few >>>> ifdefs that both jca@ and I hacked together). The library is tiny >> >> In case people wonder, the implementation is in >> gnu/llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp >> >> I suspect we should to ship a PIC/shared version of the library, along >> with /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal.a but ENOTIME >> to look further right now. > > If you see enough value in that, by all means we can add this. I don't > know if clang has the necessary plumbing for easy choice between the .a > and .so here. I didn't find such plumbing, even though ubsan_minimal/CMakeLists.txt mentions a shared version, maybe a red herring. Your Makefile proposal specifies -fPIC and that seems to work well in practice for static and static-no-pie code. >> Tentative proposal, maybe a bit premature >> >> Index: clang-local.1 >> === >> RCS file: /home/cvs/src/share/man/man1/clang-local.1,v >> retrieving revision 1.22 >> diff -u -p -p -u -r1.22 clang-local.1 >> --- clang-local.17 Sep 2021 17:39:49 - 1.22 >> +++ clang-local.15 Feb 2022 17:11:48 - >> @@ -93,6 +93,13 @@ option to treat signed integer overflows >> prevent dangerous optimizations which could remove security critical >> overflow >> checks. >> .It >> +Only ubsan_minimal support is shipped by the base system. >> +To make use of it, pass >> +.Nm clang >> +the following options: >> +.Fl fsanitize=undefined >> +.Fl fsanitize-minimal-runtime . I'm not 100% happy with this wording, I'll try to rework it. In the meantime... >> +.It >> The >> .Xr malloc 3 , >> .Xr calloc 3 , > > Excellent, I was looking for the right place to put this. > OK gnezdo@ once it works :) ... this seems to work pretty well here on amd64 and riscv64 (base-clang only archs) and sparc64 (base-gcc, but base-clang is available). I still think that this can and should be added to base. ok jca@ Minor spacing issue below, > From 46e3165e67b4e5b45e4a379abe4be2656fc8f81c Mon Sep 17 00:00:00 2001 > From: Greg Steuck > Date: Sat, 5 Feb 2022 14:15:25 -0800 > Subject: [PATCH 1/2] Add ifdefs to build ubsan_minimal on OpenBSD > > --- > gnu/llvm/compiler-rt/lib/interception/interception.h | 4 ++-- > .../compiler-rt/lib/interception/interception_linux.h| 2 +- > .../compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 2 +- > .../compiler-rt/lib/sanitizer_common/sanitizer_linux.h | 2 +- > .../lib/sanitizer_common/sanitizer_platform.h| 9 - > gnu/llvm/compiler-rt/lib/ubsan/ubsan_platform.h | 2 +- > 6 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/gnu/llvm/compiler-rt/lib/interception/interception.h > b/gnu/llvm/compiler-rt/lib/interception/interception.h > index d8dc092c45f..fb91a4cc32b 100644 > --- a/gnu/llvm/compiler-rt/lib/interception/interception.h > +++ b/gnu/llvm/compiler-rt/lib/interception/interception.h > @@ -18,7 +18,7 @@ > > #if !SANITIZER_LINUX && !SANITIZER_FREEBSD && !SANITIZER_MAC && \ > !SANITIZER_NETBSD && !SANITIZER_WINDOWS && !SANITIZER_FUCHSIA && \ > -!SANITIZER_SOLARIS > +!SANITIZER_OPENBSD && !SANITIZER_SOLARIS > # error "Interception doesn't work on this operating system." > #endif > > @@ -272,7 +272,7 @@ typedef unsigned long uptr; > #define INCLUDED_FROM_INTERCEPTION_LIB > > #if SANITIZER_LINUX || SANITIZER_FREEBSD || SANITIZER_NETBSD || \ > -SANITIZER_SOLARIS > +SANITIZER_OPENBSD || SANITIZER_SOLARIS > > # include "interception_linux.h" > # define INTERCEPT_FUNCTION(func) INTERCEPT_FUNCTION_LINUX_OR_FREEBSD(func) > diff --git a/gnu/llvm/compiler-rt/lib/interception/interception_linux.h > b/gnu/llvm/compiler-rt/lib/interception/interception_linux.h > index a08f8cb98c4..b554b53d1dc 100644 > --- a/gnu/llvm/compiler-rt/lib/interception/interception_linux.h > +++ b/gnu/llvm/co
Re: Ship ubsan_minimal library in base?
On Sat, Feb 05 2022, Jeremie Courreges-Anglas wrote: > On Fri, Feb 04 2022, Greg Steuck wrote: >> How do people feel about shipping the minimal UBSan runtime library[1] >> in the base system? It takes very little to build (Makefile + a few >> ifdefs that both jca@ and I hacked together). The library is tiny In case people wonder, the implementation is in gnu/llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp I suspect we should to ship a PIC/shared version of the library, along with /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal.a but ENOTIME to look further right now. >> and >> useful enough for finding UB in base. > > As already discussed I agree with that this should be shipped in base. > To make it easier for users to find it, I would suggest documenting that > only ubsan_minimal is supported for now (as far as I'm concerned I did > not look at non-minimal-ubsan and I don't plan to do so). Tentative proposal, maybe a bit premature Index: clang-local.1 === RCS file: /home/cvs/src/share/man/man1/clang-local.1,v retrieving revision 1.22 diff -u -p -p -u -r1.22 clang-local.1 --- clang-local.1 7 Sep 2021 17:39:49 - 1.22 +++ clang-local.1 5 Feb 2022 17:11:48 - @@ -93,6 +93,13 @@ option to treat signed integer overflows prevent dangerous optimizations which could remove security critical overflow checks. .It +Only ubsan_minimal support is shipped by the base system. +To make use of it, pass +.Nm clang +the following options: +.Fl fsanitize=undefined +.Fl fsanitize-minimal-runtime . +.It The .Xr malloc 3 , .Xr calloc 3 , >> % ls -l /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal* >> -r--r--r-- 1 root bin 51516 Feb 4 20:02 >> /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal.a >> >> % cd /usr/src/games/battlestar >> % make obj && make clean; make LDFLAGS='-fsanitize=undefined >> -fsanitize-minimal-runtime' COPTS='-g -fsanitize=undefined >> -fsanitize-minimal-runtime -fno-wrapv' >> % ./obj/battlestar >> Version 4.2, fall 1984. >> First Adventure game written by His Lordship, the honorable >> Admiral D.W. Riggle >> >> ubsan: shift-out-of-bounds > > FWIW I can't get ubsan_minimal to kick in with the wip Makefile/diff > I shared earlier. I'd love to have your latest diffs to do some > testing. Stoopid me had already fixed extern.h, no wonder why I could not reproduce this with my wip diff (seems to kick in properly now). >> % egdb ./obj/battlestar >> (gdb) b __ubsan_handle_shift_out_of_bounds_minimal >> Breakpoint 1 at 0x23630: file >> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp, >> line 104. >> (gdb) bt >> No stack. >> (gdb) r >> Starting program: /usr/obj/games/battlestar/battlestar >> Version 4.2, fall 1984. >> First Adventure game written by His Lordship, the honorable >> Admiral D.W. Riggle >> >> >> Breakpoint 1, __ubsan_handle_shift_out_of_bounds_minimal () >> at >> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 >> 104 HANDLER(shift_out_of_bounds, "shift-out-of-bounds") >> (gdb) bt >> #0 __ubsan_handle_shift_out_of_bounds_minimal () >> at >> /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 >> #1 0x07434ee9f654 in initialize (filename=) at >> /usr/src/games/battlestar/init.c:67 >> #2 0x07434ee8af06 in main (argc=1, argv=) at >> /usr/src/games/battlestar/battlestar.c:58 >> >> (gdb) up >> #1 0x0b1eddd2a654 in initialize (filename=) at >> /usr/src/games/battlestar/init.c:67 >> 67 SetBit(location[p->room].objects, p->obj); >> >> (gdb) !grep SetBit extern.h >> #define SetBit(array, index)(array[index/BITS] |= (1 << (index % BITS))) >> >> Yup, the usual, shifting too far without 1U. Hence the patch: > > The diff below does not apply (tabs vs spaces) but makes sense. ok jca@ > >> --- a/games/battlestar/extern.h >> +++ b/games/battlestar/extern.h >> @@ -39,9 +39,9 @@ >> #define OUTSIDE(position > 68 && position < 246 && position >> != 218) >> #define rnd(x) arc4random_uniform(x) >> #define max(a,b) ((a) < (b) ? (b) : (a)) >> -#define TestBit(array, index) (array[index/BITS] & (1 << (index % BITS))) >> -#define SetBit(array, index) (array[index/BITS] |=
Re: Ship ubsan_minimal library in base?
On Fri, Feb 04 2022, Greg Steuck wrote: > How do people feel about shipping the minimal UBSan runtime library[1] > in the base system? It takes very little to build (Makefile + a few > ifdefs that both jca@ and I hacked together). The library is tiny and > useful enough for finding UB in base. As already discussed I agree with that this should be shipped in base. To make it easier for users to find it, I would suggest documenting that only ubsan_minimal is supported for now (as far as I'm concerned I did not look at non-minimal-ubsan and I don't plan to do so). > % ls -l /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal* > -r--r--r-- 1 root bin 51516 Feb 4 20:02 > /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal.a > > % cd /usr/src/games/battlestar > % make obj && make clean; make LDFLAGS='-fsanitize=undefined > -fsanitize-minimal-runtime' COPTS='-g -fsanitize=undefined > -fsanitize-minimal-runtime -fno-wrapv' > % ./obj/battlestar > Version 4.2, fall 1984. > First Adventure game written by His Lordship, the honorable > Admiral D.W. Riggle > > ubsan: shift-out-of-bounds FWIW I can't get ubsan_minimal to kick in with the wip Makefile/diff I shared earlier. I'd love to have your latest diffs to do some testing. > % egdb ./obj/battlestar > (gdb) b __ubsan_handle_shift_out_of_bounds_minimal > Breakpoint 1 at 0x23630: file > /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp, > line 104. > (gdb) bt > No stack. > (gdb) r > Starting program: /usr/obj/games/battlestar/battlestar > Version 4.2, fall 1984. > First Adventure game written by His Lordship, the honorable > Admiral D.W. Riggle > > > Breakpoint 1, __ubsan_handle_shift_out_of_bounds_minimal () > at > /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 > 104 HANDLER(shift_out_of_bounds, "shift-out-of-bounds") > (gdb) bt > #0 __ubsan_handle_shift_out_of_bounds_minimal () > at > /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 > #1 0x07434ee9f654 in initialize (filename=) at > /usr/src/games/battlestar/init.c:67 > #2 0x07434ee8af06 in main (argc=1, argv=) at > /usr/src/games/battlestar/battlestar.c:58 > > (gdb) up > #1 0x0b1eddd2a654 in initialize (filename=) at > /usr/src/games/battlestar/init.c:67 > 67 SetBit(location[p->room].objects, p->obj); > > (gdb) !grep SetBit extern.h > #define SetBit(array, index)(array[index/BITS] |= (1 << (index % BITS))) > > Yup, the usual, shifting too far without 1U. Hence the patch: The diff below does not apply (tabs vs spaces) but makes sense. ok jca@ > --- a/games/battlestar/extern.h > +++ b/games/battlestar/extern.h > @@ -39,9 +39,9 @@ > #define OUTSIDE(position > 68 && position < 246 && position > != 218) > #define rnd(x) arc4random_uniform(x) > #define max(a,b) ((a) < (b) ? (b) : (a)) > -#define TestBit(array, index) (array[index/BITS] & (1 << (index % BITS))) > -#define SetBit(array, index) (array[index/BITS] |= (1 << (index % BITS))) > -#define ClearBit(array, index) (array[index/BITS] &= ~(1 << (index % BITS))) > +#define TestBit(array, index) (array[index/BITS] & (1U << (index % BITS))) > +#define SetBit(array, index) (array[index/BITS] |= (1U << (index % BITS))) > +#define ClearBit(array, index) (array[index/BITS] &= ~(1U << (index % BITS))) > /* > * These macros yield words to use with objects (followed but not preceded > * by spaces, or with no spaces if the expansion is the empty string). > > OK to fix battlestar while we are at it? > > [1] > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#minimal-runtime > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: Missing UBSan libs
On Mon, Jan 31 2022, Greg Steuck wrote: > Patrick Wildt writes: > >> regarding the missing userpace support: Since a few clang updates ago >> we import more than just the builtins of compiler-rt. This means we >> should have at least some related code in our tree, even if it is not >> built/complete. In base this seems to be gnu/llvm/compiler-rt and its subdirs. Something not present right now in the devel/llvm port. >> From the recent static analyzer mail thread it looks >> like people prefer to have such stuff in ports-clang, so, whatever. I only glanced at this thread but there is probably pushback against making clang even bigger and slower to build, which is a thing that I can understand. I'm not sure it would be a problem to build a few more llvm support libraries like ubsan. libclang_rt.profile.a comes to mind as an existing library that is built as part of the make build/release process. > This may or may not be analogous. How hard is it to build a base/ports > program with clang from ports? If it's a simple matter of > make CC=/usr/local/bin/cc CFLAGS=-fsanitize=undefined > then it makes little difference whether the base compiler includes > the libraries for UBSan reporting. > > jca@ WDYT, should I first target devel/llvm to have UBSan working or go > straight to /usr/src? Hard to tell without diving in further. :) My gut feeling is that in the end this should be provided by the base system, just like the rest of the compiler-rt / clang_rt.profile stuff*. And since we seem to already have the code in base but not in ports, I guess the answer is "base". Obviously in that case you have to replace the cmake setup with Makefiles. Those are broad hints, let me know if I can help further. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: fix active scan on iwm and iwx
On Thu, Jan 13 2022, Stefan Sperling wrote: > At present active scans (which send probe requests, as opposed to > just listening for beacons) are disabled on iwm 9k and iwx. This > was done because firmware misbehaved after association. > > zxystd from the OpenIntelWireless project has debugged the issue > and has sent me a patch against OpenBSD which fixes this problem. > The patch is below, with some small tweaks by me which have already > been reviewed by zxystd. > > It seems that firmware misbehaves if the driver sets the DTIM period > to zero. This value is read from TIM information elements (IE) in beacons. > Passive scans worked because we picked up the DTIM period from a beacon, > while probe responses received during active scans lack the TIM IE, which > resulted in a zero DTIM period being configured in firmware. We then never > updated TIM information when a beacon was recieved, letting firmware run > with a zero DTIM period until it eventually stopped working. > > I have tested this patch on iwm 8265 and iwx ax200. fwiw no regression on a 8265 too. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: DNS in acme-client
On Sun, Dec 12 2021, Florian Obser wrote:
> On 12 December 2021 21:19:21 CET, Jeremie Courreges-Anglas
> wrote:
>>
>>dnsproc.c only returns a single address even if the code pretends to
>>support multiple addresses. This leads to weird behavior in edge cases,
>>as experienced by a user on IRC.
>>
>>Take a machine with both IPv4 and IPv6 addresses configured, but no
>>IPv4 default route (on purpose). Since there is at least one IPv4
>>address different from 127.0.0.1, AI_ADDRCONFIG doesn't filter out
>>A records. Let's encrypt ACME service is dual stacked but the first and
>>only address returned by dnsproc.c is always IPv4 with our "family inet
>>inet6". In this situation acme-client can't connect over IPv4 and errors
>>out even though there's a working IPv6 default route.
>>
>
> Doctor, Doctor! When I do this, it hurts!
Ah well, the user wasn't complaining, really. :)
I just got curious because the behavior and the code seemed strange.
>>I don't know much about ACME and its requirements / good practices for
>
> I can't think of a reason to not try all address families.
Also I can't think of a reason not to try multiple addresses in the same
address family.
>>clients, but clearly acme-client doesn't behave like most of our
>>programs which try to connect to all available addresses. This break
>>statement has been there since import, but was it added on purpose?
>>Input welcome.
>>
>>
>
> I probably won't be able to look at this for a week. I am very surprised that
> this is the correct fix though.
Well I was puzzled too, I stared perhaps 30 seconds trying to understand
what this statement was doing there. Its presence defeats the point of
the loop, variables, etc around it. To me this looks like a leftover
from initial experiments. The initial copy in ntpd/config.c doesn't
have this break statement.
> I trust you checked that multiple IP addresses can be passed between
> processes?
I added some extra debug statements and could see that 2 adresses (v4
and v6) were passed to netproc.c. I was able to reproduce the problem
of the original reporter, and see that the diff fixed renewing
a certificate.
>>diff --git a/usr.sbin/acme-client/dnsproc.c b/usr.sbin/acme-client/dnsproc.c
>>index 664ef8d9b8b..c4c612e521a 100644
>>--- a/usr.sbin/acme-client/dnsproc.c
>>+++ b/usr.sbin/acme-client/dnsproc.c
>>@@ -102,7 +102,6 @@ host_dns(const char *s, struct addr *vec)
>>
>> dodbg("%s: DNS: %s", s, vec[vecsz].ip);
>> vecsz++;
>>- break;
>> }
>>
>> freeaddrinfo(res0);
>>
>>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
DNS in acme-client
dnsproc.c only returns a single address even if the code pretends to
support multiple addresses. This leads to weird behavior in edge cases,
as experienced by a user on IRC.
Take a machine with both IPv4 and IPv6 addresses configured, but no
IPv4 default route (on purpose). Since there is at least one IPv4
address different from 127.0.0.1, AI_ADDRCONFIG doesn't filter out
A records. Let's encrypt ACME service is dual stacked but the first and
only address returned by dnsproc.c is always IPv4 with our "family inet
inet6". In this situation acme-client can't connect over IPv4 and errors
out even though there's a working IPv6 default route.
I don't know much about ACME and its requirements / good practices for
clients, but clearly acme-client doesn't behave like most of our
programs which try to connect to all available addresses. This break
statement has been there since import, but was it added on purpose?
Input welcome.
diff --git a/usr.sbin/acme-client/dnsproc.c b/usr.sbin/acme-client/dnsproc.c
index 664ef8d9b8b..c4c612e521a 100644
--- a/usr.sbin/acme-client/dnsproc.c
+++ b/usr.sbin/acme-client/dnsproc.c
@@ -102,7 +102,6 @@ host_dns(const char *s, struct addr *vec)
dodbg("%s: DNS: %s", s, vec[vecsz].ip);
vecsz++;
- break;
}
freeaddrinfo(res0);
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: asr(3): strip AD flag in responses
On Mon, Nov 22 2021, Florian Obser wrote: > On 2021-11-21 22:21 +01, Jeremie Courreges-Anglas wrote: >> On Sun, Nov 21 2021, Jeremie Courreges-Anglas wrote: >>> On Sat, Nov 20 2021, Florian Obser wrote: >> >> [...] >> >>>>> Index: lib/libc/asr/res_mkquery.c >>>>> === >>>>> RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v >>>>> retrieving revision 1.13 >>>>> diff -u -p -r1.13 res_mkquery.c >>>>> --- lib/libc/asr/res_mkquery.c14 Jan 2019 06:49:42 - 1.13 >>>>> +++ lib/libc/asr/res_mkquery.c20 Nov 2021 14:24:08 - >>>>> @@ -62,6 +62,9 @@ res_mkquery(int op, const char *dname, i >>>>> h.flags |= RD_MASK; >>>>> if (ac->ac_options & RES_USE_CD) >>>>> h.flags |= CD_MASK; >>>>> + if ((ac->ac_options & RES_TRUSTAD) && >>>>> + !(ac->ac_options & RES_USE_DNSSEC)) >>>>> + h.flags |= AD_MASK; >>>> >>>> do you remember why you check for !RES_USE_DNSSEC? >>>> I'd like to leave it out. >>> >>> First, here's my understanding of RES_USE_DNSSEC: it's supposed to >>> activate EDNS and set the DO bit. The server is then supposed to reply >>> with AD set only if the data has been validated (with or without >>> DNSSEC), and possibly append add DNSSEC data if available. >>> >>> Since I didn't know the semantics of both setting AD and DO in a query >>> (I would expect such semantics to be sane) I wrote those more >>> conservative checks instead. Does this make sense? If so, maybe >>> a comment would help? >>> >>> /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */ >>> if ((ac->ac_options & RES_TRUSTAD) && >>> !(ac->ac_options & RES_USE_DNSSEC)) >>> >>>> In fact I don't think RES_USE_DNSSEC is useful >>>> at all. >>>> If you want to set the DO flag and start DNSSEC from first principles >>>> you are better of talking to the network directly, i.e. rewrite unwind. >>> >>> RES_USE_DNSSEC has been there since some time already and it's used by >>> software in the ports tree, precisely to detect AD=1 - and not so much >>> for the key records I think. >> >> BTW clearing AD might break software that depends on it for stuff like >> DANE. postfix uses RES_USE_DNSSEC for this, but also force-enables >> RES_TRUSTAD if available so it shouldn't be affected (upstream's stance >> is that you should only use a trusted resolver when running postfix). > > Ambitious... > > I actually had considerd to only do automatic trust-ad without any way > for users and programs to change it / expand it to non-localhost. > But then I thought it might be nice in a datacenter setting where you > might trust the path to the resolver. I also think that'trust-ad" is good thing to support. > And then postfix comes along :( If we don't want uptreams to use RES_TRUSTAD and thus force the admin to do a conscious choice, we could hide it as an implementation detail in asr/ or make it a noop (decoupling it from "options trust-ad" and your automatic detection). It would require careful thinking about how people use the API. Personnally I don't feel too strongly about giving people rope... >> On the other hand mail/exim knowlingly doesn't force RES_TRUSTAD. > > I don't think we'll brake anything, at leat it should not stop > progress. DANE verification has to be oportunistic, no? Good luck > sending/receiving mail while requiring that certificates are either > signed by a known CA or TLSA records are published. That will probably > lose you a whole lot of the internet... Some people in the corporate world actually start to require the use of certificates signed by a known CA. I only spotted this twice so far. Regarding DANE and TLSA, one of the goals is to be able to use strong authentication without deferring to commercial CAs. So I'm pretty sure some nerdy admins have started enforcing it where they can, just because they can. >> I don't know of other ports using RES_USE_DNSSEC and caring about the AD >> flag. >> >> The effect of RES_TRUSTAD/trust-ad on RES_USE_DNSSEC ought to be >> documented, but let's do that in another diff: the documentation of the >> latter option is already lacking. Proposal below. Feedback / oks? Index: res_init.3 =
