Re: [PATCH vhost v2 1/2] virtio_pci: fix the common map size and add check for common size

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

On Sun, Oct 08, 2023 at 05:38:41PM +0800, Xuan Zhuo wrote:
> Now, the function vp_modern_map_capability() takes the size parameter,
> which corresponds to the size of virtio_pci_common_cfg. As a result,
> this indicates the size of memory area to map.
> 
> However, if the _F_RING_RESET is negotiated, the extra items will be
> used. Therefore, we need to use the size of virtio_pci_modern_common_cfg
> to map more space.
> 
> Meanwhile, this patch removes the feature(_F_RING_ERSET and

typo

> _F_NOTIFICATION_DATA) when the common cfg size does not match
> the corresponding feature.
> 
> Fixes: 0b50cece0b78 ("virtio_pci: introduce helper to get/set queue reset")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_pci_modern.c     | 20 +++++++++++++++++++-
>  drivers/virtio/virtio_pci_modern_dev.c |  4 ++--
>  include/linux/virtio_pci_modern.h      |  1 +
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index d6bb68ba84e5..c0b9d2363ddb 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -22,8 +22,26 @@
>  static u64 vp_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +	u64 features = vp_modern_get_features(&vp_dev->mdev);
> +
> +#define check_feature(feature, field)								\
> +	do {											\
> +		if (features & BIT_ULL(feature)) {						\
> +			u32 offset = offsetofend(struct virtio_pci_modern_common_cfg, field);	\
> +			if (unlikely(vp_dev->mdev.common_len < offset))				\
> +				features &= ~BIT_ULL(feature);					\
> +		}										\
> +	} while (0)
> +
> +	/* For buggy devices, if the common len does not match the feature, we
> +	 * remove the feature.

I don't like doing this in vp_get_features. userspace won't be able
to see the actual device features at all.
Also, we should print an info message at least.

I am still debating with myself whether clearing feature bits
or just failing finalize_features (and thus, probe) is best.


> +	 */
> +	check_feature(VIRTIO_F_NOTIFICATION_DATA, queue_notify_data);
> +	check_feature(VIRTIO_F_RING_RESET, queue_reset);
> +
> +#undef check_feature

this macro's too scary. just pass offset and feature bit as
parameters to an inline function.

>  
> -	return vp_modern_get_features(&vp_dev->mdev);
> +	return features;
>  }
>  
>  static void vp_transport_features(struct virtio_device *vdev, u64 features)
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index aad7d9296e77..33f319da1558 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -291,8 +291,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev)
>  	err = -EINVAL;
>  	mdev->common = vp_modern_map_capability(mdev, common,
>  				      sizeof(struct virtio_pci_common_cfg), 4,
> -				      0, sizeof(struct virtio_pci_common_cfg),
> -				      NULL, NULL);
> +				      0, sizeof(struct virtio_pci_modern_common_cfg),
> +				      &mdev->common_len, NULL);
>  	if (!mdev->common)
>  		goto err_map_common;
>  	mdev->isr = vp_modern_map_capability(mdev, isr, sizeof(u8), 1,
> diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> index 067ac1d789bc..edf62bae0474 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -28,6 +28,7 @@ struct virtio_pci_modern_device {
>  	/* So we can sanity-check accesses. */
>  	size_t notify_len;
>  	size_t device_len;
> +	size_t common_len;
>  
>  	/* Capability for when we need to map notifications per-vq. */
>  	int notify_map_cap;
> -- 
> 2.32.0.3.g01195cf9f

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization