Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace
On 2024/2/20 21:57, Joel Granados wrote:
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.
Yes. Good catch!
+ struct iommufd_device *idev = cookie->private;
+
+ refcount_dec(>obj.users);
+ refcount_dec(>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(>obj.users);
if (cookie->domain) {
hwpt = cookie->domain->fault_data;
refcount_dec(>obj.users);
}
kfree(cookie);
}
Yeah, fixed.
+ 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(>obj.users);
+ refcount_inc(>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.
Fixed as above.
Best regards,
baolu
Re: [PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace
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
> ---
> 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(>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(>igroup->lock);
> list_del(>group_item);
> if (list_empty(>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
[PATCH v3 6/8] iommufd: IOPF-capable hw page table attach/detach/replace
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
---
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(>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(>igroup->lock);
list_del(>group_item);
if (list_empty(>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;
+ struct iommufd_device *idev = cookie->private;
