Re: [PATCH 11/14] iommufd-lu: Persist iommu hardware pagetables for live update

[Vipin Sharma]

On Tue, Feb 03, 2026 at 10:09:45PM +0000, Samiullah Khawaja wrote:
> From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> 
> The caller is expected to mark each HWPT to be preserved with an ioctl
> call, with a token that will be used in restore. At preserve time, each
> HWPT's domain is then called with iommu_domain_preserve to preserve the

Nit: Add () after iommu_domain_preserve, it makes easier to know this is
a function.

> diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c
> +static int iommufd_save_hwpts(struct iommufd_ctx *ictx,
> +			      struct iommufd_lu *iommufd_lu,
> +			      struct liveupdate_session *session)
> +{
> +	struct iommufd_hwpt_paging *hwpt, **hwpts = NULL;

We should rename hwpts to something to denote that it is a list.

> +	struct iommu_domain_ser *domain_ser;
> +	struct iommufd_hwpt_lu *hwpt_lu;
> +	struct iommufd_object *obj;
> +	unsigned int nr_hwpts = 0;
> +	unsigned long index;
> +	unsigned int i;
> +	int rc = 0;
> +
> +	if (iommufd_lu) {
> +		hwpts = kcalloc(iommufd_lu->nr_hwpts, sizeof(*hwpts),
> +				GFP_KERNEL);
> +		if (!hwpts)
> +			return -ENOMEM;
> +	}
> +
> +	xa_lock(&ictx->objects);
> +	xa_for_each(&ictx->objects, index, obj) {
> +		if (obj->type != IOMMUFD_OBJ_HWPT_PAGING)
> +			continue;
> +
> +		hwpt = container_of(obj, struct iommufd_hwpt_paging, common.obj);
> +		if (!hwpt->lu_preserve)
> +			continue;
> +
> +		if (hwpt->ioas) {
> +			/*
> +			 * Obtain exclusive access to the IOAS and IOPT while we
> +			 * set immutability
> +			 */
> +			mutex_lock(&hwpt->ioas->mutex);
> +			down_write(&hwpt->ioas->iopt.domains_rwsem);
> +			down_write(&hwpt->ioas->iopt.iova_rwsem);
> +
> +			hwpt->ioas->iopt.lu_map_immutable = true;

It feels odd that this is not cleaned up in this function as a part of
its error handling. Now it becomes caller resposibility to handle clean
up for the side effect created by this function. 

IMO, this function should clean up lu_map_immutable instread of callers
to make sure they call cleanups. You can also try exploring XA_MARK_*
APIs if that can help in cleaning up easily.

> +int iommufd_liveupdate_unregister_lufs(void)
> +{
> +	WARN_ON(iommu_liveupdate_unregister_flb(&iommufd_lu_handler));
> +
> +	return liveupdate_unregister_file_handler(&iommufd_lu_handler);

This seems little insconsistent, iommu_liveupdate_unregister_flb() has
WARN_ON and liveupdate_unregister_file_handler() does not.

Also, refer https://lore.kernel.org/all/20260226160353.6f3371bc@xxxxxxxxxxx/

> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> @@ -775,11 +775,21 @@ static int __init iommufd_init(void)
>  		if (ret)
>  			goto err_misc;
>  	}
> +
> +	if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE)) {
> +		ret = iommufd_liveupdate_register_lufs();
> +		if (ret)
> +			goto err_vfio_misc;
> +	}
> +

Do we need IS_ENABLED here? iommufd_private.h is providing definition
for both values of CONFIG_IOMMU_LIVEUPDATE.

Nit: What is lufs? Should we just rename to iommufd_liveupdate_register()?

>  	ret = iommufd_test_init();
>  	if (ret)
> -		goto err_vfio_misc;
> +		goto err_lufs;
>  	return 0;
>  
> +err_lufs:
> +	if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE))
> +		iommufd_liveupdate_unregister_lufs();
>  err_vfio_misc:
>  	if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER))
>  		misc_deregister(&vfio_misc_dev);
> @@ -791,6 +801,8 @@ static int __init iommufd_init(void)
>  static void __exit iommufd_exit(void)
>  {
>  	iommufd_test_exit();
> +	if (IS_ENABLED(CONFIG_IOMMU_LIVEUPDATE))
> +		iommufd_liveupdate_unregister_lufs();

Same as above.

> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
> @@ -1427,6 +1429,12 @@ struct iopt_pages *iopt_alloc_file_pages(struct file *file,
>  	pages->file = get_file(file);
>  	pages->start = start - start_byte;
>  	pages->type = IOPT_ADDRESS_FILE;
> +
> +	pages->seals = 0;

This is already 0.

> +	seals = memfd_get_seals(file);
> +	if (seals > 0)
> +		pages->seals = seals;
> +
>  	return pages;
>  }
>  
> --- /dev/null
> +++ b/include/linux/kho/abi/iommufd.h
> +
> +struct iommufd_lu {
> +	unsigned int nr_hwpts;

Should this be u32 or u64?

> +	struct iommufd_hwpt_lu hwpts[];
> +};
> +

Should this also be __packed?

> +#endif /* _LINUX_KHO_ABI_IOMMUFD_H */
> -- 
> 2.53.0.rc2.204.g2597b5adb4-goog
>