[PATCH 0/4] virtio: fix four bugs across mmio, pci, and vring

[PATCH 0/4] virtio: fix four bugs across mmio, pci, and vring

[Andrew Stellman]

Four independent fixes across the virtio subsystem:

1. virtio-mmio: vm_reset() returns without polling for the status
   register to read back as 0, violating the spec. Add a poll loop
   matching the existing pattern in virtio-pci's vp_reset().

2. virtio-pci: vp_find_vqs_intx() uses a sequential counter instead
   of avq->vq_index for the admin VQ, unlike the MSI-X path.

3. virtio-pci: vp_interrupt() returns IRQ_NONE for config-change-only
   interrupts because it unconditionally returns vp_vring_interrupt()'s
   result. Track the config-change handling and return IRQ_HANDLED.

4. virtio_ring: vring_transport_features() clears VIRTIO_F_RING_RESET
   during feature negotiation because it is missing from the whitelist.

Andrew Stellman (4):
  virtio-mmio: wait for status readback after reset
  virtio-pci: use avq->vq_index for admin VQ in INTx path
  virtio-pci: return IRQ_HANDLED for config-change interrupts
  virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

 drivers/virtio/virtio_mmio.c       |  3 +++
 drivers/virtio/virtio_pci_common.c | 12 +++++++++---
 drivers/virtio/virtio_ring.c       |  2 ++
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/4] virtio-mmio: wait for status readback after reset

[Andrew Stellman]

The virtio specification requires that after writing 0 to the status
register, the driver must wait until the device has actually completed
the reset (status reads back as 0) before proceeding. vm_reset() writes
0 but returns immediately without confirming the device has reset.

Add a poll loop matching the pattern already used in virtio-pci's
vp_reset(), which calls msleep(1) in a loop until the status register
reads 0.

Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
---
 drivers/virtio/virtio_mmio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 595c227..a477977 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -56,6 +56,7 @@
 
 #include <linux/acpi.h>
 #include <linux/dma-mapping.h>
+#include <linux/delay.h>
 #include <linux/highmem.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -254,6 +255,8 @@ static void vm_reset(struct virtio_device *vdev)
 
 	/* 0 status means a reset. */
 	writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+	while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
+		msleep(1);
 }
 
 
-- 
2.34.1

Re: [PATCH 1/4] virtio-mmio: wait for status readback after reset

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 08:39:01AM -0400, Andrew Stellman wrote:
> The virtio specification requires that after writing 0 to the status
> register, the driver must wait until the device has actually completed
> the reset (status reads back as 0) before proceeding.

It does? But where?
I see:
    Reading from this register returns the current device status
    flags.
    Writing non-zero values to this register sets the status flags,
    indicating the OS/driver progress. Writing zero (0x0) to this
    register triggers a device reset. The device
    sets \field{QueuePFN} to zero (0x0) for all queues in the device.



> vm_reset() writes
> 0 but returns immediately without confirming the device has reset.
> 
> Add a poll loop matching the pattern already used in virtio-pci's
> vp_reset(), which calls msleep(1) in a loop until the status register
> reads 0.
> 
> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> ---
>  drivers/virtio/virtio_mmio.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 595c227..a477977 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -56,6 +56,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/delay.h>
>  #include <linux/highmem.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -254,6 +255,8 @@ static void vm_reset(struct virtio_device *vdev)
>  
>  	/* 0 status means a reset. */
>  	writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
> +	while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
> +		msleep(1);
>  }
>  
>  
> -- 
> 2.34.1

[PATCH 2/4] virtio-pci: use avq->vq_index for admin VQ in INTx path

[Andrew Stellman]

vp_find_vqs_intx() sets up the admin virtqueue using queue_idx++
(a sequential counter) instead of avq->vq_index (the actual transport
queue index). The MSI-X path in vp_find_vqs_msix() correctly uses
avq->vq_index. When the admin VQ index does not equal the next
sequential queue_idx value, the INTx path binds the admin VQ to the
wrong transport queue.

Use avq->vq_index to match the MSI-X path.

Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
---
 drivers/virtio/virtio_pci_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index da97b6a..0b9d66b 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -497,7 +497,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 	if (!avq_num)
 		return 0;
 	sprintf(avq->name, "avq.%u", avq->vq_index);
-	vq = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done, avq->name,
+	vq = vp_setup_vq(vdev, avq->vq_index, vp_modern_avq_done, avq->name,
 			 false, VIRTIO_MSI_NO_VECTOR,
 			 &vp_dev->admin_vq.info);
 	if (IS_ERR(vq)) {
-- 
2.34.1

Re: [PATCH 2/4] virtio-pci: use avq->vq_index for admin VQ in INTx path

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 08:39:02AM -0400, Andrew Stellman wrote:
> vp_find_vqs_intx() sets up the admin virtqueue using queue_idx++
> (a sequential counter) instead of avq->vq_index (the actual transport
> queue index). The MSI-X path in vp_find_vqs_msix() correctly uses
> avq->vq_index. When the admin VQ index does not equal the next
> sequential queue_idx value, the INTx path binds the admin VQ to the
> wrong transport queue.
> 
> Use avq->vq_index to match the MSI-X path.
> 
> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>


Cc Jiri. fixes tag?

> ---
>  drivers/virtio/virtio_pci_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index da97b6a..0b9d66b 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -497,7 +497,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
>  	if (!avq_num)
>  		return 0;
>  	sprintf(avq->name, "avq.%u", avq->vq_index);
> -	vq = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done, avq->name,
> +	vq = vp_setup_vq(vdev, avq->vq_index, vp_modern_avq_done, avq->name,
>  			 false, VIRTIO_MSI_NO_VECTOR,
>  			 &vp_dev->admin_vq.info);
>  	if (IS_ERR(vq)) {
> -- 
> 2.34.1

Re: [PATCH 2/4] virtio-pci: use avq->vq_index for admin VQ in INTx path

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 01:34:23PM -0400, Andrew Stellman wrote:
> Fixes: af22bbe1f4a5 ("virtio: create admin queues alongside other virtqueues")


and how was this tested?

> 
> On Tue, Apr 7, 2026 at 12:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Apr 07, 2026 at 08:39:02AM -0400, Andrew Stellman wrote:
>     > vp_find_vqs_intx() sets up the admin virtqueue using queue_idx++
>     > (a sequential counter) instead of avq->vq_index (the actual transport
>     > queue index). The MSI-X path in vp_find_vqs_msix() correctly uses
>     > avq->vq_index. When the admin VQ index does not equal the next
>     > sequential queue_idx value, the INTx path binds the admin VQ to the
>     > wrong transport queue.
>     >
>     > Use avq->vq_index to match the MSI-X path.
>     >
>     > Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> 
> 
>     Cc Jiri. fixes tag?
> 
>     > ---
>     >  drivers/virtio/virtio_pci_common.c | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/
>     virtio_pci_common.c
>     > index da97b6a..0b9d66b 100644
>     > --- a/drivers/virtio/virtio_pci_common.c
>     > +++ b/drivers/virtio/virtio_pci_common.c
>     > @@ -497,7 +497,7 @@ static int vp_find_vqs_intx(struct virtio_device
>     *vdev, unsigned int nvqs,
>     >       if (!avq_num)
>     >               return 0;
>     >       sprintf(avq->name, "avq.%u", avq->vq_index);
>     > -     vq = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done, avq->name,
>     > +     vq = vp_setup_vq(vdev, avq->vq_index, vp_modern_avq_done, avq->
>     name,
>     >                        false, VIRTIO_MSI_NO_VECTOR,
>     >                        &vp_dev->admin_vq.info);
>     >       if (IS_ERR(vq)) {
>     > --
>     > 2.34.1
> 
> 

[PATCH 3/4] virtio-pci: return IRQ_HANDLED for config-change interrupts

[Andrew Stellman]

vp_interrupt() unconditionally returns the result of
vp_vring_interrupt(). When a config-change interrupt fires but no vring
activity is pending, vp_vring_interrupt() returns IRQ_NONE — even
though the interrupt was legitimately handled by vp_config_changed().
Over time this causes the IRQ subsystem to flag the line as spurious.

Track the return value explicitly: set ret to IRQ_HANDLED when the
config-change bit is set, OR it with the vring result, and return the
combined value.

Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
---
 drivers/virtio/virtio_pci_common.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 0b9d66b..1d1ab02 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -106,6 +106,7 @@ static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
 static irqreturn_t vp_interrupt(int irq, void *opaque)
 {
 	struct virtio_pci_device *vp_dev = opaque;
+	irqreturn_t ret = IRQ_NONE;
 	u8 isr;
 
 	/* reading the ISR has the effect of also clearing it so it's very
@@ -117,10 +118,15 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
 		return IRQ_NONE;
 
 	/* Configuration change?  Tell driver if it wants to know. */
-	if (isr & VIRTIO_PCI_ISR_CONFIG)
+	if (isr & VIRTIO_PCI_ISR_CONFIG) {
 		vp_config_changed(irq, opaque);
+		ret = IRQ_HANDLED;
+	}
 
-	return vp_vring_interrupt(irq, opaque);
+	if (vp_vring_interrupt(irq, opaque) == IRQ_HANDLED)
+		ret = IRQ_HANDLED;
+
+	return ret;
 }
 
 static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
-- 
2.34.1

Re: [PATCH 3/4] virtio-pci: return IRQ_HANDLED for config-change interrupts

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 08:39:03AM -0400, Andrew Stellman wrote:
> vp_interrupt() unconditionally returns the result of
> vp_vring_interrupt(). When a config-change interrupt fires but no vring
> activity is pending, vp_vring_interrupt() returns IRQ_NONE — even
> though the interrupt was legitimately handled by vp_config_changed().
> Over time this causes the IRQ subsystem to flag the line as spurious.
> 
> Track the return value explicitly: set ret to IRQ_HANDLED when the
> config-change bit is set, OR it with the vring result, and return the
> combined value.
> 
> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>

Fixes tag?

> ---
>  drivers/virtio/virtio_pci_common.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 0b9d66b..1d1ab02 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -106,6 +106,7 @@ static irqreturn_t vp_vring_interrupt(int irq, void *opaque)
>  static irqreturn_t vp_interrupt(int irq, void *opaque)
>  {
>  	struct virtio_pci_device *vp_dev = opaque;
> +	irqreturn_t ret = IRQ_NONE;
>  	u8 isr;
>  
>  	/* reading the ISR has the effect of also clearing it so it's very
> @@ -117,10 +118,15 @@ static irqreturn_t vp_interrupt(int irq, void *opaque)
>  		return IRQ_NONE;
>  
>  	/* Configuration change?  Tell driver if it wants to know. */
> -	if (isr & VIRTIO_PCI_ISR_CONFIG)
> +	if (isr & VIRTIO_PCI_ISR_CONFIG) {
>  		vp_config_changed(irq, opaque);
> +		ret = IRQ_HANDLED;
> +	}
>  
> -	return vp_vring_interrupt(irq, opaque);
> +	if (vp_vring_interrupt(irq, opaque) == IRQ_HANDLED)
> +		ret = IRQ_HANDLED;
> +
> +	return ret;
>  }
>  
>  static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> -- 
> 2.34.1

Re: [PATCH 3/4] virtio-pci: return IRQ_HANDLED for config-change interrupts

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 01:35:26PM -0400, Andrew Stellman wrote:
> Fixes: 77cf524654a8 ("virtio_pci: split up vp_interrupt")
> 
> Should I resend as v2 with the tag included, or is adding it on your end
> easier?


how was this tested? I don't think a unit test is sufficient here, FYI.


> On Tue, Apr 7, 2026 at 12:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Apr 07, 2026 at 08:39:03AM -0400, Andrew Stellman wrote:
>     > vp_interrupt() unconditionally returns the result of
>     > vp_vring_interrupt(). When a config-change interrupt fires but no vring
>     > activity is pending, vp_vring_interrupt() returns IRQ_NONE — even
>     > though the interrupt was legitimately handled by vp_config_changed().
>     > Over time this causes the IRQ subsystem to flag the line as spurious.
>     >
>     > Track the return value explicitly: set ret to IRQ_HANDLED when the
>     > config-change bit is set, OR it with the vring result, and return the
>     > combined value.
>     >
>     > Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> 
>     Fixes tag?
> 
>     > ---
>     >  drivers/virtio/virtio_pci_common.c | 10 ++++++++--
>     >  1 file changed, 8 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/
>     virtio_pci_common.c
>     > index 0b9d66b..1d1ab02 100644
>     > --- a/drivers/virtio/virtio_pci_common.c
>     > +++ b/drivers/virtio/virtio_pci_common.c
>     > @@ -106,6 +106,7 @@ static irqreturn_t vp_vring_interrupt(int irq, void
>     *opaque)
>     >  static irqreturn_t vp_interrupt(int irq, void *opaque)
>     >  {
>     >       struct virtio_pci_device *vp_dev = opaque;
>     > +     irqreturn_t ret = IRQ_NONE;
>     >       u8 isr;
>     > 
>     >       /* reading the ISR has the effect of also clearing it so it's very
>     > @@ -117,10 +118,15 @@ static irqreturn_t vp_interrupt(int irq, void
>     *opaque)
>     >               return IRQ_NONE;
>     > 
>     >       /* Configuration change?  Tell driver if it wants to know. */
>     > -     if (isr & VIRTIO_PCI_ISR_CONFIG)
>     > +     if (isr & VIRTIO_PCI_ISR_CONFIG) {
>     >               vp_config_changed(irq, opaque);
>     > +             ret = IRQ_HANDLED;
>     > +     }
>     > 
>     > -     return vp_vring_interrupt(irq, opaque);
>     > +     if (vp_vring_interrupt(irq, opaque) == IRQ_HANDLED)
>     > +             ret = IRQ_HANDLED;
>     > +
>     > +     return ret;
>     >  }
>     > 
>     >  static int vp_request_msix_vectors(struct virtio_device *vdev, int
>     nvectors,
>     > --
>     > 2.34.1
> 
> 

[PATCH 4/4] virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

[Andrew Stellman]

vring_transport_features() whitelists known transport feature bits and
clears the rest via __virtio_clear_bit(). VIRTIO_F_RING_RESET is
missing from the whitelist, so it is unconditionally cleared during
feature negotiation. Drivers that depend on ring reset capability
silently lose the feature.

Add VIRTIO_F_RING_RESET to the switch statement, matching the other
transport-level features.

Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
---
 drivers/virtio/virtio_ring.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fbca7ce..2cb643f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3524,6 +3524,8 @@ void vring_transport_features(struct virtio_device *vdev)
 			break;
 		case VIRTIO_F_IN_ORDER:
 			break;
+		case VIRTIO_F_RING_RESET:
+			break;
 		default:
 			/* We don't understand this bit. */
 			__virtio_clear_bit(vdev, i);
-- 
2.34.1

Re: [PATCH 4/4] virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 08:39:04AM -0400, Andrew Stellman wrote:
> vring_transport_features() whitelists known transport feature bits and
> clears the rest via __virtio_clear_bit(). VIRTIO_F_RING_RESET is
> missing from the whitelist, so it is unconditionally cleared during
> feature negotiation. Drivers that depend on ring reset capability
> silently lose the feature.

Hmm was this observed in practice or just from code analysis?
And on which transport?
Because


static void vp_transport_features(struct virtio_device *vdev, u64 features)
{               
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
        struct pci_dev *pci_dev = vp_dev->pci_dev;
                        
        if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
                        pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
                __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
        
        if (features & BIT_ULL(VIRTIO_F_RING_RESET))
                __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
        

...



}




> Add VIRTIO_F_RING_RESET to the switch statement, matching the other
> transport-level features.
> 
> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> ---
>  drivers/virtio/virtio_ring.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fbca7ce..2cb643f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -3524,6 +3524,8 @@ void vring_transport_features(struct virtio_device *vdev)
>  			break;
>  		case VIRTIO_F_IN_ORDER:
>  			break;
> +		case VIRTIO_F_RING_RESET:
> +			break;
>  		default:
>  			/* We don't understand this bit. */
>  			__virtio_clear_bit(vdev, i);
> -- 
> 2.34.1

Re: [PATCH 4/4] virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 01:33:39PM -0400, Andrew Stellman wrote:
> Code analysis only — not observed in practice. And you're right that on PCI it
> doesn't matter: vp_transport_features() re-sets the bit after
> vring_transport_features() clears it, so PCI never actually loses the feature.

Oh. please do test patches, or note they were not tested.
how were rest of patches here tested?

> 
> The gap is for transports that call vring_transport_features() without
> independently re-setting VIRTIO_F_RING_RESET afterward. Whether any current
> transport hits this in practice I'm not sure — it may just be a whitelist
> consistency fix at this point.
> 
> Happy to add a Fixes tag and resend, or drop it if you think the PCI-level
> re-set makes it unnecessary.
> 
> Fixes: 04ca0b0b16f1 ("virtio_pci: support VIRTIO_F_RING_RESET")


surely not this because pci is fine.

> On Tue, Apr 7, 2026 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Apr 07, 2026 at 08:39:04AM -0400, Andrew Stellman wrote:
>     > vring_transport_features() whitelists known transport feature bits and
>     > clears the rest via __virtio_clear_bit(). VIRTIO_F_RING_RESET is
>     > missing from the whitelist, so it is unconditionally cleared during
>     > feature negotiation. Drivers that depend on ring reset capability
>     > silently lose the feature.
> 
>     Hmm was this observed in practice or just from code analysis?
>     And on which transport?
>     Because
> 
> 
>     static void vp_transport_features(struct virtio_device *vdev, u64 features)
>     {               
>             struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>             struct pci_dev *pci_dev = vp_dev->pci_dev;
> 
>             if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
>                             pci_find_ext_capability(pci_dev,
>     PCI_EXT_CAP_ID_SRIOV))
>                     __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> 
>             if (features & BIT_ULL(VIRTIO_F_RING_RESET))
>                     __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> 
> 
>     ...
> 
> 
> 
>     }
> 
> 
> 
> 
>     > Add VIRTIO_F_RING_RESET to the switch statement, matching the other
>     > transport-level features.
>     >
>     > Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
>     > ---
>     >  drivers/virtio/virtio_ring.c | 2 ++
>     >  1 file changed, 2 insertions(+)
>     >
>     > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>     > index fbca7ce..2cb643f 100644
>     > --- a/drivers/virtio/virtio_ring.c
>     > +++ b/drivers/virtio/virtio_ring.c
>     > @@ -3524,6 +3524,8 @@ void vring_transport_features(struct virtio_device
>     *vdev)
>     >                       break;
>     >               case VIRTIO_F_IN_ORDER:
>     >                       break;
>     > +             case VIRTIO_F_RING_RESET:
>     > +                     break;
>     >               default:
>     >                       /* We don't understand this bit. */
>     >                       __virtio_clear_bit(vdev, i);
>     > --
>     > 2.34.1
> 
> 

Re: [PATCH 4/4] virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

[Andrew Stellman]

[-- Attachment #1.1: Type: text/plain, Size: 4285 bytes --]

No, sorry, I tested it, only with a unit test after finding it via static
analysis, not the "real" way using the whole kernel build. (Also replied
with the test for 0002.)

Found with static analysis, reproduced with a unit test (save attached file
to drivers/virtio/test0004.c):

% cd drivers/virtio
% git checkout virtio_ring.c
Updated 0 paths from the index
% sed -n '/^void vring_transport_features/,/^}/p' virtio_ring.c >
extracted.c && gcc -Wall -Werror -o test0004 test0004.c && ./test0004
FAIL: VIRTIO_F_RING_RESET cleared during negotiation
% git apply 0004-virtio_ring-preserve-VIRTIO_F_RING_RESET-in-transpor.patch
% sed -n '/^void vring_transport_features/,/^}/p' virtio_ring.c >
extracted.c && gcc -Wall -Werror -o test0004 test0004.c && ./test0004
PASS: VIRTIO_F_RING_RESET preserved

If this doesn't work, feel free to ditch.

On Tue, Apr 7, 2026 at 2:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:

> On Tue, Apr 07, 2026 at 01:33:39PM -0400, Andrew Stellman wrote:
> > Code analysis only — not observed in practice. And you're right that on
> PCI it
> > doesn't matter: vp_transport_features() re-sets the bit after
> > vring_transport_features() clears it, so PCI never actually loses the
> feature.
>
> Oh. please do test patches, or note they were not tested.
> how were rest of patches here tested?
>
> >
> > The gap is for transports that call vring_transport_features() without
> > independently re-setting VIRTIO_F_RING_RESET afterward. Whether any
> current
> > transport hits this in practice I'm not sure — it may just be a whitelist
> > consistency fix at this point.
> >
> > Happy to add a Fixes tag and resend, or drop it if you think the
> PCI-level
> > re-set makes it unnecessary.
> >
> > Fixes: 04ca0b0b16f1 ("virtio_pci: support VIRTIO_F_RING_RESET")
>
>
> surely not this because pci is fine.
>
> > On Tue, Apr 7, 2026 at 12:22 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >
> >     On Tue, Apr 07, 2026 at 08:39:04AM -0400, Andrew Stellman wrote:
> >     > vring_transport_features() whitelists known transport feature bits
> and
> >     > clears the rest via __virtio_clear_bit(). VIRTIO_F_RING_RESET is
> >     > missing from the whitelist, so it is unconditionally cleared during
> >     > feature negotiation. Drivers that depend on ring reset capability
> >     > silently lose the feature.
> >
> >     Hmm was this observed in practice or just from code analysis?
> >     And on which transport?
> >     Because
> >
> >
> >     static void vp_transport_features(struct virtio_device *vdev, u64
> features)
> >     {
> >             struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> >             struct pci_dev *pci_dev = vp_dev->pci_dev;
> >
> >             if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> >                             pci_find_ext_capability(pci_dev,
> >     PCI_EXT_CAP_ID_SRIOV))
> >                     __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> >
> >             if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> >                     __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> >
> >
> >     ...
> >
> >
> >
> >     }
> >
> >
> >
> >
> >     > Add VIRTIO_F_RING_RESET to the switch statement, matching the other
> >     > transport-level features.
> >     >
> >     > Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> >     > ---
> >     >  drivers/virtio/virtio_ring.c | 2 ++
> >     >  1 file changed, 2 insertions(+)
> >     >
> >     > diff --git a/drivers/virtio/virtio_ring.c
> b/drivers/virtio/virtio_ring.c
> >     > index fbca7ce..2cb643f 100644
> >     > --- a/drivers/virtio/virtio_ring.c
> >     > +++ b/drivers/virtio/virtio_ring.c
> >     > @@ -3524,6 +3524,8 @@ void vring_transport_features(struct
> virtio_device
> >     *vdev)
> >     >                       break;
> >     >               case VIRTIO_F_IN_ORDER:
> >     >                       break;
> >     > +             case VIRTIO_F_RING_RESET:
> >     > +                     break;
> >     >               default:
> >     >                       /* We don't understand this bit. */
> >     >                       __virtio_clear_bit(vdev, i);
> >     > --
> >     > 2.34.1
> >
> >
>
>

[-- Attachment #1.2: Type: text/html, Size: 5661 bytes --]

[-- Attachment #2: test0004.c --]
[-- Type: application/octet-stream, Size: 1406 bytes --]

// SPDX-License-Identifier: GPL-2.0-only
/* Test: virtio_ring: preserve VIRTIO_F_RING_RESET in transport features
 *
 * VIRTIO_F_RING_RESET is missing from the vring_transport_features()
 * whitelist, so it gets silently cleared during feature negotiation.
 *
 * sed -n '/^void vring_transport_features/,/^}/p' drivers/virtio/virtio_ring.c > extracted.c && gcc -Wall -Werror -o test0004 test0004.c && ./test0004
 */
#include <stdio.h>
#include <stdint.h>

typedef uint64_t u64;
struct virtio_device { u64 features; };

static void __virtio_clear_bit(struct virtio_device *vdev, unsigned int bit)
{
	vdev->features &= ~(1ULL << bit);
}

#define VIRTIO_TRANSPORT_F_START	28
#define VIRTIO_TRANSPORT_F_END		42
#define VIRTIO_RING_F_INDIRECT_DESC	28
#define VIRTIO_RING_F_EVENT_IDX		29
#define VIRTIO_F_VERSION_1		32
#define VIRTIO_F_ACCESS_PLATFORM	33
#define VIRTIO_F_RING_PACKED		34
#define VIRTIO_F_IN_ORDER		35
#define VIRTIO_F_ORDER_PLATFORM		36
#define VIRTIO_F_NOTIFICATION_DATA	38
#define VIRTIO_F_RING_RESET		40

#include "extracted.c"

int main(void)
{
	struct virtio_device vdev;

	vdev.features = (1ULL << VIRTIO_F_VERSION_1) | (1ULL << VIRTIO_F_RING_RESET);
	vring_transport_features(&vdev);

	if (!(vdev.features & (1ULL << VIRTIO_F_RING_RESET))) {
		printf("FAIL: VIRTIO_F_RING_RESET cleared during negotiation\n");
		return 1;
	}
	printf("PASS: VIRTIO_F_RING_RESET preserved\n");
	return 0;
}

Re: [PATCH 4/4] virtio_ring: preserve VIRTIO_F_RING_RESET in transport features

[Michael S. Tsirkin]

On Tue, Apr 07, 2026 at 04:00:00PM -0400, Andrew Stellman wrote:
> No, sorry, I tested it, only with a unit test after finding it via static
> analysis, not the "real" way using the whole kernel build. (Also replied with
> the test for 0002.)
> 
> Found with static analysis, reproduced with a unit test (save attached file to
> drivers/virtio/test0004.c):
> 
> % cd drivers/virtio          
> % git checkout virtio_ring.c
> Updated 0 paths from the index
> % sed -n '/^void vring_transport_features/,/^}/p' virtio_ring.c > extracted.c &
> & gcc -Wall -Werror -o test0004 test0004.c && ./test0004
> FAIL: VIRTIO_F_RING_RESET cleared during negotiation
> % git apply 0004-virtio_ring-preserve-VIRTIO_F_RING_RESET-in-transpor.patch
> % sed -n '/^void vring_transport_features/,/^}/p' virtio_ring.c > extracted.c &
> & gcc -Wall -Werror -o test0004 test0004.c && ./test0004
> PASS: VIRTIO_F_RING_RESET preserved
> 
> If this doesn't work, feel free to ditch.

I want to know if there's a bug or not, and what does the patch achieve.
The current code does not make it clear at all.


> On Tue, Apr 7, 2026 at 2:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Tue, Apr 07, 2026 at 01:33:39PM -0400, Andrew Stellman wrote:
>     > Code analysis only — not observed in practice. And you're right that on
>     PCI it
>     > doesn't matter: vp_transport_features() re-sets the bit after
>     > vring_transport_features() clears it, so PCI never actually loses the
>     feature.
> 
>     Oh. please do test patches, or note they were not tested.
>     how were rest of patches here tested?
> 
>     >
>     > The gap is for transports that call vring_transport_features() without
>     > independently re-setting VIRTIO_F_RING_RESET afterward. Whether any
>     current
>     > transport hits this in practice I'm not sure — it may just be a whitelist
>     > consistency fix at this point.
>     >
>     > Happy to add a Fixes tag and resend, or drop it if you think the
>     PCI-level
>     > re-set makes it unnecessary.
>     >
>     > Fixes: 04ca0b0b16f1 ("virtio_pci: support VIRTIO_F_RING_RESET")
> 
> 
>     surely not this because pci is fine.
> 
>     > On Tue, Apr 7, 2026 at 12:22 PM Michael S. Tsirkin <mst@redhat.com>
>     wrote:
>     >
>     >     On Tue, Apr 07, 2026 at 08:39:04AM -0400, Andrew Stellman wrote:
>     >     > vring_transport_features() whitelists known transport feature bits
>     and
>     >     > clears the rest via __virtio_clear_bit(). VIRTIO_F_RING_RESET is
>     >     > missing from the whitelist, so it is unconditionally cleared during
>     >     > feature negotiation. Drivers that depend on ring reset capability
>     >     > silently lose the feature.
>     >
>     >     Hmm was this observed in practice or just from code analysis?
>     >     And on which transport?
>     >     Because
>     >
>     >
>     >     static void vp_transport_features(struct virtio_device *vdev, u64
>     features)
>     >     {               
>     >             struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>     >             struct pci_dev *pci_dev = vp_dev->pci_dev;
>     >
>     >             if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
>     >                             pci_find_ext_capability(pci_dev,
>     >     PCI_EXT_CAP_ID_SRIOV))
>     >                     __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
>     >
>     >             if (features & BIT_ULL(VIRTIO_F_RING_RESET))
>     >                     __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
>     >
>     >
>     >     ...
>     >
>     >
>     >
>     >     }
>     >
>     >
>     >
>     >
>     >     > Add VIRTIO_F_RING_RESET to the switch statement, matching the other
>     >     > transport-level features.
>     >     >
>     >     > Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
>     >     > ---
>     >     >  drivers/virtio/virtio_ring.c | 2 ++
>     >     >  1 file changed, 2 insertions(+)
>     >     >
>     >     > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/
>     virtio_ring.c
>     >     > index fbca7ce..2cb643f 100644
>     >     > --- a/drivers/virtio/virtio_ring.c
>     >     > +++ b/drivers/virtio/virtio_ring.c
>     >     > @@ -3524,6 +3524,8 @@ void vring_transport_features(struct
>     virtio_device
>     >     *vdev)
>     >     >                       break;
>     >     >               case VIRTIO_F_IN_ORDER:
>     >     >                       break;
>     >     > +             case VIRTIO_F_RING_RESET:
>     >     > +                     break;
>     >     >               default:
>     >     >                       /* We don't understand this bit. */
>     >     >                       __virtio_clear_bit(vdev, i);
>     >     > --
>     >     > 2.34.1
>     >
>     >
> 
>