Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace

From: Joel Granados
Date: Tue Feb 20 2024 - 08:58:19 EST

Next message: Jonathan Cameron: "Re: [RFC PATCH v6 10/12] ACPICA: ACPI 6.5: Add support for RAS2 table"
Previous message: Qais Yousef: "Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom"
Next in thread: Baolu Lu: "Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jan 22, 2024 at 03:39:01PM +0800, Lu Baolu wrote:
> The iopf-capable hw page table attach/detach/replace should use the iommu
> iopf-specific interfaces. The pointer to iommufd_device is stored in the
> private field of the attachment cookie, so that it can be easily retrieved
> in the fault handling paths. The references to iommufd_device and
> iommufd_hw_pagetable objects are held until the cookie is released, which
> happens after the hw_pagetable is detached from the device and all
> outstanding iopf's are responded to. This guarantees that both the device
> and hw_pagetable are valid before domain detachment and outstanding faults
> are handled.
>
> The iopf-capable hw page tables can only be attached to devices that
> support the IOMMU_DEV_FEAT_IOPF feature. On the first attachment of an
> iopf-capable hw_pagetable to the device, the IOPF feature is enabled on
> the device. Similarly, after the last iopf-capable hwpt is detached from
> the device, the IOPF feature is disabled on the device.
>
> The current implementation allows a replacement between iopf-capable and
> non-iopf-capable hw page tables. This matches the nested translation use
> case, where a parent domain is attached by default and can then be
> replaced with a nested user domain with iopf support.
>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 7 ++
> drivers/iommu/iommufd/device.c | 15 ++-
> drivers/iommu/iommufd/fault.c | 122 ++++++++++++++++++++++++
> 3 files changed, 141 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 2780bed0c6b1..9844a1289c01 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -398,6 +398,7 @@ struct iommufd_device {
> /* always the physical device */
> struct device *dev;
> bool enforce_cache_coherency;
> + bool iopf_enabled;
> /* outstanding faults awaiting response indexed by fault group id */
> struct xarray faults;
> };
> @@ -459,6 +460,12 @@ iommufd_get_fault(struct iommufd_ucmd *ucmd, u32 id)
> int iommufd_fault_alloc(struct iommufd_ucmd *ucmd);
> 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);
> +void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev);
> +int iommufd_fault_domain_replace_dev(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev);
>
> #ifdef CONFIG_IOMMUFD_TEST
> int iommufd_test(struct iommufd_ucmd *ucmd);
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index d70913ee8fdf..c4737e876ebc 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -377,7 +377,10 @@ int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
> * attachment.
> */
> if (list_empty(&idev->igroup->device_list)) {
> - rc = iommu_attach_group(hwpt->domain, idev->igroup->group);
> + if (hwpt->fault_capable)
> + rc = iommufd_fault_domain_attach_dev(hwpt, idev);
> + else
> + rc = iommu_attach_group(hwpt->domain, idev->igroup->group);
> if (rc)
> goto err_unresv;
> idev->igroup->hwpt = hwpt;
> @@ -403,7 +406,10 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev)
> mutex_lock(&idev->igroup->lock);
> list_del(&idev->group_item);
> if (list_empty(&idev->igroup->device_list)) {
> - iommu_detach_group(hwpt->domain, idev->igroup->group);
> + if (hwpt->fault_capable)
> + iommufd_fault_domain_detach_dev(hwpt, idev);
> + else
> + iommu_detach_group(hwpt->domain, idev->igroup->group);
> idev->igroup->hwpt = NULL;
> }
> if (hwpt_is_paging(hwpt))
> @@ -498,7 +504,10 @@ iommufd_device_do_replace(struct iommufd_device *idev,
> goto err_unlock;
> }
>
> - rc = iommu_group_replace_domain(igroup->group, hwpt->domain);
> + if (old_hwpt->fault_capable || hwpt->fault_capable)
> + rc = iommufd_fault_domain_replace_dev(hwpt, idev);
> + else
> + rc = iommu_group_replace_domain(igroup->group, hwpt->domain);
> if (rc)
> goto err_unresv;
>
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index e752d1c49dde..a4a49f3cd4c2 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -267,3 +267,125 @@ int iommufd_fault_iopf_handler(struct iopf_group *group)
>
> return 0;
> }
> +
> +static void release_attach_cookie(struct iopf_attach_cookie *cookie)
> +{
> + struct iommufd_hw_pagetable *hwpt = cookie->domain->fault_data;
There is a possibility here of cookie->domain being NULL. When you call
release_attach_cookie from iommufd_fault_domain_attach_dev if
idev->iopf_enabled is false. In this case, you have not set the domain
yet.

> + struct iommufd_device *idev = cookie->private;
> +
> + refcount_dec(&idev->obj.users);
> + refcount_dec(&hwpt->obj.users);
You should decrease this ref count only if the cookie actually had a
domain.

This function could be something like this:

static void release_attach_cookie(struct iopf_attach_cookie *cookie)
{
struct iommufd_hw_pagetable *hwpt;
struct iommufd_device *idev = cookie->private;

refcount_dec(&idev->obj.users);
if (cookie->domain) {
hwpt = cookie->domain->fault_data;
refcount_dec(&hwpt->obj.users);
}
kfree(cookie);
}

> + kfree(cookie);
> +}
> +
> +static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
> +{
> + int ret;
> +
> + if (idev->iopf_enabled)
> + return 0;
> +
> + ret = iommu_dev_enable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
> + if (ret)
> + return ret;
> +
> + idev->iopf_enabled = true;
> +
> + return 0;
> +}
> +
> +static void iommufd_fault_iopf_disable(struct iommufd_device *idev)
> +{
> + if (!idev->iopf_enabled)
> + return;
> +
> + iommu_dev_disable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
> + idev->iopf_enabled = false;
> +}
> +
> +int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev)
> +{
> + struct iopf_attach_cookie *cookie;
> + int ret;
> +
> + cookie = kzalloc(sizeof(*cookie), GFP_KERNEL);
> + if (!cookie)
> + return -ENOMEM;
> +
> + refcount_inc(&hwpt->obj.users);
> + refcount_inc(&idev->obj.users);
> + cookie->release = release_attach_cookie;
> + cookie->private = idev;
> +
> + if (!idev->iopf_enabled) {
> + ret = iommufd_fault_iopf_enable(idev);
> + if (ret)
> + goto out_put_cookie;
You have not set domain here and release_attach_cookie will try to
access a null address.

> + }
> +
> + ret = iopf_domain_attach(hwpt->domain, idev->dev, IOMMU_NO_PASID, cookie);
> + if (ret)
> + goto out_disable_iopf;
> +
> + return 0;
> +out_disable_iopf:
> + iommufd_fault_iopf_disable(idev);
> +out_put_cookie:
> + release_attach_cookie(cookie);
> +
> + return ret;
> +}
> +
> +void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev)
> +{
> + iopf_domain_detach(hwpt->domain, idev->dev, IOMMU_NO_PASID);
> + iommufd_fault_iopf_disable(idev);
> +}
> +
> +int iommufd_fault_domain_replace_dev(struct iommufd_hw_pagetable *hwpt,
> + struct iommufd_device *idev)
> +{
> + bool iopf_enabled_originally = idev->iopf_enabled;
> + struct iopf_attach_cookie *cookie = NULL;
> + int ret;
> +
> + if (hwpt->fault_capable) {
> + cookie = kzalloc(sizeof(*cookie), GFP_KERNEL);
> + if (!cookie)
> + return -ENOMEM;
> +
> + refcount_inc(&hwpt->obj.users);
> + refcount_inc(&idev->obj.users);
> + cookie->release = release_attach_cookie;
> + cookie->private = idev;
> +
> + if (!idev->iopf_enabled) {
> + ret = iommufd_fault_iopf_enable(idev);
> + if (ret) {
> + release_attach_cookie(cookie);
> + return ret;
> + }
> + }
> + }
> +
> + ret = iopf_domain_replace(hwpt->domain, idev->dev, IOMMU_NO_PASID, cookie);
> + if (ret) {
> + goto out_put_cookie;
> + }
> +
> + if (iopf_enabled_originally && !hwpt->fault_capable)
> + iommufd_fault_iopf_disable(idev);
> +
> + return 0;
> +out_put_cookie:
> + if (hwpt->fault_capable)
> + release_attach_cookie(cookie);
> + if (iopf_enabled_originally && !idev->iopf_enabled)
> + iommufd_fault_iopf_enable(idev);
> + else if (!iopf_enabled_originally && idev->iopf_enabled)
> + iommufd_fault_iopf_disable(idev);
> +
> + return ret;
> +}
> --
> 2.34.1
>

--

Joel Granados

Attachment: signature.asc
Description: PGP signature

Next message: Jonathan Cameron: "Re: [RFC PATCH v6 10/12] ACPICA: ACPI 6.5: Add support for RAS2 table"
Previous message: Qais Yousef: "Re: [PATCH] sched: cpufreq: Rename map_util_perf to apply_dvfs_headroom"
Next in thread: Baolu Lu: "Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]