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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 12:45:20PM -0700, Nicolin Chen wrote:
> On Tue, Sep 22, 2026 at 01:12:37PM +0000, Mostafa Saleh wrote:
> > Move parsing of IDRs to functions so that it can be re-used
> > from the hypervisor.
> > 
> > As the new functions operate on structs from both the hypervisor
> > and the kernel which would be different, we rely on the compilation
> > unit to having ARM_SMMU_OBJ point to the correct struct; some
> 
> s/to having/to have

Will do.

> 
> > best-effort static asserts were added .
> 
> s/added \./added\.

Will do.

> 
> > +#ifndef __ARM_SMMU_V3_COMMON_LIB_H
> > +#define __ARM_SMMU_V3_COMMON_LIB_H
> > +
> > +#include <linux/build_bug.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/kernel.h>
> > +
> > +/*
> > + * The IDR probe functions are used by the kernel and the
> > + * hypervisor drivers where ARM_SMMU_OBJ might be defined
> > + * differently.
> > + * Ensure fields used by them are defined and has the correct
> > + * types.
> 
> s/has/have
> 
> We have 80 cols per line to write comments :)

Will do.

> 
> > + */
> > +#ifndef __KVM_NVHE_HYPERVISOR__
> > +typedef struct arm_smmu_device ARM_SMMU_OBJ;
> > +#endif
> 
> It's probably safer to include arm-smmu-v3.h so everything would
> be self-defined.
> 

Yes, I was not sure about that, I was thinking of making this file
included strictly after the struct is defined first but I didn't
find a suitable place for that.
So, I can just add the include here before the typedef.

> Also, Jason's suggestion in v7 was hyp_arm_smmu_v3_device, which
> looks nicer than ARM_SMMU_OBJ...
> 

I think having another name makes the code more readable (not sure if
some tools can be confused also) than renaming the hypervisor struct
to the kernel one. That makes it clear what is the intent of this.

> > +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, features), u32));
> > +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, options), u32));
> > +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, oas), unsigned long));
> > +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, pgsize_bitmap), unsigned long));
> > +static_assert(__same_type(typeof_member(ARM_SMMU_OBJ, base), void __iomem *));
> > +
> > +void arm_smmu_device_iidr_probe(ARM_SMMU_OBJ *smmu);
> > +u32 arm_smmu_idr0_probe(ARM_SMMU_OBJ *smmu);
> > +void arm_smmu_idr3_probe(ARM_SMMU_OBJ *smmu);
> > +u32 arm_smmu_idr5_probe(ARM_SMMU_OBJ *smmu);
> 
> Can we use "arm_smmu_device_xyz_probe" matching with the existing
> arm_smmu_device_iidr_probe?

Sure.

> 
> > +	if (coherent && !disable_msipolling &&
> > +	    smmu->features & ARM_SMMU_FEAT_MSI)
> > +		smmu->options |= ARM_SMMU_OPT_MSIPOLL;
> 
> Will pKVM ever use MSIPOLL?

No, this version does not support MSI and hides it.
And this check can not be moved because disable_msipolling is a
module_param.
Although it might be possible to pass it as an argument to the
function and assume (smmu->features & ARM_SMMU_FEAT_COHERENCY) is set
based on FW before the IDR probe similarly, no strong opinion, so this
part can all be moved as is.

> 
> > +	if (smmu->features & ARM_SMMU_FEAT_HYP &&
> > +	    cpus_have_cap(ARM64_HAS_VIRT_HOST_EXTN))
> > +		smmu->features |= ARM_SMMU_FEAT_E2H;
> 
> Why is ARM64_HAS_VIRT_HOST_EXTN left behind?
> 

cpus_have_cap() can not be used in the hypervisor.
Also, ARM_SMMU_FEAT_E2H is not exactly FEAT_HYP. As it defines the
world the translation lives in based on the kernel EL.

With pKVM at EL2 ARM64_HAS_VIRT_HOST_EXTN is always true anyway.
And the hypervisor never owns a page table itself, so it never
checks this feature.
Otherwise, I think we can move this check and use cpus_have_final_cap()
instead as it can be used in the hypervisor.

> > -	if (!(reg & (IDR0_S1P | IDR0_S2P))) {
> > +	if (!(smmu->features & (ARM_SMMU_FEAT_TRANS_S1 | ARM_SMMU_FEAT_TRANS_S2))) {
> >  		dev_err(smmu->dev, "no translation support!\n");
> >  		return -ENXIO;
> 
> This change seems unnecessary. The code above and below this line
> still uses "reg" returned by idr0_probe(). So, the original code
> should have read well:

True, I will change it back.

Thanks,
Mostafa
> 
>         if (!!(reg & IDR0_COHACC) != coherent)
>                 dev_warn(smmu->dev, "IDR0.COHACC overridden by FW configuration (%s)\n",
>                          str_true_false(coherent));
> 
> 	if (!(reg & (IDR0_S1P | IDR0_S2P))) {
>                 dev_err(smmu->dev, "no translation support!\n");
>                 return -ENXIO;
>         }
> 
>         /* We only support the AArch64 table format at present */
>         if (!(FIELD_GET(IDR0_TTF, reg) & IDR0_TTF_AARCH64)) {
>                 dev_err(smmu->dev, "AArch64 table format not supported!\n");
>                 return -ENXIO;
>         }
> 
> Nicolin