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

[James Morse <james.morse@arm.com>]

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.

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.


>> +      - 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.


> 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.


Thanks,

James