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 <[email protected]>
---
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;
+ struct iommufd_device *idev = cookie->private;
+
+ refcount_dec(&idev->obj.users);
+ refcount_dec(&hwpt->obj.users);
+ 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;
+ }
+
+ 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
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 <[email protected]>
> ---
> 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
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(&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);
> }
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(&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.
Fixed as above.
Best regards,
baolu