Re: [PATCH 10/14] iommufd-lu: Implement ioctl to let userspace mark an HWPT to be preserved

[Samiullah Khawaja <skhawaja@google.com>]

On Wed, Mar 25, 2026 at 02:37:37PM +0000, Pranjal Shrivastava wrote:
>On Tue, Feb 03, 2026 at 10:09:44PM +0000, Samiullah Khawaja wrote:
>> From: YiFei Zhu <zhuyifei@google.com>
>>
>> Userspace provides a token, which will then be used at restore to
>> identify this HWPT. The restoration logic is not implemented and will be
>> added later.
>>
>> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>> ---
>>  drivers/iommu/iommufd/Makefile          |  1 +
>>  drivers/iommu/iommufd/iommufd_private.h | 13 +++++++
>>  drivers/iommu/iommufd/liveupdate.c      | 49 +++++++++++++++++++++++++
>>  drivers/iommu/iommufd/main.c            |  2 +
>>  include/uapi/linux/iommufd.h            | 19 ++++++++++
>>  5 files changed, 84 insertions(+)
>>  create mode 100644 drivers/iommu/iommufd/liveupdate.c
>>
>> diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
>> index 71d692c9a8f4..c3bf0b6452d3 100644
>> --- a/drivers/iommu/iommufd/Makefile
>> +++ b/drivers/iommu/iommufd/Makefile
>> @@ -17,3 +17,4 @@ obj-$(CONFIG_IOMMUFD_DRIVER) += iova_bitmap.o
>>
>>  iommufd_driver-y := driver.o
>>  obj-$(CONFIG_IOMMUFD_DRIVER_CORE) += iommufd_driver.o
>> +obj-$(CONFIG_IOMMU_LIVEUPDATE) += liveupdate.o
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index eb6d1a70f673..6424e7cea5b2 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -374,6 +374,10 @@ struct iommufd_hwpt_paging {
>>  	bool auto_domain : 1;
>>  	bool enforce_cache_coherency : 1;
>>  	bool nest_parent : 1;
>> +#ifdef CONFIG_IOMMU_LIVEUPDATE
>> +	bool lu_preserve : 1;
>> +	u32 lu_token;
>
>Did we downsize the token? Shouldn't this be u64 as everywhere else?

Note that this is different from the token that is used to preserve the
FD into LUO. This token is used to mark the HWPT for preservation, that
is it will be preserved when the FD is preserved.

I will add more text in the commit message to make it clear.

For consistency I will make it u64.
>
>> +#endif
>>  	/* Head at iommufd_ioas::hwpt_list */
>>  	struct list_head hwpt_item;
>>  	struct iommufd_sw_msi_maps present_sw_msi;
>> @@ -707,6 +711,15 @@ iommufd_get_vdevice(struct iommufd_ctx *ictx, u32 id)
>>  			    struct iommufd_vdevice, obj);
>>  }
>>
>> +#ifdef CONFIG_IOMMU_LIVEUPDATE
>> +int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd);
>> +#else
>> +static inline int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd)
>> +{
>> +	return -ENOTTY;
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_IOMMUFD_TEST
>>  int iommufd_test(struct iommufd_ucmd *ucmd);
>>  void iommufd_selftest_destroy(struct iommufd_object *obj);
>> diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c
>> new file mode 100644
>> index 000000000000..ae74f5b54735
>> --- /dev/null
>> +++ b/drivers/iommu/iommufd/liveupdate.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#define pr_fmt(fmt) "iommufd: " fmt
>> +
>> +#include <linux/file.h>
>> +#include <linux/iommufd.h>
>> +#include <linux/liveupdate.h>
>> +
>> +#include "iommufd_private.h"
>> +
>> +int iommufd_hwpt_lu_set_preserve(struct iommufd_ucmd *ucmd)
>> +{
>> +	struct iommu_hwpt_lu_set_preserve *cmd = ucmd->cmd;
>> +	struct iommufd_hwpt_paging *hwpt_target, *hwpt;
>> +	struct iommufd_ctx *ictx = ucmd->ictx;
>> +	struct iommufd_object *obj;
>> +	unsigned long index;
>> +	int rc = 0;
>> +
>> +	hwpt_target = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
>> +	if (IS_ERR(hwpt_target))
>> +		return PTR_ERR(hwpt_target);
>> +
>> +	xa_lock(&ictx->objects);
>> +	xa_for_each(&ictx->objects, index, obj) {
>> +		if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
>> +			continue;
>
>Couldn't these be HWPT_NESTED? Are we explicitly skipping HWPT_NESTED
>here? ARM SMMUv3 heavily relies on IOMMU_DOMAIN_NESTED to back vIOMMUs
>and hold critical guest translation state. We'd need to support
>HWPT_NESTED for arm-smmu-v3.

For this series, I am not handling the NESTED and vIOMMU usecases. I
will be sending a separate series to handle those, this is mentioned in
cover letter also in the Future work.

Will add a note in commit message also.
>
>> +
>> +		hwpt = container_of(obj, struct iommufd_hwpt_paging, common.obj);
>> +
>> +		if (hwpt == hwpt_target)
>> +			continue;
>> +		if (!hwpt->lu_preserve)
>> +			continue;
>> +		if (hwpt->lu_token == cmd->hwpt_token) {
>> +			rc = -EADDRINUSE;
>> +			goto out;
>> +		}
>
>I see that this entire loop is to avoid collisions but could we improve
>this? We are doing an O(N) linear search over the entire ictx->objects
>xarray while holding xa_lock on every setup call.
>
>If the kernel requires a strict 1:1 mapping of lu_token to hwpt,
>wouldn't it be much better to track these in a dedicated xarray?
>
>Just thinking out loud, if we added a dedicated lu_tokens xarray to
>iommufd_ctx, we could drop the linear search and the lock entirely,
>letting the xarray handle the collision natively like this:
>
>	rc = xa_insert(&ictx->lu_tokens, cmd->hwpt_token, hwpt_target, GFP_KERNEL);
>	if (rc == -EBUSY) {
>		rc = -EADDRINUSE;
>		goto out;
>	} else if (rc) {
>		goto out;
>	}
>
>This ensures instant collision detection without iterating the global
>object pool. When the HWPT is eventually destroyed (or un-preserved), we
>simply call xa_erase(&ictx->lu_tokens, hwpt->lu_token).

Agreed. We can call xa_erase when it is destroyed. This can also be used
during actual preservation without taking the objects lock.
>
>> +	}
>> +
>> +	hwpt_target->lu_preserve = true;
>
>I don't see a way to unset hwpt->lu_preserve once it's been set. What if
>a VMM marks a HWPT for preservation, but then the guest decides to rmmod
>the device before the actual kexec? The VMM would need a way to
>unpreserve it so we don't carry stale state across the live update?
>
>Are we relying on the VMM to always call IOMMU_DESTROY on that HWPT when
>it's no longer needed for preservation? A clever VMM optimizing for perf
>might just pool or cache detached HWPTs for future reuse. If that HWPT
>goes back into a free pool and gets re-attached to a new device later,
>the sticky lu_preserve state will inadvertently leak across the kexec..

As mentioned earlier, the HWPT is not being preserved in this call. So
when VMM dies or rmmod happens, this HWPT will be destroyed following
the normal flow.

I will add this in commit message.
>
>> +	hwpt_target->lu_token = cmd->hwpt_token;
>> +
>> +out:
>> +	xa_unlock(&ictx->objects);
>> +	iommufd_put_object(ictx, &hwpt_target->common.obj);
>> +	return rc;
>> +}
>> +
>> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
>> index 5cc4b08c25f5..e1a9b3051f65 100644
>> --- a/drivers/iommu/iommufd/main.c
>> +++ b/drivers/iommu/iommufd/main.c
>> @@ -493,6 +493,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
>>  		 __reserved),
>>  	IOCTL_OP(IOMMU_VIOMMU_ALLOC, iommufd_viommu_alloc_ioctl,
>>  		 struct iommu_viommu_alloc, out_viommu_id),
>> +	IOCTL_OP(IOMMU_HWPT_LU_SET_PRESERVE, iommufd_hwpt_lu_set_preserve,
>> +		 struct iommu_hwpt_lu_set_preserve, hwpt_token),
>>  #ifdef CONFIG_IOMMUFD_TEST
>>  	IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last),
>>  #endif
>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>> index 2c41920b641d..25d8cff987eb 100644
>> --- a/include/uapi/linux/iommufd.h
>> +++ b/include/uapi/linux/iommufd.h
>> @@ -57,6 +57,7 @@ enum {
>>  	IOMMUFD_CMD_IOAS_CHANGE_PROCESS = 0x92,
>>  	IOMMUFD_CMD_VEVENTQ_ALLOC = 0x93,
>>  	IOMMUFD_CMD_HW_QUEUE_ALLOC = 0x94,
>> +	IOMMUFD_CMD_HWPT_LU_SET_PRESERVE = 0x95,
>>  };
>>
>>  /**
>> @@ -1299,4 +1300,22 @@ struct iommu_hw_queue_alloc {
>>  	__aligned_u64 length;
>>  };
>>  #define IOMMU_HW_QUEUE_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HW_QUEUE_ALLOC)
>> +
>> +/**
>> + * struct iommu_hwpt_lu_set_preserve - ioctl(IOMMU_HWPT_LU_SET_PRESERVE)
>
>Nit: The IOCTL is called "IOMMU_HWPT_LU_SET_PRESERVE" which subtly
>implies the existence of a "GET_PRESERVE". Should we perhaps just call
>it IOMMU_HWPT_LU_PRESERVE?

LU_PRESERVE would imply that it is being preserved. Maybe
"IOMMU_HWPT_LU_MARK_PRESERVE"?
>
>> + * @size: sizeof(struct iommu_hwpt_lu_set_preserve)
>> + * @hwpt_id: Iommufd object ID of the target HWPT
>> + * @hwpt_token: Token to identify this hwpt upon restore
>> + *
>> + * The target HWPT will be preserved during iommufd preservation.
>> + *
>> + * The hwpt_token is provided by userspace. If userspace enters a token
>> + * already in use within this iommufd, -EADDRINUSE is returned from this ioctl.
>> + */
>> +struct iommu_hwpt_lu_set_preserve {
>> +	__u32 size;
>> +	__u32 hwpt_id;
>> +	__u32 hwpt_token;
>> +};
>
>Nit: Let's make sure we follow the 64-bit alignment as enforced in the
>rest of this file, note the __u32 __reserved fields in existing IOCTL
>structs.

Agreed. Will update
>
>> +#define IOMMU_HWPT_LU_SET_PRESERVE _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_LU_SET_PRESERVE)
>>  #endif
>
>Thanks,
>Praan