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@xxxxxxxxxx> wrote:
>
>
> On 16/01/2023 20:03, Eugenio Perez Martin wrote:
> > On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <elic@xxxxxxxxxx> wrote:
> >>> 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 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@xxxxxxxxxx>
> >>> ---
> >>>   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
>