Re: [PATCH v2 1/3] iommu/io-pgtable-arm: Move selftests to a separate file

[Mostafa Saleh <smostafa@google.com>]

On Wed, Sep 17, 2025 at 04:38:18PM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 17, 2025 at 07:11:38PM +0000, Mostafa Saleh wrote:
> > +static void __init arm_lpae_dump_ops(struct io_pgtable_ops *ops)
> > +{
> > +	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> 
> Can be:
> 
>        struct io_pgtable_cfg *cfg =
>                &io_pgtable_ops_to_pgtable(pgtbl_ops)->cfg;

There are a bunch of other formatting issues here also, but I wanted to
move the code as is, so with “--color-moved” you can see the exact
difference, otherwise, it’s harder to review.

I can add a patch before to fix those + the printing as you suggested
next.

> 
> > +
> > +	pr_err("cfg: pgsize_bitmap 0x%lx, ias %u-bit\n",
> > +		cfg->pgsize_bitmap, cfg->ias);
> > +	pr_err("data: %d levels, 0x%zx pgd_size, %u pg_shift, %u bits_per_level, pgd @ %p\n",
> > +		ARM_LPAE_MAX_LEVELS - data->start_level, ARM_LPAE_PGD_SIZE(data),
> > +		ilog2(ARM_LPAE_GRANULE(data)), data->bits_per_level, data->pgd);
> 
> The entire struct arm_lpae_io_pgtable is exposed to a public header
> just for this one print.. Seems undesirable.
> 
> Drop the print? Honestly, I prefer this, given the maturity let's not
> compromise modularity to print someting nobody will ever read..
> 
> Alternatively call a kunit-only exported function to do the print
> directly from ops so. smmuv3 has an example how to do that
> 
> Either way a precursor patch to adjust it will allow this patch to
> avoid publishing most stuff.

I don’t have a strong opinion about this, but I am more inclined to
keep the prints considering it’s a low-level test for the page table,
and such parameters would be useful to understand the failures.

Moving arm_lpae_dump_ops() to the core library will leak the kunit
struct, but we can drop the kunit_err and rely on pr_err().

I know this is not relevant to this series, but the KVM driver will
need to expose arm_lpae_io_pgtable anyway.

>
> > +/*
> > + * Calculate the right shift amount to get to the portion describing level l
> > + * in a virtual address mapped by the pagetable in d.
> > + */
> > +#define ARM_LPAE_LVL_SHIFT(l,d)						\
> > +	(((ARM_LPAE_MAX_LEVELS - (l)) * (d)->bits_per_level) +		\
> > +	ilog2(sizeof(arm_lpae_iopte)))
> 
> Didn't see this being used?

Yes, my bad, I will move it and the other one back.

Thanks,
Mostafa

> 
> > +#define ARM_LPAE_GRANULE(d)						\
> > +	(sizeof(arm_lpae_iopte) << (d)->bits_per_level)
> 
> Only used by the above print
> 
> > +#define ARM_LPAE_PGD_SIZE(d)						\
> > +	(sizeof(arm_lpae_iopte) << (d)->pgd_bits)
> 
> Only used by the above print
> 
> > +#define ARM_LPAE_PTES_PER_TABLE(d)					\
> > +	(ARM_LPAE_GRANULE(d) >> ilog2(sizeof(arm_lpae_iopte)))
> 
> Didn't see this being used?
> 
> > +typedef u64 arm_lpae_iopte;
> 
> Only used by the defines and the above print.
> 
> Jason