Messages in this thread | ![/](/images/icornerl.gif) | | From | Eugenio Perez Martin <> | Date | Tue, 17 Jan 2023 08:25:18 +0100 | Subject | Re: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr |
| |
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 >
| ![\](/images/icornerr.gif) |