Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains

[Zhangfei Gao]

On Wed, 21 Feb 2024 at 10:06, Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
>
> On 2024/2/21 9:28, Zhangfei Gao wrote:
> > On Wed, 21 Feb 2024 at 07:58, Zhang, Tina<tina.zhang@xxxxxxxxx>  wrote:
> >
> >>>>   struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct
> >>>> mm_struct *mm)  {
> >>>> +       struct iommu_mm_data *iommu_mm;
> >>>>          struct iommu_domain *domain;
> >>>>          struct iommu_sva *handle;
> >>>>          int ret;
> >>>>
> >>>> +       mutex_lock(&iommu_sva_lock);
> >>>> +
> >>>>          /* Allocate mm->pasid if necessary. */
> >>>> -       ret = iommu_sva_alloc_pasid(mm, dev);
> >>>> -       if (ret)
> >>>> -               return ERR_PTR(ret);
> >>>> +       iommu_mm = iommu_alloc_mm_data(mm, dev);
> >>>> +       if (IS_ERR(iommu_mm)) {
> >>>> +               ret = PTR_ERR(iommu_mm);
> >>>> +               goto out_unlock;
> >>>> +       }
> >>>>
> >>>>          handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> >>>> -       if (!handle)
> >>>> -               return ERR_PTR(-ENOMEM);
> >>>> -
> >>>> -       mutex_lock(&iommu_sva_lock);
> >>>> -       /* Search for an existing domain. */
> >>>> -       domain = iommu_get_domain_for_dev_pasid(dev, mm->pasid,
> >>>> -                                               IOMMU_DOMAIN_SVA);
> >>>> -       if (IS_ERR(domain)) {
> >>>> -               ret = PTR_ERR(domain);
> >>>> +       if (!handle) {
> >>>> +               ret = -ENOMEM;
> >>>>                  goto out_unlock;
> >>>>          }
> >>>>
> >>>> -       if (domain) {
> >>>> -               domain->users++;
> >>>> -               goto out;
> >>> Our multi bind test case broke since 6.8-rc1.
> >>> The test case can use same domain & pasid, return different handle,
> >>> 6.7 simply  domain->users ++ and return.
> >>>
> >>>> +       /* Search for an existing domain. */
> >>>> +       list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next)
> >>> {
> >>>> +               ret = iommu_attach_device_pasid(domain, dev,
> >>>> + iommu_mm->pasid);
> >>> Now iommu_attach_device_pasid return BUSY since the same pasid.
> >>> And then iommu_sva_bind_device attach ret=-16
> >> Sounds like the test case tries to bind a device to a same mm multiple times without unbinding the device and the expectation is that it can always return a valid handle to pass the test. Right?
> > Yes
> >
> > The device can bind to the same mm multi-times and return different handle,
> > Since the refcount, no need to unbind and bind sequently,
> > The unbind can happen later with the handle.
>
> Is there any real use case to bind an mm to the pasid of a device
> multiple times? If there are cases, is it better to handle this in the
> uacce driver?

Yes, it is required for multi-thread, the device can provide
multi-queue to speed up.

>
>  From iommu core's perspective, it doesn't make sense to attach the same
> domain to the same device (or pasid) multiple times.

But is it the refcount domain->user++ used for?
Is there any reason not doing this.

Thanks