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 <[email protected]> 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 <[email protected]>
> >
> > 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
> >
