> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx> > Sent: Thursday, June 6, 2024 2:07 PM > > On 6/5/24 4:15 PM, Tian, Kevin wrote: > >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> > >> Sent: Monday, May 27, 2024 12:05 PM > >> > >> - list_for_each_entry(handle, &mm->iommu_mm->sva_handles, > >> handle_item) { > >> - if (handle->dev == dev) { > >> - refcount_inc(&handle->users); > >> - mutex_unlock(&iommu_sva_lock); > >> - return handle; > >> - } > >> + /* A bond already exists, just take a reference`. */ > >> + attach_handle = iommu_attach_handle_get(group, iommu_mm- > >>> pasid, IOMMU_DOMAIN_SVA); > >> + if (!IS_ERR(attach_handle)) { > >> + handle = container_of(attach_handle, struct iommu_sva, > >> handle); > >> + refcount_inc(&handle->users); > >> + mutex_unlock(&iommu_sva_lock); > >> + return handle; > >> } > > > > It's counter-intuitive to move forward when an error is returned. > > > > e.g. if it's -EBUSY indicating the pasid already used for another type then > > following attempts shouldn't been tried. > > > > probably we should have iommu_attach_handle_get() return NULL > > instead of -ENOENT when the entry is free? then: > > > > attach_handle = iommu_attach_handle_get(); > > if (IS_ERR(attach_handle)) { > > ret = PTR_ERR(attach_handle); > > goto out_unlock; > > } else if (attach_handle) { > > /* matched and increase handle->users */ > > } > > > > /* free entry falls through */ > > But then there is one potential issue with the design that 'handle' > > can be optional in iommu_attach_device_pasid(). In that case > > xa_load returns NULL then we cannot differentiate a real unused > > PASID vs. one which has been attached w/o an handle. > > The PASID should be allocated exclusively. This means that once a PASID > is assigned to A, it shouldn't be assigned to B at the same time. If a > single PASID is used for multiple purposes, it's likely a bug in the > system. yes there is a bug but catching it here would make diagnostic easier. > > So the logic of iommu_attach_handle_get() here is: has an SVA domain > already been installed for this PASID? If so, just reuse it. Otherwise, > try to install a new SVA domain. > > > Does it suggest that having the caller to always provide a handle > > makes more sense? > > I'm neutral on this since only sva bind and iopf path delivery currently > require an attach handle. > let's hear Jason's opinion.