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