Re: [PATCH v2 04/16] iommu: Implement device and IOMMU HW preservation

[Pranjal Shrivastava <praan@google.com>]

On Thu, May 07, 2026 at 06:47:07PM +0000, Samiullah Khawaja wrote:
> On Thu, May 07, 2026 at 10:07:43AM +0800, Baolu Lu wrote:
> > On 4/28/26 01:56, Samiullah Khawaja wrote:
> > > Add IOMMU ops to preserve/unpreserve a device. These can be implemented
> > > by the IOMMU drivers that support preservation of devices that have
> > > their IOMMU domains preserved. During device preservation the state of
> > > the associated IOMMU is also preserved as dependency.
> > > 
> > > Signed-off-by: Samiullah Khawaja<skhawaja@google.com>
> > > ---
> > >  drivers/iommu/liveupdate.c       | 162 +++++++++++++++++++++++++++++++
> > >  include/linux/iommu-liveupdate.h |  33 +++++++
> > >  include/linux/iommu.h            |  20 ++++
> > >  3 files changed, 215 insertions(+)
> > > 
> 
> [snip]
> > > +
> > > +int iommu_preserve_device(struct iommu_domain *domain,
> > > +			  struct device *dev, u64 *preserved_state)
> > > +{
> > > +	struct iommu_flb_obj *flb_obj;
> > > +	struct iommu_device_ser *device_ser;
> > > +	struct dev_iommu *iommu;
> > > +	struct pci_dev *pdev;
> > > +	int ret;
> > > +
> > > +	if (!dev_is_pci(dev))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (!domain->preserved_state)
> > > +		return -EINVAL;
> > > +
> > > +	if (!iommu_group_dma_owner_claimed(dev->iommu_group))
> > > +		return -EINVAL;
> > > +
> > > +	pdev = to_pci_dev(dev);
> > > +	iommu = dev->iommu;
> > > +	if (!iommu->iommu_dev->ops->preserve_device ||
> > > +	    !iommu->iommu_dev->ops->preserve)
> > > +		return -EOPNOTSUPP;
> 
> I will check for unpreserve ops here also.

[snip]

> > > +	pdev = to_pci_dev(dev);
> > > +	iommu = dev->iommu;
> > > +	if (!iommu->iommu_dev->ops->unpreserve_device ||
> > > +	    !iommu->iommu_dev->ops->unpreserve)
> > > +		return;
> > 
> > Is it considered a driver bug if it implements the preserve hooks but
> > not unpreserve ones? This would at least cause a silent memory leak. How
> > about adding a WARN like this?
> 
> I think the preservation should not be done if the unpreserve ops are
> not implemented by the driver. I will add these checks in the _preserve
> functions and return EOPNOTSUPP if it is not implemented by the driver.
> > 
> >    if (WARN_ON_ONCE(!iommu->iommu_dev->ops->unpreserve_device ||
> >                     !iommu->iommu_dev->ops->unpreserve))
> >        return;
> 
> We can add this warning here also.

I'd rather have the iommu device registration fail instead of solely
relying on these WARN_ONs here. We can simply add a snippet inside
iommu_device_register():

#ifdef CONFIG_IOMMU_LIVEUPDATE
if ((iommu->ops->preserve && !iommu->ops->unpreserve) ||
    (!iommu->ops->preserve && iommu->ops->unpreserve)) {
        pr_err("IOMMU: %s: Asymmetric live-update operations detected\n",
               dev_name(iommu->dev));
        return -EINVAL;
}
#endif

This prevents a half-baked iommu driver from ever spinning up, completely
eliminating the need to check for it inside the live-update session paths.
We anyway don't support hot-pluggable liveupdate ops atm.

Thanks,
Praan