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

[Samiullah Khawaja <skhawaja@google.com>]

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

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