Jarkko Sakkinen <jarkko@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> > > Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>> > > ---
>> > > 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
