Re: [PATCH 1/2] vdpa/mlx5: Fix mr->initialized semantics

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 8/7/2023 8:00 PM, Jason Wang wrote:
> On Fri, Aug 4, 2023 at 1:58 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/3/2023 1:03 AM, Jason Wang wrote:
>>> On Thu, Aug 3, 2023 at 1:13 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>> 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 <gal@nvidia.com>
>>>> ---
>>>>    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->group2asid[MLX5_VDPA_CVQ_GROUP]);
>>>> +       mlx5_vdpa_destroy_mr_asid(mvdev, mvdev->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.
>>>
>>> One example is that, if we only do dup_iotlb() but not try to create
>>> dma mr here, we will break virtio-vdpa:
>> For this case, I guess we may need another way to support virtio-vdpa
>> 1:1 mapping rather than overloading virtio device reset semantics, see:
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg953755.html
>>
>>   > Conceptually, the address mapping is not a part of the abstraction for
>>   > a virtio device now. So resetting the memory mapping during virtio
>>   > device reset seems wrong.
>>
>> where we want to keep memory mapping intact across virtio device reset
>> for best live migration latency/downtime. I wonder would it work to
>> reset the mapping in vhost-vdpa life cycle out of virtio reset, say
>> introduce a .reset_map() op to restore 1:1 mapping within
>> vhost_vdpa_remove_as() right after vhost_vdpa_iotlb_unmap()? Then we can
>> move the iotlb reset logic to there without worry breaking virtio-vdpa.
> It looks to me we don't need a new ops. We can simply do set_map()
> twice
What does it mean, first set_map(0, -1ULL) with zero iotlb entry passed 
in to destroy all iotlb mappings previously added, and second set_map(0, 
-1ULL) to restore 1:1 DMA MR? But userspace (maybe a buggy one but 
doesn't do harm) apart from vhost-vdpa itself can do unmap twice anyway, 
this is supported today I think. Why there'll be such obscure 
distinction, or what's the benefit to treat second .set_map() as 
recreating 1:1 mapping?

>   or do you mean it would be faster?
I think with .reset_map() we at least can avoid indefinite latency 
hiccup from destroying and recreating 1:1 mapping with the unwarranted 
2rd unmap call. And .reset_map() should work with both .dma_map() and 
.set_map() APIs with clear semantics.

Regards,
-Siwei
>
> Thanks
>
>> Thanks,
>> -Siwei
>>
>>> commit 6f5312f801836e6af9bcbb0bdb44dc423e129206
>>> Author: Eli Cohen <elic@nvidia.com>
>>> Date:   Wed Jun 2 11:58:54 2021 +0300
>>>
>>>       vdpa/mlx5: Add support for running with virtio_vdpa
>>>
>>>       In order to support running vdpa using vritio_vdpa driver, we need  to
>>>       create a different kind of MR, one that has 1:1 mapping, since the
>>>       addresses referring to virtqueues are dma addresses.
>>>
>>>       We create the 1:1 MR in mlx5_vdpa_dev_add() only in case firmware
>>>       supports the general capability umem_uid_0. The reason for that is that
>>>       1:1 MRs must be created with uid == 0 while virtqueue objects can be
>>>       created with uid == 0 only when the firmware capability is on.
>>>
>>>       If the set_map() callback is called with new translations provided
>>>       through iotlb, the driver will destroy the 1:1 MR and create a regular
>>>       one.
>>>
>>>       Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>       Link: https://lore.kernel.org/r/20210602085854.62690-1-elic@nvidia.com
>>>       Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>       Acked-by: Jason Wang <jasowang@redhat.com>
>>>
>>> Thanks
>>>
>>>
>>>> +}
>>>> +
>>>> +static int _mlx5_vdpa_create_dvq_mr(struct mlx5_vdpa_dev *mvdev,
>>>> +                                   struct vhost_iotlb *iotlb,
>>>> +                                   unsigned int asid)
>>>>    {
>>>>           struct mlx5_vdpa_mr *mr = &mvdev->mr;
>>>>           int err;
>>>>
>>>> -       if (mr->initialized)
>>>> +       if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] != asid)
>>>>                   return 0;
>>>>
>>>> -       if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
>>>> -               if (iotlb)
>>>> -                       err = create_user_mr(mvdev, iotlb);
>>>> -               else
>>>> -                       err = create_dma_mr(mvdev, mr);
>>>> +       if (mr->initialized)
>>>> +               return 0;
>>>>
>>>> -               if (err)
>>>> -                       return err;
>>>> -       }
>>>> +       if (iotlb)
>>>> +               err = create_user_mr(mvdev, iotlb);
>>>> +       else
>>>> +               err = create_dma_mr(mvdev, mr);
>>>>
>>>> -       if (mvdev->group2asid[MLX5_VDPA_CVQ_GROUP] == asid) {
>>>> -               err = dup_iotlb(mvdev, iotlb);
>>>> -               if (err)
>>>> -                       goto out_err;
>>>> -       }
>>>> +       if (err)
>>>> +               return err;
>>>>
>>>>           mr->initialized = true;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int _mlx5_vdpa_create_mr(struct mlx5_vdpa_dev *mvdev,
>>>> +                               struct vhost_iotlb *iotlb, unsigned int asid)
>>>> +{
>>>> +       int err;
>>>> +
>>>> +       err = _mlx5_vdpa_create_dvq_mr(mvdev, iotlb, asid);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       err = _mlx5_vdpa_create_cvq_mr(mvdev, iotlb, asid);
>>>> +       if (err)
>>>> +               goto out_err;
>>>> +
>>>>           return 0;
>>>>
>>>>    out_err:
>>>> -       if (mvdev->group2asid[MLX5_VDPA_DATAVQ_GROUP] == asid) {
>>>> -               if (iotlb)
>>>> -                       destroy_user_mr(mvdev, mr);
>>>> -               else
>>>> -                       destroy_dma_mr(mvdev, mr);
>>>> -       }
>>>> +       _mlx5_vdpa_destroy_dvq_mr(mvdev, asid);
>>>>
>>>>           return err;
>>>>    }
>>>> --
>>>> 2.41.0
>>>>
>>> _______________________________________________
>>> Virtualization mailing list
>>> Virtualization@lists.linux-foundation.org
>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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