Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

On 3/12/24 12:22, Rob Herring wrote:

On Tue, Mar 12, 2024 at 09:32:50PM +1100, Michael Ellerman wrote:

quoted

Rob Herring [off-list ref] writes:

quoted

On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:

quoted

On 3/7/24 16:52, Rob Herring wrote:

quoted

On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:

quoted

Stefan Berger [off-list ref] writes:

quoted

linux,sml-base holds the address of a buffer with the TPM log. This
buffer may become invalid after a kexec and therefore embed the whole TPM
log in linux,sml-log. This helps to protect the log since it is properly
carried across a kexec with both of the kexec syscalls.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
   arch/powerpc/kernel/prom_init.c | 8 ++------
   1 file changed, 2 insertions(+), 6 deletions(-)

quoted

quoted

Also adding the new linux,sml-log property should be accompanied by a
change to the device tree binding.

The syntax is not very obvious to me, but possibly something like?

diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
index 50a3fd31241c..cd75037948bc 100644
--- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
+++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml

@@ -74,8 +74,6 @@ required:
     - ibm,my-dma-window
     - ibm,my-drc-index
     - ibm,loc-code
-  - linux,sml-base
-  - linux,sml-size

Dropping required properties is an ABI break. If you drop them, an older
OS version won't work.

1) On PowerVM and KVM on Power these two properties were added in the Linux
code. I replaced the creation of these properties with creation of
linux,sml-log (1/2 in this series). I also replaced the handling of
these two (2/2 in this series) for these two platforms but leaving it for
powernv systems where the firmware creates these.

Okay, I guess your case is not a ABI break if the kernel is populating
it and the same kernel consumes it.

You failed to answer my question on using /reserved-memory. Again, why
can't that be used? That is the standard way we prevent chunks of memory
from being clobbered.

Yes I think that would mostly work. I don't see support for
/reserved-memory in kexec-tools, so that would need fixing I think.

My logic was that the memory is not special. It's just a buffer we
allocated during early boot to store the log. There isn't anything else
in the system that relies on that memory remaining untouched. So it
seemed cleaner to just put the log in the device tree, rather than a
pointer to it.

My issue is we already have 2 ways to describe the log to the OS. I
don't see a good reason to add a 3rd way. (Though it might actually be a
4th way, because the chosen property for the last attempt was accepted
to dtschema yet the code has been abandoned.)

I have a revert for this in my tree...

If you put the log into the DT, then the memory for the log remains
untouched too because the FDT remains untouched. For reserved-memory
regions, the OS is free to free them if it knows what the region is and
that it is no longer needed. IOW, if freeing the log memory is desired,
then the suggested approach doesn't work.

The log, once embedded in the FDT, would stay there forever. The TPM subsystem looks at it when initializing and will look at it again when initializing after a kexec soft reboot.

quoted

Having the log external to the device tree creates several problems,
like the crash kernel region colliding with it, it being clobbered by
kexec, etc.

We have multiple regions to pass/maintain thru kexec, so how does having
one less really matter?

I had tried it with the newer kexec syscall. Yes, there was a way of doing it but the older kexec call is still around and since that one is also still being used the problems with corrupted memory for example still persisted. So that's why we now embed it in the FDT because both syscalls carry that across unharmed.

Rob