Re: [PATCH 1/6] dt-bindings: firmware: Add arm,errata-management

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 31/03/2023 18:58, James Morse wrote:
> Hi Krzysztof
> 
> On 31/03/2023 09:29, Krzysztof Kozlowski wrote:
>> On 30/03/2023 18:51, James Morse wrote:
>>> The Errata Management SMCCC interface allows firmware to advertise whether
>>> the OS is affected by an erratum, or if a higher exception level has
>>> mitigated the issue. This allows properties of the device that are not
>>> discoverable by the OS to be described. e.g. some errata depend on the
>>> behaviour of the interconnect, which is not visible to the OS.
>>>
>>> Deployed devices may find it significantly harder to update EL3
>>> firmware than the device tree. Erratum workarounds typically have to
>>> fail safe, and assume the platform is affected putting correctness
>>> above performance.
>>>
>>> Instead of adding a device-tree entry for any CPU errata that is
>>> relevant (or not) to the platform, allow the device-tree to describe
>>> firmware's responses for the SMCCC interface. This could be used as
>>> the data source for the firmware interface, or be parsed by the OS if
>>> the firmware interface is missing.
>>>
>>> Most errata can be detected from CPU id registers. These mechanisms
>>> are only needed for the rare cases that external knowledge is needed.
> 
>>> diff --git a/Documentation/devicetree/bindings/firmware/arm,errata-management.yaml b/Documentation/devicetree/bindings/firmware/arm,errata-management.yaml
>>> new file mode 100644
>>> index 000000000000..9baeb3d35213
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/firmware/arm,errata-management.yaml
>>> @@ -0,0 +1,77 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/firmware/arm,errata-management.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>
>> Except missing testing...
> 
> After a couple of hours of testing this, I went blind and missed that it was still
> complaining about spaces.
> 
> 
>>> +
>>> +title: Errata Management Firmware Interface
>>> +
>>> +maintainers:
>>> +  - James Morse <james.morse@arm.com>
>>> +
>>> +description: |+
>>
>> Do not need '|+'.
>>
>>> +  The SMC-CC has an erratum discovery interface that allows the OS to discover
>>> +  whether a particular CPU is affected by a specific erratum when the
>>> +  configurations affected is only known by firmware. See the specification of
>>> +  the same title on developer.arm.com, document DEN0100.
>>> +  Provide the values that should be used by the interface, either to supplement
>>> +  firmware, or override the values firmware provides.
>>
>> Why? If you have the discovery interface, don't add stuff to the DT, but
>> use that interface.
> 
> A DT property was explicitly requested by Marc Z on the RFC:
> https://lore.kernel.org/linux-arm-kernel/86mt5dxxbc.wl-maz@kernel.org/
> 
> The concern is that platforms where the CPU is affected, but the issue is masked by the
> interconnect will never bother with a firmware interface. The kernel can't know this, so
> has to enable the workaround at the cost of performance.

It would have to bother DT, so same problem... DT is not optimization
mechanism for SW decisions.

> 
> Adding a DT property to describe the hardware state of the erratum avoids the need for
> separate kernel builds to work around the missing firmware.

The purpose of DT is not to avoid separate kernel builds thus this is
not an argument whether property fits DT or it doesn't.

> 
> 
>>> +      - const: arm,cpu-erratum-list
>>> +
>>> +  arm,erratum-affected:
>>> +    description: Erratum numbers that this CPU is affected by.
>>
>> Isn't this explicit from CPU compatible?
> 
> I'll drop it from the compatible. The concern is arm add erratum in other IP to this
> interface, hence shoving 'cpu' in a few places to future proof it against changes in the spec.
> 
> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    minItems: 1
> 
>> What do the numbers mean?
> 
> The numbers are unique identifiers issued by the CPU designer to identify the part
> affected and the erratum. See the cover-letter for links to arms documents for all these
> CPUs, or Documentation/arm64/silicon-errata.txt for a table of those the kernel works around.

The answer should be in description, not in cover letter.

> 
> 
>> maxItems?
> 
> If there are zero entries, the table can be omitted, hence minItems.
> 
> This is the first erratum workaround that needs to know non-discoverable CPU properties,
> but there will be others.


Best regards,
Krzysztof