Re: [PATCH v4 2/9] iommu: Replace sva_iommu with iommu_attach_handle

[Baolu Lu <baolu.lu@linux.intel.com>]

On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
> On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
>> On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
>>> On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
>>>> +	/* A bond already exists, just take a reference`. */
>>>> +	handle = iommu_attach_handle_get(group, iommu_mm->pasid);
>>>> +	if (handle) {
>>>> +		mutex_unlock(&iommu_sva_lock);
>>>> +		return handle;
>>>>    	}
>>> At least in this context this is not enough we need to ensure that the
>>> domain on the PASID is actually an SVA domain and it was installed by
>>> this mechanism, not an iommufd domain for instance.
>>>
>>> ie you probably need a type field in the iommu_attach_handle to tell
>>> what the priv is.
>>>
>>> Otherwise this seems like a great idea!
>> Yes, you are right. For the SVA case, I will add the following changes.
>> The IOMMUFD path will also need such enhancement. I will update it in
>> the next version.
> The only use for this is the PRI callbacks right? Maybe instead of
> adding a handle type let's just check domain->iopf_handler  ?
> 
> Ie SVA will pass &ommu_sva_iopf_handler as its "type"

Sorry that I don't fully understand the proposal here.

We need to get the attach handle at least in below cases:

1. In the iommu_sva_bind_device() path so that the existing bind could
    be reused.

2. In the iommu_report_device_fault() path so that the context-specific
    data could be used in the fault handler.

The problem is that the context code (SVA, IOMMUFD, etc.) needs to make
sure that the attach handle is really what it has installed during
domain attachment. The context code needs some mechanism to include some
kind of "owner cookie" in the attach handle, so that it could check
against it later for valid use.

Best regards,
baolu