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

["Michael S. Tsirkin" <mst@redhat.com>]

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