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

[Samiullah Khawaja]

On Fri, May 01, 2026 at 10:42:57PM +0000, David Matlack wrote:
> On 2026-04-27 05:56 PM, Samiullah Khawaja wrote:
> > Add IOMMU ops to preserve/unpreserve a device. These can be implemented
>
> Can you make this comment more specific about what is being preserved?
> Saying it preserves a device is vague and maybe even misleading. It's
> more about  about preserving a device's attachment to a specific domain
> correct?

There is attachment ID, but the preservation of device can have IOMMU
driver specific things, so in core I mostly mention "preseve device
specific state". In later patches in this series, we save PASID table
using the same callback. I will add more details in the commit message.
> > 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>
>
> > diff --git a/drivers/iommu/liveupdate.c b/drivers/iommu/liveupdate.c
> > index f71f14518248..765d042e22e3 100644
> > --- a/drivers/iommu/liveupdate.c
> > +++ b/drivers/iommu/liveupdate.c
>
> > +static struct iommu_device_ser *alloc_iommu_device_ser(struct iommu_flb_obj *flb)
>
> It is unforunate that struct iommu_device_ser has nothing to do with
> struct iommu_device. The former represents an a PCI device, while the
> latter represents an IOMMU.

Yes, I went through various iterations of trying to name it in a
different way but keeping the "iommu_" prefix and the "device" state
naturally falls into this. Not sure if iommu_pci_device_ser is suitable?
> > diff --git a/include/linux/iommu-liveupdate.h b/include/linux/iommu-liveupdate.h
> > index 6019cfc27428..279c7ab04f09 100644
> > --- a/include/linux/iommu-liveupdate.h
> > +++ b/include/linux/iommu-liveupdate.h
>
> >  int iommu_domain_preserve(struct iommu_domain *domain, struct iommu_domain_ser **ser);
> >  void iommu_domain_unpreserve(struct iommu_domain *domain);
> > +int iommu_preserve_device(struct iommu_domain *domain,
> > +			  struct device *dev, u64 *preserved_state);
> > +void iommu_unpreserve_device(struct iommu_domain *domain, struct device *dev);
>
> The naming scheme is inconsistent... Maybe it can be:
>
>  iommu_preserve_domain()
>  iommu_unpreserve_domain()
>  iommu_preserve_device() or iommu_preserve_device_attachment()
>  iommu_unpreserve_device() or iommu_unpreserve_device_attachment()

Agreed, but I am trying to follow the already existing naming scheme
for domains that is used for APIs in iommu.c

iommu_domain_free()
iommu_domain_init()
iommu_domain_preserve()

But I think this is rare, I will update to this as you mentioned above:

iommu_preserve_domain()
iommu_unpreserve_domain()
iommu_preserve_device()
iommu_unpreserve_device()
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 3853a3946733..1c424b32c5fc 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
>
> > +#ifdef CONFIG_IOMMU_LIVEUPDATE
> > +	int (*preserve_device)(struct device *dev, struct iommu_device_ser *device_ser);
> > +	void (*unpreserve_device)(struct device *dev, struct iommu_device_ser *device_ser);
> > +	int (*preserve)(struct iommu_device *iommu, struct iommu_hw_ser *iommu_ser);
> > +	void (*unpreserve)(struct iommu_device *iommu, struct iommu_hw_ser *iommu_ser);
> > +#endif
>
> Maybe we can make these names a little more specific:
>
>  preserve_device_attachment()
>  unpreserve_device_attachment()

Attachment is too specific. See my comment above.
>  preserve_iommu()
>  unpreserve_iommu()

These are part of iommu_ops and having preserve_iommu() instead of
preserve() is redundant I think. Note ops like capable(), hw_info() in
the same struct.
> ?