virtio

* [new tag] [email protected] -> [email protected]

The following changes since commit 839d8810747bbf39e0a5a7f223b67bffa7945f8d:

Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging (2011-10-30 15:54:59 -0700)

are available in the git repository at:

git://github.com/rustyrussell/linux.git master

Alexey Kardashevskiy (1):
virtio-pci: Use PCI MMIO instead of PIO when available

Amit Shah (8):
virtio: console: Fix indentation
virtio: console: Ignore port name update request if name already set
virtio: console: Use wait_event_freezable instead of _interruptible
virtio: console: Fix return type for get_inbuf()
virtio: console: make get_inbuf() return port->inbuf if present
virtio: console: rename variable
virtio: console: make discard_port_data() use get_inbuf()
virtio: console: add port stats for bytes received, sent and discarded

Christian Borntraeger (1):
virtio: console: wait for first console port for early console output

Krishna Kumar (1):
virtio: Dont add "config" to list for !per_vq_vector

Michael S. Tsirkin (1):
virtio-blk: use ida to allocate disk index

Pawel Moll (1):
virtio: Add platform bus driver for memory mapped virtio device

Sasha Levin (3):
virtio-console: Use virtio_config_val() for retrieving config
virtio_config: Add virtio_config_val_len()
virtio-net: Use virtio_config_val() for retrieving config

Wang Sheng-Hui (2):
virtio.h: correct comment for struct virtio_driver
virtio: modify vring_init and vring_size to take account of the layout containing *_event_idx

 Documentation/devicetree/bindings/virtio/mmio.txt |   17 +
 drivers/block/virtio_blk.c                        |   30 +-
 drivers/char/virtio_console.c                     |  120 ++++--
 drivers/net/virtio_net.c                          |   14 +-
 drivers/virtio/Kconfig                            |   11 +
 drivers/virtio/Makefile                           |    1 +
 drivers/virtio/virtio_mmio.c                      |  479 +++++++++++++++++++++
 drivers/virtio/virtio_pci.c                       |   20 +-
 include/linux/virtio.h                            |    4 +-
 include/linux/virtio_config.h                     |    3 +
 include/linux/virtio_mmio.h                       |  111 +++++
 include/linux/virtio_ring.h                       |    6 +-
 12 files changed, 749 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/virtio/mmio.txt
 create mode 100644 drivers/virtio/virtio_mmio.c
 create mode 100644 include/linux/virtio_mmio.h

Quoted 4 lines · Rusty Russell

* [new tag] [email protected] -> [email protected]

The following changes since commit 839d8810747bbf39e0a5a7f223b67bffa7945f8d:

Quoted 8 lines

Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging (2011-10-30 15:54:59 -0700)

are available in the git repository at:

git://github.com/rustyrussell/linux.git master

Alexey Kardashevskiy (1):
virtio-pci: Use PCI MMIO instead of PIO when available

I missed this one - wasn't Cc'd neither me, kvm or virtio mailing lists.

It's well known that mmio is much slower than pio on kvm, since mmio needs to be emulated to get at the address.
So I'm expecting this will cause a performance regression. IMO we should keep using PIO for VQ and interrupt status access if PIO is available.

Another consideration is that in an attempt to pack data densely in the PIO space the layout became messy. It would be better to have common config space and per-device config space in separate pages, possibly with padding between them.

So I'd like a bit more discussion on this patch, I'm concerned that if this is released in 3.2 as is we'll have to support this forever. How about a revert for now?

Quoted 28 lines · Michael S. Tsirkin

Quoted 4 lines · Rusty Russell

* [new tag] [email protected] -> [email protected]

The following changes since commit 839d8810747bbf39e0a5a7f223b67bffa7945f8d:

Quoted 8 lines

Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging (2011-10-30 15:54:59 -0700)

are available in the git repository at:

git://github.com/rustyrussell/linux.git master

Alexey Kardashevskiy (1):
virtio-pci: Use PCI MMIO instead of PIO when available

I missed this one - wasn't Cc'd neither me, kvm or virtio mailing lists.

It's well known that mmio is much slower than pio on kvm, since mmio needs to be emulated to get at the address.
So I'm expecting this will cause a performance regression. IMO we should keep using PIO for VQ and interrupt status access if PIO is available.

Another consideration is that in an attempt to pack data densely in the PIO space the layout became messy. It would be better to have common config space and per-device config space in separate pages, possibly with padding between them.

Quoted 3 lines

So I'd like a bit more discussion on this patch, I'm concerned that if this is released in 3.2 as is we'll have to support this forever. How about a revert for now?

Another thing, the patch tries to map BAR 2 and use it as the configuration space.

It's both not documented properly anywhere, and is not fully backwards compatible - we currently use BAR 2 as part of our MSIX handling in the kvm tool and I'm sure we're not the only ones to assume virtio-pci only uses BAR 0.

A proper solution would be for example a configuration in the PIO config space which points to the MMIO BAR to use instead.

Unless Michael pointed this patch out, it would have broken (at least) the kvm tool in a non obvious way which would require a rather long session of 'git bisect' to figure out whats wrong.

Signature · 1 line

Sasha.

Quoted 41 lines · Sasha Levin

Quoted 28 lines · Michael S. Tsirkin

Quoted 4 lines · Rusty Russell

* [new tag] [email protected] -> [email protected]

The following changes since commit 839d8810747bbf39e0a5a7f223b67bffa7945f8d:

Quoted 8 lines

Merge branch 'i2c-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/jdelvare/staging (2011-10-30 15:54:59 -0700)

are available in the git repository at:

git://github.com/rustyrussell/linux.git master

Alexey Kardashevskiy (1):
virtio-pci: Use PCI MMIO instead of PIO when available

I missed this one - wasn't Cc'd neither me, kvm or virtio mailing lists.

It's well known that mmio is much slower than pio on kvm, since mmio needs to be emulated to get at the address.
So I'm expecting this will cause a performance regression. IMO we should keep using PIO for VQ and interrupt status access if PIO is available.

Another consideration is that in an attempt to pack data densely in the PIO space the layout became messy. It would be better to have common config space and per-device config space in separate pages, possibly with padding between them.

Quoted 3 lines

So I'd like a bit more discussion on this patch, I'm concerned that if this is released in 3.2 as is we'll have to support this forever. How about a revert for now?

Another thing, the patch tries to map BAR 2 and use it as the configuration space.

It's both not documented properly anywhere, and is not fully backwards compatible - we currently use BAR 2 as part of our MSIX handling in the kvm tool and I'm sure we're not the only ones to assume virtio-pci only uses BAR 0.

Quoted 2 lines

A proper solution would be for example a configuration in the PIO config space which points to the MMIO BAR to use instead.

I think it makes sense to put the configuration in PCI configuration space, using vendor-specific capability. This way we can reuse existing functionality for scanning capability lists.

Quoted 2 lines · Sasha Levin

A proper solution would be for example a configuration in the PIO config space which points to the MMIO BAR to use instead.

I think it makes sense to put the configuration in PCI configuration space, using vendor-specific capability. This way we can reuse existing functionality for scanning capability lists.

Quoted 3 lines

Unless Michael pointed this patch out, it would have broken (at least) the kvm tool in a non obvious way which would require a rather long session of 'git bisect' to figure out whats wrong.

Quoted 2 lines · Sasha Levin

A proper solution would be for example a configuration in the PIO config space which points to the MMIO BAR to use instead.

I think it makes sense to put the configuration in PCI configuration space, using vendor-specific capability. This way we can reuse existing functionality for scanning capability lists.

Quoted 3 lines

Unless Michael pointed this patch out, it would have broken (at least) the kvm tool in a non obvious way which would require a rather long session of 'git bisect' to figure out whats wrong.

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · Sasha Levin

Unless Michael pointed this patch out, it would have broken (at least) the kvm tool in a non obvious way which would require a rather long session of 'git bisect' to figure out whats wrong.

I assumed not, until QEMU provided the second BAR it would simply fall back. Or am I completely misunderstanding?

Since the PCI spec allows you to have the MSI-X table and the PBA in completely different regions pointed by two different BARs, we did so in kvm tools and have BAR 1 pointing to the MSI-X table, and BAR 2 pointing to the PBA ( - we *already* provided the second BAR and used it for something else).

Since the PCI spec allows you to have the MSI-X table and the PBA in completely different regions pointed by two different BARs, we did so in kvm tools and have BAR 1 pointing to the MSI-X table, and BAR 2 pointing to the PBA ( - we *already* provided the second BAR and used it for something else).

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes also need arch updates for non-x86.

We also will need to update the spec.

See the first chunk for layout documentation.

Posting here for early feedback.

In particular:

Do we need to require offset to be aligned?
Does iowrite16 work with unaligned accesses on all architectures? Does using ioread/write as we do add overhead as compared to plain PIO accesses?

Jesse - are you OK with the pci_iomap_range API proposed here (see last chunks)?
I noticed lots of architectures duplicate the implementation of pci_iomap - makes sense to clean that up?

Thanks,

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/virtio_pci.h +0 -0

index ea66f3f..b8a9de2 100644

include/linux/virtio_pci.h +7 -0

+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0x1f

include/linux/virtio_pci.h +11 -0

+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		3
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	5
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		4
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		8
+
 #endif

drivers/virtio/virtio_pci.c +98 -1

index 4bcc8b8..d4ed130 100644
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;

@@ -57,8 +61,142 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+
+	/* 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");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_NOTIFY_CFG);
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_COMMON_CFG);
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,

Hi Michael,

Quoted 37 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes also need arch updates for non-x86.

We also will need to update the spec.

See the first chunk for layout documentation.

Posting here for early feedback.

In particular:

Do we need to require offset to be aligned?
Does iowrite16 work with unaligned accesses on all architectures? Does using ioread/write as we do add overhead as compared to plain PIO accesses?

Jesse - are you OK with the pci_iomap_range API proposed here (see last chunks)?
I noticed lots of architectures duplicate the implementation of pci_iomap - makes sense to clean that up?

Thanks,

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/virtio_pci.h +0 -0

index ea66f3f..b8a9de2 100644

include/linux/virtio_pci.h line 100

Quoted 9 lines

include/linux/virtio_pci.h +6 -0

@@ -92,4 +92,38 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/*
+ * Layout for Virtio PCI vendor specific capability (little-endian):
+ * 5 bit virtio capability id.
+ * 3 bit BAR index register, specifying which BAR to use.
+ * 4 byte cfg offset within the BAR.

Why not make the 3 bits of the BIR as the lower 3 bits of the offset like its done in the PCI spec?

This way you also get 'free' 32bit alignment and simpler masking.

Quoted 11 lines

include/linux/virtio_pci.h +11 -0

+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0x1f
+#define VIRTIO_PCI_CAP_ID_SHIFT		0
+
+/* IDs for different capabilities. If a specific configuration
+ * is missing, legacy PIO path is used. */

include/linux/virtio_pci.h line 117

Quoted 6 lines

include/linux/virtio_pci.h +6 -0

+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	0
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	1
+/* Notifications and ISR access */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2

I think that separating notification here is very kvm specific.

Maybe it
could be replaced by allowing virtio to signal eventfds or something similar?

Quoted 135 lines

include/linux/virtio_pci.h +11 -0

+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		3
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	5
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		4
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		8
+
 #endif

drivers/virtio/virtio_pci.c +104 -1

index 4bcc8b8..d4ed130 100644
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;

@@ -57,8 +61,142 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+
+	/* 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");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_NOTIFY_CFG);
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_COMMON_CFG);
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_DEVICE_CFG);
+
+	if (!vp_dev->notify_map || !vp_dev->common_map ||
+	    !vp_dev->device_map) {
+		/*
+		 * If not all capabilities present, map legacy PIO.

drivers/virtio/virtio_pci.c struct virtio_pci_device · line 168

Quoted 4 lines

drivers/virtio/virtio_pci.c +4 -0

+		 * Legacy access is at BAR 0. We never need to map
+		 * more than 256 bytes there, since legacy config space
+		 * used PIO which has this size limit.
+		 * */

Whats the point of putting each of those new configs into a separate cap structure if you must have all 3 of them for the new config layout to work anyway?

Also, why not use this change to simplify the config space? The attempts to squeeze it into the limited PIO space made it quite messy, and this can be a good chance to fix some things like the MSI-X and offsets issue.

Signature · 3 lines

-- 

Sasha.

Quoted 53 lines · Sasha Levin

Hi Michael,

Quoted 37 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes also need arch updates for non-x86.

We also will need to update the spec.

See the first chunk for layout documentation.

Posting here for early feedback.

In particular:

Do we need to require offset to be aligned?
Does iowrite16 work with unaligned accesses on all architectures? Does using ioread/write as we do add overhead as compared to plain PIO accesses?

Jesse - are you OK with the pci_iomap_range API proposed here (see last chunks)?
I noticed lots of architectures duplicate the implementation of pci_iomap - makes sense to clean that up?

Thanks,

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/virtio_pci.h +0 -0

index ea66f3f..b8a9de2 100644

include/linux/virtio_pci.h line 100

Quoted 9 lines

include/linux/virtio_pci.h +6 -0

@@ -92,4 +92,38 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/*
+ * Layout for Virtio PCI vendor specific capability (little-endian):
+ * 5 bit virtio capability id.
+ * 3 bit BAR index register, specifying which BAR to use.
+ * 4 byte cfg offset within the BAR.

Why not make the 3 bits of the BIR as the lower 3 bits of the offset like its done in the PCI spec?

Quoted 1 line

This way you also get 'free' 32bit alignment and simpler masking.

3 bit gets us 64 bit alignment, no? Seems a bit more than we need:
we need at most 16 bit alignment.

Quoted 12 lines

Quoted 12 lines

include/linux/virtio_pci.h +11 -0

+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0x1f
+#define VIRTIO_PCI_CAP_ID_SHIFT		0
+
+/* IDs for different capabilities. If a specific configuration
+ * is missing, legacy PIO path is used. */

Quoted 8 lines

include/linux/virtio_pci.h line 117

Quoted 6 lines

include/linux/virtio_pci.h +6 -0

+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	0
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	1
+/* Notifications and ISR access */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2

I think that separating notification here is very kvm specific.

I don't see how it's kvm specific. This just makes it possible to split data path and configuration. Good for security as well. I was actually debating a separate configuration for ISR and NOTIFICATION.

Quoted 3 lines

Maybe it
could be replaced by allowing virtio to signal eventfds or something similar?

No idea what you mean - it's a pci device, how should it signal eventfds?

Quoted 136 lines

Quoted 136 lines

include/linux/virtio_pci.h +11 -0

+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		3
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	5
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		4
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		8
+
 #endif

drivers/virtio/virtio_pci.c +104 -1

index 4bcc8b8..d4ed130 100644
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;

@@ -57,8 +61,142 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+
+	/* 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");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_NOTIFY_CFG);
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_COMMON_CFG);
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_DEVICE_CFG);
+
+	if (!vp_dev->notify_map || !vp_dev->common_map ||
+	    !vp_dev->device_map) {
+		/*
+		 * If not all capabilities present, map legacy PIO.

Quoted 8 lines

drivers/virtio/virtio_pci.c struct virtio_pci_device · line 168

Quoted 4 lines

drivers/virtio/virtio_pci.c +4 -0

+		 * Legacy access is at BAR 0. We never need to map
+		 * more than 256 bytes there, since legacy config space
+		 * used PIO which has this size limit.
+		 * */

Whats the point of putting each of those new configs into a separate cap structure if you must have all 3 of them for the new config layout to work anyway?

Pls look closer, we don't require all 3 of them. For example, we can have BAR0 DEVICE and COMMON, and notifications will use BAR0.

By using an identical structure we make is much easier to reuse and extend it. For example we might add a separate structure for ISR.

Quoted 4 lines

Also, why not use this change to simplify the config space? The attempts to squeeze it into the limited PIO space made it quite messy, and this can be a good chance to fix some things like the MSI-X and offsets issue.

For MSI-X this is exactly what this change does, by splitting device-specific parts out. So if DEVICE capability is present, this tells us where the device specific data starts and enabling MSI-X does not relocate it.

Are there other fields we want to move?

Quoted 54 lines · Michael S. Tsirkin

Quoted 53 lines · Sasha Levin

Hi Michael,

Quoted 37 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes also need arch updates for non-x86.

We also will need to update the spec.

See the first chunk for layout documentation.

Posting here for early feedback.

In particular:

Do we need to require offset to be aligned?
Does iowrite16 work with unaligned accesses on all architectures? Does using ioread/write as we do add overhead as compared to plain PIO accesses?

Jesse - are you OK with the pci_iomap_range API proposed here (see last chunks)?
I noticed lots of architectures duplicate the implementation of pci_iomap - makes sense to clean that up?

Thanks,

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/virtio_pci.h +0 -0

index ea66f3f..b8a9de2 100644

include/linux/virtio_pci.h line 100

Quoted 9 lines

include/linux/virtio_pci.h +6 -0

@@ -92,4 +92,38 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/*
+ * Layout for Virtio PCI vendor specific capability (little-endian):
+ * 5 bit virtio capability id.
+ * 3 bit BAR index register, specifying which BAR to use.
+ * 4 byte cfg offset within the BAR.

Why not make the 3 bits of the BIR as the lower 3 bits of the offset like its done in the PCI spec?

Quoted 4 lines

Quoted 1 line

This way you also get 'free' 32bit alignment and simpler masking.

3 bit gets us 64 bit alignment, no? Seems a bit more than we need:
we need at most 16 bit alignment.

Yup, it's actually 64 bit, but whats the problem with that?

It will also
look more similar to the PCI spec.

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

include/linux/virtio_pci.h +7 -0

+ * 4 byte cfg size.
+ */
+
+/* A single virtio device has multiple vendor specific capabilities, we use the
+ * 5 bit ID field to distinguish between these. */
+#define VIRTIO_PCI_CAP_ID		3
+#define VIRTIO_PCI_CAP_ID_MASK		0x1f

include/linux/virtio_pci.h line 117

Quoted 10 lines

include/linux/virtio_pci.h +10 -0

+#define VIRTIO_PCI_CAP_ID_SHIFT		0
+
+/* IDs for different capabilities. If a specific configuration
+ * is missing, legacy PIO path is used. */
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	0
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	1
+/* Notifications and ISR access */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2

I think that separating notification here is very kvm specific.

Quoted 3 lines

I don't see how it's kvm specific. This just makes it possible to split data path and configuration. Good for security as well. I was actually debating a separate configuration for ISR and NOTIFICATION.

It's not really a clean split between data and config, you still have device status stuck between notification and ISR, which isn't really in the data path.

A split as you suggested sounds like a good idea, but we just need to move some fields around to make it clean.

Quoted 5 lines

Quoted 3 lines

Maybe it
could be replaced by allowing virtio to signal eventfds or something similar?

No idea what you mean - it's a pci device, how should it signal eventfds?

I'm not really sure what I was thinking about, ignore that :)

Quoted 145 lines

Quoted 136 lines

Quoted 136 lines

include/linux/virtio_pci.h +11 -0

+
+/* Index of the BAR including this configuration */
+#define VIRTIO_PCI_CAP_CFG_BIR		3
+#define VIRTIO_PCI_CAP_CFG_BIR_MASK	(0x7)
+#define VIRTIO_PCI_CAP_CFG_BIR_SHIFT	5
+
+/* Offset within the BAR */
+#define VIRTIO_PCI_CAP_CFG_OFF		4
+/* Size of the configuration space */
+#define VIRTIO_PCI_CAP_CFG_SIZE		8
+
 #endif

drivers/virtio/virtio_pci.c +104 -1

index 4bcc8b8..d4ed130 100644
@@ -37,8 +37,12 @@ struct virtio_pci_device
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
-	/* the IO mapping for the PCI config space */
+	/* the IO address for the common PCI config space */
 	void __iomem *ioaddr;
+	/* the IO address for device specific config */
+	void __iomem *ioaddr_device;
+	/* the IO address to use for notifications operations */
+	void __iomem *ioaddr_notify;
 
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;

@@ -57,8 +61,142 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+
+	/* Various IO mappings: used for resource tracking only. */
+
+	/* Legacy BAR0: typically PIO. */
+	void __iomem *legacy_map;
+
+	/* Mappings specified by device capabilities: typically in MMIO */
+	void __iomem *notify_map;
+	void __iomem *common_map;
+	void __iomem *device_map;
 };
 
+/*
+ * With PIO, device-specific config moves as MSI-X is enabled/disabled.
+ * Use this accessor to keep pointer to that config in sync.
+ */
+static void virtio_pci_set_msix_enabled(struct virtio_pci_device *vp_dev, int enabled)
+{
+	vp_dev->msix_enabled = enabled;
+	if (vp_dev->device_map)
+		vp_dev->ioaddr_device = vp_dev->device_map;
+	else
+		vp_dev->ioaddr_device = vp_dev->legacy_map +
+			VIRTIO_PCI_CONFIG(vp_dev);
+}
+
+static void __iomem *virtio_pci_map_cfg(struct virtio_pci_device *vp_dev, u8 cap_id)
+{
+        u32 size;
+        u32 offset;
+        u8 bir;
+        u8 cap_len;
+	int pos;
+	struct pci_dev *dev = vp_dev->pci_dev;
+	void __iomem *p;
+
+	for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+	     pos > 0;
+	     pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+		u8 id;
+		pci_read_config_byte(dev, pos + PCI_VNDR_CAP_LEN, &cap_len);
+		if (cap_len < VIRTIO_PCI_CAP_ID + 1)
+			continue;
+		pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_ID, &id);
+		id >>= VIRTIO_PCI_CAP_ID_SHIFT;
+		id &= VIRTIO_PCI_CAP_ID_MASK;
+		if (id == cap_id)
+			break;
+	}
+
+	if (pos <= 0)
+		return NULL;
+
+	if (cap_len < VIRTIO_PCI_CAP_CFG_BIR + 1)
+		goto err;
+        pci_read_config_byte(dev, pos + VIRTIO_PCI_CAP_CFG_BIR, &bir);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_OFF + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_OFF, &offset);
+	if (cap_len < VIRTIO_PCI_CAP_CFG_SIZE + 4)
+		goto err;
+        pci_read_config_dword(dev, pos + VIRTIO_PCI_CAP_CFG_SIZE, &size);
+        bir >>= VIRTIO_PCI_CAP_CFG_BIR_SHIFT;
+        bir &= VIRTIO_PCI_CAP_CFG_BIR_MASK;
+
+	/* 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");
+	return p;
+err:
+	dev_err(&vp_dev->vdev.dev, "Unable to parse virtio pci capability");
+	return NULL;
+}
+
+static void virtio_pci_iounmap(struct virtio_pci_device *vp_dev)
+{
+	if (vp_dev->legacy_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->legacy_map);
+	if (vp_dev->notify_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->notify_map);
+	if (vp_dev->common_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->common_map);
+	if (vp_dev->device_map)
+		pci_iounmap(vp_dev->pci_dev, vp_dev->device_map);
+}
+
+static int virtio_pci_iomap(struct virtio_pci_device *vp_dev)
+{
+	vp_dev->notify_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_NOTIFY_CFG);
+	vp_dev->common_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_COMMON_CFG);
+	vp_dev->device_map = virtio_pci_map_cfg(vp_dev,
+						     VIRTIO_PCI_CAP_DEVICE_CFG);
+
+	if (!vp_dev->notify_map || !vp_dev->common_map ||
+	    !vp_dev->device_map) {
+		/*
+		 * If not all capabilities present, map legacy PIO.

Quoted 8 lines

drivers/virtio/virtio_pci.c struct virtio_pci_device · line 168

Quoted 4 lines

drivers/virtio/virtio_pci.c +4 -0

+		 * Legacy access is at BAR 0. We never need to map
+		 * more than 256 bytes there, since legacy config space
+		 * used PIO which has this size limit.
+		 * */

Whats the point of putting each of those new configs into a separate cap structure if you must have all 3 of them for the new config layout to work anyway?

Quoted 5 lines

Pls look closer, we don't require all 3 of them. For example, we can have BAR0 DEVICE and COMMON, and notifications will use BAR0.

By using an identical structure we make is much easier to reuse and extend it. For example we might add a separate structure for ISR.

Right.

Quoted 9 lines

Quoted 4 lines

Also, why not use this change to simplify the config space? The attempts to squeeze it into the limited PIO space made it quite messy, and this can be a good chance to fix some things like the MSI-X and offsets issue.

For MSI-X this is exactly what this change does, by splitting device-specific parts out. So if DEVICE capability is present, this tells us where the device specific data starts and enabling MSI-X does not relocate it.

You still have relocations within the shared config space, once 64bit features or other future extensions are implemented.

Quoted 1 line

Are there other fields we want to move?

We can do several things:

1. Since we're no longer limited in space - remove queue selector and just have a full queue table.

2. Move device specific features into the device specific region. Currently the features field is a mix between virtio-pci and device specific features.

I did more similar things in the spec suggestion I've sent, could you look at the second part for more ideas please?

Signature · 1 line

Sasha.

2. Move device specific features into the device specific region. Currently the features field is a mix between virtio-pci and device specific features.

I did more similar things in the spec suggestion I've sent,

could you
look at the second part for more ideas please?

Signature · 1 line

Sasha.

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Quoted 5 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

How about posting the spec change instead of patches?

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Quoted 1 line

How about posting the spec change instead of patches?

We need both of course but I typically start with patches. Easier to see whether it's even possible to implement the spec without overhauling all of the code.

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Michael S. Tsirkin

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Quoted 5 lines

Quoted 1 line

How about posting the spec change instead of patches?

We need both of course but I typically start with patches. Easier to see whether it's even possible to implement the spec without overhauling all of the code.

Patches are okay for someone familiar with the code. For others, they require more effort.

Signature · 1 line

error compiling committee.c: too many arguments to function

Add a flexible mechanism to specify virtio configuration layout, using pci vendor-specific capability. A separate capability is used for each of common, device specific and data-path accesses.

Warning: compiled only.
This patch also needs to be split up, pci_iomap changes also need arch updates for non-x86.

We also will need to update the spec.

See the first chunk for layout documentation.

Posting here for early feedback.

In particular:

Do we need to require offset to be aligned?
Does iowrite16 work with unaligned accesses on all architectures? Does using ioread/write as we do add overhead as compared to plain PIO accesses?

Jesse - are you OK with the pci_iomap_range API proposed here (see last chunks)?
I noticed lots of architectures duplicate the implementation of pci_iomap - makes sense to clean that up?

PDF will follow.

Quoted 1 line

PDF will follow.

Attached for the lyx challenged :)

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

It can then go into MMIO space which is cheaper than PIO.

A legacy portion of the configuration is mirrored in the first BAR, to keep legacy drivers working. Any new fields can be added in existing structures at the end, so they won't affect legacy.

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

As this solves the dynamic placement of MSIX vectors and high feature bits,
I thought it's easier to just reserve space for that programming than give it a separate structure. This can be changed by a patch on top.

Note that data path is split from configuration.

PDF will follow.
----

virtio-spec.lyx +4 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320781133

+<lyxtabular version="3" rows="4" columns="34"> +<features tabularvalignment="middle">
+<column alignment="center" valignment="top" width="0pt"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0pt"> +<row>

virtio-spec.lyx +13 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320779667
+Purpose:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320780912
+
+\emph on
+Capability ID

+\begin_layout Plain Layout
+ie.
+ once you enable MSI-X on the device, the other fields move.
+ If you turn it off again, they move back!

+
+\change_deleted 1986246365 1320784929
+If more than 31 feature bits are supported, the device indicates so by setting
+ feature bit 31 (see

Quoted 10 lines · Michael S. Tsirkin

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

Quoted 6 lines

It can then go into MMIO space which is cheaper than PIO.

A legacy portion of the configuration is mirrored in the first BAR, to keep legacy drivers working. Any new fields can be added in existing structures at the end, so they won't affect legacy.

If newer specs add more structures at the end of the config space, and use the same config space for legacy, that space now becomes device specific config space and not new-shiny-feature space, so we must remember to handle those cases.

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

Quoted 7 lines

As this solves the dynamic placement of MSIX vectors and high feature bits,
I thought it's easier to just reserve space for that programming than give it a separate structure. This can be changed by a patch on top.

Quoted 5 lines

Note that data path is split from configuration.

PDF will follow.
----

The device initialization sequence might use an update as well.

Maybe
also a description of how device handles missing structure IDs.

[snip]

Quoted 8 lines

virtio-spec.lyx +8 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320781133
+These registers are specified using vendor-specific PCI capability located
+ on capability list in PCI configuration space of the device.
+ This virtio structure capability uses little-endian format; all bits are
+ read-only:

virtio-spec.lyx Device-specific setup, including reading the Device Feature Bits, discovery · line 767

Quoted 6 lines

+\end_layout

virtio-spec.lyx +5 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320772579
+\begin_inset Tabular

Just a note, these tables are way too wide to work properly in PDFs :)

Quoted 37 lines

+<lyxtabular version="3" rows="4" columns="34"> +<features tabularvalignment="middle">
+<column alignment="center" valignment="top" width="0pt"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0pt"> +<row>

[snip]

Quoted 13 lines

virtio-spec.lyx +13 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320779667
+Purpose:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320780912
+
+\emph on
+Capability ID

virtio-spec.lyx Device-specific setup, including reading the Device Feature Bits, discovery · line 2224

Quoted 10 lines

virtio-spec.lyx +10 -0

+\emph default
+, 
+\emph on
+Next Capability Pointer
+\emph default
+, 
+\emph on
+Capability Length
+\emph default
+ - these fields are specified by PCI local bus specification, Rev 3.0

I'm not sure what capability length is, can't find it in the spec either.

[snip]

Quoted 4 lines

+\begin_layout Plain Layout
+ie.
+ once you enable MSI-X on the device, the other fields move.
+ If you turn it off again, they move back!

Is it still true? We're talking about the new layout here (there are several of this footnote, this one is located right *before* the section which talks about legacy config space.

[snip]

Quoted 4 lines

+
+\change_deleted 1986246365 1320784929
+If more than 31 feature bits are supported, the device indicates so by setting
+ feature bit 31 (see

The bit numbers below this text should be corrected as well.

Signature · 1 line

Sasha.

Quoted 1 line · Sasha Levin

The device initialization sequence might use an update as well.

What is needed? Add an item where the driver scans the PCI capability list to detect the layout?

Quoted 4 lines

Maybe
also a description of how device handles missing structure IDs.

[snip]

Hmm. I just have
'Drivers should fall back on this legacy structure if a
Virtio Structure capability is missing in the PCI capability list'.

What else would be helpful? An example?

Quoted 4 lines

I'm not sure what capability length is, can't find it in the spec either.

[snip]

It's the legth of the vendor specific capability structure in bytes. 'the byte immediately following the “Next”
pointer in the capability structure is defined to be a length field'
It's on page 330 in my copy.

Quoted 5 lines · Michael S. Tsirkin

Quoted 1 line · Sasha Levin

The device initialization sequence might use an update as well.

What is needed? Add an item where the driver scans the PCI capability list to detect the layout?

Yup, something similar like that - just to make it obvious.

Quoted 10 lines

Quoted 4 lines

Maybe
also a description of how device handles missing structure IDs.

[snip]

Hmm. I just have
'Drivers should fall back on this legacy structure if a
Virtio Structure capability is missing in the PCI capability list'.

Quoted 1 line

What else would be helpful? An example?

I just remembered from your patch that you could define some structure IDs, but not necessarily all of them, if it's not longer the case then ignore me. If it is, then the scenario of a missing structure should be specified (For example, if the ISR structure wasn't defined, fall back to...).

Quoted 9 lines

Quoted 4 lines

I'm not sure what capability length is, can't find it in the spec either.

[snip]

It's the legth of the vendor specific capability structure in bytes. 'the byte immediately following the “Next”
pointer in the capability structure is defined to be a length field'
It's on page 330 in my copy.

Right, I was looking only at the regular cap definition.

Signature · 1 line

Sasha.

Quoted 2 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

It's not yet released so maybe it's not an issue yet. If it's too late I can re-add them to legacy too.

Pekka, 64 features aren't yet used and we are discussing changing the layout for that field. Mind taking it out of kvm tool for now?

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

It's not yet released so maybe it's not an issue yet. If it's too late I can re-add them to legacy too.

Quoted 3 lines

Pekka, 64 features aren't yet used and we are discussing changing the layout for that field. Mind taking it out of kvm tool for now?

Sasha, why did we add 64-bit features to the KVM tool? Wasn't it part of the virtio spec? Does QEMU not use them? How badly will older versions of the KVM tool break if you drop 64-bit features?

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Pekka

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Pekka

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Pekka

Quoted 11 lines · Pekka Enberg

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Quoted 1 line

Pekka

So what happened? Did you guys do this? Need to know what to do to make progress. IIUC Rusty removed the new fields in 0.9.3.
Does your tool still use them? Did any version of the tool released by distros do so?

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Pekka Enberg

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Quoted 6 lines

Quoted 1 line

Pekka

So what happened? Did you guys do this? Need to know what to do to make progress. IIUC Rusty removed the new fields in 0.9.3.
Does your tool still use them? Did any version of the tool released by distros do so?

Yup, they were removed quite a while ago.

Quoted 11 lines · Pekka Enberg

Quoted 5 lines · Sasha Levin

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

As long as we are able to run older versions of the KVM tool with newer kernels and vice versa, I see no reason why we can't drop 64-bit features from the KVM tool.

Quoted 1 line

Pekka

So what happened? Did you guys do this? Need to know what to do to make progress. IIUC Rusty removed the new fields in 0.9.3.
Does your tool still use them? Did any version of the tool released by distros do so?

They don't exist in kernel code either, for same reason as above.

Nothing will break if we remove it since no one really used it, we were probably the first and only implementation of the spec which considered them :)

Quoted 2 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

It's not yet released so maybe it's not an issue yet. If it's too late I can re-add them to legacy too.

Pekka, 64 features aren't yet used and we are discussing changing the layout for that field. Mind taking it out of kvm tool for now?

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

Quoted 8 lines · Sasha Levin

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

Quoted 3 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

What about the Windows drivers?

Quoted 9 lines · Avi Kivity

Quoted 8 lines · Sasha Levin

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

Quoted 5 lines

Quoted 3 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

What about the Windows drivers?

While 64-bit features were defined, there was no actual devices to use them.

So even if Windows drivers had the ability to support 64-bit features, no device ever needed it therefore no device ever activated it.

A potential issue might arise when we remap feature bit 31 to do something else, and when activated it would fool Windows drivers to think that 64-bit features are now activated, while they're not - but thats not something that should happen since legacy layout shouldn't have that field enabled as per the spec (no new features in legacy code).

I was unable to check if it was actually implemented in the drivers
because
http://git.kernel.org/?p=virt/kvm/kvm-guest-drivers-windows.git;a=summary is not quite there (*cough*).

Signature · 1 line

Sasha.

Quoted 26 lines · Sasha Levin

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Sasha Levin

Quoted 3 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

What about the Windows drivers?

While 64-bit features were defined, there was no actual devices to use them.

So even if Windows drivers had the ability to support 64-bit features, no device ever needed it therefore no device ever activated it.

A potential issue might arise when we remap feature bit 31 to do something else, and when activated it would fool Windows drivers to think that 64-bit features are now activated, while they're not - but thats not something that should happen since legacy layout shouldn't have that field enabled as per the spec (no new features in legacy code).

Quoted 3 lines

I was unable to check if it was actually implemented in the drivers
because
http://git.kernel.org/?p=virt/kvm/kvm-guest-drivers-windows.git;a=summary is not quite there (*cough*).

I'm pretty sure windows drivers didn't yet get to it. This was out for a very short time.

Quoted 4 lines

Signature · 1 line

Sasha.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines · Avi Kivity

Quoted 8 lines · Sasha Levin

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

Quoted 5 lines

Quoted 3 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

What about the Windows drivers?

While 64-bit features were defined, there was no actual devices to use them.

So even if Windows drivers had the ability to support 64-bit features, no device ever needed it therefore no device ever activated it.

A potential issue might arise when we remap feature bit 31 to do something else, and when activated it would fool Windows drivers to think that 64-bit features are now activated, while they're not - but thats not something that should happen since legacy layout shouldn't have that field enabled as per the spec (no new features in legacy code).

I was unable to check if it was actually implemented in the drivers
because
http://git.kernel.org/?p=virt/kvm/kvm-guest-drivers-windows.git;a=summary is not quite there (*cough*).

Signature · 1 line

Sasha.

I was unable to check if it was actually implemented in the drivers
because
http://git.kernel.org/?p=virt/kvm/kvm-guest-drivers-windows.git;a=summary is not quite there (*cough*).

Signature · 1 line

Sasha.

I was unable to check if it was actually implemented in the drivers
because
http://git.kernel.org/?p=virt/kvm/kvm-guest-drivers-windows.git;a=summary is not quite there (*cough*).

Signature · 1 line

Sasha.

Quoted 8 lines · Sasha Levin

Quoted 7 lines

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

Quoted 3 lines

KVM tool actually has support for 64bit features, we can probably remove that when Pekka isn't looking :)

What about the Windows drivers?

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

It can then go into MMIO space which is cheaper than PIO.

A legacy portion of the configuration is mirrored in the first BAR, to keep legacy drivers working. Any new fields can be added in existing structures at the end, so they won't affect legacy.

Alternatively we can add new structures with new structure IDs, pointed to from PCI configuration space.

As we don't yet have devices or drivers with 64 bit features, I decided we don't need high feature bits in legacy space. This also frees up feature bit 31 as we don't need it to enable high feature bits anymore.

As this solves the dynamic placement of MSIX vectors and high feature bits,
I thought it's easier to just reserve space for that programming than give it a separate structure. This can be changed by a patch on top.

Note that data path is split from configuration.

PDF will follow.
----

virtio-spec.lyx +4 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320781133

+<lyxtabular version="3" rows="4" columns="34"> +<features tabularvalignment="middle">
+<column alignment="center" valignment="top" width="0pt"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0"> +<column alignment="center" valignment="top" width="0pt"> +<row>

virtio-spec.lyx +13 -0

+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320779667
+Purpose:
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 1986246365 1320780912
+
+\emph on
+Capability ID

+\begin_layout Plain Layout
+ie.
+ once you enable MSI-X on the device, the other fields move.
+ If you turn it off again, they move back!

+
+\change_deleted 1986246365 1320784929
+If more than 31 feature bits are supported, the device indicates so by setting
+ feature bit 31 (see

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

Quoted 5 lines · Michael S. Tsirkin

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

Quoted 4 lines

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

I'm also wondering it it's ok to move virtio configuration out of virtio space and into PCI space for archs that don't have PCI (such as ARM).

Would it mean they get stuck with legacy configuration (and no new features)? Or is there an alternative for them?

Signature · 3 lines

-- 

Sasha.

Quoted 11 lines · Sasha Levin

Quoted 9 lines · Michael S. Tsirkin

Here's a spec change documenting what my C patch does (almost - I tweaked the layout a bit, but the idea is the same). Some more cleanups are needed but I thought I'd send it for early flames/comments.

The idea is simple: we split functionally unrelated register groups to independent structures, and let the device place is anywhere using a capability in PCI configuration space.

Quoted 2 lines

I'm also wondering it it's ok to move virtio configuration out of virtio space and into PCI space for archs that don't have PCI (such as ARM).

A bit of confusion here: no register is moved from memory into configuration space. Configuration space only gains a list of pointers into memory/pio space.

Quoted 2 lines

Would it mean they get stuck with legacy configuration (and no new features)? Or is there an alternative for them?

The change only affects the layout of virtio PCI. Arches that don't have PCI don't use virtio PCI, presumably?

BTW, the spec only covers x86 ATM, this needs to be fixed.

From what I see there is a WIP by Pawel Moll <[email protected]> to add