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

[Samiullah Khawaja]

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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>i

Thanks,
Sami