Hi Baolu, On Wed, Sep 07, 2022 at 09:27:30AM +0800, Baolu Lu wrote: > Hi Jean, > > On 2022/9/7 0:36, Jean-Philippe Brucker wrote: > > On Tue, Sep 06, 2022 at 08:44:54PM +0800, Lu Baolu wrote: > > > +/** > > > + * iommu_sva_bind_device() - Bind a process address space to a device > > > + * @dev: the device > > > + * @mm: the mm to bind, caller must hold a reference to mm_users > > > + * > > > + * Create a bond between device and address space, allowing the device to access > > > + * the mm using the returned PASID. If a bond already exists between @device and > > > + * @mm, it is returned and an additional reference is taken. Caller must call > > > + * iommu_sva_unbind_device() to release each reference. > > This isn't true anymore. How about storing handle in the domain? > > Yes, agreed. How about making the comments like this: > > /** > * iommu_sva_bind_device() - Bind a process address space to a device > * @dev: the device > * @mm: the mm to bind, caller must hold a reference to mm_users > * > * Create a bond between device and address space, allowing the device to > * access the mm using the pasid returned by iommu_sva_get_pasid(). If a > * bond already exists between @device and @mm, an additional internal > * reference is taken. The reference will be released when the caller calls > * iommu_sva_unbind_device(). Sure, that works. I'd keep "Caller must call iommu_sva_unbind_device() to release each reference" > > Storing the handle in the domain looks odd. Conceptually an iommu domain > represents a hardware page table and the SVA handle represents a > relationship between device and the page table for a consumer. It's > better to make them separated. Right > > In a separated series, probably we can discuss the possibility of > removing handle from the driver APIs. Just simply return the sva domain > instead. > > struct iommu_domain *iommu_sva_bind_device(struct device *dev, > struct mm_struct *mm); > void iommu_sva_unbind_device(struct device *dev, > struct iommu_domain *domain); > u32 iommu_sva_get_pasid(struct iommu_domain *domain); > > If you think it's appropriate, I can send out the code for discussion. Yes, I don't see a reason to keep struct iommu_sva at the moment. I believe we needed to keep track of bonds themselves for sva_ops and driver data but those are gone now. Is iommu_domain still going to represent both a device context (whole PASID table) and individual address spaces, or are you planning to move away from that? What happens when a driver does: d1 = iommu_domain_alloc() iommu_attach_device(d1) d2 = iommu_sva_bind_device() iommu_detach_device(d1) Does detach (a) only disable the non-PASID address space? (b) disable everything? (c) fail because the driver didn't unbind first? I'm asking because the SMMU driver is still using smmu_domain to represent all address spaces + the non-PASID one, and using the same type "iommu_domain" for the new object makes things unreadable. I think internally we'll want to use distinct variable names, something like "domain" and "address_space". If (a) is not the direction you're going, then it may be worth renaming the API as well. I'm also not sure why set_dev_pasid() is a domain_ops of the SVA domain, but acts on the parent domain which contains the PASID table. Shouldn't it be an IOMMU op like remove_dev_pasid(), or on the parent domain? Thanks, Jean > > > > > (Maybe also drop my Reviewed-by tags since this has changed significantly, > > I tend to ignore patches that have them) > > I am sorry that after your review, the SVA domain and attach/detach > device pasid interfaces have undergone some changes. They mainly exist > in the following patches. Can you please help to take a look. > > iommu/sva: Refactoring iommu_sva_bind/unbind_device() > arm-smmu-v3/sva: Add SVA domain support > iommu: Add IOMMU SVA domain support > iommu: Add attach/detach_dev_pasid iommu interfaces > > Best regards, > baolu
