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

Vipin Sharma
2026-03-23T20:28:19Z

On Tue, Feb 03, 2026 at 10:09:45PM +0000, Samiullah Khawaja wrote:
> From: YiFei Zhu <zhuyifei@google.com>
>
> 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@shazbot.org/

> 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
>