Re: [PATCH v2 12/16] iommufd: Implement ioctl to mark HWPT for preservation

[Pranjal Shrivastava]

On Mon, Apr 27, 2026 at 05:56:29PM +0000, Samiullah Khawaja wrote:
> From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> 
> Userspace provides a token to mark the HWPT for preservation. Note that
> this token is not the LUO token that is used to preserve the iommufd.
> Once all the required HWPT are marked for preservation, the user can
> preserve the iommufd into LUO. The iommufd will preserve the HWPTs that
> are marked for preservation.
> 
> The marked HWPTs are tracked using a new XArray mark protected by a new
> liveupdate mutex. This mutex will also be used during iommufd
> preservation to protect against any race with the mark preserve ioctl.
> 
> The HWPT token will be used during restore to identify this HWPT. The
> restoration logic is not implemented and will be added later.
> 
> Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
> Signed-off-by: Samiullah Khawaja <skhawaja@xxxxxxxxxx>

[snip]

> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 6ac1965199e9..111f4d42e210 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -44,6 +44,11 @@ struct iommufd_ctx {
>  	struct file *file;
>  	struct xarray objects;
>  	struct xarray groups;
> +#ifdef CONFIG_IOMMU_LIVEUPDATE
> +#define IOMMUFD_OBJ_LIVEUPDATE_MARK XA_MARK_1
> +	/* @liveupdate_mutex: Protects the preservation of HWPTs. */
> +	struct mutex liveupdate_mutex;
> +#endif
>  	wait_queue_head_t destroy_wait;
>  	struct rw_semaphore ioas_creation_lock;
>  	struct maple_tree mt_mmap;
> @@ -373,6 +378,10 @@ struct iommufd_hwpt_paging {
>  	bool auto_domain : 1;
>  	bool enforce_cache_coherency : 1;
>  	bool nest_parent : 1;
> +#ifdef CONFIG_IOMMU_LIVEUPDATE
> +	bool liveupdate_preserve : 1;

Nit: Is this bool a remnant from v1 that made into this refactor? I don't
see us using it anywhere?

> +	u64 liveupdate_token;
> +#endif
>  	/* Head at iommufd_ioas::hwpt_list */
>  	struct list_head hwpt_item;
>  	struct iommufd_sw_msi_maps present_sw_msi;
> @@ -706,6 +715,15 @@ iommufd_get_vdevice(struct iommufd_ctx *ictx, u32 id)
>  			    struct iommufd_vdevice, obj);
>  }
>

[...]

> +int iommufd_hwpt_liveupdate_mark_preserve(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_hwpt_liveupdate_mark_preserve *cmd = ucmd->cmd;
> +	struct iommufd_hwpt_paging *hwpt_target;
> +	struct iommufd_hwpt_paging *hwpt_paging;
> +	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);
> +
> +	mutex_lock(&ictx->liveupdate_mutex);
> +
> +	xa_lock(&ictx->objects);
> +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
> +		if (WARN_ON_ONCE(obj->type != IOMMUFD_OBJ_HWPT_PAGING))
> +			continue;
> +
> +		hwpt_paging = to_hwpt_paging(container_of(obj, struct iommufd_hw_pagetable, obj));
> +		if (hwpt_paging->liveupdate_token == cmd->hwpt_token) {
> +			rc = -EADDRINUSE;
> +			goto out_unlock;
> +		}
> +	}
> +

Nit: What happens if the user calls this IOCTL on an HWPT that is already
marked but passes a different token? 

The xa_for_each_marked loop will not match the new token (so it bypasses
-EADDRINUSE case) and the code will silently overwrite the HWPT's 
existing token?

Since there is no unmark / update UAPI, It can be a fair policy as well,
but maybe we should call it out explicitly, maybe a log like:
dev_warn("Overwriting token to %d ...") ?

If NOT, then we should avoid overwriting the token and scream with
retval like -EINVAL & dev_err in dmesg.

> +	__xa_set_mark(&ictx->objects, hwpt_target->common.obj.id, IOMMUFD_OBJ_LIVEUPDATE_MARK);
> +	hwpt_target->liveupdate_token = cmd->hwpt_token;
> +
> +out_unlock:
> +	xa_unlock(&ictx->objects);
> +	mutex_unlock(&ictx->liveupdate_mutex);
> +	iommufd_put_object(ictx, &hwpt_target->common.obj);
> +	return rc;
> +}

With those two nits addressed,

Reviewed-by: Pranjal Shrivastava <praan@xxxxxxxxxx>

Thanks,
Praan