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

[Samiullah Khawaja <skhawaja@google.com>]

On Tue, May 19, 2026 at 11:05:02PM +0000, Pranjal Shrivastava wrote:
>On Mon, Apr 27, 2026 at 05:56:29PM +0000, Samiullah Khawaja wrote:
>> From: YiFei Zhu <zhuyifei@google.com>
>>
>> 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@google.com>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>

[snip]
>
>> +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 ...") ?

Agreed. I will do this.
>
>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@google.com>
>
>Thanks,
>Praan

Thanks,
Sami