Re: [PATCH 02/14] iommu: Implement IOMMU core liveupdate skeleton

[Samiullah Khawaja]

On Tue, Mar 24, 2026 at 12:06:10PM -0700, Vipin Sharma wrote:
> On Tue, Mar 17, 2026 at 08:33:39PM +0000, Samiullah Khawaja wrote:
> > On Tue, Mar 17, 2026 at 12:58:27PM -0700, Vipin Sharma wrote:
> > > On Tue, Feb 03, 2026 at 10:09:36PM +0000, Samiullah Khawaja wrote:
> > > > diff --git a/drivers/iommu/liveupdate.c b/drivers/iommu/liveupdate.c
> > > > +int iommu_for_each_preserved_device(iommu_preserved_device_iter_fn fn,
> > > > +				    void *arg)
> > > > +{
> > > > +	struct iommu_lu_flb_obj *obj;
> > > > +	struct devices_ser *devices;
> > > > +	int ret, i, idx;
> > > > +
> > > > +	ret = liveupdate_flb_get_incoming(&iommu_flb, (void **)&obj);
> > > > +	if (ret)
> > > > +		return -ENOENT;
> > > > +
> > > > +	devices = __va(obj->ser->devices_phys);
> > > > +	for (i = 0, idx = 0; i < obj->ser->nr_devices; ++i, ++idx) {
> > > > +		if (idx >= MAX_DEVICE_SERS) {
> > > > +			devices = __va(devices->objs.next_objs);
> > > > +			idx = 0;
> > > > +		}
> > > > +
> > > > +		if (devices->devices[idx].obj.deleted)
> > > > +			continue;
> > > > +
> > > > +		ret = fn(&devices->devices[idx], arg);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(iommu_for_each_preserved_device);
> > > Also, should this function be introduced in the patch where it is
> > > getting used? Other changes in this patch are already big and complex.
> > > Same for iommu_get_device_preserved_data() and
> > > iommu_get_preserved_data().
> >
> > These are used by the drivers, but part of core. So need to be in
> > this patch :(.
>
> Sorry, I am not understanding why it has to be in this patch? Can it be
> its own patch?

I will move it to a separate patch as per discussion in other thread.

See:
https://lore.kernel.org/all/abrk39_M8k45myXJ@xxxxxxxxxx/
> > Note that this patch is adding core skeleton only, focusing on helpers
> > for the serialized state. This patch is not preserving any real state of
> > iommu, domain or devices. For example, the domains are saved through
> > generic page table in a separate patch, and the drivers preserve the
> > state of devices and associated iommu in separate patches.
> >
> > I will add this text in the commit message to clarify the purpose of
> > this patch.
> > >
> > > I think this patch can be split in three.
> > > Patch 1: Preserve iommu_domain
> > > Patch 2: Preserve pci device and iommu device
> > > Patch 3: The helper functions I mentioned above.
>
> I understand that this patch is adding some helper functions and not
> doing any actual preservation. I am suggesting to split this helper
> function patch into three for easier review based on the above suggestion.
> If I am not wrong this is biggest patch in series of approx 500 line
> changes.

After moving the getter helpers out to the separate patch as mentioned
above, the size of this patch should significantly reduce. I will split
the remaining into domain and device + iommu preservation helper patches
as you suggested.
> > > > +static void iommu_unpreserve_locked(struct iommu_device *iommu)
> > > > +{
> > > > +	struct iommu_ser *iommu_ser = iommu->outgoing_preserved_state;
> > > > +
> > > > +	iommu_ser->obj.ref_count--;
> > >
> > > Should there be a null check?
> >
> > Hmm.. There is a dependency of unpreservation of iommus with devices, so
> > this should never be NULL unless used independently.
> >
> > But I think I will add it here to protect against that.
>
> Okay. Since, it is a static function, I am fine either way.
>
> > > > +void iommu_unpreserve_device(struct iommu_domain *domain, struct device *dev)
> > > > +{
> > > > +	struct iommu_lu_flb_obj *flb_obj;
> > > > +	struct device_ser *device_ser;
> > > > +	struct dev_iommu *iommu;
> > > > +	struct pci_dev *pdev;
> > > > +	int ret;
> > > > +
> > > > +	if (!dev_is_pci(dev))
> > > > +		return;
> > > > +
> > > > +	pdev = to_pci_dev(dev);
> > > > +	iommu = dev->iommu;
> > > > +	if (!iommu->iommu_dev->ops->unpreserve_device ||
> > > > +	    !iommu->iommu_dev->ops->unpreserve)
> > > > +		return;
> > > > +
> > > > +	ret = liveupdate_flb_get_outgoing(&iommu_flb, (void **)&flb_obj);
> > > > +	if (WARN_ON(ret))
> > >
> > > Why WARN_ON here and not other places? Do we need it?
> >
> > Basically this means that the upper layer (iommufd/vfio) is asking to
> > unpreserve a device, but there is no FLB found. This should not happen
> > and should generate a warning.
>
> Yeah, but other places iommu_domain_[preserve|unpreserve](),
> iommu_presreve_locked(), and iommu_preserve_device() are also using this
> function. I am having a confusion on why it is important in his
> function and not others. Those functions are also called by upper layer.

Ah yes.. That makes sense. I will add it to the unpreserve variants of
functions you listed, as during preservation the FLB obj should have
been created.
>