From: Stefan Fritsch <sf@openbsd.org>
Subject: Virtio: Prefer 1.x over 0.9 (please test)
To: tech@openbsd.org
Date: Thu, 16 Jan 2025 13:15:46 +0100

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.

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 */