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

[Baolu Lu]

On 2024/2/21 10:45, Zhangfei Gao wrote:
> 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.

I was just thinking about whether to do this in the iommu core, or in
the upper layers, like uacce or iommufd. It seems that there is no need
to attach a domain to a device or pasid again if it has already been
attached.

Best regards,
baolu