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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Aug 25, 2026 at 05:59:50PM -0300, Jason Gunthorpe wrote:
> > [ ... 21 lines skipped ... ]
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-common-lib.c
> > @@ -0,0 +1,184 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2015 ARM Limited
> > + *
> > + * Author: Will Deacon <will.deacon@arm.com>
> > + * Arm SMMUv3 driver functions shared with hypervisor.
> > + */
> > +
> > +#include "arm-smmu-v3.h"
> > +#include <asm-generic/errno-base.h>
> > +
> > +#include <linux/string.h>
> > +
> > +u32 smmu_idr0_features(u32 reg)
> > +{
> 
> I think you should do more to make this shared C file usable by both
> compiles with more of the infrastructure available.
> 
> For instance, something like this at the top:
> 
>  #ifdef __KVM_NVHE_HYPERVISOR__
>  #include "pkvm/arm_smmu_v3.h"
>  #define arm_smmu_device  hyp_arm_smmu_v3_device
>  #else
>  #include "arm-smmu-v3.h"
>  #endif
> 
> (heh you used _ and the driver uses - in file names)

Yes, that's a bit ugly, I can rename it to arm-smmu-v3-hyp.h

> 
> Then the existing code using 'arm_smmu_device' just works fine since
> the hyp version has the same structure members. Now just move the code
> as-is.
> 
> Otherwise we still have alot of cut and paste duplication:
> 
> 	reg = readl_relaxed(smmu->base + ARM_SMMU_IDR0);
> 
> 	smmu->features |= smmu_idr0_features(reg);
> 	if (!(smmu->features & (ARM_SMMU_FEAT_TT_LE | ARM_SMMU_FEAT_TT_BE))) {
> 		dev_err(smmu->dev, "unknown/unsupported TT endianness!\n");
> 		return -ENXIO;
> 	}
> 
> vs
> 
> 	reg = readl_relaxed(smmu->base + ARM_SMMU_IDR0);
> 
> 	smmu->features |= smmu_idr0_features(reg);
> 	if (!(smmu->features & (ARM_SMMU_FEAT_TT_LE | ARM_SMMU_FEAT_TT_BE)))
> 		return -ENXIO;

I am not sure I get this, the only difference is the printk?

We can do more with that, but as discussed last time, I am worried that
re-defining structs using macros just makes things harder for
maintenance and more fragile to changes, causing build failures,
and mismatch in types between the 2 structs.
I can experiment with that when working on the next version and see
if the there is significant benefits.

Thanks,
Mostafa

> 
> Then you can put the prior patches tlbi stuff here instead of the
> header file, which is important because after I get the CONT and
> errata stuff worked in it becomes really quite big.
> 
> -- 
> Jason