On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote:

Jarkko Sakkinen [off-list ref] writes:

quoted

On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:

quoted

On 7/10/23 17:23, Jarkko Sakkinen wrote:

quoted

On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:

quoted

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.

Adding more text is not the right way to clarify things. I'd start
by explaining shortly SML and then move to the handover. It can't
be that hard, right?

Just adding new paragraphs would probably just make it even more
confusing.

BR, Jarkko