Re: [RFC PATCH 08/12] virtio_pci: support VIRTIO_F_DMB

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

On Sun, Aug 09, 2026 at 06:20:06PM +0000, Alexander Graf wrote:
> Let a modern virtio-pci device place its virtqueues and the buffers they
> reference in a Device Memory Buffer of its own: accept VIRTIO_F_DMB from
> vp_transport_features(), and implement the get_dmb_shm_id config op on
> top of vp_modern_get_dmb_shm_id().
> 
> Refuse a device whose common configuration is too short to hold
> dmb_shm_id, which would put that read outside what vp_modern_probe()
> mapped. The accept commits before the id can be read, so a region we
> fail to locate afterwards fails virtio_features_ok() and probe sets the
> FAILED status bit.
> 
> Four conditions gate the accept:
> 
>   1) VIRTIO_F_ACCESS_PLATFORM, because the feature is only defined
>      together with it.
>   2) VIRTIO_F_ORDER_PLATFORM where the device offers it. Without it
>      the ring emits the weaker barriers that assume the device sees
>      memory the way another CPU does, and a region that is not
>      ordinary host memory breaks that assumption.

I don't get this last sentence. Not really?


>   3) CONFIG_VIRTIO_DMB, so a device offering the feature to a kernel
>      built without it is driven as an ordinary device.
>   4) VIRTIO_F_VERSION_1, because virtio_features_ok() returns early
>      without it, which would leave the feature negotiated and the
>      region never built. This transport refuses such a device anyway.


What is missing is actually validating that the region is cache coherent.

For regular pci devices, which this patch seems to try to handle,
this is not the case.


Maybe this feature is CONFIG_VIRTIO_DMB_COHERENT actually.


> 
> vp_dmb_ordering_ok() asks the device with vp_modern_get_features()
> instead of reading the feature word vp_transport_features() is handed.
> That word holds what the driver accepts, so a device offering
> VIRTIO_F_ORDER_PLATFORM to a driver that declined it would read there as
> a device that never offered it.
> 
> Link: https://lore.kernel.org/virtio-comment/20260804161202.38619-1-graf@amazon.com/
> Assisted-by: Kiro:claude-opus-5 checkpatch sparse
> Signed-off-by: Alexander Graf <graf@amazon.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 65 ++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 565d37b630b3..c43c1fc6e843 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -364,6 +364,36 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
>  	}
>  }
>  
> +/*
> + * The proposal

proposal?

> makes accepting VIRTIO_F_DMB conditional on accepting
> + * VIRTIO_F_ORDER_PLATFORM where the device offers it.  Without it the barriers
> + * the ring emits order accesses only as seen by a device that can be assumed
> + * to run on identical CPUs in an SMP configuration, which a device whose
> + * region is not ordinary host memory is not.  Accepting
> + * VIRTIO_F_ORDER_PLATFORM is otherwise only a SHOULD, so nothing else couples
> + * the two.

This thought process kind of thing (what we considered and discarded)
is fine in the commit log but not in the code.


> + *
> + * The offer is read back from the device rather than taken from the feature
> + * word vp_transport_features() is given, because that word is what the driver
> + * still wants rather than what the device offered.  The two differ exactly
> + * where this has to hold: virtio_dev_probe() calls finalize_features() a
> + * second time when a driver's validate() changed the set, and a validate()
> + * that declined VIRTIO_F_ORDER_PLATFORM leaves the bit absent from the word
> + * as well, which would read as an offer that never happened.  virtio_balloon
> + * declines VIRTIO_F_ACCESS_PLATFORM from validate() today, so the shape is
> + * not hypothetical.
> + */
> +static bool vp_dmb_ordering_ok(struct virtio_device *vdev)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> +	if (__virtio_test_bit(vdev, VIRTIO_F_ORDER_PLATFORM))
> +		return true;
> +
> +	return !(vp_modern_get_features(&vp_dev->mdev) &
> +		 BIT_ULL(VIRTIO_F_ORDER_PLATFORM));
> +}
> +
>  static void vp_transport_features(struct virtio_device *vdev, u64 features)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> @@ -378,6 +408,27 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
>  
>  	if (features & BIT_ULL(VIRTIO_F_ADMIN_VQ))
>  		__virtio_set_bit(vdev, VIRTIO_F_ADMIN_VQ);
> +
> +	/*
> +	 * VIRTIO_F_DMB is only defined together with
> +	 * VIRTIO_F_ACCESS_PLATFORM, so accept it only when the driver accepts
> +	 * that too.  vring_transport_features() has already run, so the bit in
> +	 * vdev is the one the driver accepts rather than the one the device
> +	 * offered, and the proposal words the requirement against what the
> +	 * driver accepts.  VIRTIO_F_ORDER_PLATFORM is required where the device
> +	 * offers it, for the reason vp_dmb_ordering_ok() gives.
> +	 * VIRTIO_F_VERSION_1 is required because the core locates and releases
> +	 * the region from virtio_features_ok(), which returns before it gets
> +	 * that far for a device without VERSION_1, so accepting the feature
> +	 * without it would leave the feature negotiated and the region never
> +	 * built.
> +	 */
> +	if (IS_ENABLED(CONFIG_VIRTIO_DMB) &&
> +	    (features & BIT_ULL(VIRTIO_F_DMB)) &&
> +	    __virtio_test_bit(vdev, VIRTIO_F_ACCESS_PLATFORM) &&
> +	    (features & BIT_ULL(VIRTIO_F_VERSION_1)) &&
> +	    vp_dmb_ordering_ok(vdev))
> +		__virtio_set_bit(vdev, VIRTIO_F_DMB);


So maybe let's not couple them in the spec and our lives will be easier.
!VIRTIO_F_ACCESS_PLATFORM is generally a PV thing.

>  }
>  
>  static int __vp_check_common_size_one_feature(struct virtio_device *vdev, u32 fbit,
> @@ -413,6 +464,9 @@ static int vp_check_common_size(struct virtio_device *vdev)
>  	if (vp_check_common_size_one_feature(vdev, VIRTIO_F_ADMIN_VQ, admin_queue_num))
>  		return -EINVAL;
>  
> +	if (vp_check_common_size_one_feature(vdev, VIRTIO_F_DMB, dmb_shm_id))
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> @@ -878,6 +932,15 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
>  	return true;
>  }
>  
> +static int vp_get_dmb_shm_id(struct virtio_device *vdev, u16 *id)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> +	*id = vp_modern_get_dmb_shm_id(&vp_dev->mdev);
> +
> +	return 0;
> +}
> +
>  /*
>   * virtio_pci_admin_has_dev_parts - Checks whether the device parts
>   * functionality is supported
> @@ -1241,6 +1304,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>  	.set_vq_affinity = vp_set_vq_affinity,
>  	.get_vq_affinity = vp_get_vq_affinity,
>  	.get_shm_region  = vp_get_shm_region,
> +	.get_dmb_shm_id = vp_get_dmb_shm_id,
>  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
>  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
>  };
> @@ -1261,6 +1325,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  	.set_vq_affinity = vp_set_vq_affinity,
>  	.get_vq_affinity = vp_get_vq_affinity,
>  	.get_shm_region  = vp_get_shm_region,
> +	.get_dmb_shm_id = vp_get_dmb_shm_id,
>  	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
>  	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
>  };