On Sun, Sep 20, 2026 at 4:41 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Sun, Sep 20, 2026 at 02:57:46PM +0800, Peng Hao wrote:
> > Since commit 69b9461512246 ("virtio_pci_modern: allow configuring
> > extended features"), vp_modern_set_extended_features() writes all 128
> > feature bits on every device. Each 32-bit word requires one write to
> > guest_feature_select and another to guest_feature, even when the upper
> > 64 feature bits are all zero.
> >
> > Feature negotiation starts after a device reset, which clears the
> > device-side driver features. There is therefore no need to write
> > trailing zero 64-bit words on the first feature finalization.
> >
> > finalize_features() can, however, be called again without an intervening
> > reset when a driver's validate callback changes the negotiated features.
> > Remember how many words were written since the last reset and include
> > that range in the next write, so that a repeated finalization can clear
> > features which were previously enabled.
> >
> > This avoids four MMIO writes for devices which negotiate no feature
> > above bit 63, while preserving the full-overwrite behavior when the
> > negotiated feature range shrinks.
> >
> > Signed-off-by: Peng Hao <[email protected]>
>
> I don't get it. pci updates features in units of 32 bit dwords.
>
> Most devices only use the 1st 32 bit dword.
>
> /scm/linux$ git grep _F_ include/uapi/linux/virtio*h|grep [4-9][0-9]|wc -l
> 21
> /scm/linux$ git grep _F_ include/uapi/linux/virtio*h|wc -l
> 190
>
> Why are you tracking 64-bit qwords?
>
No good reason -- I tracked the unit the features array happens to be
stored in rather than the unit the register interface uses. You're
right, and it's not just cosmetic: qword granularity is strictly worse
here, because VIRTIO_F_VERSION_1 is bit 32. Every modern device has
dword 1 set, so the qword count can never fall below 2, and the v1 patch
therefore saved nothing at all for a device using bits 64..95. Counting
dwords fixes that:
virtio-net, nothing above bit 63 8 -> 4 writes (v1: 4)
virtio-net + UDP tunnel GSO (65..68) 8 -> 6 writes (v1: 8, no saving)
v2 counts dwords throughout. The struct field becomes
u8 driver_features_dwords;
initialised to 1 at probe and reset to 1 in vp_modern_set_status(status
== 0), and the write loop runs to max_t(int, dwords,
mdev->driver_features_dwords).
Thanks,
Peng Hao
>
> > ---
> > drivers/virtio/virtio_pci_modern_dev.c | 22 +++++++++++++++++++++-
> > include/linux/virtio_pci_modern.h | 3 +++
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern_dev.c
> > b/drivers/virtio/virtio_pci_modern_dev.c
> > index 413a8c353463..8211e491a17b 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_u64s = 1;
> > +
> > if (mdev->device_id_check) {
> > devid = mdev->device_id_check(pci_dev);
> > if (devid < 0)
> > @@ -446,14 +448,29 @@ void vp_modern_set_extended_features(struct
> > virtio_pci_modern_device *mdev,
> > const u64 *features)
> > {
> > struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> > + u8 u64s = VIRTIO_FEATURES_U64S;
> > + u8 write_u64s;
> > int i;
> >
> > - for (i = 0; i < VIRTIO_FEATURES_BITS / 32; i++) {
> > + /*
> > + * A reset clears the device-side driver features, so trailing zero
> > + * words need not be written. Include words written by the previous
> > + * call, though, so a repeated feature finalization can clear features
> > + * that were previously enabled.
> > + */
> > + while (u64s > 1 && !features[u64s - 1])
> > + u64s--;
> > +
> > + write_u64s = max(u64s, mdev->driver_features_u64s);
> > +
> > + for (i = 0; i < write_u64s * 2; i++) {
> > u32 cur = features[i >> 1] >> (32 * (i & 1));
> >
> > vp_iowrite32(i, &cfg->guest_feature_select);
> > vp_iowrite32(cur, &cfg->guest_feature);
> > }
> > +
> > + mdev->driver_features_u64s = u64s;
> > }
> > EXPORT_SYMBOL_GPL(vp_modern_set_extended_features);
> >
> > @@ -495,6 +512,9 @@ void vp_modern_set_status(struct
> > virtio_pci_modern_device *mdev,
> > {
> > struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> >
> > + if (!status)
> > + mdev->driver_features_u64s = 1;
> > +
> > /*
> > * 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..8df287d545c9 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_u64s: Number of 64-bit driver feature words programmed
> > + * since the last device 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_u64s;
> > };
> >
> > /*
> > --
> > 2.43.7
>
