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

Robin Murphy
2024-02-15T20:11:57Z

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.

We're still unconditionally setting the "use incoming" value for MTCFG,
ALLOCCFG, PRIVCFG and INSTCFG without checking them, so there's no logic
in pretending SHCFG is any different from its peers simply because its
encoding is slightly less convenient. If the micro-optimisation of not
setting it when we know it's going to be ignored anyway starts getting
in the way, just drop that.

Thanks,
Robin.