Re: [PATCH v4 07/18] iommu/vt-d: Implement device and iommu preserve/unpreserve ops

[Samiullah Khawaja]

On Wed, Aug 26, 2026 at 03:10:31PM +0800, Baolu Lu wrote:
> On 8/8/26 10:27, Samiullah Khawaja wrote:
> > Add implementation of the device and iommu presevation in a separate
> > file. Also set the device and iommu preserve/unpreserve ops in the
> > struct iommu_ops.
>
> This patch, on its own, does not fully implement the preserve operation.
> At this point, the callback preserves all root/context entries used for
> live update, but it does not yet clear non-preserved entries and
> invalidate the related caches; that cleanup is supposed to be done in
> the next patch.
>
> So I think we should either:
>
> - add a brief note here to make this dependency explicit, or
> - merge these two patches into a single patch for completeness.
>
> ?

That is a good point. I kept the cleanup patch separate as it has to do
with shutdown and basically getting ready to go into kexec. This patch
mostly focuses on the "preservation of the memory" being used by various
IOMMU data structures.

But I agree, we should add a note about the cleanup here. I will do that
in the next revision.
> > Signed-off-by: Samiullah Khawaja <skhawaja@xxxxxxxxxx>
> > ---

[snip]

> > +#else
> > +static inline int intel_iommu_preserve_device(struct device *dev,
> > +					      struct iommu_device_ser *device_ser)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline void intel_iommu_unpreserve_device(struct device *dev,
> > +						 struct iommu_device_ser *device_ser)
> > +{
> > +}
> > +
> > +static inline int intel_iommu_preserve(struct iommu_device *iommu,
> > +				       struct iommu_hw_ser *iommu_ser)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline void intel_iommu_unpreserve(struct iommu_device *iommu,
> > +					  struct iommu_hw_ser *iommu_ser)
> > +{
> > +}
> > +#endif
>
> I believe the #else ... #endif stubs are unnecessary and can be removed.
>
> Specially iommu core checks driver support via callback pointer
> presence:
>
>        if (!iommu->iommu_dev->ops->preserve_device ||
>            !iommu->iommu_dev->ops->unpreserve_device ||
>            !iommu->iommu_dev->ops->preserve ||
>            !iommu->iommu_dev->ops->unpreserve)
>                return -EOPNOTSUPP
>
> So when CONFIG_IOMMU_LIVEUPDATE is disabled, simply not wiring these
> callbacks is sufficient.

Agreed. Will remove these.
> > +
> >  #ifdef CONFIG_INTEL_IOMMU_SVM
> >  void intel_svm_check(struct intel_iommu *iommu);
> >  struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
> > +

[snip]

> > +/**
> > + * intel_iommu_unpreserve_device() - Intel IOMMU callback to unpreserve device state
> > + * @dev: Target device
> > + * @device_ser: Struct containing serialized device state
> > + */
> > +void intel_iommu_unpreserve_device(struct device *dev,
> > +				   struct iommu_device_ser *device_ser)
> > +{
> > +}
>
> Please add a comment explaining why this callback is intentionally
> empty.

Agreed. Will add in the next revision.
> > +
> > +/**
> > + * intel_iommu_preserve() - Intel IOMMU callback to preserve hardware state
> > + * @iommu_dev: Generic IOMMU device handle
> > + * @ser: Struct to populate with serialized hardware state
> > + *
> > + * Return: 0 on success, or negative error code.
> > + */
> > +int intel_iommu_preserve(struct iommu_device *iommu_dev,
> > +			 struct iommu_hw_ser *ser)
> > +{
> > +	struct intel_iommu *iommu;
> > +	int ret;
> > +

[snip]

> >  /**
> >   * struct iommu_hw_ser - Serialized state of an IOMMU instance
> >   * @hdr: Common object header
> >   * @token: Unique token for the IOMMU
> >   * @type: IOMMU type serialized state belongs to
> > + * @intel: Intel specific serialization data
> >   */
> >  struct iommu_hw_ser {
> >  	struct iommu_hdr_ser hdr;
> >  	u64 token;
> >  	u64 type;
> > +	union {
> > +		struct iommu_intel_ser intel;
> > +	};
> >  } __packed;
> >  /**
>
> Thanks,
> baolu

Thanks,
Sami