Re: [PATCH v2] virtio_pci_modern: skip trailing zero feature dwords

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

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

So .. why does all this matter? how many exits do you save
during a guest boot? is it worth the complexity?

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

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

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?

> +
>  	/*
>  	 * 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