Re: [PATCH v2 02/16] iommu: Implement IOMMU Live update FLB callbacks

[Samiullah Khawaja <skhawaja@google.com>]

On Fri, May 01, 2026 at 09:45:19PM +0000, David Matlack wrote:
>On 2026-04-27 05:56 PM, Samiullah Khawaja wrote:
>> Add liveupdate FLB for IOMMU state preservation. Use KHO preserve memory
>> alloc/free helper functions to allocate memory for the IOMMU Live update
>> FLB object and the serialization structs for device, domain and iommu.
>>
>> During retrieve, walk through the preserved obj array headers and
>> restore each folio. Also recreate the FLB obj.
>>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>
>> +static void *iommu_liveupdate_restore_array(u64 array_phys)
>> +{
>> +	struct iommu_array_hdr_ser *array_hdr;
>> +	void *vaddr = array_phys ? phys_to_virt(array_phys) : NULL;
>> +
>> +	while (array_phys) {
>> +		/*
>> +		 * Failure to restore preserved IOMMU state is considered fatal.
>> +		 *
>> +		 * This is because the IOMMU translations for preserved IOMMUs
>> +		 * were kept enabled in the previous kernel and the preserved
>> +		 * devices have their IOMMU domains still present. Not being
>> +		 * able to restore means that the memory mapped into preserved
>> +		 * domains might be already corrupted by the preserved devices.
>> +		 *
>> +		 * There is no way to confirm the integrity of the memory that
>> +		 * was mapped. BUG_ON is the safest option at this point.
>> +		 */
>> +		BUG_ON(!kho_restore_folio(array_phys));
>> +		array_hdr = phys_to_virt(array_phys);
>> +		array_phys = array_hdr->next_array_phys;
>> +	}
>> +
>> +	return vaddr;
>> +}
>
>> +static int iommu_liveupdate_flb_retrieve(struct liveupdate_flb_op_args *argp)
>> +{
>> +	struct iommu_flb_obj *obj;
>> +	struct iommu_flb_ser *ser;
>> +
>> +	obj = kzalloc_obj(*obj, GFP_KERNEL);
>> +	if (!obj)
>> +		return -ENOMEM;
>
>Should this be considered fatal for the same reason
>iommu_liveupdate_restore_array() is considered fatal? If anything in
>iommu_liveupdate_flb_retrieve() fails then the risk of corruption as
>described in iommu_liveupdate_restore_array() is possible.

This one is a little tricky, it is not really fatal as it doesn't
represent a corruption or unexpected behaviour. I think if this fails
the probe of IOMMUs should fail and with a rescan, the user can retry
this.

I will handle this properly in the caller to make sure a retry can
happen for these.
>
>> +
>> +	/* Data must be present and valid from the previous kernel */
>> +	BUG_ON(!kho_restore_folio(argp->data));
>> +
>> +	mutex_init(&obj->lock);
>> +	ser = phys_to_virt(argp->data);
>> +	obj->ser = ser;
>> +
>> +	obj->curr_domain_array = iommu_liveupdate_restore_array(ser->iommu_domain_array_phys);
>> +	obj->curr_device_array = iommu_liveupdate_restore_array(ser->device_array_phys);
>> +	obj->curr_iommu_array = iommu_liveupdate_restore_array(ser->iommu_array_phys);
>> +	argp->obj = obj;
>> +	return 0;
>> +}
>> +
>> +static struct liveupdate_flb_ops iommu_flb_ops = {
>> +	.preserve = iommu_liveupdate_flb_preserve,
>> +	.unpreserve = iommu_liveupdate_flb_unpreserve,
>> +	.finish = iommu_liveupdate_flb_finish,
>> +	.retrieve = iommu_liveupdate_flb_retrieve,
>
>nit: I think it's helpful to put these in the order they are expected to
>be called.
>
>  .preserve = iommu_liveupdate_flb_preserve,
>  .unpreserve = iommu_liveupdate_flb_unpreserve,
>  .retrieve = iommu_liveupdate_flb_retrieve,
>  .finish = iommu_liveupdate_flb_finish,

Agreed. I will do this.
>
>> diff --git a/include/linux/kho/abi/iommu.h b/include/linux/kho/abi/iommu.h
>> new file mode 100644
>> index 000000000000..37b967820f14
>
>> +enum iommu_type_ser {
>> +	IOMMU_INVALID,
>> +};
>
>Please document this enum.

Agreed. Will do.
>
>> +
>> +/**
>> + * struct iommu_hdr_ser - Common header for all serialized IOMMU objects
>> + * @ref_count: Reference count for the object
>> + * @deleted: Flag indicating if the object is deleted
>> + * @incoming: Flag indicating if the object was preserved in previous kernel
>> + */
>> +struct iommu_hdr_ser {
>> +	u32 ref_count;
>> +	u32 deleted:1;
>> +	u32 incoming:1;
>
>Are C bitfields safe to use in Live Update ABI?
>
>> +} __packed;
>
>> +/**
>> + * struct iommu_flb_obj - FLB object allocated in current kernel pointing to
>> + *			  preserved state in FLB
>> + * @lock: Mutex protecting the object
>> + * @ser: Pointer to the serialized state in FLB
>> + * @curr_iommu_array: Pointer to the current array of IOMMU instances
>> + * @curr_domain_array: Pointer to the current array of domains
>> + * @curr_device_array: Pointer to the current array of devices
>> + */
>> +struct iommu_flb_obj {
>> +	/* @lock: Protects the serialized objects during concurrent preservation */
>> +	struct mutex lock;
>> +	struct iommu_flb_ser *ser;
>> +
>> +	struct iommu_hw_array_ser *curr_iommu_array;
>> +	struct iommu_domain_array_ser *curr_domain_array;
>> +	struct iommu_device_array_ser *curr_device_array;
>> +} __packed;
>
>This struct is not ABI so it should not be __packed nor defined in this
>file. I haven't read the whole series yet but this definition can
>probably go in drivers/iommu/liveupdate.c.

Agreed on both points. I think it can be moved to liveupdate.c. Will do.

Thanks,
Sami