Re: panic(9): set panicstr atomically
Scott Cheloha wrote:
> On Sat, May 22, 2021 at 01:35:53AM +0200, Alexander Bluhm wrote:
> > On Fri, May 21, 2021 at 02:00:54PM -0500, Scott Cheloha wrote:
> > > Given all of this, would it be better if secondary CPUs spin in
> > > panic(9) instead of trying to print anything?
> >
> > The panic code should be as primitive as possible. The garbled
> > output also tells me something. Two CPUs are failing simultaneosly.
> > Please don't suppress that information.
> >
> > The crash is the problem, not the ugly printing.
>
> I get where you're coming from in principle (simpler is better) but I
> think you're prioritizing a minor concern over the bigger picture.
Actually, I think you've got it wrong, and risk capturing the least
important panic.
I suspect we can check for not now, by looking at what the first character
in the panics looks like, to decode which panic happened first. I think
I did this before, and the first one through the gate wasn't the important one.
Doesn't it break parts of ddb with (;;) CPU_BUSY_CYCLE() ?
> I think it is strictly better for one CPU to run the panic code than
> to have two CPUs doing the same. If the panic code is primitive then
> it probably isn't MP-safe.
But it is primitive. I suspect parallel panic's are not corrupting a
large amount of state.
> Further, what about kernels without ddb(4)? What happens if two or
> more CPUs all simultaneously call reboot() at the end of panic(9)? It
> would vary by platform at a minimum. Wouldn't deterministic behavior
> be better in that case?
Which kernels are those? Just ramdisks, which are MP. This situation
does not happen.
> We can keep the garbled printing if you like, I can see how it works
> as a diagnostic feature, but I think the losing CPUs should spin in
> panic. Fewer things can go wrong.
How many times must it be repeated: Then you only see the first cpu
which manages to reach panic(). Any other cpu no longer provides a
diagnostic. They'll be spinning and noone knows why, the message is
entirely lost.
I would like to reiterate that I would be very happy if every cpuinfo
has an independet panic buffer, and *THAT* buffer can be updated async
by every cpu first, before deciding to print. Heck, in that case the
printing code can show only "one" panic message, and then say
"more panics: use show panic".
At which point the garble is gone, we can hopefully gravitate
towards people knowing to run "show panic" to see multiple reasons.
> What about something like this?
>
> Index: subr_prf.c
> ===
> RCS file: /cvs/src/sys/kern/subr_prf.c,v
> retrieving revision 1.103
> diff -u -p -r1.103 subr_prf.c
> --- subr_prf.c16 May 2021 15:10:20 - 1.103
> +++ subr_prf.c25 May 2021 03:09:19 -
> @@ -184,31 +184,50 @@ void
> panic(const char *fmt, ...)
> {
> static char panicbuf[512];
> - int bootopt;
> + static struct cpu_info *paniccpu;
> + struct cpu_info *ci;
> + int bootopt, recursive, spin;
> va_list ap;
>
> - /* do not trigger assertions, we know that we are inconsistent */
> - splassert_ctl = 0;
> + ci = curcpu();
> + recursive = 0;
> + spin = 0;
>
> - /* make sure we see kernel printf output */
> - printf_flags |= TOCONS;
> + if (atomic_cas_ptr(, NULL, ci) != NULL) {
> + if (paniccpu != ci)
> + spin = 1;
> + else
> + recursive = 1;
> + } else {
> + panicstr = panicbuf;
> +
> + /* do not trigger assertions, we know that we are inconsistent
> */
> + splassert_ctl = 0;
> +
> + /* make sure we see kernel printf output */
> + printf_flags |= TOCONS;
> + }
>
> bootopt = RB_AUTOBOOT | RB_DUMP;
> - va_start(ap, fmt);
> - if (panicstr)
> +
> + if (recursive || spin) {
> bootopt |= RB_NOSYNC;
> - else {
> + printf("panic: ");
> + va_start(ap, fmt);
> + vprintf(fmt, ap);
> + va_end(ap);
> + printf("\n");
> + } else {
> + va_start(ap, fmt);
> vsnprintf(panicbuf, sizeof panicbuf, fmt, ap);
> - panicstr = panicbuf;
> + va_end(ap);
> + printf("panic: %s\n", panicbuf);
> }
> - va_end(ap);
> -
> - printf("panic: ");
> - va_start(ap, fmt);
> - vprintf(fmt, ap);
> - printf("\n");
> - va_end(ap);
>
> + if (spin) {
> + for (;;)
> + CPU_BUSY_CYCLE();
> + }
> #ifdef DDB
> if (db_panic)
> db_enter();
>
Re: panic(9): set panicstr atomically
On Mon, May 24, 2021 at 10:12:53PM -0500, Scott Cheloha wrote: > On Sat, May 22, 2021 at 01:35:53AM +0200, Alexander Bluhm wrote: > > On Fri, May 21, 2021 at 02:00:54PM -0500, Scott Cheloha wrote: > > > Given all of this, would it be better if secondary CPUs spin in > > > panic(9) instead of trying to print anything? > > > > The panic code should be as primitive as possible. The garbled > > output also tells me something. Two CPUs are failing simultaneosly. > > Please don't suppress that information. > > > > The crash is the problem, not the ugly printing. > > I get where you're coming from in principle (simpler is better) but I > think you're prioritizing a minor concern over the bigger picture. To be perfectly clear, I'm not talking about the garbled printing anymore. I'm talking about *all* the code that can run from panic(). There is a lot of code. I think it would be better if we prevented multiple CPUs from running that code simultaneously by having secondary CPUs spin in panic() as visa@ suggested.
Re: panic(9): set panicstr atomically
On Sat, May 22, 2021 at 01:35:53AM +0200, Alexander Bluhm wrote:
> On Fri, May 21, 2021 at 02:00:54PM -0500, Scott Cheloha wrote:
> > Given all of this, would it be better if secondary CPUs spin in
> > panic(9) instead of trying to print anything?
>
> The panic code should be as primitive as possible. The garbled
> output also tells me something. Two CPUs are failing simultaneosly.
> Please don't suppress that information.
>
> The crash is the problem, not the ugly printing.
I get where you're coming from in principle (simpler is better) but I
think you're prioritizing a minor concern over the bigger picture.
I think it is strictly better for one CPU to run the panic code than
to have two CPUs doing the same. If the panic code is primitive then
it probably isn't MP-safe.
Further, what about kernels without ddb(4)? What happens if two or
more CPUs all simultaneously call reboot() at the end of panic(9)? It
would vary by platform at a minimum. Wouldn't deterministic behavior
be better in that case?
We can keep the garbled printing if you like, I can see how it works
as a diagnostic feature, but I think the losing CPUs should spin in
panic. Fewer things can go wrong.
What about something like this?
Index: subr_prf.c
===
RCS file: /cvs/src/sys/kern/subr_prf.c,v
retrieving revision 1.103
diff -u -p -r1.103 subr_prf.c
--- subr_prf.c 16 May 2021 15:10:20 - 1.103
+++ subr_prf.c 25 May 2021 03:09:19 -
@@ -184,31 +184,50 @@ void
panic(const char *fmt, ...)
{
static char panicbuf[512];
- int bootopt;
+ static struct cpu_info *paniccpu;
+ struct cpu_info *ci;
+ int bootopt, recursive, spin;
va_list ap;
- /* do not trigger assertions, we know that we are inconsistent */
- splassert_ctl = 0;
+ ci = curcpu();
+ recursive = 0;
+ spin = 0;
- /* make sure we see kernel printf output */
- printf_flags |= TOCONS;
+ if (atomic_cas_ptr(, NULL, ci) != NULL) {
+ if (paniccpu != ci)
+ spin = 1;
+ else
+ recursive = 1;
+ } else {
+ panicstr = panicbuf;
+
+ /* do not trigger assertions, we know that we are inconsistent
*/
+ splassert_ctl = 0;
+
+ /* make sure we see kernel printf output */
+ printf_flags |= TOCONS;
+ }
bootopt = RB_AUTOBOOT | RB_DUMP;
- va_start(ap, fmt);
- if (panicstr)
+
+ if (recursive || spin) {
bootopt |= RB_NOSYNC;
- else {
+ printf("panic: ");
+ va_start(ap, fmt);
+ vprintf(fmt, ap);
+ va_end(ap);
+ printf("\n");
+ } else {
+ va_start(ap, fmt);
vsnprintf(panicbuf, sizeof panicbuf, fmt, ap);
- panicstr = panicbuf;
+ va_end(ap);
+ printf("panic: %s\n", panicbuf);
}
- va_end(ap);
-
- printf("panic: ");
- va_start(ap, fmt);
- vprintf(fmt, ap);
- printf("\n");
- va_end(ap);
+ if (spin) {
+ for (;;)
+ CPU_BUSY_CYCLE();
+ }
#ifdef DDB
if (db_panic)
db_enter();
sys/dev/pv: remove unused arg in various virtio drivers
I'm working on some vio(4) stuff and found that the virtio_alloc_vq
function defined in sys/dev/pv/virtiovar.h contains a vestigal function
arg "int maxsegsize" that was inherited from the original NetBSD code
imported.
It's original use was to set a vq_maxsegsize struct member to advise
drivers on how large their virtio queue buffers should be, but in
practice drivers have used their own local logic when calling things
like dma_alloc(9).
Setting vq_maxsegsize in the virtio_alloc_vq function was removed just
under 4 years ago by sf@ in virtio.c [1] and the struct member was
removed in virtiovar.h [2].
I checked NetBSD out of curiousity. The same struct member has lingered
for ~10 years in their tree [3]. They set a vq_maxsegsize member on
their virtio queue struct, but nothing ever references it. They also
carry this unused arg.
The diff below removes the argument and updates all callers. No new
functionality is added. I've only had a chance to test by booting a
kernel under vmm(4)/vmd(8) and not on other hypervisors and didn't find
any regressions.
OK? Or more testing?
-dv
[1]
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/dev/pv/virtio.c.diff?r1=1.4=1.5
[2]
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/dev/pv/virtio.c.diff?r1=1.4=1.5
[3] https://github.com/NetBSD/src/search?q=vq_maxsegsize
Index: sys/dev/pv/if_vio.c
===
RCS file: /cvs/src/sys/dev/pv/if_vio.c,v
retrieving revision 1.19
diff -u -p -r1.19 if_vio.c
--- sys/dev/pv/if_vio.c 12 Dec 2020 11:48:53 - 1.19
+++ sys/dev/pv/if_vio.c 25 May 2021 03:03:30 -
@@ -555,12 +555,11 @@ vio_attach(struct device *parent, struct
else
ifp->if_hardmtu = MCLBYTES - sc->sc_hdr_size - ETHER_HDR_LEN;
- if (virtio_alloc_vq(vsc, >sc_vq[VQRX], 0, MCLBYTES, 2, "rx") != 0)
+ if (virtio_alloc_vq(vsc, >sc_vq[VQRX], 0, 2, "rx") != 0)
goto err;
vsc->sc_nvqs = 1;
sc->sc_vq[VQRX].vq_done = vio_rx_intr;
if (virtio_alloc_vq(vsc, >sc_vq[VQTX], 1,
- sc->sc_hdr_size + ifp->if_hardmtu + ETHER_HDR_LEN,
VIRTIO_NET_TX_MAXNSEGS + 1, "tx") != 0) {
goto err;
}
@@ -573,7 +572,7 @@ vio_attach(struct device *parent, struct
virtio_stop_vq_intr(vsc, >sc_vq[VQTX]);
if (virtio_has_feature(vsc, VIRTIO_NET_F_CTRL_VQ)
&& virtio_has_feature(vsc, VIRTIO_NET_F_CTRL_RX)) {
- if (virtio_alloc_vq(vsc, >sc_vq[VQCTL], 2, NBPG, 1,
+ if (virtio_alloc_vq(vsc, >sc_vq[VQCTL], 2, 1,
"control") == 0) {
sc->sc_vq[VQCTL].vq_done = vio_ctrleof;
virtio_start_vq_intr(vsc, >sc_vq[VQCTL]);
Index: sys/dev/pv/vioblk.c
===
RCS file: /cvs/src/sys/dev/pv/vioblk.c,v
retrieving revision 1.32
diff -u -p -r1.32 vioblk.c
--- sys/dev/pv/vioblk.c 15 Oct 2020 13:22:13 - 1.32
+++ sys/dev/pv/vioblk.c 25 May 2021 03:03:30 -
@@ -207,8 +207,8 @@ vioblk_attach(struct device *parent, str
sc->sc_capacity = virtio_read_device_config_8(vsc,
VIRTIO_BLK_CONFIG_CAPACITY);
- if (virtio_alloc_vq(vsc, >sc_vq[0], 0, MAXPHYS, ALLOC_SEGS,
- "I/O request") != 0) {
+ if (virtio_alloc_vq(vsc, >sc_vq[0], 0, ALLOC_SEGS, "I/O request")
+ != 0) {
printf("\nCan't alloc virtqueue\n");
goto err;
}
Index: sys/dev/pv/viomb.c
===
RCS file: /cvs/src/sys/dev/pv/viomb.c,v
retrieving revision 1.7
diff -u -p -r1.7 viomb.c
--- sys/dev/pv/viomb.c 4 Sep 2020 13:10:16 - 1.7
+++ sys/dev/pv/viomb.c 25 May 2021 03:03:30 -
@@ -162,12 +162,12 @@ viomb_attach(struct device *parent, stru
if (virtio_negotiate_features(vsc, viomb_feature_names) != 0)
goto err;
- if ((virtio_alloc_vq(vsc, >sc_vq[VQ_INFLATE], VQ_INFLATE,
-sizeof(u_int32_t) * PGS_PER_REQ, 1, "inflate") != 0))
+ if ((virtio_alloc_vq(vsc, >sc_vq[VQ_INFLATE], VQ_INFLATE, 1,
+ "inflate") != 0))
goto err;
vsc->sc_nvqs++;
- if ((virtio_alloc_vq(vsc, >sc_vq[VQ_DEFLATE], VQ_DEFLATE,
-sizeof(u_int32_t) * PGS_PER_REQ, 1, "deflate") != 0))
+ if ((virtio_alloc_vq(vsc, >sc_vq[VQ_DEFLATE], VQ_DEFLATE, 1,
+ "deflate") != 0))
goto err;
vsc->sc_nvqs++;
Index: sys/dev/pv/viornd.c
===
RCS file: /cvs/src/sys/dev/pv/viornd.c,v
retrieving revision 1.4
diff -u -p -r1.4 viornd.c
--- sys/dev/pv/viornd.c 29 May 2020 04:42:25 - 1.4
+++ sys/dev/pv/viornd.c 25 May 2021 03:03:30 -
@@ -126,8 +126,7 @@ viornd_attach(struct device *parent, str
goto err2;
}
- if (virtio_alloc_vq(vsc, >sc_vq, 0, VIORND_BUFSIZE, 1,
-
Re: audio devices for armv7
> Date: Mon, 24 May 2021 22:37:14 +0200 > From: Peter Hessler > > After the recent uaudio dma fixes, I tried out audio playing on my armv7 > system. Tested on the built-in audio port on hw.product=Tinker-RK3288, > sounds fine. > > OK? > > (N.B. 'twrget' is not a typo, even if it looks like one) ok kettenis@ > Index: etc/etc.armv7/MAKEDEV.md > === > RCS file: /cvs/src/etc/etc.armv7/MAKEDEV.md,v > retrieving revision 1.19 > diff -u -p -u -p -r1.19 MAKEDEV.md > --- etc/etc.armv7/MAKEDEV.md 23 Jan 2021 05:08:33 - 1.19 > +++ etc/etc.armv7/MAKEDEV.md 24 May 2021 20:29:34 - > @@ -105,6 +105,7 @@ _std(1, 2, 8, 6) > dnl > dnl *** armv7 specific targets > dnl > +twrget(all, au, audio, 0, 1, 2)dnl > target(all, ch, 0)dnl > target(all, vscsi, 0)dnl > target(all, diskmap)dnl > > > > -- > "Amnesia used to be my favorite word, but then I forgot it." > >
audio devices for armv7
After the recent uaudio dma fixes, I tried out audio playing on my armv7 system. Tested on the built-in audio port on hw.product=Tinker-RK3288, sounds fine. OK? (N.B. 'twrget' is not a typo, even if it looks like one) Index: etc/etc.armv7/MAKEDEV.md === RCS file: /cvs/src/etc/etc.armv7/MAKEDEV.md,v retrieving revision 1.19 diff -u -p -u -p -r1.19 MAKEDEV.md --- etc/etc.armv7/MAKEDEV.md23 Jan 2021 05:08:33 - 1.19 +++ etc/etc.armv7/MAKEDEV.md24 May 2021 20:29:34 - @@ -105,6 +105,7 @@ _std(1, 2, 8, 6) dnl dnl *** armv7 specific targets dnl +twrget(all, au, audio, 0, 1, 2)dnl target(all, ch, 0)dnl target(all, vscsi, 0)dnl target(all, diskmap)dnl -- "Amnesia used to be my favorite word, but then I forgot it."
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 05:07:50PM BST, Theo de Raadt wrote: > Raf Czlonka wrote: > > > On Mon, May 24, 2021 at 04:10:00PM BST, Theo de Raadt wrote: > > > But does it matter? > > > > Did this[0] matter? > > If you aren't curious enough to read the Makefile, devlist2h.awk, > usbdevs.h, and usbdevs_data.h to recognize the full production > of your changes (to wit, #defines AND structured strings in every > kernel), then I don't understand what you are doing. Hi Theo, All I was trying to do is to have the vendor ID "pretty"-printed - I thought that part was obvious. Did I realise what the full impact of such change would be? Of course not - otherwise, I would not have emailed the patch in the first place. Errare humanum est. Raf
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
Raf Czlonka wrote: > On Mon, May 24, 2021 at 04:10:00PM BST, Theo de Raadt wrote: > > But does it matter? > > Did this[0] matter? If you aren't curious enough to read the Makefile, devlist2h.awk, usbdevs.h, and usbdevs_data.h to recognize the full production of your changes (to wit, #defines AND structured strings in every kernel), then I don't understand what you are doing.
Re: Removal of old users and groups in the upgrade notes
Ping.
On Sun, May 09, 2021 at 01:07:15PM BST, Raf Czlonka wrote:
> Hello,
>
> This is both a general question and specific example of removal of
> old users and groups.
>
> With the release of 6.7, rebound(8) got tedued[0] ;^)
> However, there were no specific instructions regarding removal of
> _rebound user and group, or the /etc/rebound.conf file, in the
> upgrade notes - I had the latter added to my /etc/sysclean.ignore
> file and didn't notice it until today.
>
> Grepping for '{user,group}del' under 'faq/upgrade*', shows a handful
> of examples - some are straight after a user or a group has been
> retired[1], others when the UID/GID got recycled[2].
>
> Probably too little too late but, should the 6.7 upgrade page get:
>
> # rm /etc/rebound.conf
> # userdel _rebound
> # groupdel _rebound
>
> instructions added, i.e. need I bother you with a diff, or will you
> simply add it once the UID/GID gets recycled?
>
> [0] https://www.openbsd.org/faq/upgrade67.html
> [1] https://www.openbsd.org/faq/upgrade61.html
> [2] https://www.openbsd.org/faq/upgrade64.html
>
> Cheers,
>
> Raf
Re: [PATCH] [src] etc/services - duplicates
Ping. On Sun, May 16, 2021 at 07:10:22PM BST, Raf Czlonka wrote: > Hello, > > During recent services(5)-related threads, I glanced over the file > and noticed a duplicate - namely(sic!), "nameserver" is being used > both as the a service name, as well as an alias for "domain". > > nameserver 42/tcp name# IEN 116 > domain 53/tcp nameserver # name-domain server > domain 53/udp nameserver > > The above entries had remained unchanged since the file has been > imported into the tree[0]. As I found out some minutes later, NetBSD > have removed the duplicate in 1999[1]. > > As you can see from their commit[1], there is another duplicate > which has been removed from that file - "readnews". There, they > have removed it from: > > netnews 532/tcp > > and, even nowadays, still have it as a local alias[2]: > > readnews119/tcp untp > > on top of the usual[2]: > > nntp119/tcp# Network News Transfer > nntp119/udp# Network News Transfer > > while IANA entries look as follows[3]: > > nntp119 tcp Network News Transfer > nntp119 udp Network News Transfer > netnews 532 tcp readnews > netnews 532 udp readnews > > FreeBSD[4] and DragonFly BSD[5]: > > nntp119/tcpusenet #Network News Transfer Protocol > nntp119/udpusenet #Network News Transfer Protocol > netnews 532/tcpreadnews > netnews 532/udpreadnews > > To sum it up, I wasn't sure whether to remove it from: > > nntp119/tcp readnews untp > > or: > > netnews 532/tcp readnews > > Perhaps add "usenet" alias to the "nntp" entry while there...? > > Either way, I'm leaving it "as is", at least for now. > > In terms of the actual diff, I've also taken the liberty to update > the comment to the more modern/familiar - "Domain Name Server" - > as, nowadays, it is being used universally[2][3][4][5]. > > [0] https://cvsweb.openbsd.org/~checkout~/src/etc/services?rev=1.1 > [1] > http://cvsweb.netbsd.org/bsdweb.cgi/src/etc/services.diff?r1=1.30=1.31_with_tag=MAIN=h > [2] http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/etc/services?rev=1.103 > [3] > https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.txt > [4] https://cgit.freebsd.org/src/plain/usr.sbin/services_mkdb/services > [5] > https://gitweb.dragonflybsd.org/dragonfly.git/blob_plain/HEAD:/etc/services > > Regards, > > Raf > > Index: etc/services > === > RCS file: /cvs/src/etc/services,v > retrieving revision 1.102 > diff -u -p -r1.102 services > --- etc/services 12 May 2021 06:50:33 - 1.102 > +++ etc/services 16 May 2021 17:46:38 - > @@ -33,8 +33,8 @@ nameserver 42/tcp name# IEN 116 > whois43/tcp nicname > tacacs 49/tcp tacas+ # Login Host Protocol > (TACACS) > tacacs 49/udp tacas+ # Login Host Protocol > (TACACS) > -domain 53/tcp nameserver # name-domain server > -domain 53/udp nameserver > +domain 53/tcp # Domain Name Server > +domain 53/udp # Domain Name Server > mtp 57/tcp # deprecated > bootps 67/tcp # BOOTP server > bootps 67/udp
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 04:37:57PM BST, Stuart Henderson wrote: > On 2021/05/24 16:27, Raf Czlonka wrote: > > On Mon, May 24, 2021 at 04:10:00PM BST, Theo de Raadt wrote: > > > But does it matter? > > > > Did this[0] matter? > > > [0] > > https://cvsweb.openbsd.org/src/sys/dev/usb/usbdevs.diff?r1=1.698=1.699=date=h > > Yes, that one is used in a driver. Thanks! R.
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 04:28:36PM BST, Jonathan Gray wrote: > On Mon, May 24, 2021 at 03:52:44PM +0100, Raf Czlonka wrote: > > Hello, > > > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. > > 0x1ea7 / 7847 is 'SEMITEK INTERNATIONAL (HK) HOLDING LTD.' in > https://usb.org/sites/default/files/vendor_ids033021.pdf Hi Jonathan, Elsewhere[0][1][2], this mouse shows up as what's below but it's most likely due to the fact that it's several years old. Now I see that there's been an update[3] - vendor renamed or sold? Either way, thanks for the link! [0] https://devicehunt.com/view/type/usb/vendor/1EA7 [1] https://www.devicekb.com/en/hardware/usb-vendors/vid_1ea7 [2] http://www.linux-usb.org/usb.ids [3] https://usb-ids.gowdy.us/read/UD/1ea7 Cheers, Raf > > > > Regards, > > > > Raf > > > > Index: sys/dev/usb/usbdevs > > === > > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > > retrieving revision 1.740 > > diff -u -p -r1.740 usbdevs > > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > > @@ -618,6 +618,7 @@ vendor SELUXIT 0x1d6f Seluxit > > vendor METAGEEK0x1dd5 MetaGeek > > vendor SIMCOM 0x1e0e SIMCom Wireless Solutions Co., Ltd. > > vendor FESTO 0x1e29 Festo > > +vendor SHARKOON0x1ea7 SHARKOON Technologies GmbH > > vendor MODACOM 0x1eb8 Modacom > > vendor AIRTIES 0x1eda AirTies > > vendor LAKESHORE 0x1fb9 Lake Shore > > > >
Re: vio.4: mention support provided by vmd(8)
On Sun, May 23, 2021 at 09:50:46PM -0400, Dave Voutila wrote: > Seems only right that vio.4 mention it's the driver used for the virtio > networking device provided by vmd(8). > > OK? > ok mlarkin > > Index: vio.4 > === > RCS file: /cvs/src/share/man/man4/vio.4,v > retrieving revision 1.15 > diff -u -p -r1.15 vio.4 > --- vio.4 24 Sep 2015 13:11:48 - 1.15 > +++ vio.4 24 May 2021 01:48:44 - > @@ -27,7 +27,8 @@ The > .Nm > driver provides support for the > .Xr virtio 4 > -network interface provided by bhyve, KVM, QEMU, and VirtualBox. > +network interface provided by bhyve, KVM, QEMU, VirtualBox, and > +.Xr vmd 8 . > .Pp > Setting the bit 0x2 in the flags disables the RingEventIndex feature. > This can be tried as a workaround for possible bugs in host implementations > of >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On 2021/05/24 16:27, Raf Czlonka wrote: > On Mon, May 24, 2021 at 04:10:00PM BST, Theo de Raadt wrote: > > But does it matter? > > Did this[0] matter? > [0] > https://cvsweb.openbsd.org/src/sys/dev/usb/usbdevs.diff?r1=1.698=1.699=date=h Yes, that one is used in a driver.
Re: vmd(8): add MTU feature support to vionet device
On Mon, May 24, 2021 at 08:25:04AM +0200, Claudio Jeker wrote:
> On Sun, May 23, 2021 at 10:25:38PM -0400, Dave Voutila wrote:
> > The following diff adds in virtio 1.1's VIRTIO_NET_F_MTU feature support
> > to vmd(8)'s virtio networking device. This allows for communicating an MTU
> > to the guest driver and then enforcing it in the emulated device.
> >
> > When the feature is offered, per Virtio v1.1, 5.1.4.1 [1]:
> >
> > "The device MUST NOT pass received packets that exceed mtu (plus low
> > level ethernet header length) size with gso_type NONE or ECN after
> > VIRTIO_NET_F_MTU has been successfully negotiated."
> >
> > (GSO is not supported or negotiated, so it's always NONE. This is
> > primarly because the vmd vionet device also doesn't support or negotiate
> > checksum offloading.)
> >
> > The prior logic in place simply checked the packet was of a allowable
> > size, which meant the largest IP packet (65535) plus an ethernet header.
> >
> > If testing the diff, you can change the VIONET_MTU definition to
> > something other than 1500 and check that a non-OpenBSD guest defaults to
> > using the value and forbids setting it higher. This is easy in an Alpine
> > or Debian Linux guest using:
> >
> > a) to view the mtu: ip link
> > b) to set the mtu: sudo ip link set dev mtu
> >
> > For example:
> >
> > dave@debian:~$ sudo ip link set dev enp0s2 mtu 1501
> > Error: mtu greater than device maximum.
> >
> > Since the diff lacks context of the goto, it jumps to section that
> > advances to the next ring
> >
> > Currently, vio(4) does not negotiate this feature and won't obey it. I'm
> > working on that separately.
> >
> > OK? Feedback?
> >
> > [1]
> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-204
> >
> > Index: virtio.c
> > ===
> > RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> > retrieving revision 1.87
> > diff -u -p -r1.87 virtio.c
> > --- virtio.c18 May 2021 11:06:43 - 1.87
> > +++ virtio.c24 May 2021 01:31:22 -
> > @@ -60,6 +60,7 @@ int nr_vioblk;
> >
> > #define MAXPHYS(64 * 1024) /* max raw I/O transfer size */
> >
> > +#define VIRTIO_NET_F_MTU (1<<3)
> > #define VIRTIO_NET_F_MAC (1<<5)
> >
> > #define VMMCI_F_TIMESYNC (1<<0)
> > @@ -1046,6 +1047,26 @@ virtio_net_io(int dir, uint16_t reg, uin
> > *data = dev->mac[reg -
> > VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI];
> > break;
> > + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 10:
> > + if (sz == 2) {
> > + *data = VIONET_MTU;
> > + } else if (sz == 1) {
> > + *data &= 0xFF00;
> > + *data |= (uint32_t)(VIONET_MTU) & 0xFF;
> > + } else {
> > + log_warnx("%s: illegal read of vionet_mtu",
> > + __progname);
> > + }
> > + break;
> > + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11:
> > + if (sz == 1) {
> > + *data &= 0xFF00;
> > + *data = (uint32_t)(VIONET_MTU >> 8) & 0xFF;
> > + } else {
> > + log_warnx("%s: illegal read of vionet_mtu",
> > + __progname);
> > + }
> > + break;
>
> Is it possible to get proper defines for these two options?
> This VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11 is ugly.
>
We could fix the + 11 part, but about the best we could do would be something
like the following:
VIRTIO_CONFIG_NET_MTU
VIRTIO_CONFIG_NET_MTU + 1
VIRTIO_CONFIG_NET_MTU + 2
VIRTIO_CONFIG_NET_MTU + 3
Since this is a pci config space access and I've seen Linux use 1, 2, and 4 byte
accesses. But, yes, we could improve the actual name.
Once dv@ gets this in I'll go back and redo the other devices (since we do a
similar thing for those as well).
-ml
> > case VIRTIO_CONFIG_DEVICE_FEATURES:
> > *data = dev->cfg.device_feature;
> > break;
> > @@ -1437,7 +1458,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> > size_t pktsz, chunk_size = 0;
> > ssize_t dhcpsz;
> > int ret, num_enq, ofs, spc;
> > - char *vr, *pkt, *dhcppkt;
> > + char *vr, *pkt = NULL, *dhcppkt;
> > struct vring_desc *desc, *pkt_desc, *hdr_desc;
> > struct vring_avail *avail;
> > struct vring_used *used;
> > @@ -1505,12 +1526,13 @@ vionet_notify_tx(struct vionet_dev *dev)
> > /* Remove virtio header descriptor len */
> > pktsz -= hdr_desc->len;
> >
> > - /* Only allow buffer len < max IP packet + Ethernet header */
> > - if (pktsz > IP_MAXPACKET + ETHER_HDR_LEN) {
> > + /* Drop frames larger than our MTU + ethernet header */
> > + if
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 03:52:44PM +0100, Raf Czlonka wrote: > Hello, > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. 0x1ea7 / 7847 is 'SEMITEK INTERNATIONAL (HK) HOLDING LTD.' in https://usb.org/sites/default/files/vendor_ids033021.pdf > > Regards, > > Raf > > Index: sys/dev/usb/usbdevs > === > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > retrieving revision 1.740 > diff -u -p -r1.740 usbdevs > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > @@ -618,6 +618,7 @@ vendor SELUXIT0x1d6f Seluxit > vendor METAGEEK 0x1dd5 MetaGeek > vendor SIMCOM0x1e0e SIMCom Wireless Solutions Co., Ltd. > vendor FESTO 0x1e29 Festo > +vendor SHARKOON 0x1ea7 SHARKOON Technologies GmbH > vendor MODACOM 0x1eb8 Modacom > vendor AIRTIES 0x1eda AirTies > vendor LAKESHORE 0x1fb9 Lake Shore > >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 04:10:00PM BST, Theo de Raadt wrote: > But does it matter? Did this[0] matter? Well, in the grand scheme of things, not many things do, really. Or is it just about the length of the vendor ID? If the latter, then yes - a bit unfortunate that it's on the longer side... I didn't come up with the name. Maybe remove all without any product IDs? Your call. [0] https://cvsweb.openbsd.org/src/sys/dev/usb/usbdevs.diff?r1=1.698=1.699=date=h R. > It adds sizeof pointer + 28 bytes to every OpenBSD kernel. > > I have seriously considered deleting usbdevs device-naming support, > because the cost keeps growing without bound. > > Raf Czlonka wrote: > > > On Mon, May 24, 2021 at 04:00:20PM BST, Theo de Raadt wrote: > > > Without proof it is required, no. > > > > Sure - hope this will suffice. > > > > Before: > > > > uhidev1 at uhub3 port 2 configuration 1 interface 0 "vendor 0x1ea7 2.4G > > Mouse" rev 1.10/2.00 addr 4 > > > > After: > > > > uhidev1 at uhub3 port 2 configuration 1 interface 0 "SHARKOON > > Technologies GmbH 2.4G Mouse" rev 1.10/2.00 addr 4 > > > > Cheers, > > > > Raf > > > > > Raf Czlonka wrote: > > > > > > > Hello, > > > > > > > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. > > > > > > > > Regards, > > > > > > > > Raf > > > > > > > > Index: sys/dev/usb/usbdevs > > > > === > > > > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > > > > retrieving revision 1.740 > > > > diff -u -p -r1.740 usbdevs > > > > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > > > > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > > > > @@ -618,6 +618,7 @@ vendor SELUXIT 0x1d6f Seluxit > > > > vendor METAGEEK0x1dd5 MetaGeek > > > > vendor SIMCOM 0x1e0e SIMCom Wireless Solutions Co., Ltd. > > > > vendor FESTO 0x1e29 Festo > > > > +vendor SHARKOON0x1ea7 SHARKOON Technologies GmbH > > > > vendor MODACOM 0x1eb8 Modacom > > > > vendor AIRTIES 0x1eda AirTies > > > > vendor LAKESHORE 0x1fb9 Lake Shore > > > >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
> Date: Mon, 24 May 2021 15:52:44 +0100 > From: Raf Czlonka > > Hello, > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. Not really self-explanatory. Why do you need this? We typically don't add strings for devices unless we need the vendor ID or device ID in a driver. > Index: sys/dev/usb/usbdevs > === > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > retrieving revision 1.740 > diff -u -p -r1.740 usbdevs > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > @@ -618,6 +618,7 @@ vendor SELUXIT0x1d6f Seluxit > vendor METAGEEK 0x1dd5 MetaGeek > vendor SIMCOM0x1e0e SIMCom Wireless Solutions Co., Ltd. > vendor FESTO 0x1e29 Festo > +vendor SHARKOON 0x1ea7 SHARKOON Technologies GmbH > vendor MODACOM 0x1eb8 Modacom > vendor AIRTIES 0x1eda AirTies > vendor LAKESHORE 0x1fb9 Lake Shore > >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
But does it matter? It adds sizeof pointer + 28 bytes to every OpenBSD kernel. I have seriously considered deleting usbdevs device-naming support, because the cost keeps growing without bound. Raf Czlonka wrote: > On Mon, May 24, 2021 at 04:00:20PM BST, Theo de Raadt wrote: > > Without proof it is required, no. > > Sure - hope this will suffice. > > Before: > > uhidev1 at uhub3 port 2 configuration 1 interface 0 "vendor 0x1ea7 2.4G > Mouse" rev 1.10/2.00 addr 4 > > After: > > uhidev1 at uhub3 port 2 configuration 1 interface 0 "SHARKOON > Technologies GmbH 2.4G Mouse" rev 1.10/2.00 addr 4 > > Cheers, > > Raf > > > Raf Czlonka wrote: > > > > > Hello, > > > > > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. > > > > > > Regards, > > > > > > Raf > > > > > > Index: sys/dev/usb/usbdevs > > > === > > > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > > > retrieving revision 1.740 > > > diff -u -p -r1.740 usbdevs > > > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > > > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > > > @@ -618,6 +618,7 @@ vendor SELUXIT0x1d6f Seluxit > > > vendor METAGEEK 0x1dd5 MetaGeek > > > vendor SIMCOM0x1e0e SIMCom Wireless Solutions Co., Ltd. > > > vendor FESTO 0x1e29 Festo > > > +vendor SHARKOON 0x1ea7 SHARKOON Technologies GmbH > > > vendor MODACOM 0x1eb8 Modacom > > > vendor AIRTIES 0x1eda AirTies > > > vendor LAKESHORE 0x1fb9 Lake Shore > > >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
On Mon, May 24, 2021 at 04:00:20PM BST, Theo de Raadt wrote: > Without proof it is required, no. Sure - hope this will suffice. Before: uhidev1 at uhub3 port 2 configuration 1 interface 0 "vendor 0x1ea7 2.4G Mouse" rev 1.10/2.00 addr 4 After: uhidev1 at uhub3 port 2 configuration 1 interface 0 "SHARKOON Technologies GmbH 2.4G Mouse" rev 1.10/2.00 addr 4 Cheers, Raf > Raf Czlonka wrote: > > > Hello, > > > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. > > > > Regards, > > > > Raf > > > > Index: sys/dev/usb/usbdevs > > === > > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > > retrieving revision 1.740 > > diff -u -p -r1.740 usbdevs > > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > > @@ -618,6 +618,7 @@ vendor SELUXIT 0x1d6f Seluxit > > vendor METAGEEK0x1dd5 MetaGeek > > vendor SIMCOM 0x1e0e SIMCom Wireless Solutions Co., Ltd. > > vendor FESTO 0x1e29 Festo > > +vendor SHARKOON0x1ea7 SHARKOON Technologies GmbH > > vendor MODACOM 0x1eb8 Modacom > > vendor AIRTIES 0x1eda AirTies > > vendor LAKESHORE 0x1fb9 Lake Shore > >
Re: [PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
Without proof it is required, no. Raf Czlonka wrote: > Hello, > > Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. > > Regards, > > Raf > > Index: sys/dev/usb/usbdevs > === > RCS file: /cvs/src/sys/dev/usb/usbdevs,v > retrieving revision 1.740 > diff -u -p -r1.740 usbdevs > --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 > +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - > @@ -618,6 +618,7 @@ vendor SELUXIT0x1d6f Seluxit > vendor METAGEEK 0x1dd5 MetaGeek > vendor SIMCOM0x1e0e SIMCom Wireless Solutions Co., Ltd. > vendor FESTO 0x1e29 Festo > +vendor SHARKOON 0x1ea7 SHARKOON Technologies GmbH > vendor MODACOM 0x1eb8 Modacom > vendor AIRTIES 0x1eda AirTies > vendor LAKESHORE 0x1fb9 Lake Shore >
[PATCH] [src] sys/dev/usb/usbdevs - add "SHARKOON Technologies GmbH" vendor ID
Hello, Pretty self-explanatory - add "SHARKOON Technologies GmbH" vendor ID. Regards, Raf Index: sys/dev/usb/usbdevs === RCS file: /cvs/src/sys/dev/usb/usbdevs,v retrieving revision 1.740 diff -u -p -r1.740 usbdevs --- sys/dev/usb/usbdevs 18 May 2021 14:23:03 - 1.740 +++ sys/dev/usb/usbdevs 24 May 2021 14:37:14 - @@ -618,6 +618,7 @@ vendor SELUXIT 0x1d6f Seluxit vendor METAGEEK0x1dd5 MetaGeek vendor SIMCOM 0x1e0e SIMCom Wireless Solutions Co., Ltd. vendor FESTO 0x1e29 Festo +vendor SHARKOON0x1ea7 SHARKOON Technologies GmbH vendor MODACOM 0x1eb8 Modacom vendor AIRTIES 0x1eda AirTies vendor LAKESHORE 0x1fb9 Lake Shore
usbhidctl: add -R flag to dump raw report descriptor bytes
This is useful for parsing the report descriptor with a different
tool to find issues with our HID parser.
I've found https://eleccelerator.com/usbdescreqparser/ to be
helpful.
diff --git usr.bin/usbhidctl/usbhid.c usr.bin/usbhidctl/usbhid.c
index 921f211a280..bd0b5da0222 100644
--- usr.bin/usbhidctl/usbhid.c
+++ usr.bin/usbhidctl/usbhid.c
@@ -106,6 +106,11 @@ static struct {
#define REPORT_MAXVAL 2
};
+struct report_desc {
+ uint32_t size;
+ uint8_t data[1];
+};
+
/*
* Extract 16-bit unsigned usage ID from a numeric string. Returns -1
* if string failed to parse correctly.
@@ -747,7 +752,7 @@ usage(void)
fprintf(stderr, "usage: %s -f device [-t table] [-alv]\n",
__progname);
- fprintf(stderr, " %s -f device [-t table] [-v] -r\n",
+ fprintf(stderr, " %s -f device [-t table] [-v] [-r|-R]\n",
__progname);
fprintf(stderr,
" %s -f device [-t table] [-lnv] name ...\n",
@@ -764,16 +769,16 @@ main(int argc, char **argv)
char const *dev;
char const *table;
size_t varnum;
- int aflag, lflag, nflag, rflag, wflag;
- int ch, hidfd;
+ int aflag, lflag, nflag, rflag, Rflag, wflag;
+ int ch, hidfd, x;
report_desc_t repdesc;
char devnamebuf[PATH_MAX];
struct Susbvar variables[128];
- wflag = aflag = nflag = verbose = rflag = lflag = 0;
+ wflag = aflag = nflag = verbose = rflag = Rflag = lflag = 0;
dev = NULL;
table = NULL;
- while ((ch = getopt(argc, argv, "?af:lnrt:vw")) != -1) {
+ while ((ch = getopt(argc, argv, "?af:lnRrt:vw")) != -1) {
switch (ch) {
case 'a':
aflag = 1;
@@ -790,6 +795,9 @@ main(int argc, char **argv)
case 'r':
rflag = 1;
break;
+ case 'R':
+ Rflag = 1;
+ break;
case 't':
table = optarg;
break;
@@ -807,7 +815,8 @@ main(int argc, char **argv)
}
argc -= optind;
argv += optind;
- if (dev == NULL || (lflag && (wflag || rflag))) {
+ if (dev == NULL || (lflag && (wflag || rflag || Rflag)) ||
+ (rflag && Rflag)) {
/*
* No device specified, or attempting to loop and set
* or dump report at the same time
@@ -942,6 +951,12 @@ main(int argc, char **argv)
if (repdesc == 0)
errx(1, "USB_GET_REPORT_DESC");
+ if (Rflag) {
+ for (x = 0; x < repdesc->size; x++)
+ printf("%s0x%02x", x > 0 ? " " : "", repdesc->data[x]);
+ printf("\n");
+ }
+
if (lflag) {
devloop(hidfd, repdesc, variables, varnum);
/* NOTREACHED */
@@ -951,10 +966,11 @@ main(int argc, char **argv)
/* Report mode header */
printf("Report descriptor:\n");
- devshow(hidfd, repdesc, variables, varnum,
- 1 << hid_input |
- 1 << hid_output |
- 1 << hid_feature);
+ if (!Rflag)
+ devshow(hidfd, repdesc, variables, varnum,
+ 1 << hid_input |
+ 1 << hid_output |
+ 1 << hid_feature);
if (rflag) {
/* Report mode trailer */
diff --git usr.bin/usbhidctl/usbhidctl.1 usr.bin/usbhidctl/usbhidctl.1
index 5b8e59f7bd7..54f5a49270d 100644
--- usr.bin/usbhidctl/usbhidctl.1
+++ usr.bin/usbhidctl/usbhidctl.1
@@ -47,6 +47,11 @@
.Nm
.Fl f Ar device
.Op Fl t Ar table
+.Op Fl v
+.Fl R
+.Nm
+.Fl f Ar device
+.Op Fl t Ar table
.Op Fl lnv
.Ar name ...
.Nm
@@ -90,6 +95,8 @@ Suppress printing of the item name when querying specific
items.
Only output the current value.
.It Fl r
Dump the USB HID report descriptor.
+.It Fl R
+Dump the USB HID report descriptor as hexadecimal bytes.
.It Fl t Ar table
Specify a path name for the HID usage table file.
.It Fl v
hidms: don't ignore mice with no x/y coordinates
A bug was reported where a Kensington USB trackball didn't work
properly:
uhidev4 at uhub0 port 6 configuration 1 interface 0 "Kensington Expert
Wireless TB" rev 2.00/1.02 addr 9
uhidev4: iclass 3/1, 3 report ids
ums3 at uhidev4 reportid 1
ums3: mouse has no X report
ums4 at uhidev4 reportid 2: 0 button
wsmouse3 at ums4 mux 0
After looking at the HID report descriptor, this device is weird in
that it puts the buttons and wheel on one report and the trackball
X/Y coordinates an another. This causes uhidev to attach two ums
devices, but the first one fails because there are no X/Y reports
found.
The proper fix is probably to make ums act like umt and use
UHIDEV_CLAIM_MULTIPLE_REPORTID to find all of the necessary reports
and attach to multiple at once if needed. I started working on this
but all of the logic in hidms_setup gets tricky when it has to look
at multiple reports. So an easier fix is to just not consider a
mouse with no X/Y reports invalid.
Now the device attaches to the first button/wheel report:
uhidev4 at uhub4 port 4 configuration 1 interface 0 "Kensington Expert
Wireless TB" rev 2.00/1.02 addr 3
uhidev4: iclass 3/1, 3 report ids
ums1 at uhidev4 reportid 1: 5 buttons, Z and W dir
wsmouse1 at ums1 mux 0
ums2 at uhidev4 reportid 2: 0 buttons
wsmouse2 at ums2 mux 0
Checking dmesglog for 'no X report' yields a lot of results, so this
may help on other devices.
diff --git sys/dev/hid/hidms.c sys/dev/hid/hidms.c
index ab9cd9c797e..92ca89537da 100644
--- sys/dev/hid/hidms.c
+++ sys/dev/hid/hidms.c
@@ -76,10 +76,9 @@ hidms_setup(struct device *self, struct hidms *ms, uint32_t
quirks,
ms->sc_flags = quirks;
if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X), id,
- hid_input, >sc_loc_x, )) {
- printf("\n%s: mouse has no X report\n", self->dv_xname);
- return ENXIO;
- }
+ hid_input, >sc_loc_x, ))
+ ms->sc_loc_x.size = 0;
+
switch(flags & MOUSE_FLAGS_MASK) {
case 0:
ms->sc_flags |= HIDMS_ABSX;
@@ -93,10 +92,9 @@ hidms_setup(struct device *self, struct hidms *ms, uint32_t
quirks,
}
if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y), id,
- hid_input, >sc_loc_y, )) {
- printf("\n%s: mouse has no Y report\n", self->dv_xname);
- return ENXIO;
- }
+ hid_input, >sc_loc_y, ))
+ ms->sc_loc_y.size = 0;
+
switch(flags & MOUSE_FLAGS_MASK) {
case 0:
ms->sc_flags |= HIDMS_ABSY;
@@ -292,7 +290,7 @@ hidms_attach(struct hidms *ms, const struct
wsmouse_accessops *ops)
#endif
printf(": %d button%s",
- ms->sc_num_buttons, ms->sc_num_buttons <= 1 ? "" : "s");
+ ms->sc_num_buttons, ms->sc_num_buttons == 1 ? "" : "s");
switch (ms->sc_flags & (HIDMS_Z | HIDMS_W)) {
case HIDMS_Z:
printf(", Z dir");
ld.so: program headers: do not rely on DYNAMIC coming before GNU_RELRO
When tinkering with ld.so crashes due to file corruption the other day
I tested a few changes but did not want to replace /usr/libexec/ld.so
and since recompiling executable to change their interpreter is not
always an option, I went for https://github.com/NixOS/patchelf which
allows me to manipulate executables in place.
The tool works just fine and as a byproduct rearanges program headers;
that in itself is fine except our run-time link-editor is not happy with
such valid executables:
ELF mandates nothing but the file header be at a fixed location, hence
ld.so(1) must not assume any specific order for headers, segments, etc.
Looping over the program header table to parse segment headers,
_dl_boot() creates the executable object upon DYNAMIC and expects it to
be set upon GNU_RELRO, resulting in a NULL dereference iff that order is
reversed.
Store relocation bits in temporary variables and update the executable
object once all segment headers are parsed to lift this dependency.
Under __mips__ _dl_boot() later on uses the same temporary variable, so
move nothing but the declaration out of MI code so as to not alter the
MD code's logic/behaviour.
This fix is needed for the following work on OpenBSD:
$ patchelf --set-interpreter $PWD/obj/ld.so ./my-ldso-test
$ readelf -l ./my-ldso-test | grep interpreter
[Requesting program interpreter: /usr/src/libexec/ld.so/obj/ld.so]
$ ./my-ldso-test
it works!
amd64 and arm64 regress is happy and all my patched executables work
with this.
Feedback? Objections? OK?
diff d7231fb4fb547dd287a884c56ae7c8b10f9145fe
f023dbe355bef379d55eb93eddbb2702559d5bdb
blob - 18bd30af759bffbc4e3fbfee9ffc29906f0d1bb0
blob + b66dbb169aad9afffa1283d480ad9276aff9072a
--- libexec/ld.so/loader.c
+++ libexec/ld.so/loader.c
@@ -464,6 +464,7 @@ _dl_boot(const char **argv, char **envp, const long dy
int failed;
struct dep_node *n;
Elf_Addr minva, maxva, exe_loff, exec_end, cur_exec_end;
+ Elf_Addr relro_addr = 0, relro_size = 0;
Elf_Phdr *ptls = NULL;
int align;
@@ -552,8 +553,8 @@ _dl_boot(const char **argv, char **envp, const long dy
ptls = phdp;
break;
case PT_GNU_RELRO:
- exe_obj->relro_addr = phdp->p_vaddr + exe_loff;
- exe_obj->relro_size = phdp->p_memsz;
+ relro_addr = phdp->p_vaddr + exe_loff;
+ relro_size = phdp->p_memsz;
break;
}
phdp++;
@@ -561,6 +562,8 @@ _dl_boot(const char **argv, char **envp, const long dy
exe_obj->load_list = load_list;
exe_obj->obj_flags |= DF_1_GLOBAL;
exe_obj->load_size = maxva - minva;
+ exe_obj->relro_addr = relro_addr;
+ exe_obj->relro_size = relro_size;
_dl_set_sod(exe_obj->load_name, _obj->sod);
#ifdef __i386__
@@ -638,7 +641,7 @@ _dl_boot(const char **argv, char **envp, const long dy
debug_map->r_ldbase = dyn_loff;
_dl_debug_map = debug_map;
#ifdef __mips__
- Elf_Addr relro_addr = exe_obj->relro_addr;
+ relro_addr = exe_obj->relro_addr;
if (dynp->d_tag == DT_DEBUG &&
((Elf_Addr)map_link + sizeof(*map_link) <= relro_addr ||
(Elf_Addr)map_link >= relro_addr + exe_obj->relro_size)) {
Re: vmd(8): add MTU feature support to vionet device
On Sun, May 23, 2021 at 10:25:38PM -0400, Dave Voutila wrote:
> The following diff adds in virtio 1.1's VIRTIO_NET_F_MTU feature support
> to vmd(8)'s virtio networking device. This allows for communicating an MTU
> to the guest driver and then enforcing it in the emulated device.
>
> When the feature is offered, per Virtio v1.1, 5.1.4.1 [1]:
>
> "The device MUST NOT pass received packets that exceed mtu (plus low
> level ethernet header length) size with gso_type NONE or ECN after
> VIRTIO_NET_F_MTU has been successfully negotiated."
>
> (GSO is not supported or negotiated, so it's always NONE. This is
> primarly because the vmd vionet device also doesn't support or negotiate
> checksum offloading.)
>
> The prior logic in place simply checked the packet was of a allowable
> size, which meant the largest IP packet (65535) plus an ethernet header.
>
> If testing the diff, you can change the VIONET_MTU definition to
> something other than 1500 and check that a non-OpenBSD guest defaults to
> using the value and forbids setting it higher. This is easy in an Alpine
> or Debian Linux guest using:
>
> a) to view the mtu: ip link
> b) to set the mtu: sudo ip link set dev mtu
>
> For example:
>
> dave@debian:~$ sudo ip link set dev enp0s2 mtu 1501
> Error: mtu greater than device maximum.
>
> Since the diff lacks context of the goto, it jumps to section that
> advances to the next ring
>
> Currently, vio(4) does not negotiate this feature and won't obey it. I'm
> working on that separately.
>
> OK? Feedback?
>
> [1]
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-204
>
> Index: virtio.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 virtio.c
> --- virtio.c 18 May 2021 11:06:43 - 1.87
> +++ virtio.c 24 May 2021 01:31:22 -
> @@ -60,6 +60,7 @@ int nr_vioblk;
>
> #define MAXPHYS (64 * 1024) /* max raw I/O transfer size */
>
> +#define VIRTIO_NET_F_MTU (1<<3)
> #define VIRTIO_NET_F_MAC (1<<5)
>
> #define VMMCI_F_TIMESYNC (1<<0)
> @@ -1046,6 +1047,26 @@ virtio_net_io(int dir, uint16_t reg, uin
> *data = dev->mac[reg -
> VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI];
> break;
> + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 10:
> + if (sz == 2) {
> + *data = VIONET_MTU;
> + } else if (sz == 1) {
> + *data &= 0xFF00;
> + *data |= (uint32_t)(VIONET_MTU) & 0xFF;
> + } else {
> + log_warnx("%s: illegal read of vionet_mtu",
> + __progname);
> + }
> + break;
> + case VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11:
> + if (sz == 1) {
> + *data &= 0xFF00;
> + *data = (uint32_t)(VIONET_MTU >> 8) & 0xFF;
> + } else {
> + log_warnx("%s: illegal read of vionet_mtu",
> + __progname);
> + }
> + break;
Is it possible to get proper defines for these two options?
This VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI + 11 is ugly.
> case VIRTIO_CONFIG_DEVICE_FEATURES:
> *data = dev->cfg.device_feature;
> break;
> @@ -1437,7 +1458,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> size_t pktsz, chunk_size = 0;
> ssize_t dhcpsz;
> int ret, num_enq, ofs, spc;
> - char *vr, *pkt, *dhcppkt;
> + char *vr, *pkt = NULL, *dhcppkt;
> struct vring_desc *desc, *pkt_desc, *hdr_desc;
> struct vring_avail *avail;
> struct vring_used *used;
> @@ -1505,12 +1526,13 @@ vionet_notify_tx(struct vionet_dev *dev)
> /* Remove virtio header descriptor len */
> pktsz -= hdr_desc->len;
>
> - /* Only allow buffer len < max IP packet + Ethernet header */
> - if (pktsz > IP_MAXPACKET + ETHER_HDR_LEN) {
> + /* Drop frames larger than our MTU + ethernet header */
> + if (pktsz > VIONET_MTU + ETHER_HDR_LEN) {
> log_warnx("%s: invalid packet size %lu", __func__,
> pktsz);
> - goto out;
> + goto drop_packet;
> }
> +
> pkt = malloc(pktsz);
> if (pkt == NULL) {
> log_warn("malloc error alloc packet buf");
> @@ -1590,6 +1612,7 @@ vionet_notify_tx(struct vionet_dev *dev)
> goto out;
> }
>
> + drop_packet:
> ret = 1;
> dev->cfg.isr_status = 1;
> used->ring[used->idx &
