| Message ID | 20240912131255.13305-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
> 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. > --- 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?
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.
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index cd56745f3003..50ce6c6e61be 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -55,25 +55,6 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) mutex_unlock(&idev->iopf_lock); } -static int __fault_domain_attach_dev(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; -} - int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -86,7 +67,7 @@ int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt, if (ret) return ret; - ret = __fault_domain_attach_dev(hwpt, idev); + ret = iommufd_dev_attach_handle(hwpt, idev); if (ret) iommufd_fault_iopf_disable(idev); @@ -122,18 +103,6 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, mutex_unlock(&fault->mutex); } -static 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); -} - void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { @@ -146,32 +115,6 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt, kfree(handle); } -/* Caller to free the old iommufd_attach_handle */ -static struct iommufd_attach_handle * -__fault_domain_replace_dev(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; -} - int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt, struct iommufd_hw_pagetable *old) @@ -188,7 +131,7 @@ int iommufd_fault_domain_replace_dev(struct iommufd_device *idev, } if (hwpt->fault) { - curr = __fault_domain_replace_dev(idev, hwpt, old); + curr = iommufd_dev_replace_handle(idev, hwpt, old); ret = IS_ERR(curr) ? PTR_ERR(curr) : 0; } else { ret = iommu_replace_group_handle(idev->igroup->group, diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 1141c0633dc9..7039c86d9981 100644 --- 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; +} + static inline struct iommufd_fault * iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id) {
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 Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/iommufd/fault.c | 61 +------------------------ drivers/iommu/iommufd/iommufd_private.h | 57 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 59 deletions(-)