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

[Robin Murphy <robin.murphy@arm.com>]

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.