On Wed, 2023-08-09 at 09:42 +0800, Jason Wang wrote:

On Tue, Aug 8, 2023 at 3:24 PM Dragos Tatulea [off-list ref] wrote:

quoted

On Tue, 2023-08-08 at 10:57 +0800, Jason Wang wrote:

quoted

On Thu, Aug 3, 2023 at 7:40 PM Dragos Tatulea [off-list ref] wrote:

quoted

On Thu, 2023-08-03 at 16:03 +0800, Jason Wang wrote:

quoted

On Thu, Aug 3, 2023 at 1:13 AM Dragos Tatulea [off-list ref]
wrote:

quoted

The mr->initialized flag is shared between the control vq and data
vq
part of the mr init/uninit. But if the control vq and data vq get
placed
in different ASIDs, it can happen that initializing the control vq
will
prevent the data vq mr from being initialized.

This patch consolidates the control and data vq init parts into
their
own init functions. The mr->initialized will now be used for the
data vq
only. The control vq currently doesn't need a flag.

The uninitializing part is also taken care of: mlx5_vdpa_destroy_mr
got
split into data and control vq functions which are now also ASID
aware.

Fixes: 8fcd20c30704 ("vdpa/mlx5: Support different address spaces
for
control and data")
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Gal Pressman <redacted>
---
 drivers/vdpa/mlx5/core/mlx5_vdpa.h |  1 +
 drivers/vdpa/mlx5/core/mr.c        | 97 +++++++++++++++++++++------
---
 2 files changed, 71 insertions(+), 27 deletions(-)

diff --git a/drivers/vdpa/mlx5/core/mlx5_vdpa.h

b/drivers/vdpa/mlx5/core/mlx5_vdpa.h
index 25fc4120b618..a0420be5059f 100644

--- a/drivers/vdpa/mlx5/core/mlx5_vdpa.h
+++ b/drivers/vdpa/mlx5/core/mlx5_vdpa.h

@@ -31,6 +31,7 @@ struct mlx5_vdpa_mr {

        struct list_head head;
        unsigned long num_directs;
        unsigned long num_klms;
+       /* state of dvq mr */
        bool initialized;

        /* serialize mkey creation and destruction */

diff --git a/drivers/vdpa/mlx5/core/mr.c

b/drivers/vdpa/mlx5/core/mr.c
index 03e543229791..4ae14a248a4b 100644

--- a/drivers/vdpa/mlx5/core/mr.c
+++ b/drivers/vdpa/mlx5/core/mr.c

@@ -489,60 +489,103 @@ 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)
+static void _mlx5_vdpa_destroy_cvq_mr(struct mlx5_vdpa_dev *mvdev,
unsigned
int asid)
+{
+       if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] != asid)
+               return;
+
+       prune_iotlb(mvdev);
+}
+
+static void _mlx5_vdpa_destroy_dvq_mr(struct mlx5_vdpa_dev *mvdev,
unsigned
int asid)
 {
        struct mlx5_vdpa_mr *mr = &mvdev->mr;

-       mutex_lock(&mr->mkey_mtx);
+       if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] != asid)
+               return;
+
        if (!mr->initialized)
-               goto out;
+               return;

-       prune_iotlb(mvdev);
        if (mr->user_mr)
                destroy_user_mr(mvdev, mr);
        else
                destroy_dma_mr(mvdev, mr);

        mr->initialized = false;
-out:
+}
+
+static void mlx5_vdpa_destroy_mr_asid(struct mlx5_vdpa_dev *mvdev,
unsigned
int asid)
+{
+       struct mlx5_vdpa_mr *mr = &mvdev->mr;
+
+       mutex_lock(&mr->mkey_mtx);
+
+       _mlx5_vdpa_destroy_dvq_mr(mvdev, asid);
+       _mlx5_vdpa_destroy_cvq_mr(mvdev, asid);
+
        mutex_unlock(&mr->mkey_mtx);
 }

-static 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)
+{
+       mlx5_vdpa_destroy_mr_asid(mvdev, mvdev-

quoted

group2asid[MLX5_VDPA_CVQ_GROUP]);

+       mlx5_vdpa_destroy_mr_asid(mvdev, mvdev-

quoted

group2asid[MLX5_VDPA_DATAVQ_GROUP]);

+}
+
+static int _mlx5_vdpa_create_cvq_mr(struct mlx5_vdpa_dev *mvdev,
+                                   struct vhost_iotlb *iotlb,
+                                   unsigned int asid)
+{
+       if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] != asid)
+               return 0;
+
+       return dup_iotlb(mvdev, iotlb);

This worries me as conceptually, there should be no difference between
dvq mr and cvq mr. The virtqueue should be loosely coupled with mr.

Are you worried by the changes in this patch or about the possibility of
having

The reason for this change is that I noticed if you create one mr in one
asid
you could be blocked out from creating another one in a different asid
due
to
mr->initialized being true. To me that seemed problematic. Is it not?

My feeling is that mr.c should be device agnostic. It needs to know
nothing about the device details to work. But this patch seems to
break the layer.

But the same logic was there before (with the exception of cvq not having an
init flag anymore). So what am I missing here?

Nothing, I think you're right.

I think we can have this patch go first and tweak on top by moving CVQ
aware logic into the net specific codes.

Is this anything more than a re-org? My plan is to move the cvq mr part from
mlx5_vdpa_dev into mlx5_vdpa_net. Is there anything else that you were expecting
here?

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