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

[Nicolin Chen <nicolinc@nvidia.com>]

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.

Thanks
Nicolin