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