Download raw body.
Virtio: Prefer 1.x over 0.9 (please test)
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 */
Virtio: Prefer 1.x over 0.9 (please test)