Messages in this thread Patch in this message | | | Date | Tue, 17 Jan 2023 09:07:57 +0200 | Subject | Re: [RFC 2/3] vdpa/mlx5: conditionally delete cvq iotlb in destroy_mr | From | 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
| |