Re: [PATCH v6 04/14] iommufd: Always pass iommu_attach_handle to iommu core

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

On 2024/12/20 12:35, Nicolin Chen wrote:
> Hi Yi,
> 
> On Thu, Dec 19, 2024 at 05:27:36AM -0800, Yi Liu wrote:
>> The iommu_attach_handle is optional in the RID attach/replace API and the
>> PASID attach APIs. But it is a mandatory argument for the PASID replace API.
>> Without it, the PASID replace path cannot get the old domain. Hence, the
>> PASID path (attach/replace) requires the attach handle. As iommufd is the
>> major user of the RID attach/replace with iommu_attach_handle, this also
>> makes the iommufd always pass the attach handle for the RID path as well.
>> This keeps the RID and PASID path much aligned.
> 
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index d5c83f10d83e..6cf9c1f10e85 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -493,28 +493,42 @@ static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
>>   	if (hwpt->fault)
>>   		return iommufd_fault_domain_attach_dev(hwpt, idev);
>>   
>> -	return iommu_attach_group(hwpt->domain, idev->igroup->group);
>> +	return iommufd_dev_attach_handle(hwpt, idev);
>>   }
>>   
>>   static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
>>   					      struct iommufd_device *idev)
>>   {
>> +	struct iommufd_attach_handle *handle;
>> +
>>   	if (hwpt->fault) {
>>   		iommufd_fault_domain_detach_dev(hwpt, idev);
>>   		return;
>>   	}
>>   
>> -	iommu_detach_group(hwpt->domain, idev->igroup->group);
>> +	handle = iommufd_device_get_attach_handle(idev);
>> +	iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
>> +	kfree(handle);
>>   }
>>   
>>   static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
>>   					      struct iommufd_hw_pagetable *hwpt,
>>   					      struct iommufd_hw_pagetable *old)
>>   {
>> +	struct iommufd_attach_handle *curr;
>> +	int ret;
>> +
>>   	if (old->fault || hwpt->fault)
>>   		return iommufd_fault_domain_replace_dev(idev, hwpt, old);
>>   
>> -	return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
>> +	curr = iommufd_device_get_attach_handle(idev);
>> +
>> +	ret = iommufd_dev_replace_handle(idev, hwpt, old);
>> +	if (ret)
>> +		return ret;
> 
> These inline functions feel heavier after this rework..
> 
> I actually have the same patch for a different reason, yet haven't
> sent yet:
> https://github.com/nicolinc/iommufd/commits/wip/iommufd_msi_p1-v1
>     iommu: Turn fault_data to iommufd private pointer
>   =>iommufd: Make attach_handle generic
> 
> I think we can align with each other on these functions with one
> common patch: my series requires the fault_data and attach_handle
> are common to all HWPTs, v.s. exclusive to an hwpt->fault.

yeah, I'm ok to move the helpers to be in device.c to avoid inline.
I've three remark on your patch [1]. I'm now also considering to do
the work in one patch, hence it might be better for review w.r.t the
comment from Baolu [2].

[1] 
https://github.com/nicolinc/iommufd/commit/26f9a2a4a5e28574295ecfe057e949dcc9a58c7e#diff-c251419ce7f13e1e22e8fe08864a50d8541d6c9addaee27f806bf9786b4a4886R374
[2] 
https://lore.kernel.org/linux-iommu/a2f363e7-88e4-46ef-8755-8a5e0cc47ecc@linux.intel.com/

-- 
Regards,
Yi Liu