RE: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb

[Eli Cohen]

> From: Eugenio Pérez <eperezma@xxxxxxxxxx>
> Sent: Thursday, 12 January 2023 16:22
> To: mst@xxxxxxxxxx; Eli Cohen <elic@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Parav Pandit <parav@xxxxxxxxxx>;
> lulu@xxxxxxxxxx; jasowang@xxxxxxxxxx; virtualization@lists.linux-
> foundation.org; sgarzare@xxxxxxxxxx; si-wei.liu@xxxxxxxxxx
> Subject: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
> 
> Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock at
> this moment, but:
> * These iotlb changes / queries are not in the fast data path.
> * reslock belongs to netdev, while dup_iotlb seems generic.
> * It's located in a different file than the lock it needs to hold
> 
> Justifies the lock acquisition.
> 

Following this reasoning we should take the spinlock wherever we reference an iotlb.
Question if it make sense that the iotlb could change while it is being referenced.
Can you identify a specific case for this? 
 
> Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC
> setting")
> Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> ---
>  drivers/vdpa/mlx5/core/mr.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index 878ee94efa78..e9c8a7f8ee1d 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -454,13 +454,15 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> struct vhost_iotlb *src)
>  {
>  	struct vhost_iotlb_map *map;
>  	u64 start = 0, last = ULLONG_MAX;
> -	int err;
> +	int err = 0;
> +
> +	spin_lock(&mvdev->cvq.iommu_lock);
> 
>  	vhost_iotlb_reset(mvdev->cvq.iotlb);
> 
>  	if (!src) {
>  		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> start, VHOST_ACCESS_RW);
> -		return err;
> +		goto out;
>  	}
> 
>  	for (map = vhost_iotlb_itree_first(src, start, last); map;
> @@ -468,9 +470,12 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> struct vhost_iotlb *src)
>  		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start,
> map->last,
>  					    map->addr, map->perm);
>  		if (err)
> -			return err;
> +			goto out;
>  	}
> -	return 0;
> +
> +out:
> +	spin_unlock(&mvdev->cvq.iommu_lock);
> +	return err;
>  }
> 
>  static void prune_iotlb(struct mlx5_vdpa_dev *mvdev)
> --
> 2.31.1