Re: [PATCH v2] virtio_pci_modern: skip trailing zero feature dwords

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

On Mon, Sep 28, 2026 at 08:31:15PM +0800, Hao Peng wrote:
> On Fri, Sep 25, 2026 at 12:05 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Sep 24, 2026 at 03:14:12PM +0800, Peng Hao wrote:
> > > Since commit 69b9461512246 ("virtio_pci_modern: allow configuring
> > > extended features"), vp_modern_set_extended_features() writes all four
> > > feature dwords on every device, even when the upper ones are zero.
> > >
> > > Feature negotiation follows a device reset, which clears the device-side
> > > driver features, so trailing zero dwords need not be written at all.
> > > finalize_features() can be called again without an intervening reset,
> > > though, when a driver's validate callback narrows the features, so also
> > > write any dword written since the last reset, to clear what the previous
> > > call had enabled.
> > >
> > > Devices negotiating nothing above bit 63 save four MMIO writes; those
> > > using the 64..95 range (e.g. the UDP tunnel GSO features) save two.
> > > Counting dwords rather than 64-bit words is what makes the latter work:
> > > with VIRTIO_F_VERSION_1 at bit 32 the second dword is set on every modern
> > > device, so a qword count never drops below two.
> > >
> > > Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> >
> > So .. why does all this matter? how many exits do you save
> > during a guest boot? is it worth the complexity?
> >
> it depends on the number of modern virtio-pci devices:
>   - A device using only feature bits 0..63 previously required eight
> MMIO writes and now requires four, saving four
>     MMIO writes, normally four VM-exits.
>   - A device using bits 64..95 saves two MMIO writes/exits.
>   - Thus, for example, a guest with five ordinary modern virtio-pci
> devices saves about 20 exits during their
>     initial feature negotiation.

Out of some 10000-100000 that boot takes? Why does this matter?

> 
> > > ---
> > >  drivers/virtio/virtio_pci_modern_dev.c | 32 ++++++++++++++++++++++----
> > >  include/linux/virtio_pci_modern.h      |  3 +++
> > >  2 files changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> > > index 413a8c353463..7b4da37b6ae5 100644
> > > --- a/drivers/virtio/virtio_pci_modern_dev.c
> > > +++ b/drivers/virtio/virtio_pci_modern_dev.c
> > > @@ -230,6 +230,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
> > >
> > >       check_offsets();
> > >
> > > +     mdev->driver_features_dwords = 1;
> > > +
> > >       if (mdev->device_id_check) {
> > >               devid = mdev->device_id_check(pci_dev);
> > >               if (devid < 0)
> > > @@ -437,6 +439,11 @@ vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,
> > >  }
> > >  EXPORT_SYMBOL_GPL(vp_modern_get_driver_extended_features);
> > >
> > > +static u32 vp_modern_features_dword(const u64 *features, int dword)
> > > +{
> > > +     return features[dword / 2] >> (32 * (dword % 2));
> > > +}
> > > +
> > >  /*
> > >   * vp_modern_set_extended_features - set features to device
> > >   * @mdev: the modern virtio-pci device
> > > @@ -446,14 +453,27 @@ void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev,
> > >                                    const u64 *features)
> > >  {
> > >       struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> > > -     int i;
> > > +     int dwords = VIRTIO_FEATURES_BITS / 32;
> > > +     int i, write_dwords;
> > >
> > > -     for (i = 0; i < VIRTIO_FEATURES_BITS / 32; i++) {
> > > -             u32 cur = features[i >> 1] >> (32 * (i & 1));
> >
> >
> > below is arguing with previous version of code
> > instead of straight explaining what is this code doing.
> > pls rewrite this comment.
> >
> > > +     /*
> > > +      * A device reset
> >
> > what "a device reset"? who did reset and when?
> >
> > > clears the driver features, so trailing all-zero
> >
> > trailing?
> >
> > > +      * dwords need not be written out.  Include any dword written since
> > > +      * that reset,
> >
> > what "that" reset?
> >
> The reset in question is performed by the virtio core.
> register_virtio_device() calls virtio_reset_device() before
>   driver matching and feature negotiation. For modern virtio-pci,
> vp_reset() writes zero to device_status and waits
>   for the device to report zero. That reset clears all driver feature registers.
> > > though, so that a repeated finalization can clear
> > > +      * features which were enabled by the previous one.
> > > +      */
> >
> > > +     while (dwords > 1 && !vp_modern_features_dword(features, dwords - 1))
> > > +             dwords--;
> >
> dwords is the number of registers needed to represent the new feature
> set. The loop examines
>  register dwords - 1 and reduces the count while the highest register
> is zero. Register 0
>  remains part of the range.
> > what's all this > 1, - 1?
> >
> > > +
> > > +     write_dwords = max_t(int, dwords, mdev->driver_features_dwords);
> >
> > and what is this. i have a vague idea but needs a comment.
> >
> > >
> > > +     for (i = 0; i < write_dwords; i++) {
> > >               vp_iowrite32(i, &cfg->guest_feature_select);
> > > -             vp_iowrite32(cur, &cfg->guest_feature);
> > > +             vp_iowrite32(vp_modern_features_dword(features, i),
> > > +                          &cfg->guest_feature);
> > >       }
> > > +
> > > +     mdev->driver_features_dwords = dwords;
> >
> >
> > contradicts the comment near driver_features_dwords.
> >
> > >  }
> > >  EXPORT_SYMBOL_GPL(vp_modern_set_extended_features);
> > >
> > > @@ -495,6 +515,10 @@ void vp_modern_set_status(struct virtio_pci_modern_device *mdev,
> > >  {
> > >       struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> > >
> > > +     /* A reset clears the device's copy of the driver features. */
> > > +     if (!status)
> > > +             mdev->driver_features_dwords = 1;
> >
> > so why 1 not 0?
> >
> Initializing driver_features_dwords to 1 after reset was unnecessarily
> confusing. The reset leaves no
> previously programmed range to clear, so the new version sets it to 0.
> > > +
> > >       /*
> > >        * Per memory-barriers.txt, wmb() is not needed to guarantee
> > >        * that the cache coherent memory writes have completed
> > > diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> > > index 9a3f2fc53bd6..b9f4783f0e02 100644
> > > --- a/include/linux/virtio_pci_modern.h
> > > +++ b/include/linux/virtio_pci_modern.h
> > > @@ -27,6 +27,8 @@
> > >   *               Returns the found device id or ERRNO
> > >   * @dma_mask:            Optional mask instead of the traditional DMA_BIT_MASK(64),
> > >   *               for vendor devices with DMA space address limitations
> > > + * @driver_features_dwords: Number of 32-bit driver feature words written
> > > + *               to the device since the last reset
> > >   */
> > >  struct virtio_pci_modern_device {
> > >       struct pci_dev *pci_dev;
> > > @@ -49,6 +51,7 @@ struct virtio_pci_modern_device {
> > >
> > >       int (*device_id_check)(struct pci_dev *pdev);
> > >       u64 dma_mask;
> > > +     u8 driver_features_dwords;
> > >  };
> > >
> > >  /*
> > > --
> > > 2.43.7
> >