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 <[email protected]>
---
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
> From: Eugenio Pérez <[email protected]>
> Sent: Thursday, 12 January 2023 16:22
> To: [email protected]; Eli Cohen <[email protected]>
> Cc: [email protected]; Parav Pandit <[email protected]>;
> [email protected]; [email protected]; [email protected]
> foundation.org; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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
On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <[email protected]> wrote:
>
> > From: Eugenio Pérez <[email protected]>
> > Sent: Thursday, 12 January 2023 16:22
> > To: [email protected]; Eli Cohen <[email protected]>
> > Cc: [email protected]; Parav Pandit <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > foundation.org; [email protected]; [email protected]
> > 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 <[email protected]>
> > ---
> > 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
>
On 16/01/2023 20:03, Eugenio Perez Martin wrote:
> On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <[email protected]> wrote:
>>> From: Eugenio Pérez <[email protected]>
>>> Sent: Thursday, 12 January 2023 16:22
>>> To: [email protected]; Eli Cohen <[email protected]>
>>> Cc: [email protected]; Parav Pandit <[email protected]>;
>>> [email protected]; [email protected]; [email protected]
>>> foundation.org; [email protected]; [email protected]
>>> 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 <[email protected]>
>>> ---
>>> 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
On Tue, Jan 17, 2023 at 8:08 AM Eli Cohen <[email protected]> wrote:
>
>
> On 16/01/2023 20:03, Eugenio Perez Martin wrote:
> > On Mon, Jan 16, 2023 at 8:03 AM Eli Cohen <[email protected]> wrote:
> >>> From: Eugenio Pérez <[email protected]>
> >>> Sent: Thursday, 12 January 2023 16:22
> >>> To: [email protected]; Eli Cohen <[email protected]>
> >>> Cc: [email protected]; Parav Pandit <[email protected]>;
> >>> [email protected]; [email protected]; [email protected]
> >>> foundation.org; [email protected]; [email protected]
> >>> 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 <[email protected]>
> >>> ---
> >>> 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
>