Re: [PATCH RFCv2 06/13] iommufd: Make attach_handle generic

From: Eric Auger
Date: Wed Jan 29 2025 - 08:15:16 EST

Next message: Ayush Singh: "[PATCH v2] arm64: defconfig: Enable gb_beagleplay"
Previous message: Robert Richter: "Re: [PATCH v1 06/29] cxl/region: Factor out code to find the root decoder"
In reply to: Yi Liu: "Re: [PATCH RFCv2 06/13] iommufd: Make attach_handle generic"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 1/11/25 4:32 AM, Nicolin Chen wrote:
> "attach_handle" was added exclusively for the iommufd_fault_iopf_handler()
> used by IOPF/PRI use cases, along with the "fault_data". Now, the iommufd
> version of sw_msi function will resue the attach_handle and fault_data for
reuse
> a non-fault case.
>
> Move the attach_handle part out of the fault.c file to make it generic for
> all cases. Simplify the remaining fault specific routine to attach/detach.
>
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 40 +-------
> drivers/iommu/iommufd/device.c | 105 +++++++++++++++++++++
> drivers/iommu/iommufd/fault.c | 120 +++---------------------
> 3 files changed, 122 insertions(+), 143 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index b6d706cf2c66..063c0a42f54f 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -472,42 +472,12 @@ void iommufd_fault_destroy(struct iommufd_object *obj);
> int iommufd_fault_iopf_handler(struct iopf_group *group);
>
> int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_device *idev);
> + struct iommufd_device *idev,
> + bool enable_iopf);
> void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_device *idev);
> -int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
> - struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_hw_pagetable *old);
> -
> -static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_device *idev)
> -{
> - if (hwpt->fault)
> - return iommufd_fault_domain_attach_dev(hwpt, idev);
> -
> - return iommu_attach_group(hwpt->domain, idev->igroup->group);
> -}
> -
> -static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_device *idev)
> -{
> - if (hwpt->fault) {
> - iommufd_fault_domain_detach_dev(hwpt, idev);
> - return;
> - }
> -
> - iommu_detach_group(hwpt->domain, idev->igroup->group);
> -}
> -
> -static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
> - struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_hw_pagetable *old)
> -{
> - if (old->fault || hwpt->fault)
> - return iommufd_fault_domain_replace_dev(idev, hwpt, old);
> -
> - return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
> -}
> + struct iommufd_device *idev,
> + struct iommufd_attach_handle *handle,
> + bool disable_iopf);
>
> static inline struct iommufd_viommu *
> iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id)
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index dfd0898fb6c1..38b31b652147 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -352,6 +352,111 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
> return 0;
> }
>
> +/* The device attach/detach/replace helpers for attach_handle */
> +
> +static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev)
> +{
> + struct iommufd_attach_handle *handle;
> + int rc;
> +
> + if (hwpt->fault) {
> + rc = iommufd_fault_domain_attach_dev(hwpt, idev, true);
why don't we simply call iommufd_fault_iopf_enable(idev)
also it looks there is a redundant check of hwpt_fault here and in

iommufd_fault_domain_attach_dev

Besides the addition of enable_iopf param is not documented anywhere

> + if (rc)
> + return rc;
> + }
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle) {
> + rc = -ENOMEM;
> + goto out_fault_detach;
> + }
> +
> + handle->idev = idev;
> + rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,
> + &handle->handle);
> + if (rc)
> + goto out_free_handle;
> +
> + return 0;
> +
> +out_free_handle:
> + kfree(handle);
> + handle = NULL;
> +out_fault_detach:
> + if (hwpt->fault)
> + iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
> + return rc;
> +}
> +
> +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);
> +}
> +
> +static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev)
> +{
> + struct iommufd_attach_handle *handle;
> +
> + handle = iommufd_device_get_attach_handle(idev);
> + iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
> + if (hwpt->fault)
> + iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
same here, pretty difficult to understand what this

iommufd_fault_domain_detach_dev does
To me calling iommufd_auto_response_faults and iommufd_fault_iopf_disable would be more readable or rename iommufd_fault_domain_detach_dev().
Also compared to the original code, there is a new check on handle. Why is it necessary.

Globally I feel that patch pretty hard to read. Would be nice to split if possible to ease the review process.

Thanks

Eric

> + kfree(handle);
> +}
> +
> +static int iommufd_hwpt_replace_device(struct iommufd_device *idev,
> + struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_hw_pagetable *old)
> +{
> + struct iommufd_attach_handle *old_handle =
> + iommufd_device_get_attach_handle(idev);
> + struct iommufd_attach_handle *handle;
> + int rc;
> +
> + if (hwpt->fault) {
> + rc = iommufd_fault_domain_attach_dev(hwpt, idev, !old->fault);
> + if (rc)
> + return rc;
> + }
> +
> + handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> + if (!handle) {
> + rc = -ENOMEM;
> + goto out_fault_detach;
> + }
> +
> + handle->idev = idev;
> + rc = iommu_replace_group_handle(idev->igroup->group, hwpt->domain,
> + &handle->handle);
> + if (rc)
> + goto out_free_handle;
> +
> + if (old->fault)
> + iommufd_fault_domain_detach_dev(old, idev, old_handle,
> + !hwpt->fault);
> + kfree(old_handle);
> +
> + return 0;
> +
> +out_free_handle:
> + kfree(handle);
> + handle = NULL;
> +out_fault_detach:
> + if (hwpt->fault)
> + iommufd_fault_domain_detach_dev(hwpt, idev, handle,
> + !old->fault);
> + return rc;
> +}
> +
> int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> struct iommufd_device *idev)
> {
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 06aa83a75e94..1d9bd3024b57 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -60,42 +60,17 @@ 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)
> + struct iommufd_device *idev,
> + bool enable_iopf)
> {
> - int ret;
> + int rc = 0;
>
> if (!hwpt->fault)
> return -EINVAL;
> -
> - ret = iommufd_fault_iopf_enable(idev);
> - if (ret)
> - return ret;
> -
> - ret = __fault_domain_attach_dev(hwpt, idev);
> - if (ret)
> - iommufd_fault_iopf_disable(idev);
> -
> - return ret;
> + if (enable_iopf)
> + rc = iommufd_fault_iopf_enable(idev);
> + return rc;
> }
>
> static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt,
> @@ -127,86 +102,15 @@ 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)
> + struct iommufd_device *idev,
> + struct iommufd_attach_handle *handle,
> + bool disable_iopf)
> {
> - struct iommufd_attach_handle *handle;
> -
> - handle = iommufd_device_get_attach_handle(idev);
> - iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
> - iommufd_auto_response_faults(hwpt, handle);
> - iommufd_fault_iopf_disable(idev);
> - kfree(handle);
> -}
> -
> -static int __fault_domain_replace_dev(struct iommufd_device *idev,
> - struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_hw_pagetable *old)
> -{
> - struct iommufd_attach_handle *handle, *curr = NULL;
> - int ret;
> -
> - if (old->fault)
> - curr = iommufd_device_get_attach_handle(idev);
> -
> - if (hwpt->fault) {
> - handle = kzalloc(sizeof(*handle), GFP_KERNEL);
> - if (!handle)
> - return -ENOMEM;
> -
> - handle->idev = idev;
> - ret = iommu_replace_group_handle(idev->igroup->group,
> - hwpt->domain, &handle->handle);
> - } else {
> - ret = iommu_replace_group_handle(idev->igroup->group,
> - hwpt->domain, NULL);
> - }
> -
> - if (!ret && curr) {
> - iommufd_auto_response_faults(old, curr);
> - kfree(curr);
> - }
> -
> - return ret;
> -}
> -
> -int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
> - struct iommufd_hw_pagetable *hwpt,
> - struct iommufd_hw_pagetable *old)
> -{
> - bool iopf_off = !hwpt->fault && old->fault;
> - bool iopf_on = hwpt->fault && !old->fault;
> - int ret;
> -
> - if (iopf_on) {
> - ret = iommufd_fault_iopf_enable(idev);
> - if (ret)
> - return ret;
> - }
> -
> - ret = __fault_domain_replace_dev(idev, hwpt, old);
> - if (ret) {
> - if (iopf_on)
> - iommufd_fault_iopf_disable(idev);
> - return ret;
> - }
> -
> - if (iopf_off)
> + if (handle)
> + iommufd_auto_response_faults(hwpt, handle);
> + if (disable_iopf)
> iommufd_fault_iopf_disable(idev);
> -
> - return 0;
> }
>
> void iommufd_fault_destroy(struct iommufd_object *obj)

Next message: Ayush Singh: "[PATCH v2] arm64: defconfig: Enable gb_beagleplay"
Previous message: Robert Richter: "Re: [PATCH v1 06/29] cxl/region: Factor out code to find the root decoder"
In reply to: Yi Liu: "Re: [PATCH RFCv2 06/13] iommufd: Make attach_handle generic"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]