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

[Vipin Sharma <vipinsh@google.com>]

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?
> 
> 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.

> > > +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 this
function and not others. Those functions are also called by upper layer.