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

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

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