Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian

[Michael Ellerman <mpe@ellerman.id.au>]

Stefan Berger <stefanb@linux.ibm.com> writes:
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
>> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>>> Measurement Log) from OF to Linux, before Linux shuts down Open
>>> Firmware.
>>>
>>> This involves creating a buffer to hold the SML, and creating two device
>>> tree properties to record its base address and size. The kernel then
>>> later reads those properties from the device tree to find the SML.
>>>
>>> When the code was initially added in commit 4a727429abec ("PPC64: Add
>>> support for instantiating SML from Open Firmware") the powerpc kernel
>>> was always built big endian, so the properties were created big endian
>>> by default.
>>>
>>> However since then little endian support was added to powerpc, and now
>>> the code lacks conversions to big endian when creating the properties.
>>>
>>> This means on little endian kernels the device tree properties are
>>> little endian, which is contrary to the device tree spec, and in
>>> contrast to all other device tree properties.
>>>
>>> To cope with that a workaround was added in tpm_read_log_of() to skip
>>> the endian conversion if the properties were created via the SML
>>> handover.
>>>
>>> A better solution is to encode the properties as big endian as they
>>> should be, and remove the workaround.
>>>
>>> Typically changing the encoding of a property like this would present
>>> problems for kexec. However the SML is not propagated across kexec, so
>>> changing the encoding of the properties is a non-issue.
>>>
>>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>>>   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>>>   2 files changed, 10 insertions(+), 21 deletions(-)
>> 
>> Split into two patches (producer and consumer).
>
> I think this wouldn't be right since it would break the system when
> only one patch is applied since it would be reading the fields in the
> wrong endianess.

Yes that's right.

Typically we would split the patch by subsystem, but in this case it
will just create a known-broken point in the git history, which helps no
one.

cheers