RFC: PCI using capabilitities

Here's the patch series I ended up with. I haven't coded up the QEMU side yet, so no idea if the new driver works.

Questions:
(1) Do we win from separating ISR, NOTIFY and COMMON?

(2) I used a "u8 bar"; should I use a bir and pack it instead? BIR seems a little obscure (noone else in the kernel source seems to refer to it).

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.

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

Signed-off-by Rusty Russell <[email protected]>

---
 include/asm-generic/io.h    |    4 ++++
 include/asm-generic/iomap.h |   11 +++++++++++
 lib/iomap.c                 |   41 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 51 insertions(+), 5 deletions(-)

include/asm-generic/io.h +4 -0

index 9120887..3cf1787 100644
@@ -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

index 98dcd76..6f192d4 100644
@@ -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)
 {

lib/iomap.c +36 -5

index 5dbcb4b..93ae915 100644
@@ -243,26 +243,37 @@ EXPORT_SYMBOL(ioport_unmap);
 
 #ifdef CONFIG_PCI
 /**
- * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
  * @bar: BAR number
- * @maxlen: length of the memory to map
+ * @offset: map memory at the given offset in BAR
+ * @minlen: min length of the memory to map
+ * @maxlen: max length of the memory to map
  *
  * Using this function you will get a __iomem address to your device BAR.
  * You can access it using ioread*() and iowrite*(). These functions hide
  * the details if this is a MMIO or PIO address space and will just do what
  * you expect from them in the correct way.
  *
+ * @minlen specifies the minimum length to map. We check that BAR is
+ * large enough.
  * @maxlen specifies the maximum length to map. If you want to get access to
- * the complete BAR without checking for its length first, pass %0 here.
+ * the complete BAR from offset to the end, pass %0 here.
  * */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+			      unsigned offset,
+			      unsigned long minlen,
+			      unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(dev, bar);
 	resource_size_t len = pci_resource_len(dev, bar);
 	unsigned long flags = pci_resource_flags(dev, bar);
 
-	if (!len || !start)
+	if (len <= offset || !start)
+		return NULL;
+	len -= offset;
+	start += offset;
+	if (len < minlen)
 		return NULL;
 	if (maxlen && len > maxlen)
 		len = maxlen;

@@ -277,10 +288,30 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	return NULL;
 }
 
+/**
+ * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, 0, maxlen);
+}
+
 void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
 {
 	IO_COND(addr, /* nothing */, iounmap(addr));
 }
 EXPORT_SYMBOL(pci_iomap);
+EXPORT_SYMBOL(pci_iomap_range);
 EXPORT_SYMBOL(pci_iounmap);
 #endif /* CONFIG_PCI */

Quoted 7 lines · Rusty Russell

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.

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

Signed-off-by Rusty Russell <[email protected]>

I think that we should add a forcenocache flag. This will let devices put the cap structure in the prefetcheable BAR. That has an advantage that it can be located anywhere in the 2^64 space, while non-prefetcheable BARs are limited to lower 4G for devices behind a PCI-to-PCI bridge.

Quoted 6 lines · Rusty Russell

I think that we should add a forcenocache flag. This will let devices put the cap structure in the prefetcheable BAR. That has an advantage that it can be located anywhere in the 2^64 space, while non-prefetcheable BARs are limited to lower 4G for devices behind a PCI-to-PCI bridge.

OK, want to respin that patch (or patch on top and I'll fold?)

Based on patch by Michael S. Tsirkin <[email protected]>, but I found it hard to follow so changed to use structures which are more self-documenting.

Signed-off-by Rusty Russell <[email protected]>

---
 include/linux/virtio_pci.h |   41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

include/linux/virtio_pci.h +6 -0

@@ -92,4 +92,45 @@
 /* The alignment to use between consumer and producer parts of vring.
  * x86 pagesize again. */
 #define VIRTIO_PCI_VRING_ALIGN		4096
+
+/* IDs for different capabilities.  Must all exist. */
+/* FIXME: Do we win from separating ISR, NOTIFY and COMMON? */
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	1
+/* Notifications */

include/linux/virtio_pci.h +14 -0

+	u8 cfg_type;	/* One of the VIRTIO_PCI_CAP_*_CFG. */
+/* FIXME: Should we use a bir, instead of raw bar number? */
+	u8 bar;		/* Where to find it. */
+	__le32 offset;	/* Offset within bar. */
+	__le32 length;	/* Length. */
+};
+
+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
+struct virtio_pci_common_cfg {
+	/* About the whole device. */
+	__le32 device_feature_select;	/* read-write */
+	__le32 device_feature;		/* read-only */
+	__le32 guest_feature_select;	/* read-write */
+	__le32 guest_feature;		/* read-only */

With module option to override. I assume I can call pci_find_capability() before pci_request_regions?

Quoted 2 lines · Rusty Russell

With module option to override. I assume I can call pci_find_capability() before pci_request_regions?

I don't think thats safe.

Quoted 2 lines · Rusty Russell

With module option to override. I assume I can call pci_find_capability() before pci_request_regions?

I don't think thats safe.

Signature · 1 line

Sasha.

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

Quoted 15 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val>>  32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Quoted 5 lines

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val>>  32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Quoted 6 lines

Quoted 2 lines

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

yes. PCI(e) support atomic 64-bit ops; it's dependent on CPU & chipset interface to PCI that determines ability to generate a 64-bit length xaction.

Quoted 15 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Quoted 5 lines

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

Signature · 1 line

MST

Quoted 19 lines · Michael S. Tsirkin

Quoted 18 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Quoted 5 lines

Quoted 2 lines

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

Ping. Going to change this in the layout struct?

Quoted 26 lines · Michael S. Tsirkin

Quoted 24 lines · Michael S. Tsirkin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Quoted 7 lines

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

Quoted 1 line

Ping. Going to change this in the layout struct?

Not sure what you mean.... We use a queue_enable field, so it doesn't matter if the accesses aren't atomic for that.

Cheers,
Rusty.

Quoted 28 lines · Rusty Russell

Quoted 26 lines · Michael S. Tsirkin

Quoted 24 lines · Michael S. Tsirkin

Quoted 13 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Review

Quoted 7 lines · Rusty Russell

(quoted diff) +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Quoted 7 lines

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

I think PCI can optionally support atomic 64 bit accesses, but not all architectures can generate them.

Quoted 7 lines

Quoted 1 line

Ping. Going to change this in the layout struct?

Not sure what you mean.... We use a queue_enable field, so it doesn't matter if the accesses aren't atomic for that.

Cheers,
Rusty.

I mean the struct should have separate _lo and _hi fields.

Otherwise I have to do:

(quoted diff) +6 -0

+    case offsetof(struct virtio_pci_common_cfg, queue_desc):
+	 assert(size == 4);
+        return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) & low;
+    case offsetof(struct virtio_pci_common_cfg, queue_desc) + 4:
+	 assert(size == 4);
+        return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) >> 32;

Would be nicer as:

(quoted diff) +6 -0

+    case offsetof(struct virtio_pci_common_cfg, queue_desc_lo):
+	 assert(size == sizeof cfg.queue_desc_lo);
+        return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) & low;
+    case offsetof(struct virtio_pci_common_cfg, queue_desc_hi):
+	 assert(size ==  sizeof cfg.queue_desc_hi);
+        return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) >> 32;

Agree?

Quoted 11 lines · Michael S. Tsirkin

drivers/virtio/virtio_pci.c line 117

Quoted 7 lines · Rusty Russell

drivers/virtio/virtio_pci.c new file +7 -0

+/* There is no iowrite64.  We use two 32-bit ops. */
+static void iowrite64(u64 val, const __le64 *addr)
+{
+	iowrite32((u32)val, (__le32 *)addr);
+	iowrite32(val >> 32, (__le32 *)addr + 1);
+}
+

Let's put addr_lo/addr_hi in the structure then, to make the fact this field is not atomic explicit?

Good point, assuming I haven't missed something.

Are 64-bit accesses actually unknown in PCI-land? Or is this a limited availability thing?

Thanks,
Rusty.

By the way, a generic question on virtio-pci: we now have:

/* virtio config->get() implementation */
static void vp_get(struct virtio_device *vdev, unsigned offset,
void *buf, unsigned len)

{
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
        void __iomem *ioaddr = vp_dev->ioaddr +
                                VIRTIO_PCI_CONFIG(vp_dev) + offset;
        u8 *ptr = buf;
        int i;

        for (i = 0; i < len; i++)
                ptr[i] = ioread8(ioaddr + i);
}

This means that if configuration is read while it is changed, we might get an inconsistent state, with parts of a 64 bit field coming from old and parts from new value.

Isn't this a problem?

Quoted 21 lines · Michael S. Tsirkin

By the way, a generic question on virtio-pci: we now have:

/* virtio config->get() implementation */
static void vp_get(struct virtio_device *vdev, unsigned offset,
void *buf, unsigned len)

{
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
        void __iomem *ioaddr = vp_dev->ioaddr +
                                VIRTIO_PCI_CONFIG(vp_dev) + offset;
        u8 *ptr = buf;
        int i;

        for (i = 0; i < len; i++)
                ptr[i] = ioread8(ioaddr + i);
}

This means that if configuration is read while it is changed, we might get an inconsistent state, with parts of a 64 bit field coming from old and parts from new value.

Quoted 1 line

Isn't this a problem?

I don't think so; it's the caller's problem if they need to do locking. Is there a caller which needs this?

Or am I missing something?
Rusty.

On Tue, Dec 13, 2011 at 12:51:20PM +1030, Rusty Russell wrote:
I mean like this in block:

        /* Host must always specify the capacity. */
        vdev->config->get(vdev, offsetof(struct virtio_blk_config,
capacity),
                          &capacity, sizeof(capacity));

        /* If capacity is too big, truncate with warning. */
        if ((sector_t)capacity != capacity) {
                dev_warn(&vdev->dev, "Capacity %llu too large:
truncating\n",
                         (unsigned long long)capacity);
                capacity = (sector_t)-1;
        }

Now let's assume capacity field is changed from 0x8000 to 0x10000 on host. Is it possible that we read two upper bytes before the change so we see 0x0000....
and 2 lower bytes after the change
so we see 0x....0000 and resulting capacity appears
to be 0?

If no why not?

Quoted 19 lines · Michael S. Tsirkin

On Tue, Dec 13, 2011 at 12:51:20PM +1030, Rusty Russell wrote:
I mean like this in block:

        /* Host must always specify the capacity. */
        vdev->config->get(vdev, offsetof(struct virtio_blk_config,
capacity),
                          &capacity, sizeof(capacity));

        /* If capacity is too big, truncate with warning. */
        if ((sector_t)capacity != capacity) {
                dev_warn(&vdev->dev, "Capacity %llu too large:
truncating\n",
                         (unsigned long long)capacity);
                capacity = (sector_t)-1;
        }

Quoted 8 lines

Now let's assume capacity field is changed from 0x8000 to 0x10000 on host. Is it possible that we read two upper bytes before the change so we see 0x0000....
and 2 lower bytes after the change
so we see 0x....0000 and resulting capacity appears
to be 0?

If no why not?

Same issue in reverse with the guest setting the MAC address in virtio_net, if the host were reading it. And virtio_balloon? We have ignored it, so far.

Perhaps a new feature VIRTIO_F_UNSTABLE? Which (unlike other features) appears and vanishes around config writes by either side? Kind of a hack though...

Cheers,
Rusty.

Quoted 3 lines · Rusty Russell

Perhaps a new feature VIRTIO_F_UNSTABLE? Which (unlike other features) appears and vanishes around config writes by either side? Kind of a hack though...

Not sure how this can work in such a setup: when would guest check this bit to avoid races?
A separate registers also seems nicer than a flag.

Some other possible design choices:
- a flag to signal config accesses in progress by guest host would need to buffer changes and apply them in one go when flag is cleared
- a register to make host get/set config in guest memory
- use a control vq for all devices

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell

Perhaps a new feature VIRTIO_F_UNSTABLE? Which (unlike other features) appears and vanishes around config writes by either side? Kind of a hack though...

Not sure how this can work in such a setup: when would guest check this bit to avoid races?
A separate registers also seems nicer than a flag.

Quoted 6 lines

Some other possible design choices:
- a flag to signal config accesses in progress by guest host would need to buffer changes and apply them in one go when flag is cleared
- a register to make host get/set config in guest memory
- use a control vq for all devices

- seqlock-style generation count register(s)?
Has the advantage of
being a noop if things never change.

- continue to ignore it ;)

And yes, it's a more general problem than virtio_pci...

Cheers,
Rusty.

Quoted 17 lines · Rusty Russell

Quoted 15 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell

Perhaps a new feature VIRTIO_F_UNSTABLE? Which (unlike other features) appears and vanishes around config writes by either side? Kind of a hack though...

Not sure how this can work in such a setup: when would guest check this bit to avoid races?
A separate registers also seems nicer than a flag.

Some other possible design choices:
- a flag to signal config accesses in progress by guest host would need to buffer changes and apply them in one go when flag is cleared
- a register to make host get/set config in guest memory
- use a control vq for all devices

Quoted 3 lines

- seqlock-style generation count register(s)?
Has the advantage of
being a noop if things never change.

The counter can be in guest memory, right? So we don't pay extra exits.

Quoted 1 line

- continue to ignore it ;)

Since you decided on a config layout redesign it seems a good time to fix architectural problems ...

Quoted 18 lines · Michael S. Tsirkin

Quoted 17 lines · Rusty Russell

Quoted 15 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell

Perhaps a new feature VIRTIO_F_UNSTABLE? Which (unlike other features) appears and vanishes around config writes by either side? Kind of a hack though...

Not sure how this can work in such a setup: when would guest check this bit to avoid races?
A separate registers also seems nicer than a flag.

Some other possible design choices:
- a flag to signal config accesses in progress by guest host would need to buffer changes and apply them in one go when flag is cleared
- a register to make host get/set config in guest memory
- use a control vq for all devices

Quoted 3 lines

Quoted 3 lines

- seqlock-style generation count register(s)?
Has the advantage of
being a noop if things never change.

Actually, the host doesn't need anything, since it can always lock out the guest while it updates the area.
It's the guest which can't do atomic updates.

Quoted 2 lines

The counter can be in guest memory, right? So we don't pay extra exits.

Could be, but I'm not delighted about the design.

What does the host do
if the guest screws things up? How long do you wait for them to complete the seqlock? Or does it save the old version for use in the duration?

And we don't have any atomic guest write problems that I know of. We can solve it in future (by specifying a config queue).

Quoted 4 lines

Quoted 1 line

- continue to ignore it ;)

Since you decided on a config layout redesign it seems a good time to fix architectural problems ...

Yes, indeed.

Cheers,
Rusty.

Quoted 5 lines · Rusty Russell

On Mon, 19 Dec 2011 11:13:26 +0200, "Michael S. Tsirkin" <[email protected]> wrote:

Actually, the host doesn't need anything, since it can always lock out the guest while it updates the area.
It's the guest which can't do atomic updates.

There are 2 cases
- updates by guest accesses by host
- accesses by guest updates by host

Both are problematic because the guest accesses are split. Consider the example I gave at the beginning was with capacity read by guest. Host can not solve it without guest changes, right?

Quoted 4 lines

Quoted 2 lines

The counter can be in guest memory, right? So we don't pay extra exits.

Could be, but I'm not delighted about the design.

OK, so this is an argument for always using a control vq, right?

Quoted 4 lines

What does the host do
if the guest screws things up? How long do you wait for them to complete the seqlock? Or does it save the old version for use in the duration?

Yes, it will have to only apply the change when seqlock is dropped.

Don't have == not reported as observed in the field? It seems clear from code that we do have a race, correct?

Quoted 6 lines

Quoted 4 lines

What does the host do
if the guest screws things up? How long do you wait for them to complete the seqlock? Or does it save the old version for use in the duration?

Yes, it will have to only apply the change when seqlock is dropped.

If the seqlock is in normal memory, how does it get notified? It would have to poll. That's annoying, since you don't know when to give up and declare the device terminally broken.

Quoted 7 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell

On Mon, 19 Dec 2011 11:13:26 +0200, "Michael S. Tsirkin" <[email protected]> wrote:

Actually, the host doesn't need anything, since it can always lock out the guest while it updates the area.
It's the guest which can't do atomic updates.

Quoted 7 lines

There are 2 cases
- updates by guest accesses by host
- accesses by guest updates by host

Both are problematic because the guest accesses are split. Consider the example I gave at the beginning was with capacity read by guest. Host can not solve it without guest changes, right?

Indeed, my brain fart. Let's pretend I didn't say that, and you didn't have to explain it to me in baby words :)

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

The counter can be in guest memory, right? So we don't pay extra exits.

Could be, but I'm not delighted about the design.

Quoted 1 line

OK, so this is an argument for always using a control vq, right?

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

Quoted 4 lines

Quoted 2 lines

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field. This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

We should also change the ring (to a single ring, I think). Descriptors to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags).

We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes.  The idea that we can alter fields in the device-specific config area is flawed.  There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space.  Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field.  This solves half the problem, of sync guest writes.  For the other half, I suggest a generation counter; odd means inconsistent.  The guest can poll.

BenH also convinced me we should finally make the config space LE if we're going to change things.  Since PCI is the most common transport, guest-endian confuses people.  And it sucks for really weird machines.

We should also change the ring (to a single ring, I think).  Descriptors to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing.  We should support inline, chained or indirect.  Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec).  That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Euh why ?

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Euh why ?

Because it's a paravirt hack that ends up hitting corner cases. It's not possible to do virtio-pci passthrough under nested virtualization unless we use an IOMMU. Imagine passing virtio-net from L0 into the L2 guest (i.e. PCI-passthrough). If virtio-pci is really "PCI" this should be possible but it's not when we use physical addresses instead of bus addresses.

Stefan

Quoted 17 lines · Stefan Hajnoczi

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Euh why ?

Because it's a paravirt hack that ends up hitting corner cases.  It's not possible to do virtio-pci passthrough under nested virtualization unless we use an IOMMU.  Imagine passing virtio-net from L0 into the L2 guest (i.e. PCI-passthrough).  If virtio-pci is really "PCI" this should be possible but it's not when we use physical addresses instead of bus addresses.

Quoted 1 line

Stefan

It won't be hard to show siginificant performance regression if we do this. Hard to justify for something as niche as nested virt.

Quoted 18 lines · Michael S. Tsirkin

Quoted 17 lines · Stefan Hajnoczi

Quoted 7 lines · Benjamin Herrenschmidt

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Euh why ?

Because it's a paravirt hack that ends up hitting corner cases.  It's not possible to do virtio-pci passthrough under nested virtualization unless we use an IOMMU.  Imagine passing virtio-net from L0 into the L2 guest (i.e. PCI-passthrough).  If virtio-pci is really "PCI" this should be possible but it's not when we use physical addresses instead of bus addresses.

Quoted 4 lines

Quoted 1 line

Stefan

It won't be hard to show siginificant performance regression if we do this. Hard to justify for something as niche as nested virt.

For x86 this should be mostly a nop.

For ppc and SPARC architectures maybe you're right. I still think it's a design flaw because if virtio v2 doesn't use bus addresses then it will simply not be possible to do passthrough for nested virt and other cases we haven't hit yet.

Stefan

Quoted 2 lines

It won't be hard to show siginificant performance regression if we do this. Hard to justify for something as niche as nested virt.

For x86 this should be mostly a nop.

For ppc and SPARC architectures
maybe you're right. I still think it's a design flaw because if virtio v2 doesn't use bus addresses then it will simply not be possible to do passthrough for nested virt and other cases we haven't hit yet.

Quoted 6 lines · Benjamin Herrenschmidt

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Quoted 1 line

Euh why ?

Because it's a paravirt hack that ends up hitting corner cases. It's not possible to do virtio-pci passthrough under nested virtualization unless we use an IOMMU. Imagine passing virtio-net from L0 into the L2 guest (i.e. PCI-passthrough). If virtio-pci is really "PCI" this should be possible but it's not when we use physical addresses instead of bus addresses.

Quoted 8 lines · Stefan Hajnoczi

Quoted 6 lines · Benjamin Herrenschmidt

Quoted 4 lines · Stefan Hajnoczi

This is also an opportunity to stop using CPU physical addresses in the ring and instead perform DMA like a normal PCI device (use bus addresses).

Quoted 8 lines

Quoted 1 line

Euh why ?

Because it's a paravirt hack that ends up hitting corner cases.  It's not possible to do virtio-pci passthrough under nested virtualization unless we use an IOMMU.  Imagine passing virtio-net from L0 into the L2 guest (i.e. PCI-passthrough).  If virtio-pci is really "PCI" this should be possible but it's not when we use physical addresses instead of bus addresses.

Is this just an academic exercise or is there any actual value in doing this ?

Using an iommu is going to slaugher your performance, so at the very least it should be kept an option.

Quoted 8 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Quoted 1 line

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field.

This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

We should also change the ring (to a single ring, I think). Descriptors to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Thoughts?
Rusty.

Quoted 4 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

Are we going to keep guest endian for e.g. virtio net header? If yes the benefit of switching config space is not that big. And changes in devices would affect non-PCI transports.

Quite possibly all or some of these things help performance but do we have to change the spec before we have experimental proof?

Quoted 7 lines

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Thoughts?
Rusty.

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1.

Is there any reason why we shouldn't be
able to experiment with inline within virtio1 and see whether that gets us anything?
If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Quoted 8 lines · Michael S. Tsirkin

Quoted 4 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

Are we going to keep guest endian for e.g. virtio net header? If yes the benefit of switching config space is not that big. And changes in devices would affect non-PCI transports.

I think the concept of "guest endian" is broken by design. What does that mean when running for example an ARM or a ppc 440 "guest" which could be either endian ? Since you can't hard code your guest endian how do you obtain/negociate it ? Also you now have to deal with dual endian in the host, makes everything trickier.

Just make everything LE.

Quoted 3 lines

Quite possibly all or some of these things help performance but do we have to change the spec before we have experimental proof?

Well, I would argue that the network driver world has proven countless times that those are good ideas :-)

But by all mean, let's do a
prototype implementation with virtio-net for example and bench it.

I don't think you need a single ring. For multiqueue net, you definitely want multiple rings and you do want rings to remain uni-directional.

One other thing that can be useful is to separate the completion ring from the actual ring of DMA descriptors, making the former completely read-only by the guest and the later completely read only by the host.

For example take the ehea ethernet rx model. It has 3 rx "rings" per queue. One contains the completions, it's a toggle-valid model so we never write back to clear valid, it contains infos from the parser, the tokenID of the packet and the index as to where in which ring the data is, wich is either inline in the completion ring (small packet), header inline & data in a data ring or completely in a data ring. Then you have two data rings which are simply rings of SG list entries (more or less).

We typically pre-populate the data rings with skb's for 1500 and 9000 bytes packets. Small packets come in immediately in the completion ring, and large packets via the data ring.

That's just -an- example. There's many other to take inspiration from. Network folks have beaten to death the problem of ring efficiency vs. CPU caches.

Quoted 5 lines

Quoted 5 lines

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Quoted 5 lines

Quoted 2 lines

Thoughts?
Rusty.

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

That was 10 years ago...

Quoted 3 lines

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1.

Maybe we have learned some more since then ? :-)

Quoted 10 lines

Is there any reason why we shouldn't be
able to experiment with inline within virtio1 and see whether that gets us anything?
If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Another advantage of inline data is that it makes things a lot easier for cases where only small amount of data need to be exchanged, such as control/status rings, maybe virtio-tty (which I'm working on), etc...

Cheers,
Ben.

Quoted 3 lines

We typically pre-populate the data rings with skb's for 1500 and 9000 bytes packets. Small packets come in immediately in the completion ring, and large packets via the data ring.

Won't real workloads suffer from packet reordering?

Quoted 2 lines

Well, I would argue that the network driver world has proven countless times that those are good ideas :-)

Below you seem to suggest that separate rings like virtio has now is better than a single ring like Rusty suggested.

Are you familiar with current virtio ring structure? How is this different?

I'd suggest doing a simple user space app that creates such a ring, forks, and produce / consume. We can then run that through perf and analyze the cache behaviour, maximum throughput, etc....

Cheers,
Ben.

Quoted 6 lines · Benjamin Herrenschmidt

I'd suggest doing a simple user space app that creates such a ring, forks, and produce / consume. We can then run that through perf and analyze the cache behaviour, maximum throughput, etc....

Cheers,
Ben.

Sure. You can also use my tools/virtio hack: this rebuilds virtio ring code in userspace, has the advantage of reusing actual kernel code, so it's up to date, has same barriers, etc.

Quoted 11 lines · Michael S. Tsirkin

Quoted 6 lines · Benjamin Herrenschmidt

I'd suggest doing a simple user space app that creates such a ring, forks, and produce / consume. We can then run that through perf and analyze the cache behaviour, maximum throughput, etc....

Cheers,
Ben.

Sure. You can also use my tools/virtio hack: this rebuilds virtio ring code in userspace, has the advantage of reusing actual kernel code, so it's up to date, has same barriers, etc.

Yes, Rusty mentioned it today :-)

We'll play around in the next couple of weeks. With LCA, I'm not sure how much I'll get done next week but heh...

I'm curious to see if we can still have a one-size-fit-all between unordered blk requests with large data/descriptor access ratio and ordered network buffers with a much smaller one.

Cheers,
Ben.

Quoted 11 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Quoted 5 lines

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field.

Are you sure? I think net writes a mac address.

Quoted 3 lines

This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

So we get the counter until it's even, get the config, if it's changed repeat? Yes it works. However, I would like to have a way to detect config change just by looking at memory. ATM we need to read ISR to know. If we used a VQ, the advantage would be that the device can work with a single MSIX vector shared by all VQs.

If we do require config VQ anyway, why not use it to notify guest of config changes? Guest could pre-post an in buffer and host uses that.

Quoted 3 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

Are we going to keep guest endian for e.g. virtio net header? If yes the benefit of switching config space is not that big. And changes in devices would affect non-PCI transports.

Quoted 5 lines

We should also change the ring (to a single ring, I think). Descriptors to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Quite possibly all or some of these things help performance but do we have to change the spec before we have experimental proof?

I did experiment with a single ring using tools/virtio and I didn't see a measureable performance gain.

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Since
host doesn't change desriptors, we could also
preformat some descriptors in the current design.

There is a fragmentation problem in theory but it can be alleviated with a smart allocator.

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1. Is there any reason why we shouldn't be able to experiment with inline within virtio1 and see whether that gets us anything?

If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Quoted 3 lines

Are we going to keep guest endian for e.g. virtio net header? If yes the benefit of switching config space is not that big. And changes in devices would affect non-PCI transports.

Yep. It would only make sense if we do it for everything. And yes, it'll mess up everyone who is BE, so it needs to be a feature bit for them.

Interesting. It is simpler and more standard than our current design, but that's not sufficient unless there are other reasons. Needs further discussion and testing.

Quoted 2 lines

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Well, a simple host could process in-order and leave stuff in the ring I guess. A smarter host would copy and queue, maybe leave one queue entry in so it doesn't get flooded?

Quoted 2 lines

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

It was suggested by others, but I think TCP Acks are the classic one.

12 + 14 + 20 + 40 = 86 bytes with virtio_net_hdr_mrg_rxbuf at the front.

Quoted 2 lines

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

(s/2001/2011). Indeed. Noone shared my optimism that having an open process for a virtio2 would bring more players on board (my original motivation). But technical requirements are mounting up, which means we're going to get there anyway.

Quoted 5 lines

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1. Is there any reason why we shouldn't be able to experiment with inline within virtio1 and see whether that gets us anything?

Inline in the used ring is possible, but those descriptors 8 bytes, vs 24/32.

Quoted 4 lines

If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Quoted 3 lines

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Yes, I'm thinking #define VIRTIO_F_VIRTIO2 (-1). For PCI, this gets mapped into a "are we using the new config layout?". For others, it gets mapped into a transport-specific feature.

(I'm sure you get it, but for the others) This is because I want to be draw a clear line between all the legacy stuff at the same time, not have to support part of it later because someone might not flip the feature bit.

However I can see at least one advantage of what you've done :-) You never have to deal with holes in the ring.

Quoted 6 lines

Quoted 2 lines

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Well, a simple host could process in-order and leave stuff in the ring I guess. A smarter host would copy and queue, maybe leave one queue entry in so it doesn't get flooded?

What's wrong with a ring of descriptors + a ring of completion, with a single toggle valid bit to indicate whether a given descriptor is valid or not (to avoid the nasty ping pong on the ring head/tails).

Quoted 4 lines

Quoted 2 lines

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

It was suggested by others, but I think TCP Acks are the classic one.

Split headers + data too, tho that means supporting immediate + indirect.

It makes a lot of sense for command rings as well if we're going to go down that route.

Quoted 27 lines

12 + 14 + 20 + 40 = 86 bytes with virtio_net_hdr_mrg_rxbuf at the front.

Quoted 2 lines

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

(s/2001/2011). Indeed. Noone shared my optimism that having an open process for a virtio2 would bring more players on board (my original motivation). But technical requirements are mounting up, which means we're going to get there anyway.

Quoted 5 lines

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1. Is there any reason why we shouldn't be able to experiment with inline within virtio1 and see whether that gets us anything?

Inline in the used ring is possible, but those descriptors 8 bytes, vs 24/32.

Quoted 7 lines

If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Quoted 3 lines

Yes, I'm thinking #define VIRTIO_F_VIRTIO2 (-1). For PCI, this gets mapped into a "are we using the new config layout?". For others, it gets mapped into a transport-specific feature.

Or we can use the PCI ProgIf to indicate a different programming interface, that way we also use that as an excuse to say that the first BAR can either be PIO or MMIO :-)

Quoted 4 lines

(I'm sure you get it, but for the others) This is because I want to be draw a clear line between all the legacy stuff at the same time, not have to support part of it later because someone might not flip the feature bit.

Cheers,
Ben.

Quoted 2 lines · Benjamin Herrenschmidt

However I can see at least one advantage of what you've done :-) You never have to deal with holes in the ring.

Another advantage is the design goal for that ring:
host never needs to copy even if it completes
descriptors out of order. And out of order is something that does not happen at all with hardware drivers. This is where paravirt is different.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Well, a simple host could process in-order and leave stuff in the ring I guess. A smarter host would copy and queue, maybe leave one queue entry in so it doesn't get flooded?

Quoted 3 lines

What's wrong with a ring of descriptors + a ring of completion, with a single toggle valid bit to indicate whether a given descriptor is valid or not (to avoid the nasty ping pong on the ring head/tails).

First, I don't understand how a valid bit avoids ping poing on the last descriptor. Second, how do you handle out of order completions?

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

It was suggested by others, but I think TCP Acks are the classic one.

Quoted 5 lines

Split headers + data too, tho that means supporting immediate + indirect.

It makes a lot of sense for command rings as well if we're going to go down that route.

I don't see why it makes sense for commands. It's a performance optimization and commands are off the data path.

We can't, legal PCI ProgIf values are defined in PCI spec.

Quoted 9 lines · Michael S. Tsirkin

Quoted 2 lines · Benjamin Herrenschmidt

However I can see at least one advantage of what you've done :-) You never have to deal with holes in the ring.

Another advantage is the design goal for that ring:
host never needs to copy even if it completes
descriptors out of order. And out of order is something that does not happen at all with hardware drivers. This is where paravirt is different.

Actually out of order can happen with tagged command queue for SCSI or ATA, tho I'm not 100% familiar with how things like AHCI handle this.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 2 lines

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Well, a simple host could process in-order and leave stuff in the ring I guess. A smarter host would copy and queue, maybe leave one queue entry in so it doesn't get flooded?

What's wrong with a ring of descriptors + a ring of completion, with a single toggle valid bit to indicate whether a given descriptor is valid or not (to avoid the nasty ping pong on the ring head/tails).

Quoted 2 lines

First, I don't understand how a valid bit avoids ping poing on the last descriptor. Second, how do you handle out of order completions?

A toggle means the valid bit is never cleared, it just changes polarity every time you go around the ring. So there's never a write back to 0.

Out of order is something I hadn't thought about (I was most probably too focused on virtio-net) and is indeed a PITA. It's doable with rings but can get nasty.

I'll give that more thought in the next week, and Rusty and I shall play with userspace models based on your tool.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

It was suggested by others, but I think TCP Acks are the classic one.

Quoted 8 lines

Quoted 5 lines

Split headers + data too, tho that means supporting immediate + indirect.

It makes a lot of sense for command rings as well if we're going to go down that route.

I don't see why it makes sense for commands. It's a performance optimization and commands are off the data path.

Oh just code simplification not having to dequeue a descriptor, allocate a buffer, etc... but a lot of that can be buried in helpers indeed.

Quoted 1 line

We can't, legal PCI ProgIf values are defined in PCI spec.

Hrm, more or less yes, I suppose we should stay away from that then, do we use revision ID for anything in virtio-land ?

Cheers,
Ben.

One othe rinteresting thing is that
in practice host often runs on the same CPU as guest.

This is the fastest way to run the host for when it
does not do a lot of work as most data is cached. And there amount of sharing is less important than reducing cache consumption.

Again this is different from real hardware.

Quoted 18 lines · Michael S. Tsirkin

Quoted 16 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field.

Quoted 1 line

Are you sure? I think net writes a mac address.

True. We'll need to disable that, and come up with another mechanism if we want it back (a new feature and a VIRTIO_NET_HDR_F_SET_MAC flag in the virtio_net header? Or would that mess up vhost_net?).

Quoted 9 lines

Quoted 3 lines

This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

So we get the counter until it's even, get the config, if it's changed repeat? Yes it works. However, I would like to have a way to detect config change just by looking at memory. ATM we need to read ISR to know. If we used a VQ, the advantage would be that the device can work with a single MSIX vector shared by all VQs.

If we use a 32-bit counter, we also get this though, right?

If counter has changed, it's a config interrupt...

Quoted 3 lines

If we do require config VQ anyway, why not use it to notify guest of config changes? Guest could pre-post an in buffer and host uses that.

We could, but it's an additional burden on each device. vqs are cheap, but not free. And the config area is so damn convenient...

Quoted 7 lines

Quoted 3 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines.

Are we going to keep guest endian for e.g. virtio net header? If yes the benefit of switching config space is not that big. And changes in devices would affect non-PCI transports.

Yep. It would only make sense if we do it for everything. And yes, it'll mess up everyone who is BE, so it needs to be a feature bit for them.

Quoted 9 lines

Quoted 5 lines

We should also change the ring (to a single ring, I think). Descriptors to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Quite possibly all or some of these things help performance but do we have to change the spec before we have experimental proof?

We change the spec last, once we know what we're doing, ideally.

Quoted 2 lines

I did experiment with a single ring using tools/virtio and I didn't see a measureable performance gain.

Interesting. It is simpler and more standard than our current design, but that's not sufficient unless there are other reasons. Needs further discussion and testing.

Quoted 2 lines

Two rings do have the advantage of not requiring host side copy, which copy would surely add to cache pressure.

Well, a simple host could process in-order and leave stuff in the ring I guess. A smarter host would copy and queue, maybe leave one queue entry in so it doesn't get flooded?

Quoted 6 lines

Since
host doesn't change desriptors, we could also
preformat some descriptors in the current design.

There is a fragmentation problem in theory but it can be alleviated with a smart allocator.

Yeah, the complexity scares me...

Quoted 2 lines

About inline - it can only help very small buffers. Which workloads do you have in mind exactly?

It was suggested by others, but I think TCP Acks are the classic one. 12 + 14 + 20 + 40 = 86 bytes with virtio_net_hdr_mrg_rxbuf at the front.

Quoted 2 lines

BTW this seems to be the reverse from what you have in Mar 2001, see [email protected] :)

(s/2001/2011). Indeed. Noone shared my optimism that having an open process for a virtio2 would bring more players on board (my original motivation). But technical requirements are mounting up, which means we're going to get there anyway.

Quoted 5 lines

I am much less concerned with what we do for configuration, but I do not believe we have learned all performance lessons from virtio ring1. Is there any reason why we shouldn't be able to experiment with inline within virtio1 and see whether that gets us anything?

Inline in the used ring is possible, but those descriptors 8 bytes, vs 24/32.

Quoted 7 lines

If we do a bunch of changes to the ring at once, we can't figure out what's right, what's wrong, or back out of mistakes later.

Since there are non PCI transports that use the ring, we really shouldn't make both the configuration and the ring changes depend on the same feature bit.

Yes, I'm thinking #define VIRTIO_F_VIRTIO2 (-1). For PCI, this gets mapped into a "are we using the new config layout?". For others, it gets mapped into a transport-specific feature.

(I'm sure you get it, but for the others) This is because I want to be draw a clear line between all the legacy stuff at the same time, not have to support part of it later because someone might not flip the feature bit.

Cheers,
Rusty.

Quoted 3 lines · Rusty Russell

If we use a 32-bit counter, we also get this though, right?

If counter has changed, it's a config interrupt...

But we need an exit to read the counter. We can put the counter in memory but this looks suspiciously like a simplified VQ - so why not use a VQ then?

Quoted 6 lines

Quoted 3 lines

If we do require config VQ anyway, why not use it to notify guest of config changes? Guest could pre-post an in buffer and host uses that.

We could, but it's an additional burden on each device. vqs are cheap, but not free. And the config area is so damn convenient...

Not if you start playing with counters, checking it twice, reinvent all kind of barriers ...

Quoted 2 lines

It was suggested by others, but I think TCP Acks are the classic one. 12 + 14 + 20 + 40 = 86 bytes with virtio_net_hdr_mrg_rxbuf at the front.

That's only the simplest IPv4, right?
Anyway, this spans multiple descriptors so this complicates allocation significantly.

Quoted 8 lines

Yes, I'm thinking #define VIRTIO_F_VIRTIO2 (-1). For PCI, this gets mapped into a "are we using the new config layout?". For others, it gets mapped into a transport-specific feature.

(I'm sure you get it, but for the others) This is because I want to be draw a clear line between all the legacy stuff at the same time, not have to support part of it later because someone might not flip the feature bit.

So my point is, config stuff and ring are completely separate, they are different layers.

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field. This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines

We should also change the ring (to a single ring, I think).

Descriptors
to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Thoughts?
Rusty.

Quoted 19 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field. This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

Quoted 3 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines

I think the more important thing to do is require accesses to integers in the config space to always be aligned and to use the appropriate accessor. Non-integer fields should be restricted to byte access.

That limits config space entries to 32-bit but also means that there is no need for a generation counter. It's also easier to deal with endian conversion that way.

But it means the backend code ends up being much simpler to write (because it behaves more like a normal PCI device).

If we're already making the change, the endianness ought to be a feature bit.

Quoted 1 line

We should also change the ring (to a single ring, I think).

Ack.

Quoted 10 lines

Descriptors
to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Ack. Long live virtio2! :-)

Regards,

Anthony Liguori

Quoted 3 lines

Thoughts?
Rusty.

Quoted 29 lines · Anthony Liguori

Quoted 19 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field. This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

Quoted 3 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines

I think the more important thing to do is require accesses to integers in the config space to always be aligned and to use the appropriate accessor. Non-integer fields should be restricted to byte access.

Quoted 3 lines

That limits config space entries to 32-bit but also means that there is no need for a generation counter. It's also easier to deal with endian conversion that way.

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

Quoted 31 lines

But it means the backend code ends up being much simpler to write (because it behaves more like a normal PCI device).

If we're already making the change, the endianness ought to be a feature bit.

Quoted 1 line

We should also change the ring (to a single ring, I think).

Ack.

Quoted 10 lines

Descriptors
to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Ack. Long live virtio2! :-)

Regards,

Anthony Liguori

Quoted 3 lines

Thoughts?
Rusty.

Quoted 30 lines · Michael S. Tsirkin

Quoted 29 lines · Anthony Liguori

Quoted 19 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin

Quoted 6 lines · Rusty Russell

Yes. The idea that we can alter fields in the device-specific config area is flawed. There may be cases where it doesn't matter, but as an idea it was holed to begin with.

We can reduce probability by doing a double read to check, but there are still cases where it will fail.

Okay - want me to propose an interface for that?

Had a brief chat with BenH (CC'd).

I think we should deprecate writing to the config space. Only balloon does it AFAICT, and I can't quite figure out *why* it has an 'active' field. This solves half the problem, of sync guest writes. For the other half, I suggest a generation counter; odd means inconsistent. The guest can poll.

Quoted 3 lines

BenH also convinced me we should finally make the config space LE if we're going to change things. Since PCI is the most common transport, guest-endian confuses people. And it sucks for really weird machines

I think the more important thing to do is require accesses to integers in the config space to always be aligned and to use the appropriate accessor. Non-integer fields should be restricted to byte access.

Quoted 7 lines

Quoted 3 lines

That limits config space entries to 32-bit but also means that there is no need for a generation counter. It's also easier to deal with endian conversion that way.

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

This is no different than a normal network card. You have to use a secondary register to trigger an update.

Regards,

Anthony Liguori

Quoted 31 lines

Quoted 31 lines

But it means the backend code ends up being much simpler to write (because it behaves more like a normal PCI device).

If we're already making the change, the endianness ought to be a feature bit.

Quoted 1 line

We should also change the ring (to a single ring, I think).

Ack.

Quoted 10 lines

Descriptors
to 24 bytes long (8 byte cookie, 8 byte addr, 4 byte len, 4 byte flags). We might be able to squeeze it into 20 bytes but that means packing. We should support inline, chained or indirect. Let the other side ack by setting flag, cookie and len (if written).

Moreover, I think we should make all these changes at once (at least, in the spec). That makes it a big change, and it'll take longer to develop, but makes it easy in the long run to differentiate legacy and modern virtio.

Ack. Long live virtio2! :-)

Regards,

Anthony Liguori

Quoted 3 lines

Thoughts?
Rusty.

Quoted 7 lines · Anthony Liguori

Quoted 3 lines

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

This is no different than a normal network card. You have to use a secondary register to trigger an update.

Quoted 3 lines

Regards,

Anthony Liguori

Possible but doesn't let us layer nicely to allow unchanged drivers that work with all transports (new pci, old pci, non pci).

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Anthony Liguori

Quoted 3 lines

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

This is no different than a normal network card. You have to use a secondary register to trigger an update.

Regards,

Quoted 4 lines

Quoted 1 line

Anthony Liguori

Possible but doesn't let us layer nicely to allow unchanged drivers that work with all transports (new pci, old pci, non pci).

If we declare config space LE, then we have to touch all drivers. There's no way around it because the virtio API is byte-based, not word based.

This is why I'm suggesting making the virtio API (and then the virtio-pci ABI) word based. It gives us the flexibility to make endianness a property of the transport and not a property of the devices.

Regards,

Anthony Liguori

Quoted 13 lines · Anthony Liguori

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Anthony Liguori

Quoted 3 lines

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

This is no different than a normal network card. You have to use a secondary register to trigger an update.

Regards,

Anthony Liguori

Quoted 6 lines

Quoted 2 lines

Possible but doesn't let us layer nicely to allow unchanged drivers that work with all transports (new pci, old pci, non pci).

If we declare config space LE, then we have to touch all drivers. There's no way around it because the virtio API is byte-based, not word based.

Fine but don't we want to be compatible with old hypervisors?

Quoted 8 lines

This is why I'm suggesting making the virtio API (and then the virtio-pci ABI) word based. It gives us the flexibility to make endianness a property of the transport and not a property of the devices.

Regards,

Anthony Liguori

Some fields are 64 bit, this is still tricky to do atomically. What's the objection to using a config VQ?

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · Anthony Liguori

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Anthony Liguori

Quoted 3 lines

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

This is no different than a normal network card. You have to use a secondary register to trigger an update.

Regards,

Anthony Liguori

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Possible but doesn't let us layer nicely to allow unchanged drivers that work with all transports (new pci, old pci, non pci).

If we declare config space LE, then we have to touch all drivers. There's no way around it because the virtio API is byte-based, not word based.

Fine but don't we want to be compatible with old hypervisors?

We can modify the drivers to work either with a virtio1 or virtio2 transport. If the only difference is that we move to word access instead of byte access for the config space, it's a nop because drivers don't rely on sub-word access today.

Quoted 11 lines

Quoted 8 lines

This is why I'm suggesting making the virtio API (and then the virtio-pci ABI) word based. It gives us the flexibility to make endianness a property of the transport and not a property of the devices.

Regards,

Anthony Liguori

Some fields are 64 bit, this is still tricky to do atomically. What's the objection to using a config VQ?

Then we move very far away from something that looks like a PCI device. The problem we're having here is specifically where we've deviated from what a normal PCI device would do. Fixing that by deviating even further seems counter intuitive to me.

Regards,

Anthony Liguori

Quoted 1 line

Not sure what you mean. Using VQ is DMA which is pretty common for PCI.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines · Michael S. Tsirkin

Not sure what you mean. Using VQ is DMA which is pretty common for PCI.

Do you know of a network device that obtains it's mac address via a DMA transaction?

Regards,

Anthony Liguori

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines · Anthony Liguori

Quoted 2 lines · Michael S. Tsirkin

Not sure what you mean. Using VQ is DMA which is pretty common for PCI.

Do you know of a network device that obtains it's mac address via a DMA transaction?

Sure.
See mlx4_replace_mac in drivers/net/ethernet/mellanox/mlx4/port.c

Quoted 7 lines

Regards,

Anthony Liguori

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I'd say that's a special case but I see what you're getting at here.

So what about keeping the config space read-only and using control queues for
everything else?

Quoted 6 lines · Anthony Liguori

I'd say that's a special case but I see what you're getting at here.

So what about keeping the config space read-only and using control queues for
everything else?

Which is exactly what Rusty and I are proposing :-)
I would go further
and eliminate the idea of a seqlock and instead of a status queue with precise messages indicating what changed.

I would couple that with the new queue format allowing immediate data in the descriptor to avoid having to use indirect buffers for these, which means no allocation, no buffer pool etc... which makes everything a lot easier to deal with as well.

We could probably have a helper library for sending control messages which could handle waiting for a ring slot to be free (practically always the case on control queues), writing the message, sending it and waiting for a status queue confirmation message.

Cheers,
Ben.

Quoted 3 lines

We could probably have a helper library for sending control messages which could handle waiting for a ring slot to be free (practically always the case on control queues), writing the message, sending it

and

Quoted 5 lines

waiting for a status queue confirmation message.

Cheers,
Ben.

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

Quoted 2 lines · Michael S. Tsirkin

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

In theory, yes. In practice, we bandaid it.

I think in the short term we change ->get to get the entire sequence twice, and check it's the same. Theoretically, still racy, but it does cut the window. And we haven't seen the bug yet, either.

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

In theory, yes. In practice, we bandaid it.

I think in the short term we change ->get to get the entire sequence twice, and check it's the same. Theoretically, still racy, but it does cut the window. And we haven't seen the bug yet, either.

I thought about this some more. Since we always get an interrupt on config changes, it seems that a rather robust method would be to just synchronize against that. Something like the below (warning - completely untested). Still need to think about memory barriers, overflow etc. What do you think?

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

drivers/virtio/virtio_pci.c +31 -16

index 03d1984..b5df385 100644
@@ -57,6 +57,7 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+	atomic_t config_changes;
 };
 
 /* Constants for MSI-X */

@@ -125,6 +126,19 @@ static void vp_finalize_features(struct virtio_device *vdev)
 	iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
 }
 
+/* wait for pending irq handlers */
+static void vp_synchronize_vectors(struct virtio_device *vdev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	int i;
+
+	if (vp_dev->intx_enabled)
+		synchronize_irq(vp_dev->pci_dev->irq);
+
+	for (i = 0; i < vp_dev->msix_vectors; ++i)
+		synchronize_irq(vp_dev->msix_entries[i].vector);
+}
+
 /* virtio config->get() implementation */
 static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)

@@ -134,9 +148,20 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 				VIRTIO_PCI_CONFIG(vp_dev) + offset;
 	u8 *ptr = buf;
 	int i;
-
-	for (i = 0; i < len; i++)
-		ptr[i] = ioread8(ioaddr + i);
+	int uninitialized_var(c);
+	c = atomic_read(&vp_dev->config_changes);
+	/* Make sure read is done before we get the first config byte */
+	rmb();
+	do {
+		for (i = 0; i < len; i++)
+			ptr[i] = ioread8(ioaddr + i);
+		/* Synchronize with config interrupt */
+		vp_synchronize_vectors(vdev);
+		/*
+		 * For multi-byte fields, we might get a config change interrupt
+		 * between byte reads. If this happens, retry the read.
+		 */
+	} while (c != atomic_read(&vp_dev->config_changes))
 }
 
 /* the config->set() implementation.  it's symmetric to the config->get()

@@ -169,19 +194,6 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
 	iowrite8(status, vp_dev->ioaddr + VIRTIO_PCI_STATUS);
 }
 
-/* wait for pending irq handlers */
-static void vp_synchronize_vectors(struct virtio_device *vdev)
-{
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	int i;
-
-	if (vp_dev->intx_enabled)
-		synchronize_irq(vp_dev->pci_dev->irq);
-
-	for (i = 0; i < vp_dev->msix_vectors; ++i)
-		synchronize_irq(vp_dev->msix_entries[i].vector);
-}
-
 static void vp_reset(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);

@@ -213,6 +225,8 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 	drv = container_of(vp_dev->vdev.dev.driver,
 			   struct virtio_driver, driver);
 
+	atomic_inc(&vp_dev->config_changes);
+
 	if (drv && drv->config_changed)
 		drv->config_changed(&vp_dev->vdev);
 	return IRQ_HANDLED;

@@ -646,6 +660,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	vp_dev->vdev.config = &virtio_pci_config_ops;
 	vp_dev->pci_dev = pci_dev;
 	INIT_LIST_HEAD(&vp_dev->virtqueues);
+	atomic_set(&vp_dev->config_changes, 0);
 	spin_lock_init(&vp_dev->lock);
 
 	/* Disable MSI/MSIX to bring device to a known good state. */

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

In theory, yes. In practice, we bandaid it.

I think in the short term we change ->get to get the entire sequence twice, and check it's the same. Theoretically, still racy, but it does cut the window. And we haven't seen the bug yet, either.

Quoted 6 lines

I thought about this some more. Since we always get an interrupt on config changes, it seems that a rather robust method would be to just synchronize against that. Something like the below (warning - completely untested). Still need to think about memory barriers, overflow etc. What do you think?

Your interrupt may take an unpredictable amount of time to arrive, I don't see how you can use that as a guarantee.

Cheers,
Ben.

Quoted 95 lines

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

drivers/virtio/virtio_pci.c +31 -16

index 03d1984..b5df385 100644
@@ -57,6 +57,7 @@ struct virtio_pci_device
 	unsigned msix_used_vectors;
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
+	atomic_t config_changes;
 };
 
 /* Constants for MSI-X */

@@ -125,6 +126,19 @@ static void vp_finalize_features(struct virtio_device *vdev)
 	iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
 }
 
+/* wait for pending irq handlers */
+static void vp_synchronize_vectors(struct virtio_device *vdev)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	int i;
+
+	if (vp_dev->intx_enabled)
+		synchronize_irq(vp_dev->pci_dev->irq);
+
+	for (i = 0; i < vp_dev->msix_vectors; ++i)
+		synchronize_irq(vp_dev->msix_entries[i].vector);
+}
+
 /* virtio config->get() implementation */
 static void vp_get(struct virtio_device *vdev, unsigned offset,
 		   void *buf, unsigned len)

@@ -134,9 +148,20 @@ static void vp_get(struct virtio_device *vdev, unsigned offset,
 				VIRTIO_PCI_CONFIG(vp_dev) + offset;
 	u8 *ptr = buf;
 	int i;
-
-	for (i = 0; i < len; i++)
-		ptr[i] = ioread8(ioaddr + i);
+	int uninitialized_var(c);
+	c = atomic_read(&vp_dev->config_changes);
+	/* Make sure read is done before we get the first config byte */
+	rmb();
+	do {
+		for (i = 0; i < len; i++)
+			ptr[i] = ioread8(ioaddr + i);
+		/* Synchronize with config interrupt */
+		vp_synchronize_vectors(vdev);
+		/*
+		 * For multi-byte fields, we might get a config change interrupt
+		 * between byte reads. If this happens, retry the read.
+		 */
+	} while (c != atomic_read(&vp_dev->config_changes))
 }
 
 /* the config->set() implementation.  it's symmetric to the config->get()

@@ -169,19 +194,6 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
 	iowrite8(status, vp_dev->ioaddr + VIRTIO_PCI_STATUS);
 }
 
-/* wait for pending irq handlers */
-static void vp_synchronize_vectors(struct virtio_device *vdev)
-{
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	int i;
-
-	if (vp_dev->intx_enabled)
-		synchronize_irq(vp_dev->pci_dev->irq);
-
-	for (i = 0; i < vp_dev->msix_vectors; ++i)
-		synchronize_irq(vp_dev->msix_entries[i].vector);
-}
-
 static void vp_reset(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);

@@ -213,6 +225,8 @@ static irqreturn_t vp_config_changed(int irq, void *opaque)
 	drv = container_of(vp_dev->vdev.dev.driver,
 			   struct virtio_driver, driver);
 
+	atomic_inc(&vp_dev->config_changes);
+
 	if (drv && drv->config_changed)
 		drv->config_changed(&vp_dev->vdev);
 	return IRQ_HANDLED;

@@ -646,6 +660,7 @@ static int __devinit virtio_pci_probe(struct pci_dev *pci_dev,
 	vp_dev->vdev.config = &virtio_pci_config_ops;
 	vp_dev->pci_dev = pci_dev;
 	INIT_LIST_HEAD(&vp_dev->virtqueues);
+	atomic_set(&vp_dev->config_changes, 0);
 	spin_lock_init(&vp_dev->lock);
 
 	/* Disable MSI/MSIX to bring device to a known good state. */

Quoted 2 lines · Benjamin Herrenschmidt

Your interrupt may take an unpredictable amount of time to arrive, I don't see how you can use that as a guarantee.

In theory yes.
Practically, under current KVM, what would delay it?

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin

Look, we have a race currently. Let us not tie a bug fix to a huge rewrite with unclear performance benefits, please.

In theory, yes. In practice, we bandaid it.

I think in the short term we change ->get to get the entire sequence twice, and check it's the same. Theoretically, still racy, but it does cut the window. And we haven't seen the bug yet, either.

I thought about this some more. Since we always get an interrupt on config changes, it seems that a rather robust method would be to just synchronize against that. Something like the below (warning - completely untested). Still need to think about memory barriers, overflow etc. What do you think?

Quoted 7 lines · Michael S. Tsirkin

Quoted 5 lines · Anthony Liguori

Quoted 2 lines · Michael S. Tsirkin

Not sure what you mean. Using VQ is DMA which is pretty common for PCI.

Do you know of a network device that obtains it's mac address via a DMA transaction?

Quoted 2 lines

Sure.
See mlx4_replace_mac in drivers/net/ethernet/mellanox/mlx4/port.c

I'd say that's a special case but I see what you're getting at here.

So what about keeping the config space read-only and using control queues for everything else?

Regards,

Anthony Liguori

Quoted 7 lines

Quoted 7 lines

Regards,

Anthony Liguori

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines · Michael S. Tsirkin

Not sure what you mean. Using VQ is DMA which is pretty common for PCI.

Do you know of a network device that obtains it's mac address via a DMA transaction?

Possible but doesn't let us layer nicely to allow unchanged drivers that work with all transports (new pci, old pci, non pci). Something like a command VQ would be a generic transport that can be hidden behind config->set(...).

This is similar to what we have now. But it's still buggy: e.g. if guest updates MAC byte by byte, we have no way to know when it's done doing so.

I think the more important thing to do is require accesses to integers in the config space to always be aligned and to use the appropriate accessor. Non-integer fields should be restricted to byte access.

That limits config space entries to 32-bit but also means that there is no need for a generation counter.

If we're already making the change, the endianness ought to be a feature bit.