Re: [PATCH 05/14] iommupt: Implement preserve/unpreserve/restore callbacks

[Jason Gunthorpe]

On Fri, Apr 10, 2026 at 11:02:52PM +0000, Samiullah Khawaja wrote:
> On Fri, Apr 10, 2026 at 11:16:52AM -0300, Jason Gunthorpe wrote:
> > On Fri, Mar 20, 2026 at 09:57:08PM +0000, Pranjal Shrivastava wrote:
> > > > +static int __restore_tables(struct pt_range *range, void *arg,
> > > > +			    unsigned int level, struct pt_table_p *table)
> > > > +{
> > > > +	struct pt_state pts = pt_init(range, level, table);
> > > > +	int ret;
> > > > +
> > > > +	for_each_pt_level_entry(&pts) {
> > > > +		if (pts.type == PT_ENTRY_TABLE) {
> > > > +			iommu_restore_page(virt_to_phys(pts.table_lower));
> > > > +			ret = pt_descend(&pts, arg, __restore_tables);
> > > > +			if (ret)
> > > > +				return ret;
> > > 
> > > If pt_descend() returns an error, we immediately return ret. However, we
> > > have already successfully called iommu_restore_page() on pts.table_lower
> > > and potentially many other tables earlier in the loop or higher up in
> > > the tree..
> > 
> > It doesn't return an error, it just propogates errors from the
> > callbacks which this one never errors. So this is just dead code.
> > 
> > > > +int DOMAIN_NS(restore)(struct iommu_domain *domain, struct iommu_domain_ser *ser)
> > > > +{
> > > > +	struct pt_iommu *iommu_table =
> > > > +		container_of(domain, struct pt_iommu, domain);
> > > > +	struct pt_common *common = common_from_iommu(iommu_table);
> > > > +	struct pt_range range = pt_all_range(common);
> > > > +
> > > > +	iommu_restore_page(ser->top_table);
> > > > +
> > > > +	/* Free new table */
> > > > +	iommu_free_pages(range.top_table);
> > > > +
> > > > +	/* Set the restored top table */
> > > > +	pt_top_set(common, phys_to_virt(ser->top_table), ser->top_level);
> > > > +
> > > > +	/* Restore all pages*/
> > > > +	range = pt_all_range(common);
> > > > +	return pt_walk_range(&range, __restore_tables, NULL);
> > 
> > This should probably be doing something with the FEAT flags and
> > ias/oas too or do you imagine the calling driver has to deal with
> > that?
> 
> During boot the iommu_domain is recreated in driver and it sets up the
> FEAT flags and ias/oas properly. Then this generic callback is used to
> restore the page tables.
> 
> Currently the FEAT flags of a domain are not explicitly preserved, I
> will preserve them and error out here if there is a mismatch.

Hrm, that expands the ABI a bit though

If the only operation on the restored table is free then I suppose it
can be simplified quite a bit, you just need the minimal things that
the collect walker in free touches.

Is that the intention, free only?

If so then the restored iommu_domain should be some special free only
domain too.

Jason