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