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

[Samiullah Khawaja <skhawaja@google.com>]

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@google.com/
>>
>> 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.
>