virtio-pci: flexible configuration layout
Subsystems (5)
Michael S. Tsirkin
maintainerRFC PATCH v3 1/1 virtio-pci: flexible configuration layout
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.
Compiled only. Posting here for early feedback, and to allow Sasha to proceed with his "kvm tool" work.
Changes from v2:
address comments by Rusty
bugfixes by Sasha
Changes from v1:
Updated to match v3 of the spec, see:
todo:
split core changes out
--- drivers/virtio/Kconfig | 22 +++++ drivers/virtio/virtio_pci.c | 203 ++++++++++++++++++++++++++++++++++++++++--- include/asm-generic/io.h | 4 + include/asm-generic/iomap.h | 11 +++ include/linux/pci_regs.h | 4 + include/linux/virtio_pci.h | 41 +++++++++ lib/iomap.c | 41 ++++++++- 7 files changed, 307 insertions(+), 19 deletions(-)
drivers/virtio/Kconfig +22 -0
@@ -25,6 +25,28 @@ config VIRTIO_PCI If unsure, say M. +config VIRTIO_PCI_LEGACY + bool "Compatibility with Legacy PIO" + default y + depends on VIRTIO_PCI + ---help--- + Look out into your driveway. Do you have a flying car? If + so, you can happily disable this option and virtio will not + break. Otherwise, leave it set. Unless you're testing what + life will be like in The Future. + + In other words: + + Support compatibility with legacy PIO mapping in hypervisors. + As of Nov 2011, this is required by all hypervisors without + exception, so for now, disabling this option is only useful for + testing. In future hypervisors, it will be possible to disable + this option and get a slightly smaller kernel. + You know that your hypervisor is new enough if you disable this + option and the device initialization passes. + + If unsure, say Y. + config VIRTIO_BALLOON tristate "Virtio balloon driver (EXPERIMENTAL)" select VIRTIO
drivers/virtio/virtio_pci.c +189 -14
278 lines not on this page · raw message
include/asm-generic/io.h +4 -0
@@ -286,6 +286,10 @@ static inline void writesb(const void __iomem *addr, const void *buf, int len)/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */ struct pci_dev ;extern void __iomem *pci_iomap (struct pci_dev *dev,int bar,unsigned long max); +extern void __iomem *pci_iomap_range (struct pci_dev *dev,int bar, +unsigned offset, +unsigned long minlen, +unsigned long maxlen);static inline void pci_iounmap (struct pci_dev *dev,void __iomem *p) { }
include/asm-generic/iomap.h +11 -0
@@ -70,8 +70,19 @@ extern void ioport_unmap(void __iomem *);/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */ struct pci_dev ;extern void __iomem *pci_iomap (struct pci_dev *dev,int bar,unsigned long max); +extern void __iomem *pci_iomap_range (struct pci_dev *dev,int bar, +unsigned offset, +unsigned long minlen, +unsigned long maxlen);extern void pci_iounmap (struct pci_dev *dev,void __iomem *);#else +static inline void __iomem *pci_iomap_range (struct pci_dev *dev,int bar, +unsigned offset, +unsigned long minlen, +unsigned long maxlen) +{ +return NULL ; +}struct pci_dev ;static inline void __iomem *pci_iomap (struct pci_dev *dev,int bar,unsigned long max) {
include/linux/pci_regs.h +4 -0
@@ -375,6 +375,10 @@#define PCI_X_STATUS_266MHZ 0x40000000/* 266 MHz capable */ #define PCI_X_STATUS_533MHZ 0x80000000/* 533 MHz capable */ +/* Vendor specific capability */ +#define PCI_VNDR_CAP_LEN 2/* Capability length (8 bits), including +bytes: ID, NEXT and LEN itself. */ +/* PCI Bridge Subsystem ID registers */ #define PCI_SSVID_VENDOR_ID 4/* PCI-Bridge subsystem vendor id register */
include/linux/virtio_pci.h +41 -0
45 lines not on this page · raw message
lib/iomap.c +36 -5
72 lines not on this page · raw message
Signature · 1 line
1.7.5.53.gc233e
Quoted 3 lines · Michael S. Tsirkin
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.
Don't touch the spec yet, we have a long way to go :(
I want the ability for driver to set the ring size, and the device to set the alignment. That's a bigger change than you have here. I imagine it almost rips the driver into two completely different drivers.
This is the kind of thing I had in mind, for the header. Want me to code up the rest?
(I've avoided adding the constants for the new layout: a struct is more compact and more descriptive).
Cheers,
Rusty.
include/linux/virtio_pci.h +56 -22
@@ -42,56 +42,74 @@#include <linux/virtio_config.h> /* A 32-bit r/o bitmask of the features supported by the host */ -#define VIRTIO_PCI_HOST_FEATURES 0 +#define VIRTIO_PCI_LEGACY_HOST_FEATURES 0/* A 32-bit r/w bitmask of features activated by the guest */ -#define VIRTIO_PCI_GUEST_FEATURES 4 +#define VIRTIO_PCI_LEGACY_GUEST_FEATURES 4/* A 32-bit r/w PFN for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_PFN 8 +#define VIRTIO_PCI_LEGACY_QUEUE_PFN 8/* A 16-bit r/o queue size for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_NUM 12 +#define VIRTIO_PCI_LEGACY_QUEUE_NUM 12/* A 16-bit r/w queue selector */ -#define VIRTIO_PCI_QUEUE_SEL 14 +#define VIRTIO_PCI_LEGACY_QUEUE_SEL 14/* A 16-bit r/w queue notifier */ -#define VIRTIO_PCI_QUEUE_NOTIFY 16 +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY 16/* An 8-bit device status register. */ -#define VIRTIO_PCI_STATUS 18 +#define VIRTIO_PCI_LEGACY_STATUS 18/* An 8-bit r/o interrupt status register. Reading the value will return the * current contents of the ISR and will also clear it. This is effectively * a read-and-acknowledge. */ -#define VIRTIO_PCI_ISR 19 - -/* The bit of the ISR which indicates a device configuration change. */ -#define VIRTIO_PCI_ISR_CONFIG 0x2 +#define VIRTIO_PCI_LEGACY_ISR 19/* MSI-X registers: only enabled if MSI-X is enabled. */ /* A 16-bit vector for configuration changes. */ -#define VIRTIO_MSI_CONFIG_VECTOR 20 +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR 20/* A 16-bit vector for selected queue notifications. */ -#define VIRTIO_MSI_QUEUE_VECTOR 22 -/* Vector value used to disable MSI for queue */ -#define VIRTIO_MSI_NO_VECTOR 0xffff +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR 22/* The remaining space is defined by each driver as the per-driver * configuration space */ -#define VIRTIO_PCI_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) +#define VIRTIO_PCI_LEGACY_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) + +/* How many bits to shift physical queue address written to QUEUE_PFN. +* 12 is historical, and due to x86 page size. */ +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT 12 + +/* The alignment to use between consumer and producer parts of vring. +* x86 pagesize again. */ +#define VIRTIO_PCI_LEGACY_VRING_ALIGN 4096 + +#ifndef __KERNEL__ +/* Don't break compile of old userspace code. These will go away. */ +#define VIRTIO_PCI_HOST_FEATURES VIRTIO_PCI_LEGACY_HOST_FEATURES +#define VIRTIO_PCI_GUEST_FEATURES VIRTIO_PCI_LEGACY_GUEST_FEATURES +#define VIRTIO_PCI_LEGACY_QUEUE_PFN VIRTIO_PCI_QUEUE_PFN +#define VIRTIO_PCI_LEGACY_QUEUE_NUM VIRTIO_PCI_QUEUE_NUM +#define VIRTIO_PCI_LEGACY_QUEUE_SEL VIRTIO_PCI_QUEUE_SEL +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY VIRTIO_PCI_QUEUE_NOTIFY +#define VIRTIO_PCI_LEGACY_STATUS VIRTIO_PCI_STATUS +#define VIRTIO_PCI_LEGACY_ISR VIRTIO_PCI_ISR +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR VIRTIO_MSI_CONFIG_VECTOR +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR VIRTIO_MSI_QUEUE_VECTOR +#define VIRTIO_PCI_LEGACY_CONFIG (dev) VIRTIO_PCI_CONFIG(dev) +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT VIRTIO_PCI_QUEUE_ADDR_SHIFT +#define VIRTIO_PCI_LEGACY_VRING_ALIGN VIRTIO_PCI_VRING_ALIGN +#endif /* ...!KERNEL */ /* Virtio ABI version, this must match exactly */ #define VIRTIO_PCI_ABI_VERSION 0 -/* How many bits to shift physical queue address written to QUEUE_PFN. -* 12 is historical, and due to x86 page size. */ -#define VIRTIO_PCI_QUEUE_ADDR_SHIFT 12 +/* Vector value used to disable MSI for queue */ +#define VIRTIO_MSI_NO_VECTOR 0xffff -/* The alignment to use between consumer and producer parts of vring. -* x86 pagesize again. */ -#define VIRTIO_PCI_VRING_ALIGN 4096 +/* The bit of the ISR which indicates a device configuration change. */ +#define VIRTIO_PCI_ISR_CONFIG 0x2/* * Layout for Virtio PCI vendor specific capability (little-endian):
@@ -133,4 +151,20 @@#define VIRTIO_PCI_CAP_CFG_OFF_MASK 0xffffffff#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT 0 +/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ +__u64 queue_address;/* read-write */ +__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; + +/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ +__u16 queue_msix_vector;/* read-write */ +};#endif
Michael S. Tsirkin
maintainerQuoted 9 lines · Rusty Russell
Quoted 3 lines · Michael S. Tsirkin
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.Don't touch the spec yet, we have a long way to go :(
I want the ability for driver to set the ring size, and the device to set the alignment.
Did you mean driver to be able to set the alignment? This
is what BIOS guys want - after BIOS completes, guest driver gets handed control and sets its own alignment to save memory.
Quoted 1 line
That's a bigger change than you have here.
Why can't we just add the new registers at the end?
With the new capability, we have as much space as we like for that.
Quoted 1 line
I imagine it almost rips the driver into two completely different drivers.
If you insist on moving all the rest of registers around, certainly. But why do this?
Quoted 8 lines
This is the kind of thing I had in mind, for the header. Want me to code up the rest?
(I've avoided adding the constants for the new layout: a struct is more compact and more descriptive).
Cheers,
Rusty.
Renaming constants in exported headers will break userspace builds. Do we care? Why not?
Quoted 110 lines
include/linux/virtio_pci.h +46
-22@@ -42,56 +42,74 @@#include <linux/virtio_config.h> /* A 32-bit r/o bitmask of the features supported by the host */ -#define VIRTIO_PCI_HOST_FEATURES 0 +#define VIRTIO_PCI_LEGACY_HOST_FEATURES 0/* A 32-bit r/w bitmask of features activated by the guest */ -#define VIRTIO_PCI_GUEST_FEATURES 4 +#define VIRTIO_PCI_LEGACY_GUEST_FEATURES 4/* A 32-bit r/w PFN for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_PFN 8 +#define VIRTIO_PCI_LEGACY_QUEUE_PFN 8/* A 16-bit r/o queue size for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_NUM 12 +#define VIRTIO_PCI_LEGACY_QUEUE_NUM 12/* A 16-bit r/w queue selector */ -#define VIRTIO_PCI_QUEUE_SEL 14 +#define VIRTIO_PCI_LEGACY_QUEUE_SEL 14/* A 16-bit r/w queue notifier */ -#define VIRTIO_PCI_QUEUE_NOTIFY 16 +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY 16/* An 8-bit device status register. */ -#define VIRTIO_PCI_STATUS 18 +#define VIRTIO_PCI_LEGACY_STATUS 18/* An 8-bit r/o interrupt status register. Reading the value will return the * current contents of the ISR and will also clear it. This is effectively * a read-and-acknowledge. */ -#define VIRTIO_PCI_ISR 19 - -/* The bit of the ISR which indicates a device configuration change. */ -#define VIRTIO_PCI_ISR_CONFIG 0x2 +#define VIRTIO_PCI_LEGACY_ISR 19/* MSI-X registers: only enabled if MSI-X is enabled. */ /* A 16-bit vector for configuration changes. */ -#define VIRTIO_MSI_CONFIG_VECTOR 20 +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR 20/* A 16-bit vector for selected queue notifications. */ -#define VIRTIO_MSI_QUEUE_VECTOR 22 -/* Vector value used to disable MSI for queue */ -#define VIRTIO_MSI_NO_VECTOR 0xffff +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR 22/* The remaining space is defined by each driver as the per-driver * configuration space */ -#define VIRTIO_PCI_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) +#define VIRTIO_PCI_LEGACY_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) + +/* How many bits to shift physical queue address written to QUEUE_PFN. +* 12 is historical, and due to x86 page size. */ +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT 12 + +/* The alignment to use between consumer and producer parts of vring. +* x86 pagesize again. */ +#define VIRTIO_PCI_LEGACY_VRING_ALIGN 4096 + +#ifndef __KERNEL__ +/* Don't break compile of old userspace code. These will go away. */ +#define VIRTIO_PCI_HOST_FEATURES VIRTIO_PCI_LEGACY_HOST_FEATURES +#define VIRTIO_PCI_GUEST_FEATURES VIRTIO_PCI_LEGACY_GUEST_FEATURES +#define VIRTIO_PCI_LEGACY_QUEUE_PFN VIRTIO_PCI_QUEUE_PFN +#define VIRTIO_PCI_LEGACY_QUEUE_NUM VIRTIO_PCI_QUEUE_NUM +#define VIRTIO_PCI_LEGACY_QUEUE_SEL VIRTIO_PCI_QUEUE_SEL +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY VIRTIO_PCI_QUEUE_NOTIFY +#define VIRTIO_PCI_LEGACY_STATUS VIRTIO_PCI_STATUS +#define VIRTIO_PCI_LEGACY_ISR VIRTIO_PCI_ISR +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR VIRTIO_MSI_CONFIG_VECTOR +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR VIRTIO_MSI_QUEUE_VECTOR +#define VIRTIO_PCI_LEGACY_CONFIG (dev) VIRTIO_PCI_CONFIG(dev) +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT VIRTIO_PCI_QUEUE_ADDR_SHIFT +#define VIRTIO_PCI_LEGACY_VRING_ALIGN VIRTIO_PCI_VRING_ALIGN +#endif /* ...!KERNEL */ /* Virtio ABI version, this must match exactly */ #define VIRTIO_PCI_ABI_VERSION 0 -/* How many bits to shift physical queue address written to QUEUE_PFN. -* 12 is historical, and due to x86 page size. */ -#define VIRTIO_PCI_QUEUE_ADDR_SHIFT 12 +/* Vector value used to disable MSI for queue */ +#define VIRTIO_MSI_NO_VECTOR 0xffff -/* The alignment to use between consumer and producer parts of vring. -* x86 pagesize again. */ -#define VIRTIO_PCI_VRING_ALIGN 4096 +/* The bit of the ISR which indicates a device configuration change. */ +#define VIRTIO_PCI_ISR_CONFIG 0x2/* * Layout for Virtio PCI vendor specific capability (little-endian): @@ -133,4 +151,20 @@#define VIRTIO_PCI_CAP_CFG_OFF_MASK 0xffffffff#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT 0 +/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ +__u64 queue_address;/* read-write */
include/linux/virtio_pci.h line 169
Quoted 10 lines
include/linux/virtio_pci.h +10
-0+__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; + +/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ +__u16 queue_msix_vector;/* read-write */ +};
Slightly confusing as the registers are in fact little endian ...
Quoted 3 lines
include/linux/virtio_pci.h +0
-0#endif
Michael S. Tsirkin
maintainerQuoted 11 lines · Michael S. Tsirkin
Quoted 9 lines · Rusty Russell
Quoted 3 lines · Michael S. Tsirkin
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.Don't touch the spec yet, we have a long way to go :(
I want the ability for driver to set the ring size, and the device to set the alignment.
Quoted 8 lines
Did you mean driver to be able to set the alignment? This
is what BIOS guys want - after BIOS completes, guest driver gets handed control and sets its own alignment to save memory.Quoted 1 line
That's a bigger change than you have here.
Why can't we just add the new registers at the end?
With the new capability, we have as much space as we like for that.
Didn't have the time to finish the patch today, but just to clarify, we can apply something like the below on top of my patch, and then config_len can be checked to see whether the new fields like alignment are available.
We probably can make the alignment smaller and save some memory - the default value could set the default alignment.
Ring size, naturally, can just be made writeable - BIOS can put a small value there, but linux guest would just use the defaults so no need for any code changes at all.
As a bonus, the capability length is verified to be large enough. The change's pretty small, isn't it?
drivers/virtio/virtio_pci.c +20 -10
@@ -37,8 +37,9 @@ struct virtio_pci_devicestruct virtio_device vdev;struct pci_dev *pci_dev; -/* the IO address for the common PCI config space */ +/* the IO address and length for the common PCI config space */ void __iomem *ioaddr; +unsigned config_len;/* the IO address for device specific config */ void __iomem *ioaddr_device;/* the IO address to use for notifications operations */
@@ -101,22 +102,24 @@ static void __iomem *virtio_pci_legacy_map(struct virtio_pci_device *vp_dev)#endif /* -* With PIO, device-specific config moves as MSI-X is enabled/disabled. -* Use this accessor to keep pointer to that config in sync. +* With PIO, device-specific config moves as MSI-X is enabled/disabled, +* configuration region length changes as well. Use this accessor to keep +* pointer to that config and common config length, in sync. */ static void virtio_pci_set_msix_enabled (struct virtio_pci_device *vp_dev,int enabled) {void __iomem * m; vp_dev->msix_enabled = enabled; m =virtio_pci_legacy_map (vp_dev); -if (m) +if (m) { vp_dev->ioaddr_device = m +VIRTIO_PCI_CONFIG (vp_dev); -else + vp_dev->config_len =VIRTIO_PCI_CONFIG (vp_dev); + }else vp_dev->ioaddr_device = vp_dev->device_map; }static void __iomem *virtio_pci_map_cfg (struct virtio_pci_device *vp_dev,u8 cap_id, -u32 align) +u32 align,unsigned *lenp) {u32 size;u32 offset;
@@ -160,12 +163,16 @@ static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap offset &=VIRTIO_PCI_CAP_CFG_OFF_MASK ;/* Align offset appropriately */ offset &= ~(align -1 ); +if (lenp && size < *lenp) + goto err;/* It's possible for a device to supply a huge config space, * but we'll never need to map more than a page ATM. */ p =pci_iomap_range (dev, bir, offset, size,PAGE_SIZE );if (!p)dev_err (&vp_dev->vdev.dev,"Unable to map virtio pci memory" ); +if (lenp) + *lenp =min (size,PAGE_SIZE );return p; err:dev_err (&vp_dev->vdev.dev,"Unable to parse virtio pci capability" );
@@ -188,22 +195,24 @@ static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)static int virtio_pci_iomap (struct virtio_pci_device *vp_dev) { +unsigned common_len =VIRTIO_PCI_COMMON_CFG_MINSIZE ; vp_dev->isr_map =virtio_pci_map_cfg (vp_dev,VIRTIO_PCI_CAP_ISR_CFG , -sizeof (u8)); +sizeof (u8),NULL ); vp_dev->notify_map =virtio_pci_map_cfg (vp_dev,VIRTIO_PCI_CAP_NOTIFY_CFG , -sizeof (u16)); +sizeof (u16),NULL ); vp_dev->common_map =virtio_pci_map_cfg (vp_dev,VIRTIO_PCI_CAP_COMMON_CFG , -sizeof (u32)); +sizeof (u32), &common_len); vp_dev->device_map =virtio_pci_map_cfg (vp_dev,VIRTIO_PCI_CAP_DEVICE_CFG , -sizeof (u8)); +sizeof (u8),NULL );if (vp_dev->notify_map && vp_dev->isr_map && vp_dev->common_map && vp_dev->device_map) { vp_dev->ioaddr = vp_dev->common_map; + vp_dev->config_len = common_len; vp_dev->ioaddr_isr = vp_dev->isr_map; vp_dev->ioaddr_notify = vp_dev->notify_map; vp_dev->ioaddr_device = vp_dev->device_map;
@@ -221,6 +230,7 @@ static int virtio_pci_iomap(struct virtio_pci_device *vp_dev) goto err; } vp_dev->ioaddr = m; + vp_dev->config_len =VIRTIO_PCI_CONFIG (vp_dev); vp_dev->ioaddr_isr = m +VIRTIO_PCI_ISR ; vp_dev->ioaddr_notify = m +VIRTIO_PCI_QUEUE_NOTIFY ; vp_dev->ioaddr_device = m +VIRTIO_PCI_CONFIG (vp_dev);
include/linux/virtio_pci.h +2 -0
@@ -133,4 +133,6 @@#define VIRTIO_PCI_CAP_CFG_OFF_MASK 0xffffffff#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT 0 +#define VIRTIO_PCI_COMMON_CFG_MINSIZE 24 +#endif
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Rusty Russell
Quoted 3 lines · Michael S. Tsirkin
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.
Quoted 8 lines
Quoted 4 lines
Don't touch the spec yet, we have a long way to go :(
I want the ability for driver to set the ring size, and the device to set the alignment.
Did you mean driver to be able to set the alignment? This
is what BIOS guys want - after BIOS completes, guest driver gets handed control and sets its own alignment to save memory.
Yep, sorry.
But we really do want the guest to set the ring size. Because it has to be guest-physical-contiguous, the host currently sets a very small ring, because the guest is useless if it can't allocate.
Either way, it's now the driver's responsibility to write those fields.
Quoted 4 lines
Quoted 1 line
That's a bigger change than you have here.
Why can't we just add the new registers at the end?
With the new capability, we have as much space as we like for that.
We could, for sure.
Quoted 4 lines
Quoted 1 line
I imagine it almost rips the driver into two completely different drivers.
If you insist on moving all the rest of registers around, certainly. But why do this?
Because I suspect we'll be different enough anyway, once we change the way we allocate the ring, and write the alignment. It'll be *clearer* to have two completely separate paths than to fill with if() statements. And a rewrite won't hurt the driver.
But to be honest I don't really care about the Linux driver: we're steeped in this stuff and we'll get it right. But I'm *terrified* of making the spec more complex; implementations will get it wrong. I *really* want to banish the legacy stuff to an appendix where noone will ever know it's there :)
Quoted 2 lines
Renaming constants in exported headers will break userspace builds. Do we care? Why not?
As the patch shows, I decided not to do that. It's a nice heads-up, but breaking older versions of the code is just mean. Hence this:
Quoted 16 lines
Quoted 16 lines
include/linux/virtio_pci.h +16
-0+#ifndef __KERNEL__ +/* Don't break compile of old userspace code. These will go away. */ +#define VIRTIO_PCI_HOST_FEATURES VIRTIO_PCI_LEGACY_HOST_FEATURES +#define VIRTIO_PCI_GUEST_FEATURES VIRTIO_PCI_LEGACY_GUEST_FEATURES +#define VIRTIO_PCI_LEGACY_QUEUE_PFN VIRTIO_PCI_QUEUE_PFN +#define VIRTIO_PCI_LEGACY_QUEUE_NUM VIRTIO_PCI_QUEUE_NUM +#define VIRTIO_PCI_LEGACY_QUEUE_SEL VIRTIO_PCI_QUEUE_SEL +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY VIRTIO_PCI_QUEUE_NOTIFY +#define VIRTIO_PCI_LEGACY_STATUS VIRTIO_PCI_STATUS +#define VIRTIO_PCI_LEGACY_ISR VIRTIO_PCI_ISR +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR VIRTIO_MSI_CONFIG_VECTOR +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR VIRTIO_MSI_QUEUE_VECTOR +#define VIRTIO_PCI_LEGACY_CONFIG (dev) VIRTIO_PCI_CONFIG(dev) +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT VIRTIO_PCI_QUEUE_ADDR_SHIFT +#define VIRTIO_PCI_LEGACY_VRING_ALIGN VIRTIO_PCI_VRING_ALIGN +#endif /* ...!KERNEL */
...
Quoted 10 lines
Quoted 10 lines
include/linux/virtio_pci.h +10
-0+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ +__u64 queue_address;/* read-write */ +__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; +
Quoted 8 lines
include/linux/virtio_pci.h line 169
Quoted 6 lines
include/linux/virtio_pci.h +6
-0+/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ +__u16 queue_msix_vector;/* read-write */ +};Slightly confusing as the registers are in fact little endian ...
Good point, should mark them appropriately with __le16. That makes it even clearer.
Thanks,
Rusty.
Michael S. Tsirkin
maintainerQuoted 19 lines · Rusty Russell
Quoted 10 lines
Quoted 10 lines
include/linux/virtio_pci.h +10
-0+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ +__u64 queue_address;/* read-write */ +__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; +Quoted 8 lines
include/linux/virtio_pci.h line 169
Quoted 6 lines
include/linux/virtio_pci.h +6
-0+/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ +__u16 queue_msix_vector;/* read-write */ +};Slightly confusing as the registers are in fact little endian ...
Quoted 5 lines
Good point, should mark them appropriately with
__le16. That makes it even clearer.Thanks,
Rusty.
Do we still require atomic access to fields in common cfg? If yes it's a problem as some systems don't have 64 bit addresses. If no, implementations might get harder.
Signature · 1 line
MST
Michael S. Tsirkin
maintainerQuoted 2 lines · Rusty Russell
Because I suspect we'll be different enough anyway, once we change the way we allocate the ring, and write the alignment.
Well, it'd be easy to just add pa_high.
Quoted 2 lines
It'll be *clearer* to have two completely separate paths than to fill with
if()statements.
Well, look at my patches. See how each new feature basically adds *one* if statement.
Yes, we could declare all existing features to be required, this is what you are advocating. But in a couple of years more if statements have accumulated and we still don't have a flying car. Meanwhile the driver has a huge legacy section which is a huge pain to maintain.
Quoted 1 line
And a rewrite won't hurt the driver.
Wow. Is everyone going for the rewrite these days? After a lot of work we will be back at the point where we left, with a minor tweak to enable a huge number of slow devices. Meanwhile we have no time to work on real problems, such as ring fragmentation that Avi pointed out, support for very large requests.
And yes it will hurt, it will hurt bisectability and testability. What you propose is a huge patch that just changes all of it. If there's a breakage, bisect just points at a rewrite. What I would like to see is incremental patches. So I would like to
1. Split driver config from common config
2. Split isr/notifications out too
3. Copy config in MMIO
4. Add a new config
5. Add length checks
6. Add 64 bit features
7. Add alignment programming
At each point we can bisect. It worked well for event index, qemu had a flag to turn it off, each time someone came in and went 'maybe it's event index' we just tested without.
Quoted 2 lines
But to be honest I don't really care about the Linux driver: we're steeped in this stuff and we'll get it right.
Maybe. What about windows drivers?
Quoted 1 line
But I'm *terrified* of making the spec more complex;
All you do is move stuff around. Why do you think it simplifies the spec so much?
Quoted 1 line
implementations will get it wrong.
Just to clarify: you assume that if virtio pci spec is changed, then there will be many more new implementations than existing ones, so it is worth it to make life (temporarily, for several short years) harder for all the existing ones?
Look at the qemu side for example. The way I proposed - with optional capabilities, each one with a separate fallback - the change can be implemented very gradually. Each step will be bisectable, and testable.
Quoted 2 lines
I *really* want to banish the legacy stuff to an appendix where noone will ever know it's there :)
This does not make life easy for implementations as they need to implement it anyway. What makes life easy is making new features optional, so you pick just what you like.
For example, we might want to backport some new feature into rhel6.X at some point. I would like the diff to be small, and easily understandable in its impact.
Signature · 1 line
MST
Quoted 6 lines · Michael S. Tsirkin
Quoted 2 lines · Rusty Russell
It'll be *clearer* to have two completely separate paths than to fill with
if()statements.Well, look at my patches. See how each new feature basically adds *one* if statement.
Which means every new feature doubles the number of paths to test. Both in qemu and the kernel. And I know we're not very good at testing.
And you've still got alignment setting and guest-set ringsize to go, which are more complex.
Quoted 5 lines
Yes, we could declare all existing features to be required, this is what you are advocating. But in a couple of years more if statements have accumulated and we still don't have a flying car. Meanwhile the driver has a huge legacy section which is a huge pain to maintain.
We've only implemented one layout change in 4 years (MSI-X) with one more definite (64-bit features). The rate of requests is also slowing:
they mainly came from early re-implementations.
And by drawing a line under legacy mode, we don't have to test those new options with legacy mode.
Quoted 3 lines
Quoted 1 line
And a rewrite won't hurt the driver.
Wow. Is everyone going for the rewrite these days?
Ok, I'll confess that was an ill-advised comment. The virtio-pci driver is actually quite nice.
Quoted 3 lines
And yes it will hurt, it will hurt bisectability and testability. What you propose is a huge patch that just changes all of it. If there's a breakage, bisect just points at a rewrite.
I don't have a problem with bisectability; it's great in the transition.
But you're suggesting we maintain all those different variations forever, and that burden be on other implementations too.
Quoted 9 lines
What I would like to see is incremental patches. So I would like to
1. Split driver config from common config
2. Split isr/notifications out too
3. Copy config in MMIO
4. Add a new config
5. Add length checks
6. Add 64 bit features
7. Add alignment programming
Quoted 3 lines
At each point we can bisect. It worked well for event index, qemu had a flag to turn it off, each time someone came in and went 'maybe it's event index' we just tested without.
Ok, I would like to see this too. But I think the intermediate stages should be disallowed in the final patch.
Quoted 4 lines
Quoted 2 lines
But to be honest I don't really care about the Linux driver: we're steeped in this stuff and we'll get it right.
Maybe. What about windows drivers?
Yes. My point was: I think one big switch is easier than lots of little ones.
Quoted 4 lines
Quoted 1 line
But I'm *terrified* of making the spec more complex;
All you do is move stuff around. Why do you think it simplifies the spec so much?
No, but it reduces the yuk factor. Which has been important to adoption.
And that's *not* all I do: reducing the number of options definitely simplifies the spec. For example, the spec should currently say (looking at your implementation):
Notifying the device
====================
If you find a valid VIRTIO_PCI_CAP_NOTIFY_CFG capability, and you can map 2 bytes within it, those two bytes should be used to notify the device of new descriptors in its virtqueues, by writing the index of the virtqueue to that mapping.
If the capability is missing or malformed or you cannot map it, the virtqueue index should be written to the VIRTIO_PCI_QUEUE_NOTIFY offset of the legacy bar.
Vs:
Notifying the device
====================
The index of the virtqueue containing new descriptors should be written to the location specified by the VIRTIO_PCI_CAP_NOTIFY_CFG capability. (Unless the device is in legacy mode, see Appendix Y: Legacy Mode).
Quoted 6 lines
Quoted 2 lines
I *really* want to banish the legacy stuff to an appendix where noone will ever know it's there :)
This does not make life easy for implementations as they need to implement it anyway. What makes life easy is making new features optional, so you pick just what you like.
Today, that's true. Tomorrow, it's not.
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
And there are boutique implementations who don't care about compatibility with older versions, but they implement virtio because it's well specified, and clear.
Quoted 3 lines
For example, we might want to backport some new feature into rhel6.X at some point. I would like the diff to be small, and easily understandable in its impact.
Sure. And I'm determined to reduce long-term complexity.
Look, I try to be more inclusive and polite than Linus, but at some point more verbiage is wasted. We will have single Legacy Mode switch. Accept it, or fork the standard.
If you want to reuse the same structure, we're going to need to figure out how to specify the virtqueue address without a fixed alignment, and how to specify the alignment itself.
Thanks,
Rusty.
Michael S. Tsirkin
maintainerQuoted 6 lines · Rusty Russell
Quoted 4 lines
Quoted 1 line
But I'm *terrified* of making the spec more complex;
All you do is move stuff around. Why do you think it simplifies the spec so much?
No, but it reduces the yuk factor. Which has been important to adoption.
Sorry if I'm dense. Could you please clarify: do you think we can live with the slightly higher yuk factor assuming the spec moves the legacy mode into an appendix as you explain below and driver has a single 'legacy' switch?
Quoted 15 lines
And that's *not* all I do: reducing the number of options definitely simplifies the spec. For example, the spec should currently say (looking at your implementation):
Notifying the device
====================
If you find a validVIRTIO_PCI_CAP_NOTIFY_CFGcapability, and you can map 2 bytes within it, those two bytes should be used to notify the device of new descriptors in its virtqueues, by writing the index of the virtqueue to that mapping.If the capability is missing or malformed or you cannot map it, the virtqueue index should be written to the
VIRTIO_PCI_QUEUE_NOTIFYoffset of the legacy bar.
Quoted 7 lines
Vs:
Notifying the device
====================
The index of the virtqueue containing new descriptors should be written to the location specified by theVIRTIO_PCI_CAP_NOTIFY_CFGcapability. (Unless the device is in legacy mode, see Appendix Y: Legacy Mode).
Yes, I agree, this is better.
...
Quoted 3 lines
Look, I try to be more inclusive and polite than Linus, but at some point more verbiage is wasted.
We will have single Legacy Mode switch.
Sorry, I'm adding more verbiage :(
When you say a single Legacy Mode switch, you mean that the driver will assume either legacy layout or the new one, correct?
Quoted 5 lines
Accept it, or fork the standard.
If you want to reuse the same structure, we're going to need to figure out how to specify the virtqueue address without a fixed alignment, and how to specify the alignment itself.
I think I see a way to do that in a relatively painless way. Do you prefer seeing driver patches or spec? Or are you not interested in reusing the same structure at all?
Signature · 1 line
MST
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Rusty Russell
Quoted 4 lines
Quoted 1 line
But I'm *terrified* of making the spec more complex;
All you do is move stuff around. Why do you think it simplifies the spec so much?
Quoted 6 lines
Quoted 1 line
No, but it reduces the yuk factor. Which has been important to adoption.
Sorry if I'm dense. Could you please clarify: do you think we can live with the slightly higher yuk factor assuming the spec moves the legacy mode into an appendix as you explain below and driver has a single 'legacy' switch?
Yep, it's all a trade-off. A clean slate is good, but if we can make our lives in transition less painful, I'm all for it.
Quoted 3 lines
I think I see a way to do that in a relatively painless way. Do you prefer seeing driver patches or spec? Or are you not interested in reusing the same structure at all?
I think we should look at code at this point; my gut says we're going to be not-quite-similar-enough-to-be-useful. At which point, a clean-slate approach is more appealing. But the code will show, one way or another.
Thanks,
Rusty.
Michael S. Tsirkin
maintainerQuoted 10 lines · Rusty Russell
Quoted 3 lines
I think I see a way to do that in a relatively painless way. Do you prefer seeing driver patches or spec? Or are you not interested in reusing the same structure at all?
I think we should look at code at this point; my gut says we're going to be not-quite-similar-enough-to-be-useful. At which point, a clean-slate approach is more appealing. But the code will show, one way or another.
Thanks,
Rusty.
Makes sense, absolutely. So I'll hack on it and post and we can judge the result.
One small comment that I'm afraid was lost in the noise is that we should not add any 64 bit fields in the common area, because there's no generic iowrite64/ioread64.
Signature · 1 line
MST
Quoted 2 lines · Rusty Russell
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
While we can't simply remove it right away, instead of mixing our implementation for both legacy and new spec in the same code we can split the virtio-pci implementation into two:
- virtio/virtio-pci-legacy.c
- virtio/virtio-pci.c
At that point we can #ifdef the entire virtio-pci-legacy.c for now and remove it at the same time legacy virtio-pci is removed from the kernel.
I think this is something very similar to what you want done in the kernel code, so an added plus is that the usermode code will be mirroring the kernel code - which is something we try to have in the KVM tool :)
Signature · 1 line
Sasha.
Quoted 8 lines · Sasha Levin
Quoted 2 lines · Rusty Russell
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
While we can't simply remove it right away, instead of mixing our implementation for both legacy and new spec in the same code we can split the virtio-pci implementation into two:
Quoted 5 lines
-
virtio/virtio-pci-legacy.c
-virtio/virtio-pci.cAt that point we can #ifdef the entire
virtio-pci-legacy.cfor now and remove it at the same time legacy virtio-pci is removed from the kernel.
Hmm, that might be neat, but we can't tell the driver core to try virtio-pci before virtio-pci-legacy, so we need detection code in both modules (and add a "force" flag to virtio-pci-legacy to tell it to accept the device even if it's not a legacy-only one).
Then it should work...
Cheers,
Rusty.
Michael S. Tsirkin
maintainerQuoted 12 lines · Rusty Russell
Quoted 11 lines · Sasha Levin
Quoted 2 lines · Rusty Russell
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
While we can't simply remove it right away, instead of mixing our implementation for both legacy and new spec in the same code we can split the virtio-pci implementation into two:
-
virtio/virtio-pci-legacy.c
-virtio/virtio-pci.c
Quoted 7 lines
Quoted 2 lines
At that point we can #ifdef the entire
virtio-pci-legacy.cfor now and remove it at the same time legacy virtio-pci is removed from the kernel.Hmm, that might be neat, but we can't tell the driver core to try virtio-pci before virtio-pci-legacy, so we need detection code in both modules (and add a "force" flag to virtio-pci-legacy to tell it to accept the device even if it's not a legacy-only one).
This flag might need to be per device ideally, which is tricky ...
Quoted 4 lines
Then it should work...
Cheers,
Rusty.
One also wonders whether and how this will work on other OS-es.
Signature · 1 line
MST
Quoted 12 lines · Rusty Russell
Quoted 11 lines · Sasha Levin
Quoted 2 lines · Rusty Russell
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
While we can't simply remove it right away, instead of mixing our implementation for both legacy and new spec in the same code we can split the virtio-pci implementation into two:
-
virtio/virtio-pci-legacy.c
-virtio/virtio-pci.c
Quoted 7 lines
Quoted 2 lines
At that point we can #ifdef the entire
virtio-pci-legacy.cfor now and remove it at the same time legacy virtio-pci is removed from the kernel.Hmm, that might be neat, but we can't tell the driver core to try virtio-pci before virtio-pci-legacy, so we need detection code in both modules (and add a "force" flag to virtio-pci-legacy to tell it to accept the device even if it's not a legacy-only one).
I was thinking more in the direction of fallback code in virtio-pci.c to virtio-pci-legacy.c.
Something like:
#ifdef VIRTIO_PCI_LEGACY
[Create BAR0 and map it to virtio-pci-legacy.c]
#endif
So BAR0 isn't defined as long as legacy code is there, which makes falling back to legacy pretty simple.
Signature · 1 line
Sasha.
Quoted 24 lines · Sasha Levin
Quoted 12 lines · Rusty Russell
Quoted 11 lines · Sasha Levin
Quoted 2 lines · Rusty Russell
I'd like to see kvmtools remove support for legacy mode altogether, but they probably have existing users.
While we can't simply remove it right away, instead of mixing our implementation for both legacy and new spec in the same code we can split the virtio-pci implementation into two:
-
virtio/virtio-pci-legacy.c
-virtio/virtio-pci.cQuoted 7 lines
Quoted 2 lines
At that point we can #ifdef the entire
virtio-pci-legacy.cfor now and remove it at the same time legacy virtio-pci is removed from the kernel.Hmm, that might be neat, but we can't tell the driver core to try virtio-pci before virtio-pci-legacy, so we need detection code in both modules (and add a "force" flag to virtio-pci-legacy to tell it to accept the device even if it's not a legacy-only one).
I was thinking more in the direction of fallback code in
virtio-pci.ctovirtio-pci-legacy.c.
Quoted 7 lines
Something like:
#ifdefVIRTIO_PCI_LEGACY
[Create BAR0 and map it tovirtio-pci-legacy.c]
#endifSo BAR0 isn't defined as long as legacy code is there, which makes falling back to legacy pretty simple.
But it's nicer to see the driver actually labelled "virtio-pci-legacy", and such a module.
I'll code something up, see what it looks like.
Cheers,
Rusty.
Michael S. Tsirkin
maintainerinclude/linux/virtio_pci.h line 158
Quoted 5 lines · Rusty Russell
include/linux/virtio_pci.h +5
-0+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */
We currently require atomic accesses to common fields. Some architectures might not have such for 64 bit, so these need to be split I think ...
Quoted 14 lines
include/linux/virtio_pci.h +11
-0+__u64 queue_address;/* read-write */ +__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; + +/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ + __u16 queue_msix_vector;/* read-write */ +};#endif
Quoted 10 lines · Michael S. Tsirkin
include/linux/virtio_pci.h line 158
Quoted 5 lines · Rusty Russell
include/linux/virtio_pci.h +5
-0+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ We currently require atomic accesses to common fields. Some architectures might not have such for 64 bit, so these need to be split I think ...
We can consider stealing the feature implementation from virtio-mmio:
Use the first 32 bits as a selector and the last as the features themselves.
It's more complex to work with, but it provides 2**32 32 feature bits (which should be enough for a while) and it solves the atomic access issue.
Quoted 14 lines
Quoted 14 lines
include/linux/virtio_pci.h +11
-0+__u64 queue_address;/* read-write */ +__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; + +/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ + __u16 queue_msix_vector;/* read-write */ +};#endif
Signature · 1 line
Sasha.
Quoted 12 lines · Sasha Levin
Quoted 10 lines · Michael S. Tsirkin
include/linux/virtio_pci.h line 158
Quoted 5 lines · Rusty Russell
include/linux/virtio_pci.h +5
-0+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ We currently require atomic accesses to common fields. Some architectures might not have such for 64 bit, so these need to be split I think ...
Quoted 7 lines
We can consider stealing the feature implementation from virtio-mmio:
Use the first 32 bits as a selector and the last as the features themselves.It's more complex to work with, but it provides 2**32 32 feature bits (which should be enough for a while) and it solves the atomic access issue.
That works. I don't expect we'll need more than 64 features given that virtio_net hasn't seen a new one in over a year, but it's gone from 5 to 18 in 4 years, so another 32 bits at that rate only gets us another decade.
Unfortunately, it doesn't solve the queue_address problem.
We currently multiply the (32-bit) address by 4096 (the alignment). If drivers are going to reduce alignment, that makes it trickier, hence the change here to a 64. Simplicity.
We could cheat and assert that a 32-bit write there implies 0 in the upper bits, and hope that all 64 bit platforms can do a 64-bit atomic write. Or insist the value not be intepreted until the guest writes its (first) feature bit.
Perhaps we really need a "I'm done configuring!" trigger, now we can't use the feature bit field for that.
Thoughts welcome...
Rusty.
Quoted 78 lines · Rusty Russell
Quoted 3 lines · Michael S. Tsirkin
Here's an updated vesion.
I'm alternating between updating the spec and the driver, spec update to follow.Don't touch the spec yet, we have a long way to go :(
I want the ability for driver to set the ring size, and the device to set the alignment. That's a bigger change than you have here. I imagine it almost rips the driver into two completely different drivers.
This is the kind of thing I had in mind, for the header. Want me to code up the rest?
(I've avoided adding the constants for the new layout: a struct is more compact and more descriptive).
Cheers,
Rusty.include/linux/virtio_pci.h +12
-16@@ -42,56 +42,74 @@#include <linux/virtio_config.h> /* A 32-bit r/o bitmask of the features supported by the host */ -#define VIRTIO_PCI_HOST_FEATURES 0 +#define VIRTIO_PCI_LEGACY_HOST_FEATURES 0/* A 32-bit r/w bitmask of features activated by the guest */ -#define VIRTIO_PCI_GUEST_FEATURES 4 +#define VIRTIO_PCI_LEGACY_GUEST_FEATURES 4/* A 32-bit r/w PFN for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_PFN 8 +#define VIRTIO_PCI_LEGACY_QUEUE_PFN 8/* A 16-bit r/o queue size for the currently selected queue */ -#define VIRTIO_PCI_QUEUE_NUM 12 +#define VIRTIO_PCI_LEGACY_QUEUE_NUM 12/* A 16-bit r/w queue selector */ -#define VIRTIO_PCI_QUEUE_SEL 14 +#define VIRTIO_PCI_LEGACY_QUEUE_SEL 14/* A 16-bit r/w queue notifier */ -#define VIRTIO_PCI_QUEUE_NOTIFY 16 +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY 16/* An 8-bit device status register. */ -#define VIRTIO_PCI_STATUS 18 +#define VIRTIO_PCI_LEGACY_STATUS 18/* An 8-bit r/o interrupt status register. Reading the value will return the * current contents of the ISR and will also clear it. This is effectively * a read-and-acknowledge. */ -#define VIRTIO_PCI_ISR 19 - -/* The bit of the ISR which indicates a device configuration change. */ -#define VIRTIO_PCI_ISR_CONFIG 0x2 +#define VIRTIO_PCI_LEGACY_ISR 19/* MSI-X registers: only enabled if MSI-X is enabled. */ /* A 16-bit vector for configuration changes. */ -#define VIRTIO_MSI_CONFIG_VECTOR 20 +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR 20/* A 16-bit vector for selected queue notifications. */ -#define VIRTIO_MSI_QUEUE_VECTOR 22 -/* Vector value used to disable MSI for queue */ -#define VIRTIO_MSI_NO_VECTOR 0xffff +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR 22/* The remaining space is defined by each driver as the per-driver * configuration space */ -#define VIRTIO_PCI_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) +#define VIRTIO_PCI_LEGACY_CONFIG (dev) ((dev)->msix_enabled ? 24 : 20) +
include/linux/virtio_pci.h line 89
Quoted 10 lines
include/linux/virtio_pci.h +10
-0+/* How many bits to shift physical queue address written to QUEUE_PFN. +* 12 is historical, and due to x86 page size. */ +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT 12 + +/* The alignment to use between consumer and producer parts of vring. +* x86 pagesize again. */ +#define VIRTIO_PCI_LEGACY_VRING_ALIGN 4096 + +#ifndef __KERNEL__ +/* Don't break compile of old userspace code. These will go away. */
Maybe wrap it in:
#ifndef VIRTIO_PCI_NO_LEGACY
#warning Please stop using old defines
[...]
include/linux/virtio_pci.h line 102
Quoted 13 lines
include/linux/virtio_pci.h +13
-0+#define VIRTIO_PCI_HOST_FEATURES VIRTIO_PCI_LEGACY_HOST_FEATURES +#define VIRTIO_PCI_GUEST_FEATURES VIRTIO_PCI_LEGACY_GUEST_FEATURES +#define VIRTIO_PCI_LEGACY_QUEUE_PFN VIRTIO_PCI_QUEUE_PFN +#define VIRTIO_PCI_LEGACY_QUEUE_NUM VIRTIO_PCI_QUEUE_NUM +#define VIRTIO_PCI_LEGACY_QUEUE_SEL VIRTIO_PCI_QUEUE_SEL +#define VIRTIO_PCI_LEGACY_QUEUE_NOTIFY VIRTIO_PCI_QUEUE_NOTIFY +#define VIRTIO_PCI_LEGACY_STATUS VIRTIO_PCI_STATUS +#define VIRTIO_PCI_LEGACY_ISR VIRTIO_PCI_ISR +#define VIRTIO_MSI_LEGACY_CONFIG_VECTOR VIRTIO_MSI_CONFIG_VECTOR +#define VIRTIO_MSI_LEGACY_QUEUE_VECTOR VIRTIO_MSI_QUEUE_VECTOR +#define VIRTIO_PCI_LEGACY_CONFIG (dev) VIRTIO_PCI_CONFIG(dev) +#define VIRTIO_PCI_LEGACY_QUEUE_ADDR_SHIFT VIRTIO_PCI_QUEUE_ADDR_SHIFT +#define VIRTIO_PCI_LEGACY_VRING_ALIGN VIRTIO_PCI_VRING_ALIGN
#endif /* VIRTIO_PCI_NO_LEGACY */
Quoted 20 lines
include/linux/virtio_pci.h +5
-6+#endif /* ...!KERNEL */ /* Virtio ABI version, this must match exactly */ #define VIRTIO_PCI_ABI_VERSION 0 -/* How many bits to shift physical queue address written to QUEUE_PFN. -* 12 is historical, and due to x86 page size. */ -#define VIRTIO_PCI_QUEUE_ADDR_SHIFT 12 +/* Vector value used to disable MSI for queue */ +#define VIRTIO_MSI_NO_VECTOR 0xffff -/* The alignment to use between consumer and producer parts of vring. -* x86 pagesize again. */ -#define VIRTIO_PCI_VRING_ALIGN 4096 +/* The bit of the ISR which indicates a device configuration change. */ +#define VIRTIO_PCI_ISR_CONFIG 0x2/* * Layout for Virtio PCI vendor specific capability (little-endian):
include/linux/virtio_pci.h line 159
Quoted 9 lines
include/linux/virtio_pci.h +6
-0@@ -133,4 +151,20 @@#define VIRTIO_PCI_CAP_CFG_OFF_MASK 0xffffffff#define VIRTIO_PCI_CAP_CFG_OFF_SHIFT 0 +/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */ +struct virtio_pci_common_cfg { +/* About the whole device. */ +__u64 device_features;/* read-only */ +__u64 guest_features;/* read-write */ +__u64 queue_address;/* read-write */
queue address should be in the queue specific fields below
include/linux/virtio_pci.h line 168
Quoted 9 lines
include/linux/virtio_pci.h +9
-0+__u16 msix_config;/* read-write */ +__u8 device_status;/* read-write */ +__u8 unused; + +/* About a specific virtqueue. */ +__u16 queue_select;/* read-write */ +__u16 queue_align;/* read-write, power of 2. */ +__u16 queue_size;/* read-write, power of 2. */ +__u16 queue_msix_vector;/* read-write */
Maybe we should reserve a bunch of bytes here for future extensions of virtqueues. Otherwise, non virtqueue options may be added here which will cause fragmentation of virtqueue specific features.
Quoted 5 lines
include/linux/virtio_pci.h +1
-0+};#endif
Signature · 1 line
Sasha.