Re: [PATCH v6 05/25] iommu/arm-smmu-v3: Move IDR parsing to common functions

[Mostafa Saleh <smostafa@google.com>]

On Wed, May 06, 2026 at 06:56:31AM -0300, Jason Gunthorpe wrote:
> On Tue, May 05, 2026 at 04:48:08PM +0000, Mostafa Saleh wrote:
> > On Tue, May 05, 2026 at 01:27:45PM -0300, Jason Gunthorpe wrote:
> > > On Mon, May 04, 2026 at 12:16:13PM +0000, Mostafa Saleh wrote:
> > > > On Fri, May 01, 2026 at 09:47:16AM -0300, Jason Gunthorpe wrote:
> > > > > On Fri, May 01, 2026 at 11:19:07AM +0000, Mostafa Saleh wrote:
> > > > > > Move parsing of IDRs to functions so that it can be re-used
> > > > > > +unsigned long smmu_idr5_to_pgsize(u32 reg)
> > > > > > +{
> > > > > > +	unsigned long pgsize_bitmap = 0;
> > > > > > +
> > > > > > +	if (reg & IDR5_GRAN64K)
> > > > > > +		pgsize_bitmap |= SZ_64K | SZ_512M;
> > > > > > +	if (reg & IDR5_GRAN16K)
> > > > > > +		pgsize_bitmap |= SZ_16K | SZ_32M;
> > > > > > +	if (reg & IDR5_GRAN4K)
> > > > > > +		pgsize_bitmap |= SZ_4K | SZ_2M | SZ_1G;
> > > > > > +	return pgsize_bitmap;
> > > > > > +}
> > > > > 
> > > > > I think this should include:
> > > > > 
> > > > > > +	smmu->oas = smmu_idr5_to_oas(reg);
> > > > > > +	if (smmu->oas == 52)
> > > > > >  		smmu->pgsize_bitmap |= 1ULL << 42; /* 4TB */
> > > > > > -		break;
> > > > > 
> > > > > ie it should return the supported page sizes by inspecting all the
> > > > > idrs and don't leave this tricky bit to be open coded..
> > > > 
> > > > This way was easier as each function only returns one thing, otherwise
> > > > we have to pass stuff by address as we can’t pass the smmu struct as it
> > > > is not shared between the drivers.
> > > > But no strong opinion, I can change that.
> > > 
> > > Could you share the struct? With multi-compilation you do get two
> > > structs with the same name and different layout, it is OK?
> > > 
> > > That would give a cleaner setup too because you can just shove all
> > > this into a function and have it setup the same struct which is more a
> > > copy and paste than break up into little functions?
> > 
> > Yes, that’s possible, but I didn't want to contaminate the main struct,
> > also we can’t compile the main one for KVM because of things such as
> > “struct device, struct mutex...”
> > May we can do:
> > - struct arm_smmu_device_common: Basic stuff (ioaddress, features,
> >   oas...)
> > - arm_smmu_device: For the main driver which inherits arm_smmu_device_common
> > - arm_smmu_nested_device for KVM which also inherits arm_smmu_device_common
> 
> Less keen on this, it would need more usages to justify it
>  
> > What do you think?
> 
> I was thinking more like
> 
> #define arm_smmu arm_pkvm_smmu
> 
> Before the pkvm compile of the shared code. 

Oh, so we rely on arm_pkvm_smmu and arm_smmu having the same field
names and types?

I don’t think that’s better for maintainability, I can look into it
if Robin and Will are OK with that.

> 
> How does pkvm compilation work anyhow? Is it all rolled into a unique
> link, or do you have to worry about symbol conflicts with the main
> kernel?

All pkvm symbols are prefixed with __kvm_nvhe_ in the main kernel.

Thanks,
Mostafa

> 
> Jason