From: Alexander Bluhm Subject: Re: Virtio: Prefer 1.x over 0.9 (please test) To: Stefan Fritsch Cc: tech@openbsd.org Date: Mon, 20 Jan 2025 18:07:02 +0100 On Thu, Jan 16, 2025 at 01:15:46PM +0100, Stefan Fritsch wrote: > Hi, > > tl;dr > Please test this diff if you run openbsd on non-qemu/non-vmd hypervisors > that use virtio, for example on bhyve or VirtualBox. Or if you have > unusual virtio configurations. > > The virtio 1.x code has been in openbsd for several years. I think it is > time to make the drivers attach with 1.x instead of 0.9 if the hypervisor > offers both. This is important as only 1.x supports more than 32 feature > bits and virtio-network's multiqueue feature uses one of the high bits. > > This only affects virtio_pci, as there are no virtio_mmio devices that > support both versions. The diff also tweaks the matching code to be more > compliant with the virtio 1.x standard which says "Drivers MUST match any > PCI Revision ID value". The diff should not change anything on openbsd > vmd, which only offers 0.9. > > It could lead to a slight performance degradation in vioblk(4) which uses > the notify-on-empty feature as an optimization. That feature exists in 0.9 > only. However, I did some simple benchmarks and the performance difference > was lost in the noise. I also have a diff that switches vioblk(4) to use > the event-index feature instead, but that needs some more polishing and > testing. > > I have tested the diff on openbsd vmd and on qemu/kvm with various > configurations. If you run openbsd on other hypervisors that use virtio > devices, like bhyve or VirtualBox, I would be interested in test results. > Also if you run it on qemu with unusual virtio setups, like vhost-user. If > all of the virtio devices still attach successfully with the diff, > everything should be fine. > > Comments welcome. I have tested network sucessfully on KVM/qemu, vmm/vmd, and AMD SEV vmm/vmd using bounce buffers. No other systems available. bluhm > > Thanks in advance. > > Cheers, > Stefan > > diff --git a/share/man/man4/virtio.4 b/share/man/man4/virtio.4 > index 017dc55cddb..f6d80d6ee6f 100644 > --- a/share/man/man4/virtio.4 > +++ b/share/man/man4/virtio.4 > @@ -60,9 +60,8 @@ The > driver conforms to the virtio 0.9.5 specification. > The virtio 1.0 standard is only supported for PCI devices. > .Pp > -By default 0.9 is preferred over 1.0. > -This can be changed by setting the bit 0x4 in the flags. > -Setting the bit 0x8 in the flags disables 1.0 support completely. > +By default 1.x is preferred over 0.9. > +This can be changed by setting the bit 0x8 in the flags. > .Sh SEE ALSO > .Xr intro 4 > .Sh HISTORY > diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c > index 2c59e899cad..e9dad0e2fff 100644 > --- a/sys/dev/pci/virtio_pci.c > +++ b/sys/dev/pci/virtio_pci.c > @@ -362,8 +362,7 @@ virtio_pci_match(struct device *parent, void *match, void *aux) > return 1; > /* virtio 1.0 */ > if (PCI_PRODUCT(pa->pa_id) >= 0x1040 && > - PCI_PRODUCT(pa->pa_id) <= 0x107f && > - PCI_REVISION(pa->pa_class) == 1) > + PCI_PRODUCT(pa->pa_id) <= 0x107f) > return 1; > return 0; > } > @@ -595,21 +594,24 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux) > struct pci_attach_args *pa = (struct pci_attach_args *)aux; > pci_chipset_tag_t pc = pa->pa_pc; > pcitag_t tag = pa->pa_tag; > - int revision, ret = ENODEV; > + int revision, product, vendor, ret = ENODEV, flags; > pcireg_t id; > struct virtio_pci_attach_args vpa = { { 0 }, pa }; > > revision = PCI_REVISION(pa->pa_class); > - switch (revision) { > - case 0: > - /* subsystem ID shows what I am */ > + product = PCI_PRODUCT(pa->pa_id); > + vendor = PCI_VENDOR(pa->pa_id); > + if (vendor == PCI_VENDOR_OPENBSD || > + (product >= 0x1000 && product <= 0x103f && revision == 0)) { > + /* OpenBSD VMMCI and virtio 0.9 */ > id = PCI_PRODUCT(pci_conf_read(pc, tag, PCI_SUBSYS_ID_REG)); > - break; > - case 1: > - id = PCI_PRODUCT(pa->pa_id) - 0x1040; > - break; > - default: > - printf("unknown revision 0x%02x; giving up\n", revision); > + } else if (product >= 0x1040 && product <= 0x107f) { > + /* virtio 1.0 */ > + id = product - 0x1040; > + revision = 1; > + } else { > + printf("unknown device prod 0x%04x rev 0x%02x; giving up\n", > + product, revision); > return; > } > > @@ -637,15 +639,15 @@ virtio_pci_attach(struct device *parent, struct device *self, void *aux) > M_DEVBUF, M_WAITOK | M_ZERO); > > vsc->sc_ops = &virtio_pci_ops; > - if ((vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_NO_VERSION_1) == 0 && > - (revision == 1 || > - (vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_PREFER_VERSION_1))) { > + flags = vsc->sc_dev.dv_cfdata->cf_flags; > + if ((flags & VIRTIO_CF_PREFER_VERSION_09) == 0) > ret = virtio_pci_attach_10(sc, pa); > - } > if (ret != 0 && revision == 0) { > /* revision 0 means 0.9 only or both 0.9 and 1.0 */ > ret = virtio_pci_attach_09(sc, pa); > } > + if (ret != 0 && (flags & VIRTIO_CF_PREFER_VERSION_09)) > + ret = virtio_pci_attach_10(sc, pa); > if (ret != 0) { > printf(": Cannot attach (%d)\n", ret); > goto free; > diff --git a/sys/dev/pv/virtiovar.h b/sys/dev/pv/virtiovar.h > index 029f5743845..692af968557 100644 > --- a/sys/dev/pv/virtiovar.h > +++ b/sys/dev/pv/virtiovar.h > @@ -82,8 +82,7 @@ > /* flags for config(8) */ > #define VIRTIO_CF_NO_INDIRECT 1 > #define VIRTIO_CF_NO_EVENT_IDX 2 > -#define VIRTIO_CF_PREFER_VERSION_1 4 > -#define VIRTIO_CF_NO_VERSION_1 8 > +#define VIRTIO_CF_PREFER_VERSION_09 8 > > struct virtio_attach_args { > int va_devid; /* virtio device id */