[PATCH] virtio_pci_modern: skip unused trailing driver feature words

[PATCH] virtio_pci_modern: skip unused trailing driver feature words

[Peng Hao]

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 <flyingpeng@tencent.com>
---
 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

Re: [PATCH] virtio_pci_modern: skip unused trailing driver feature words

[sashiko-bot]

> 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 <flyingpeng@tencent.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920065746.86415-1-flyingpeng@tencent.com?part=1

Re: [PATCH] virtio_pci_modern: skip unused trailing driver feature words

[Michael S. Tsirkin]

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 <flyingpeng@tencent.com>

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?


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

Re: [PATCH] virtio_pci_modern: skip unused trailing driver feature words

[Hao Peng]

On Sun, Sep 20, 2026 at 4:41 PM Michael S. Tsirkin <mst@redhat.com> 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 <flyingpeng@tencent.com>
>
> 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
>