Re: [PATCH v2 08/16] iommu: Add APIs to get iommu and device preserved state

[Samiullah Khawaja <skhawaja@google.com>]

On Tue, May 19, 2026 at 03:52:43PM +0000, Pranjal Shrivastava wrote:
>On Mon, Apr 27, 2026 at 05:56:25PM +0000, Samiullah Khawaja wrote:
>> The preserved state of the device and IOMMU needs to be fetched during
>> shutdown and boot in the next kernel. Add APIs that can be used to fetch
>> the preserved state of a device and IOMMU. The APIs will only be used
>> during shutdown and after liveupdate so no locking needed.
>>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>> ---
>>  drivers/iommu/liveupdate.c       | 57 ++++++++++++++++++++++++++++++++
>>  include/linux/iommu-liveupdate.h | 31 +++++++++++++++++
>>  2 files changed, 88 insertions(+)
>>
>> diff --git a/drivers/iommu/liveupdate.c b/drivers/iommu/liveupdate.c
>> index 765d042e22e3..60ee29b0c6bd 100644
>> --- a/drivers/iommu/liveupdate.c
>> +++ b/drivers/iommu/liveupdate.c
>> @@ -17,6 +17,14 @@
>>  #define iommu_max_objs_per_page(_array) \
>>  	((PAGE_SIZE - sizeof(struct iommu_array_hdr_ser)) / sizeof((_array)->objects[0]))
>>
>> +#define iommu_liveupdate_for_each_obj(_arr, _obj, _idx)				\
>> +	for (; (_arr);								\
>> +	     (_arr) = (_arr)->hdr.next_array_phys ?				\
>> +	     phys_to_virt((_arr)->hdr.next_array_phys) : NULL)			\
>> +		for ((_idx) = 0, (_obj) = (_arr)->objects;			\
>> +		     (_idx) < (_arr)->hdr.nr_objects; (_idx)++, (_obj)++)	\
>> +			if (!(_obj)->hdr.deleted)
>> +
>
>Nit: While the usage of this macro is safe right now because we use
>goto out; to short-circuit the scan. Such naked nested loops inside an
>iterator macro can be a bit of a trap for the future.
>
>If anyone tries to reuse this macro down the road and uses a break;
>statement, it will only break the innermost object loop and continue
>scanning subsequent array blocks.
>
>We could nest two macros or it as a helper fn to prevent such bugs.

Agreed on all points. I will change this to two nested macros.
>
>
>>  static void *iommu_liveupdate_restore_array(u64 array_phys)
>>  {
>>  	struct iommu_array_hdr_ser *array_hdr;
>> @@ -201,6 +209,55 @@ void iommu_liveupdate_unregister_flb(struct liveupdate_file_handler *handler)
>>  }
>>  EXPORT_SYMBOL(iommu_liveupdate_unregister_flb);
>>
>> +int iommu_for_each_preserved_device(iommu_preserved_device_iter_fn fn,
>> +				    void *arg)
>> +{
>> +	struct iommu_flb_obj *flb_obj;
>> +	struct iommu_device_array_ser *array;
>> +	struct iommu_device_ser *device_ser;
>> +	int ret, idx;
>> +
>> +	ret = liveupdate_flb_get_incoming(&iommu_flb, (void **)&flb_obj);
>> +	if (ret)
>> +		return -ENOENT;
>> +
>> +	array = phys_to_virt(flb_obj->ser->device_array_phys);
>
>Nit: phys_to_virt doesn't return an error for NULL. While we shouldn't
>be in a case where serialized ptrs are NULL. I'm wondering how painful
>catching such bugs would be.. should we check for NULL phys_addr here?
>(and at other places)


Agreed. Will update.
>
>> +	iommu_liveupdate_for_each_obj(array, device_ser, idx) {
>> +		ret = fn(device_ser, arg);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +out:
>> +	liveupdate_flb_put_incoming(&iommu_flb);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(iommu_for_each_preserved_device);
>> +
>
>With those two nits,
>
>Reviewed-by: Pranjal Shrivastava <praan@google.com>i

Thanks,
Sami