Re: [PATCH v5 15/18] iommufd: Persist iommu hardware pagetables for live update

[Samiullah Khawaja]

On Wed, Oct 07, 2026 at 02:31:47PM -0700, Nicolin Chen wrote:
> On Mon, Sep 21, 2026 at 12:48:31AM +0000, Samiullah Khawaja wrote:
> > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> > index a33b32708afa..a4ddc29ec5ae 100644
> > --- a/drivers/iommu/iommufd/iommufd_private.h
> > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > @@ -98,6 +98,9 @@ struct io_pagetable {
> >  	/* IOVA that cannot be allocated, struct iopt_reserved */
> >  	struct rb_root_cached reserved_itree;
> >  	u8 disable_large_pages;
> > +#ifdef CONFIG_IOMMU_LIVEUPDATE
> > +	u32 nr_preserved_domains;
> > +#endif
>
> In iommufd, "num_" is used more often than "nr_".

Will update.
> >  	unsigned long iova_alignment;
> >  };
> >
> > @@ -398,6 +401,7 @@ struct iommufd_hwpt_paging {
> >  	bool enforce_cache_coherency : 1;
> >  	bool nest_parent : 1;
> >  #ifdef CONFIG_IOMMU_LIVEUPDATE
> > +	bool liveupdate_preserved;
> >  	u64 liveupdate_token;
> >  #endif
> >  	/* Head at iommufd_ioas::hwpt_list */
>
> > +static inline bool iopt_liveupdate_immutable(const struct io_pagetable *iopt)
> > +{
> > +	return iopt->nr_preserved_domains > 0;
> > +}
>
> It's used in io_pagetable file only. I'd move it out of the header:
>
> static inline bool iopt_liveupdate_immutable(const struct io_pagetable *iopt)
> {
> 	bool immutable = false;
>
> 	lockdep_assert_held(&iopt->domains_rwsem);
>
> #ifdef CONFIG_IOMMU_LIVEUPDATE
> 	/* iopt becomes immutable once liveupdate preservation starts */
> 	immutable = iopt->num_preserved_domains;
> #endif
> 	return immutable;
> }

Will update.
> > diff --git a/drivers/iommu/iommufd/liveupdate.c b/drivers/iommu/iommufd/liveupdate.c
> > +static bool ioas_set_immutable(struct iommufd_ioas *ioas, bool set)
> > +{
> > +	bool was_immutable;
> > +
> > +	down_write(&ioas->iopt.domains_rwsem);
> > +	was_immutable = ioas->iopt.nr_preserved_domains > 0;
> > +	if (set)
> > +		ioas->iopt.nr_preserved_domains++;
> > +	else if (!WARN_ON(!was_immutable))
> > +		ioas->iopt.nr_preserved_domains--;
> > +
> > +	up_write(&ioas->iopt.domains_rwsem);
> > +
> > +	return was_immutable;
> > +}
>
> Since it has to handle refcount, an unset doesn't really unset the
> flag, which is confusing.
>
> Maybe just:
>  iopt_inc_num_preserved_domains
>  iopt_dec_num_preserved_domains

Sounds good. Will update.
> > +static int check_iopt_pages_preserved(struct liveupdate_session *s,
> > +				      struct iommufd_hwpt_paging *hwpt)
>
> static int iopt_validate_preserved_memory(struct io_pagetable *iopt,
> 					  struct liveupdate_session *s)

Will update.
> > +{
> > +	u32 req_seals = F_SEAL_SEAL | F_SEAL_GROW | F_SEAL_SHRINK;
> > +	struct iopt_area *area;
> > +	int ret = 0;
> > +
> > +	down_read(&hwpt->ioas->iopt.iova_rwsem);
> > +	for (area = iopt_area_iter_first(&hwpt->ioas->iopt, 0, ULONG_MAX); area;
> > +	     area = iopt_area_iter_next(area, 0, ULONG_MAX)) {
> > +		struct iopt_pages *pages = area->pages;
>
> Move req_seals inside the for loop.

Will update.
> > +static int iommufd_liveupdate_preserve(struct liveupdate_file_op_args *args)
> [...]
> > +	iommufd_ser = mem;
> > +	iommufd_ser->nr_hwpts = nr_hwpts;
> > +
> > +	/* Preserve HWPTs */
> > +	i = 0;
> > +	xa_lock(&ictx->objects);
> > +	xa_for_each_marked(&ictx->objects, index, obj, IOMMUFD_OBJ_LIVEUPDATE_MARK) {
> [...]
> > +	}
> > +	xa_unlock(&ictx->objects);
> > +
> > +	/* Store the actual number of HWPTs that are preserved */
> > +	iommufd_ser->nr_hwpts = i;
>
> iommufd_ser->nr_hwpts is set twice. First one seems redundant.

Agreed.
> > +++ b/include/linux/kho/abi/iommufd.h
> > +/**
> > + * struct iommu_hwpt_ser - IOMMUFD HWPT serialized state
> > + * @domain_data: Physical address of the serialized state of associated domain
>
> Then, it could be domain_ser_phys?

Agreed.
> > + * @token: User provided token
> > + * @reclaimed: Whether the HWPT is reclaimed
>
> Once the hwpt is reclaimed, could we clear domain_data to indicate
> "it is reclaimed" instead of a separate field?

Agreed.
> Nicolin

Thanks,
Sami