[RFC 0/3] MLX Adress Space ID + Control VirtQueue fixes

[RFC 0/3] MLX Adress Space ID + Control VirtQueue fixes

[Eugenio Pérez]

Proposal for fixes detected when using CVQ in a different ASID.

Only tested with qemu and vhost_vdpa.

Eugenio Pérez (3):
  vdpa/mlx5: reset iotlb at dup_iotlb
  vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr
  vdpa/mlx5: take iommu_lock at dup_iotlb

 drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
 drivers/vdpa/mlx5/core/mr.c        | 20 ++++++++++++++------
 drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
 3 files changed, 21 insertions(+), 13 deletions(-)

-- 
2.31.1

[RFC 1/3] vdpa/mlx5: reset iotlb at dup_iotlb

[Eugenio Pérez]

Regular memory region changes already reset cvq iotlb at set_map.
However this is not true if CVQ and data VQs are in different ASID.

Clean the CVQ iotlb every time we hit dup_iotlb.

Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for control and data")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vdpa/mlx5/core/mr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index 0a1e0b0dc37e..ae34dcac9a3f 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -456,6 +456,8 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *src)
 	u64 start = 0, last = ULLONG_MAX;
 	int err;
 
+	vhost_iotlb_reset(mvdev->cvq.iotlb);
+
 	if (!src) {
 		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last, start, VHOST_ACCESS_RW);
 		return err;
-- 
2.31.1

RE: [RFC 1/3] vdpa/mlx5: reset iotlb at dup_iotlb

[Eli Cohen]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, 12 January 2023 16:22
> To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> Subject: [RFC 1/3] vdpa/mlx5: reset iotlb at dup_iotlb
> 
> Regular memory region changes already reset cvq iotlb at set_map.
> However this is not true if CVQ and data VQs are in different ASID.
> 
> Clean the CVQ iotlb every time we hit dup_iotlb.
> 
> Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
> control and data")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vdpa/mlx5/core/mr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index 0a1e0b0dc37e..ae34dcac9a3f 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -456,6 +456,8 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> struct vhost_iotlb *src)
>  	u64 start = 0, last = ULLONG_MAX;
>  	int err;
> 
> +	vhost_iotlb_reset(mvdev->cvq.iotlb);
> +

As far as I can see, mlx5_vdpa_destroy_mr() is called independently of the asid
and it will calls prune_iotlb() which resets the cvq iotlb. Am I missing something?

>  	if (!src) {
>  		err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> start, VHOST_ACCESS_RW);
>  		return err;
> --
> 2.31.1

Re: [RFC 1/3] vdpa/mlx5: reset iotlb at dup_iotlb

[Eugenio Perez Martin]

On Mon, Jan 16, 2023 at 7:32 AM Eli Cohen <elic@nvidia.com> wrote:
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, 12 January 2023 16:22
> > To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> > Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> > lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> > foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> > Subject: [RFC 1/3] vdpa/mlx5: reset iotlb at dup_iotlb
> >
> > Regular memory region changes already reset cvq iotlb at set_map.
> > However this is not true if CVQ and data VQs are in different ASID.
> >
> > Clean the CVQ iotlb every time we hit dup_iotlb.
> >
> > Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
> > control and data")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/core/mr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index 0a1e0b0dc37e..ae34dcac9a3f 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -456,6 +456,8 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *src)
> >       u64 start = 0, last = ULLONG_MAX;
> >       int err;
> >
> > +     vhost_iotlb_reset(mvdev->cvq.iotlb);
> > +
>
> As far as I can see, mlx5_vdpa_destroy_mr() is called independently of the asid
> and it will calls prune_iotlb() which resets the cvq iotlb. Am I missing something?
>

Sorry, my fault. It is not obvious from the patch but this prepares the code for
the next patch in the series.

With the next patch applied, prune_iotlb is not called in all
contexts. This patch may be dropped in future series.

Thanks!


> >       if (!src) {
> >               err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> > start, VHOST_ACCESS_RW);
> >               return err;
> > --
> > 2.31.1
>

[RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr

[Eugenio Pérez]

mlx5_vdpa_destroy_mr can be called by setting a map to data ASID after
populating control virtqueue ASID iotlb.  Control vq iotlb must not be
cleared, since it will not be populated again.

Adding a conditional in the function so the caller specifies if it is
resetting, cleaning, or just changing data memory.

Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for control and data")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
 drivers/vdpa/mlx5/core/mr.c        |  5 +++--
 drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
index 058fbe28107e..000b144019ec 100644
--- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
@@ -119,7 +119,7 @@ int mlx5_vdpa_handle_set_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *io
 			     bool *change_map, unsigned int asid);
 int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb *iotlb,
 			unsigned int asid);
-void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev);
+void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool delete_cvq_iotlb);
 
 #define mlx5_vdpa_warn(__dev, format, ...)                                                         \
 	dev_warn((__dev)->mdev->device, "%s:%d:(pid %d) warning: " format, __func__, __LINE__,     \
diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
index ae34dcac9a3f..878ee94efa78 100644
--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c
@@ -491,7 +491,7 @@ static void destroy_user_mr(struct mlx5_vdpa_dev *mvdev, struct mlx5_vdpa_mr *mr
 	}
 }
 
-void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
+void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool delete_cvq_iotlb)
 {
 	struct mlx5_vdpa_mr *mr = &mvdev->mr;
 
@@ -499,7 +499,8 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
 	if (!mr->initialized)
 		goto out;
 
-	prune_iotlb(mvdev);
+	if (delete_cvq_iotlb)
+		prune_iotlb(mvdev);
 	if (mr->user_mr)
 		destroy_user_mr(mvdev, mr);
 	else
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 6632651b1e54..1f1f341f602b 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2433,7 +2433,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
 		goto err_mr;
 
 	teardown_driver(ndev);
-	mlx5_vdpa_destroy_mr(mvdev);
+	mlx5_vdpa_destroy_mr(mvdev, mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
 	err = mlx5_vdpa_create_mr(mvdev, iotlb, asid);
 	if (err)
 		goto err_mr;
@@ -2449,7 +2449,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
 	return 0;
 
 err_setup:
-	mlx5_vdpa_destroy_mr(mvdev);
+	mlx5_vdpa_destroy_mr(mvdev, mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
 err_mr:
 	return err;
 }
@@ -2578,7 +2578,7 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 	return;
 
 err_setup:
-	mlx5_vdpa_destroy_mr(&ndev->mvdev);
+	mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
 err_clear:
 	up_write(&ndev->reslock);
@@ -2604,7 +2604,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 	down_write(&ndev->reslock);
 	teardown_driver(ndev);
 	clear_vqs_ready(ndev);
-	mlx5_vdpa_destroy_mr(&ndev->mvdev);
+	mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
 	ndev->mvdev.status = 0;
 	ndev->cur_num_vqs = 0;
 	ndev->mvdev.cvq.received_desc = 0;
@@ -2691,7 +2691,7 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 
 	free_resources(ndev);
-	mlx5_vdpa_destroy_mr(mvdev);
+	mlx5_vdpa_destroy_mr(mvdev, true);
 	if (!is_zero_ether_addr(ndev->config.mac)) {
 		pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
 		mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
@@ -3214,7 +3214,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 err_res2:
 	free_resources(ndev);
 err_mr:
-	mlx5_vdpa_destroy_mr(mvdev);
+	mlx5_vdpa_destroy_mr(mvdev, true);
 err_res:
 	mlx5_vdpa_free_resources(&ndev->mvdev);
 err_mpfs:
-- 
2.31.1

RE: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr

[Eli Cohen]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, 12 January 2023 16:22
> To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> Subject: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr
> 
> mlx5_vdpa_destroy_mr can be called by setting a map to data ASID after
> populating control virtqueue ASID iotlb.  Control vq iotlb must not be
> cleared, since it will not be populated again.
> 
> Adding a conditional in the function so the caller specifies if it is
> resetting, cleaning, or just changing data memory.
> 
> Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
> control and data")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
>  drivers/vdpa/mlx5/core/mr.c        |  5 +++--
>  drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> index 058fbe28107e..000b144019ec 100644
> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> @@ -119,7 +119,7 @@ int mlx5_vdpa_handle_set_map(struct
> mlx5_vdpa_dev *mvdev, struct vhost_iotlb *io
>  			     bool *change_map, unsigned int asid);
>  int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> *iotlb,
>  			unsigned int asid);
> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev);
> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> delete_cvq_iotlb);
> 
>  #define mlx5_vdpa_warn(__dev, format, ...)
> \
>  	dev_warn((__dev)->mdev->device, "%s:%d:(pid %d) warning: "
> format, __func__, __LINE__,     \
> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> index ae34dcac9a3f..878ee94efa78 100644
> --- a/drivers/vdpa/mlx5/core/mr.c
> +++ b/drivers/vdpa/mlx5/core/mr.c
> @@ -491,7 +491,7 @@ static void destroy_user_mr(struct mlx5_vdpa_dev
> *mvdev, struct mlx5_vdpa_mr *mr
>  	}
>  }
> 
> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> delete_cvq_iotlb)
>  {
>  	struct mlx5_vdpa_mr *mr = &mvdev->mr;
> 
> @@ -499,7 +499,8 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev
> *mvdev)
>  	if (!mr->initialized)
>  		goto out;
> 
> -	prune_iotlb(mvdev);
> +	if (delete_cvq_iotlb)
> +		prune_iotlb(mvdev);
>  	if (mr->user_mr)
>  		destroy_user_mr(mvdev, mr);
>  	else
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 6632651b1e54..1f1f341f602b 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2433,7 +2433,7 @@ static int mlx5_vdpa_change_map(struct
> mlx5_vdpa_dev *mvdev,
>  		goto err_mr;
> 
>  	teardown_driver(ndev);
> -	mlx5_vdpa_destroy_mr(mvdev);
> +	mlx5_vdpa_destroy_mr(mvdev, mvdev-
> >group2asid[MLX5_VDPA_CVQ_GROUP] == asid);

Looks to me we need to handle this in a more generic manner. The asid should be used conditionally for either CVQ or data VQ updates. You are protecting CVQ but same thing should hold also for data VQs iotlb. Meaning, if qemu wants to update only CVQ than data VQ translation must not be affected.

>  	err = mlx5_vdpa_create_mr(mvdev, iotlb, asid);
>  	if (err)
>  		goto err_mr;
> @@ -2449,7 +2449,7 @@ static int mlx5_vdpa_change_map(struct
> mlx5_vdpa_dev *mvdev,
>  	return 0;
> 
>  err_setup:
> -	mlx5_vdpa_destroy_mr(mvdev);
> +	mlx5_vdpa_destroy_mr(mvdev, mvdev-
> >group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
>  err_mr:
>  	return err;
>  }
> @@ -2578,7 +2578,7 @@ static void mlx5_vdpa_set_status(struct
> vdpa_device *vdev, u8 status)
>  	return;
> 
>  err_setup:
> -	mlx5_vdpa_destroy_mr(&ndev->mvdev);
> +	mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
>  	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
>  err_clear:
>  	up_write(&ndev->reslock);
> @@ -2604,7 +2604,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
> *vdev)
>  	down_write(&ndev->reslock);
>  	teardown_driver(ndev);
>  	clear_vqs_ready(ndev);
> -	mlx5_vdpa_destroy_mr(&ndev->mvdev);
> +	mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
>  	ndev->mvdev.status = 0;
>  	ndev->cur_num_vqs = 0;
>  	ndev->mvdev.cvq.received_desc = 0;
> @@ -2691,7 +2691,7 @@ static void mlx5_vdpa_free(struct vdpa_device
> *vdev)
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
> 
>  	free_resources(ndev);
> -	mlx5_vdpa_destroy_mr(mvdev);
> +	mlx5_vdpa_destroy_mr(mvdev, true);
>  	if (!is_zero_ether_addr(ndev->config.mac)) {
>  		pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
>  		mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
> @@ -3214,7 +3214,7 @@ static int mlx5_vdpa_dev_add(struct
> vdpa_mgmt_dev *v_mdev, const char *name,
>  err_res2:
>  	free_resources(ndev);
>  err_mr:
> -	mlx5_vdpa_destroy_mr(mvdev);
> +	mlx5_vdpa_destroy_mr(mvdev, true);
>  err_res:
>  	mlx5_vdpa_free_resources(&ndev->mvdev);
>  err_mpfs:
> --
> 2.31.1

Re: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr

[Eugenio Perez Martin]

On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <elic@nvidia.com> wrote:
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, 12 January 2023 16:22
> > To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> > Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> > lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> > foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> > Subject: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr
> >
> > mlx5_vdpa_destroy_mr can be called by setting a map to data ASID after
> > populating control virtqueue ASID iotlb.  Control vq iotlb must not be
> > cleared, since it will not be populated again.
> >
> > Adding a conditional in the function so the caller specifies if it is
> > resetting, cleaning, or just changing data memory.
> >
> > Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
> > control and data")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
> >  drivers/vdpa/mlx5/core/mr.c        |  5 +++--
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
> >  3 files changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > index 058fbe28107e..000b144019ec 100644
> > --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> > @@ -119,7 +119,7 @@ int mlx5_vdpa_handle_set_map(struct
> > mlx5_vdpa_dev *mvdev, struct vhost_iotlb *io
> >                            bool *change_map, unsigned int asid);
> >  int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> > *iotlb,
> >                       unsigned int asid);
> > -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev);
> > +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> > delete_cvq_iotlb);
> >
> >  #define mlx5_vdpa_warn(__dev, format, ...)
> > \
> >       dev_warn((__dev)->mdev->device, "%s:%d:(pid %d) warning: "
> > format, __func__, __LINE__,     \
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index ae34dcac9a3f..878ee94efa78 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -491,7 +491,7 @@ static void destroy_user_mr(struct mlx5_vdpa_dev
> > *mvdev, struct mlx5_vdpa_mr *mr
> >       }
> >  }
> >
> > -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> > +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> > delete_cvq_iotlb)
> >  {
> >       struct mlx5_vdpa_mr *mr = &mvdev->mr;
> >
> > @@ -499,7 +499,8 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev
> > *mvdev)
> >       if (!mr->initialized)
> >               goto out;
> >
> > -     prune_iotlb(mvdev);
> > +     if (delete_cvq_iotlb)
> > +             prune_iotlb(mvdev);
> >       if (mr->user_mr)
> >               destroy_user_mr(mvdev, mr);
> >       else
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 6632651b1e54..1f1f341f602b 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -2433,7 +2433,7 @@ static int mlx5_vdpa_change_map(struct
> > mlx5_vdpa_dev *mvdev,
> >               goto err_mr;
> >
> >       teardown_driver(ndev);
> > -     mlx5_vdpa_destroy_mr(mvdev);
> > +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
> > >group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
>
> Looks to me we need to handle this in a more generic manner. The asid should be used conditionally for either CVQ or data VQ updates. You are protecting CVQ but same thing should hold also for data VQs iotlb.

I agree. Maybe the best option is to replace the boolean indicating
the ASID we want to destroy mr? Then, at cleanup, we can iterate by
all vq groups / ASID.

> Meaning, if qemu wants to update only CVQ than data VQ translation must not be affected.

_mlx5_vdpa_create_mr. is the one that checks it If I recall correctly.
It is not obvious in this change though.

Thanks!

>
> >       err = mlx5_vdpa_create_mr(mvdev, iotlb, asid);
> >       if (err)
> >               goto err_mr;
> > @@ -2449,7 +2449,7 @@ static int mlx5_vdpa_change_map(struct
> > mlx5_vdpa_dev *mvdev,
> >       return 0;
> >
> >  err_setup:
> > -     mlx5_vdpa_destroy_mr(mvdev);
> > +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
> > >group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
> >  err_mr:
> >       return err;
> >  }
> > @@ -2578,7 +2578,7 @@ static void mlx5_vdpa_set_status(struct
> > vdpa_device *vdev, u8 status)
> >       return;
> >
> >  err_setup:
> > -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
> > +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
> >       ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> >  err_clear:
> >       up_write(&ndev->reslock);
> > @@ -2604,7 +2604,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
> > *vdev)
> >       down_write(&ndev->reslock);
> >       teardown_driver(ndev);
> >       clear_vqs_ready(ndev);
> > -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
> > +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
> >       ndev->mvdev.status = 0;
> >       ndev->cur_num_vqs = 0;
> >       ndev->mvdev.cvq.received_desc = 0;
> > @@ -2691,7 +2691,7 @@ static void mlx5_vdpa_free(struct vdpa_device
> > *vdev)
> >       ndev = to_mlx5_vdpa_ndev(mvdev);
> >
> >       free_resources(ndev);
> > -     mlx5_vdpa_destroy_mr(mvdev);
> > +     mlx5_vdpa_destroy_mr(mvdev, true);
> >       if (!is_zero_ether_addr(ndev->config.mac)) {
> >               pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
> >               mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
> > @@ -3214,7 +3214,7 @@ static int mlx5_vdpa_dev_add(struct
> > vdpa_mgmt_dev *v_mdev, const char *name,
> >  err_res2:
> >       free_resources(ndev);
> >  err_mr:
> > -     mlx5_vdpa_destroy_mr(mvdev);
> > +     mlx5_vdpa_destroy_mr(mvdev, true);
> >  err_res:
> >       mlx5_vdpa_free_resources(&ndev->mvdev);
> >  err_mpfs:
> > --
> > 2.31.1
>

Re: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr

[Eli Cohen]

On 16/01/2023 20:03, Eugenio Perez Martin wrote:
> On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <elic@nvidia.com> wrote:
>>> From: Eugenio Pérez <eperezma@redhat.com>
>>> Sent: Thursday, 12 January 2023 16:22
>>> To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
>>> Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
>>> lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
>>> foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
>>> Subject: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr
>>>
>>> mlx5_vdpa_destroy_mr can be called by setting a map to data ASID after
>>> populating control virtqueue ASID iotlb.  Control vq iotlb must not be
>>> cleared, since it will not be populated again.
>>>
>>> Adding a conditional in the function so the caller specifies if it is
>>> resetting, cleaning, or just changing data memory.
>>>
>>> Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
>>> control and data")
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
>>>   drivers/vdpa/mlx5/core/mr.c        |  5 +++--
>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
>>>   3 files changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> index 058fbe28107e..000b144019ec 100644
>>> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
>>> @@ -119,7 +119,7 @@ int mlx5_vdpa_handle_set_map(struct
>>> mlx5_vdpa_dev *mvdev, struct vhost_iotlb *io
>>>                             bool *change_map, unsigned int asid);
>>>   int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>>> *iotlb,
>>>                        unsigned int asid);
>>> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev);
>>> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
>>> delete_cvq_iotlb);
>>>
>>>   #define mlx5_vdpa_warn(__dev, format, ...)
>>> \
>>>        dev_warn((__dev)->mdev->device, "%s:%d:(pid %d) warning: "
>>> format, __func__, __LINE__,     \
>>> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
>>> index ae34dcac9a3f..878ee94efa78 100644
>>> --- a/drivers/vdpa/mlx5/core/mr.c
>>> +++ b/drivers/vdpa/mlx5/core/mr.c
>>> @@ -491,7 +491,7 @@ static void destroy_user_mr(struct mlx5_vdpa_dev
>>> *mvdev, struct mlx5_vdpa_mr *mr
>>>        }
>>>   }
>>>
>>> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
>>> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
>>> delete_cvq_iotlb)
>>>   {
>>>        struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>>
>>> @@ -499,7 +499,8 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev
>>> *mvdev)
>>>        if (!mr->initialized)
>>>                goto out;
>>>
>>> -     prune_iotlb(mvdev);
>>> +     if (delete_cvq_iotlb)
>>> +             prune_iotlb(mvdev);
>>>        if (mr->user_mr)
>>>                destroy_user_mr(mvdev, mr);
>>>        else
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 6632651b1e54..1f1f341f602b 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -2433,7 +2433,7 @@ static int mlx5_vdpa_change_map(struct
>>> mlx5_vdpa_dev *mvdev,
>>>                goto err_mr;
>>>
>>>        teardown_driver(ndev);
>>> -     mlx5_vdpa_destroy_mr(mvdev);
>>> +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
>>>> group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
>> Looks to me we need to handle this in a more generic manner. The asid should be used conditionally for either CVQ or data VQ updates. You are protecting CVQ but same thing should hold also for data VQs iotlb.
> I agree. Maybe the best option is to replace the boolean indicating
> the ASID we want to destroy mr? Then, at cleanup, we can iterate by
> all vq groups / ASID.

I think mlx5_vdpa_destroy_mr() should get the asid as an argument and 
have a logic such as:

if (asid == data asid)

     destroy_data_mr


if (asid == ctrl_vq_asid)

     prune_iotlb()


return

Since we have only two groups, one for data and for control, I don't 
think we need to iterate.

>
>> Meaning, if qemu wants to update only CVQ than data VQ translation must not be affected.
> _mlx5_vdpa_create_mr. is the one that checks it If I recall correctly.
> It is not obvious in this change though.
>
> Thanks!
>
>>>        err = mlx5_vdpa_create_mr(mvdev, iotlb, asid);
>>>        if (err)
>>>                goto err_mr;
>>> @@ -2449,7 +2449,7 @@ static int mlx5_vdpa_change_map(struct
>>> mlx5_vdpa_dev *mvdev,
>>>        return 0;
>>>
>>>   err_setup:
>>> -     mlx5_vdpa_destroy_mr(mvdev);
>>> +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
>>>> group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
>>>   err_mr:
>>>        return err;
>>>   }
>>> @@ -2578,7 +2578,7 @@ static void mlx5_vdpa_set_status(struct
>>> vdpa_device *vdev, u8 status)
>>>        return;
>>>
>>>   err_setup:
>>> -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>> +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
>>>        ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
>>>   err_clear:
>>>        up_write(&ndev->reslock);
>>> @@ -2604,7 +2604,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
>>> *vdev)
>>>        down_write(&ndev->reslock);
>>>        teardown_driver(ndev);
>>>        clear_vqs_ready(ndev);
>>> -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>> +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
>>>        ndev->mvdev.status = 0;
>>>        ndev->cur_num_vqs = 0;
>>>        ndev->mvdev.cvq.received_desc = 0;
>>> @@ -2691,7 +2691,7 @@ static void mlx5_vdpa_free(struct vdpa_device
>>> *vdev)
>>>        ndev = to_mlx5_vdpa_ndev(mvdev);
>>>
>>>        free_resources(ndev);
>>> -     mlx5_vdpa_destroy_mr(mvdev);
>>> +     mlx5_vdpa_destroy_mr(mvdev, true);
>>>        if (!is_zero_ether_addr(ndev->config.mac)) {
>>>                pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
>>>                mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
>>> @@ -3214,7 +3214,7 @@ static int mlx5_vdpa_dev_add(struct
>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>   err_res2:
>>>        free_resources(ndev);
>>>   err_mr:
>>> -     mlx5_vdpa_destroy_mr(mvdev);
>>> +     mlx5_vdpa_destroy_mr(mvdev, true);
>>>   err_res:
>>>        mlx5_vdpa_free_resources(&ndev->mvdev);
>>>   err_mpfs:
>>> --
>>> 2.31.1

Re: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr

[Eugenio Perez Martin]

On Tue, Jan 17, 2023 at 8:08 AM Eli Cohen <elic@nvidia.com> wrote:
>
>
> On 16/01/2023 20:03, Eugenio Perez Martin wrote:
> > On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <elic@nvidia.com> wrote:
> >>> From: Eugenio Pérez <eperezma@redhat.com>
> >>> Sent: Thursday, 12 January 2023 16:22
> >>> To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> >>> Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> >>> lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> >>> foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> >>> Subject: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr
> >>>
> >>> mlx5_vdpa_destroy_mr can be called by setting a map to data ASID after
> >>> populating control virtqueue ASID iotlb.  Control vq iotlb must not be
> >>> cleared, since it will not be populated again.
> >>>
> >>> Adding a conditional in the function so the caller specifies if it is
> >>> resetting, cleaning, or just changing data memory.
> >>>
> >>> Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces for
> >>> control and data")
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>   drivers/vdpa/mlx5/core/mlx5_vdpa.h |  2 +-
> >>>   drivers/vdpa/mlx5/core/mr.c        |  5 +++--
> >>>   drivers/vdpa/mlx5/net/mlx5_vnet.c  | 12 ++++++------
> >>>   3 files changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> >>> b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> >>> index 058fbe28107e..000b144019ec 100644
> >>> --- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> >>> +++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
> >>> @@ -119,7 +119,7 @@ int mlx5_vdpa_handle_set_map(struct
> >>> mlx5_vdpa_dev *mvdev, struct vhost_iotlb *io
> >>>                             bool *change_map, unsigned int asid);
> >>>   int mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
> >>> *iotlb,
> >>>                        unsigned int asid);
> >>> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev);
> >>> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> >>> delete_cvq_iotlb);
> >>>
> >>>   #define mlx5_vdpa_warn(__dev, format, ...)
> >>> \
> >>>        dev_warn((__dev)->mdev->device, "%s:%d:(pid %d) warning: "
> >>> format, __func__, __LINE__,     \
> >>> diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> >>> index ae34dcac9a3f..878ee94efa78 100644
> >>> --- a/drivers/vdpa/mlx5/core/mr.c
> >>> +++ b/drivers/vdpa/mlx5/core/mr.c
> >>> @@ -491,7 +491,7 @@ static void destroy_user_mr(struct mlx5_vdpa_dev
> >>> *mvdev, struct mlx5_vdpa_mr *mr
> >>>        }
> >>>   }
> >>>
> >>> -void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev)
> >>> +void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev *mvdev, bool
> >>> delete_cvq_iotlb)
> >>>   {
> >>>        struct mlx5_vdpa_mr *mr = &mvdev->mr;
> >>>
> >>> @@ -499,7 +499,8 @@ void mlx5_vdpa_destroy_mr(struct mlx5_vdpa_dev
> >>> *mvdev)
> >>>        if (!mr->initialized)
> >>>                goto out;
> >>>
> >>> -     prune_iotlb(mvdev);
> >>> +     if (delete_cvq_iotlb)
> >>> +             prune_iotlb(mvdev);
> >>>        if (mr->user_mr)
> >>>                destroy_user_mr(mvdev, mr);
> >>>        else
> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> index 6632651b1e54..1f1f341f602b 100644
> >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> @@ -2433,7 +2433,7 @@ static int mlx5_vdpa_change_map(struct
> >>> mlx5_vdpa_dev *mvdev,
> >>>                goto err_mr;
> >>>
> >>>        teardown_driver(ndev);
> >>> -     mlx5_vdpa_destroy_mr(mvdev);
> >>> +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
> >>>> group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
> >> Looks to me we need to handle this in a more generic manner. The asid should be used conditionally for either CVQ or data VQ updates. You are protecting CVQ but same thing should hold also for data VQs iotlb.
> > I agree. Maybe the best option is to replace the boolean indicating
> > the ASID we want to destroy mr? Then, at cleanup, we can iterate by
> > all vq groups / ASID.
>
> I think mlx5_vdpa_destroy_mr() should get the asid as an argument and
> have a logic such as:
>
> if (asid == data asid)
>
>      destroy_data_mr
>
>
> if (asid == ctrl_vq_asid)
>
>      prune_iotlb()
>
>
> return
>

I agree.

> Since we have only two groups, one for data and for control, I don't
> think we need to iterate.
>

I mean to iterate on mlx5_vdpa_reset and mlx5_vdpa_free. Or, at least,
to call mlx5_vdpa_destroy_mr(asid=data) and
mlx5_vdpa_destroy_mr(asid=control) one after another.

Thanks!

> >
> >> Meaning, if qemu wants to update only CVQ than data VQ translation must not be affected.
> > _mlx5_vdpa_create_mr. is the one that checks it If I recall correctly.
> > It is not obvious in this change though.
> >
> > Thanks!
> >
> >>>        err = mlx5_vdpa_create_mr(mvdev, iotlb, asid);
> >>>        if (err)
> >>>                goto err_mr;
> >>> @@ -2449,7 +2449,7 @@ static int mlx5_vdpa_change_map(struct
> >>> mlx5_vdpa_dev *mvdev,
> >>>        return 0;
> >>>
> >>>   err_setup:
> >>> -     mlx5_vdpa_destroy_mr(mvdev);
> >>> +     mlx5_vdpa_destroy_mr(mvdev, mvdev-
> >>>> group2asid[MLX5_VDPA_CVQ_GROUP] == asid);
> >>>   err_mr:
> >>>        return err;
> >>>   }
> >>> @@ -2578,7 +2578,7 @@ static void mlx5_vdpa_set_status(struct
> >>> vdpa_device *vdev, u8 status)
> >>>        return;
> >>>
> >>>   err_setup:
> >>> -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >>> +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
> >>>        ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> >>>   err_clear:
> >>>        up_write(&ndev->reslock);
> >>> @@ -2604,7 +2604,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
> >>> *vdev)
> >>>        down_write(&ndev->reslock);
> >>>        teardown_driver(ndev);
> >>>        clear_vqs_ready(ndev);
> >>> -     mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >>> +     mlx5_vdpa_destroy_mr(&ndev->mvdev, true);
> >>>        ndev->mvdev.status = 0;
> >>>        ndev->cur_num_vqs = 0;
> >>>        ndev->mvdev.cvq.received_desc = 0;
> >>> @@ -2691,7 +2691,7 @@ static void mlx5_vdpa_free(struct vdpa_device
> >>> *vdev)
> >>>        ndev = to_mlx5_vdpa_ndev(mvdev);
> >>>
> >>>        free_resources(ndev);
> >>> -     mlx5_vdpa_destroy_mr(mvdev);
> >>> +     mlx5_vdpa_destroy_mr(mvdev, true);
> >>>        if (!is_zero_ether_addr(ndev->config.mac)) {
> >>>                pfmdev = pci_get_drvdata(pci_physfn(mvdev->mdev->pdev));
> >>>                mlx5_mpfs_del_mac(pfmdev, ndev->config.mac);
> >>> @@ -3214,7 +3214,7 @@ static int mlx5_vdpa_dev_add(struct
> >>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>   err_res2:
> >>>        free_resources(ndev);
> >>>   err_mr:
> >>> -     mlx5_vdpa_destroy_mr(mvdev);
> >>> +     mlx5_vdpa_destroy_mr(mvdev, true);
> >>>   err_res:
> >>>        mlx5_vdpa_free_resources(&ndev->mvdev);
> >>>   err_mpfs:
> >>> --
> >>> 2.31.1
>

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

[Eugenio Pérez]

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.

Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 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

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

[Eli Cohen]

> From: Eugenio Pérez <eperezma@redhat.com>
> Sent: Thursday, 12 January 2023 16:22
> To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> 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@redhat.com>
> ---
>  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

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

[Eugenio Perez Martin]

On Mon, Jan 16, 2023 at 8:13 AM Eli Cohen <elic@nvidia.com> wrote:
>
> > From: Eugenio Pérez <eperezma@redhat.com>
> > Sent: Thursday, 12 January 2023 16:22
> > To: mst@redhat.com; Eli Cohen <elic@nvidia.com>
> > Cc: linux-kernel@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> > lulu@redhat.com; jasowang@redhat.com; virtualization@lists.linux-
> > foundation.org; sgarzare@redhat.com; si-wei.liu@oracle.com
> > 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.

vring.c:iotlb_translate takes it.

> Question if it make sense that the iotlb could change while it is being referenced.
> Can you identify a specific case for this?
>

Not at this moment, because both are protected by
mlx5_vdpa_net->reslock before access or change iotlb. So this would
require changes to be exploitable, that's true.

However, to take the lock is the expected usage for vringh, so future
changes to either mlx or vringh could miss it.

Thanks!

> > Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC
> > setting")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  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
>