[PATCH v5 01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers

[Will Deacon]

On Thu, Feb 15, 2024 at 08:11:38PM +0000, Robin Murphy wrote:
> On 2024-02-15 6:42 pm, Robin Murphy wrote:
> [...]
> > > > > +static void arm_smmu_get_ste_used(const __le64 *ent, __le64
> > > > > *used_bits)
> > > > >   {
> > > > > +    unsigned int cfg = FIELD_GET(STRTAB_STE_0_CFG,
> > > > > le64_to_cpu(ent[0]));
> > > > > +
> > > > > +    used_bits[0] = cpu_to_le64(STRTAB_STE_0_V);
> > > > > +    if (!(ent[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > > > > +        return;
> > > > > +
> > > > > +    /*
> > > > > +     * See 13.5 Summary of attribute/permission
> > > > > configuration fields for the
> > > > > +     * SHCFG behavior. It is only used for BYPASS,
> > > > > including S1DSS BYPASS,
> > > > > +     * and S2 only.
> > > > > +     */
> > > > > +    if (cfg == STRTAB_STE_0_CFG_BYPASS ||
> > > > > +        cfg == STRTAB_STE_0_CFG_S2_TRANS ||
> > > > > +        (cfg == STRTAB_STE_0_CFG_S1_TRANS &&
> > > > > +         FIELD_GET(STRTAB_STE_1_S1DSS, le64_to_cpu(ent[1])) ==
> > > > > +             STRTAB_STE_1_S1DSS_BYPASS))
> > > > > +        used_bits[1] |= cpu_to_le64(STRTAB_STE_1_SHCFG);
> > > > 
> > > > Huh, SHCFG is really getting in the way here, isn't it?
> > > 
> > > I wouldn't say that.. It is just a complicated bit of the spec. One of
> > > the things we recently did was to audit all the cache settings and, at
> > > least, we then realized that SHCFG was being subtly used by S2 as
> > > well..
> > 
> > Yeah, that really shouldn't be subtle; incoming attributes are replaced
> > by S1 translation, thus they are relevant to not-S1 configs.
> 
> That said, in this specific case I don't understand why we're worrying about
> SHCFG here at all - we're never going to make use of any value other than
> "use incoming" because we can't rely on it being implemented in the first
> place, and even if it is, we really don't want to start getting into the
> forced-coherency notion that the DMA layer can'#t understand and devicetree
> can't describe.

Yup, that's exactly what I'm thinking. We currently set it to NSH when
translation is enabled, so that the stage-2 shareability is effectively
an override. However, the device is either coherent, or it isn't, and so
we should just leave this always set to "use incoming" in my opinion,
which means we no longer need to care about qword 1 for the bypass case.

Will