On 2024/9/30 15:44, Tian, Kevin wrote:
From: Yi Liu yi.l.liu@intel.com Sent: Thursday, September 12, 2024 9:13 PM
iommufd plans to always pass in an iommu_attach_handle to the iommu core, so it's no longer fault specific, hence move the helpers out of the fault.c
again please put the reason for why iommufd plans to do so.
how about the below?
The iommu_attach_handle tracks the attached domains in the group->pasid_array, it is optional so far. But to support the domain replacement for a pasid, it needs to get the attached domain, so making iommufd always pass a handle is a choice. Before that, we would need to decouple the handle allocation and fault code as non-fault capable hwpt attach would also allocate handle.
--- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -458,6 +458,63 @@ struct iommufd_attach_handle { /* Convert an iommu attach handle to iommufd handle. */ #define to_iommufd_handle(hdl) container_of(hdl, struct iommufd_attach_handle, handle)
+static inline struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{
- struct iommu_attach_handle *handle;
- handle = iommu_attach_handle_get(idev->igroup->group,
IOMMU_NO_PASID, 0);
- if (IS_ERR(handle))
return NULL;- return to_iommufd_handle(handle);
+}
+static inline int iommufd_dev_attach_handle(struct iommufd_hw_pagetable *hwpt,
struct iommufd_device *idev)+{
- struct iommufd_attach_handle *handle;
- int ret;
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return -ENOMEM;- handle->idev = idev;
- ret = iommu_attach_group_handle(hwpt->domain, idev->igroup-
group,
&handle->handle);- if (ret)
kfree(handle);- return ret;
+}
+/* Caller to free the old iommufd_attach_handle */ +static inline struct iommufd_attach_handle * +iommufd_dev_replace_handle(struct iommufd_device *idev,
struct iommufd_hw_pagetable *hwpt,struct iommufd_hw_pagetable *old)+{
- struct iommufd_attach_handle *handle, *curr;
- int ret;
- curr = iommufd_device_get_attach_handle(idev);
- handle = kzalloc(sizeof(*handle), GFP_KERNEL);
- if (!handle)
return ERR_PTR(-ENOMEM);- handle->idev = idev;
- ret = iommu_replace_group_handle(idev->igroup->group,
hwpt->domain, &handle->handle);- if (ret) {
kfree(handle);return ERR_PTR(ret);- }
- return curr;
+}
why putting them in header file instead of C file?
put it in device.c is also fine. :) but not in the fault.c as the non fault path also needs to use handle.