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

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

Jarkko Sakkinen <jarkko@kernel.org> writes:
> On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
>> 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.
>
> I think it would help if the commit message would better explain
> what is going on. It is somewhat difficult to decipher, if you
> don't have deep knowledge of the powerpc architecture.

I mean, it's already 8 paragraphs ¯\_(ツ)_/¯

But I'm happy to expand it. I just don't really know what extra detail
is needed to make it clearer.

cheers