Re: [PATCH v5 04/15] iommupt: Add the AMD IOMMU v1 page table format

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Sep 18, 2025 at 07:05:50AM +0000, Tian, Kevin wrote:
> >  256*2^30,    396,5665  ,     389,5258    ,  92.92
> 
> data here mismatches those in coverletter, though the difference
> didn't affect the conclusion. 😊

I ran it twice, it isn't stable in micro :)

> > +if IOMMU_PT
> > +config IOMMU_PT_AMDV1
> > +	tristate "IOMMU page table for 64-bit AMD IOMMU v1"
> 
> remove "64-bit"? I don't think there is a 32-bit format ever.

I am marking the 64 bit vs 32 bit in the kconfig descriptions since
that seemed to be a pattern from iopgtable

> > +
> > +static inline unsigned int amdv1pt_table_item_lg2sz(const struct pt_state
> > *pts)
> > +{
> > +	return PT_GRANULE_LG2SZ +
> > +	       (PT_TABLEMEM_LG2SZ - ilog2(PT_ITEM_WORD_SIZE)) * pts-
> > >level;
> > +}
> > +#define pt_table_item_lg2sz amdv1pt_table_item_lg2sz
> 
> this is the same as in pt_fmt_defaults.h

Yeah, I left these behind because the order of declaration is
inconvenient, the format itself wants to call this function a few
lines down.

But I just realized it can be forward declared:

/* Body in pt_fmt_defaults.h */
static inline unsigned int pt_table_item_lg2sz(const struct pt_state *pts);

So that's fixed.

> > +static inline void
> > +amdv1pt_install_leaf_entry(struct pt_state *pts, pt_oaddr_t oa,
> > +			   unsigned int oasz_lg2,
> > +			   const struct pt_write_attrs *attrs)
> > +{
> > +	unsigned int isz_lg2 = pt_table_item_lg2sz(pts);
> > +	u64 *tablep = pt_cur_table(pts, u64) + pts->index;
> 
> check that the index is aligned to oasz_log2

I see the various formats have a mismatched collection of assertions,
I added this function:

/*
 * Format can call in the pt_install_leaf_entry() to check the arguments are all
 * aligned correctly.
 */
static inline bool pt_check_install_leaf_args(struct pt_state *pts,
					      pt_oaddr_t oa,
					      unsigned int oasz_lg2)
{
	unsigned int isz_lg2 = pt_table_item_lg2sz(pts);

	if (PT_WARN_ON(oalog2_mod(oa, oasz_lg2)))
		return false;

#ifdef pt_possible_sizes
	if (PT_WARN_ON(isz_lg2 < oasz_lg2 ||
		       oasz_lg2 > isz_lg2 + pt_num_items_lg2(pts)))
		return false;
#else
	if (PT_WARN_ON(oasz_lg2 != isz_lg2 &&
		       oasz_lg2 != isz_lg2 + pt_contig_count_lg2(pts)))
		return false;
#endif

	if (PT_WARN_ON(oalog2_mod(pts->index, oasz_lg2 - isz_lg2)))
		return false;
	return true;
}

And the format can just call it.

Made both these edits to all the formats..

Thanks,
Jason