Re: [PATCH 2/2] iommu/arm-smmu-v3: Override for Inst/Data attribute

Hi Nicolin,

Thank you for your review feedback.

On Fri, 24 Jul 2026 at 19:05, Nicolin Chen <[email protected]> wrote:
>
> On Fri, Jul 24, 2026 at 01:39:43PM +0100, Peter Griffin wrote:
> > @@ -1206,7 +1207,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> >                       cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
> >                                   STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
> >                                   STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
> > -                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
> > +                                 STRTAB_STE_1_EATS | STRTAB_STE_1_MEV |
> > +                                 STRTAB_STE_1_INSTCFG);
> >               used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);
> >
> >               /*
> > @@ -1222,7 +1224,8 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
> >       if (cfg & BIT(1)) {
> >               used_bits[1] |=
> >                       cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS |
> > -                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV);
> > +                                 STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV |
> > +                                 STRTAB_STE_1_INSTCFG);
> >               used_bits[2] |=
> >                       cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR |
> >                                   STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
>
> This adds for "stage-1" and "stage-2", while missing "bypass"?

I need to follow up again on this point, but I don't believe currently
we require the override for Laguna when it's in bypass.

>
> > @@ -1835,7 +1838,11 @@ void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> >                        STRTAB_STE_1_S1STALLD :
> >                        0) |
> >               FIELD_PREP(STRTAB_STE_1_EATS,
> > -                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> > +                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0)) |
> > +             FIELD_PREP(STRTAB_STE_1_INSTCFG,
> > +                        smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> > +                                STRTAB_STE_1_INSTCFG_DATA :
> > +                                STRTAB_STE_1_INSTCFG_INCOMING);
>
> Sashiko pointed out that this prematurely closes the cpu_to_le64()
> macro.

Will fix in v2

>
> > @@ -1887,7 +1894,11 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> >
> >       target->data[1] = cpu_to_le64(
> >               FIELD_PREP(STRTAB_STE_1_EATS,
> > -                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0));
> > +                        ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> > +             FIELD_PREP(STRTAB_STE_1_INSTCFG,
> > +                        smmu->options & ARM_SMMU_OPT_OVR_INSTCFG_DATA ?
> > +                                STRTAB_STE_1_INSTCFG_DATA :
> > +                                STRTAB_STE_1_INSTCFG_INCOMING));
>
> You might need to set in arm_smmu_make_bypass_ste() too.
>
> > @@ -5087,6 +5098,13 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> >       if (smmu->sid_bits <= STRTAB_SPLIT)
> >               smmu->features &= ~ARM_SMMU_FEAT_2_LVL_STRTAB;
> >
> > +     if (reg & IDR1_ATTR_PERMS_OVR) {
> > +             smmu->features |= ARM_SMMU_FEAT_PERMS_OVR;
>
> Where does this new feature bit get used?

Good point, the feature flag isn't actually used anywhere else except
here. The intention is to ensure the DT property
"arm,instdata-override" is not specified for a SMMU which doesn't
support the override feature. But we can perform that test without
adding a feature flag that is otherwise unused. Will fix in the next
version.

Thanks,

Peter