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

[Samiullah Khawaja]

On Mon, May 18, 2026 at 02:01:25PM +0000, Pranjal Shrivastava wrote:
> 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@xxxxxxxxxx>
> > > > ---
> > > >  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.

Hmm.. I understand your point, But I think we should let the
registration go through so the IOMMU can be used. Its just that the
liveupdate preservation will not be supported. Failing the registration
would be a big hammer that I think is not really needed.
> Thanks,
> Praan

Sami