Re: [PATCH 5/5] iommu: Retire group->domain

[Yi Liu <yi.l.liu@intel.com>]

On 2025/2/19 21:10, Jason Gunthorpe wrote:
> On Wed, Feb 19, 2025 at 08:31:52PM +0800, Yi Liu wrote:
>>>> And then iommu_get_domain_for_dev() which explains it is an older
>>>> function that depends on the caller somehow ensuring that the attached
>>>> domain cannot change. Ideally we would someday try to remove calls to
>>>> iommu_get_domain_for_dev()..
>>
>> currently, the paths hold group->mutex are mostly in the iommu drivers.
>> So what we may do is updating the iommu drivers to use a version that
>> has lockdep. While other callers keep using the iommu_get_domain_for_dev().
> 
> Honestly I would prefer to pass the old domain in as a function argument to
> attach and remove the calls entirely.

hmmm. I may drop the retire group->domain from this series to avoid
blocking the iommufd pasid series.

>>>> Since the xarray cannot change for non-lockdepable reasons, it can
>>>> just call xa_load(). Probably re-organize iommu_group_domain() to have
>>>> a __iommu_group_domain() which has the body but no lockdep assertion.
>>
>> another approach may be holding xa_lock when retrieving domain from xarray
>> in the iommu_get_domain_for_dev() helper. This approach is consistent with
>> the usage pattern of iommu_attach_handle_get(), where callers also do not
>> acquire the group->mutex. Thoughts?
> 
> It doesn't matter, the pointer is unsafe the instant the xa_lock is
> released.
> 
> The locking works in most places that don't hold the group mutex
> because they rely on a driver being probed and the dma api keeping the
> domain constant during that period.

yes. Since group->domain is set/clear under group->mutex as well. So I'm
wondering if it is ok to add kdoc to iommu_get_domain_for_dev() to note
it, and just add the xa_load && entry_decode stuffs without lockdep?

-- 
Regards,
Yi Liu