[PATCH 0/2] Preserve TPM log across kexec

This series resolves an issue on PowerVM and KVM on Power where the memory
the TPM log was held in may become inaccessible or corrupted after a kexec
soft reboot. The solution on these two platforms is to store the whole log
in the device tree because the device tree is preserved across a kexec with
either of the two kexec syscalls.

Regards,
   Stefan

Stefan Berger (2):
  powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
  tpm: of: If available Use linux,sml-log to get the log and its size

 arch/powerpc/kernel/prom_init.c |  8 ++------
 drivers/char/tpm/eventlog/of.c  | 36 ++++++++++-----------------------
 2 files changed, 13 insertions(+), 31 deletions(-)

-- 
2.43.0

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(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index e67effdba85c..41268c30de4c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
 
 	reserve_mem(base, size);
 
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
-
-	prom_debug("sml base     = 0x%llx\n", base);
+	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
+		     (void *)base, size);
 	prom_debug("sml size     = 0x%x\n", size);
 
 	prom_debug("prom_instantiate_sml: end...\n");

-- 
2.43.0

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/of.c | 36 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index 930fe43d5daf..e37196e64ef1 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c

@@ -54,8 +54,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	const u32 *sizep;
 	const u64 *basep;
 	struct tpm_bios_log *log;
+	const void *logp;
 	u32 size;
-	u64 base;
 
 	log = &chip->log;
 	if (chip->dev.parent && chip->dev.parent->of_node)

@@ -66,37 +66,23 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (of_property_read_bool(np, "powered-while-suspended"))
 		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
 
-	sizep = of_get_property(np, "linux,sml-size", NULL);
-	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (sizep == NULL && basep == NULL)
-		return tpm_read_log_memory_region(chip);
-	if (sizep == NULL || basep == NULL)
-		return -EIO;
-
-	/*
-	 * For both vtpm/tpm, firmware has log addr and log size in big
-	 * endian format. But in case of vtpm, there is a method called
-	 * sml-handover which is run during kernel init even before
-	 * device tree is setup. This sml-handover function takes care
-	 * of endianness and writes to sml-base and sml-size in little
-	 * endian format. For this reason, vtpm doesn't need conversion
-	 * but physical tpm needs the conversion.
-	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
-	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
+	logp = of_get_property(np, "linux,sml-log", &size);
+	if (logp == NULL) {
+		sizep = of_get_property(np, "linux,sml-size", NULL);
+		basep = of_get_property(np, "linux,sml-base", NULL);
+		if (sizep == NULL && basep == NULL)
+			return tpm_read_log_memory_region(chip);
+		if (sizep == NULL || basep == NULL)
+			return -EIO;
+		logp = __va(be64_to_cpup((__force __be64 *)basep));
 		size = be32_to_cpup((__force __be32 *)sizep);
-		base = be64_to_cpup((__force __be64 *)basep);
-	} else {
-		size = *sizep;
-		base = *basep;
 	}
-
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
 		return -EIO;
 	}
 
-	log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL);
+	log->bios_event_log = devm_kmemdup(&chip->dev, logp, size, GFP_KERNEL);
 	if (!log->bios_event_log)
 		return -ENOMEM;
 

-- 
2.43.0

On 3/6/24 10:55, Stefan Berger wrote:

This series resolves an issue on PowerVM and KVM on Power where the memory
the TPM log was held in may become inaccessible or corrupted after a kexec
soft reboot. The solution on these two platforms is to store the whole log
in the device tree because the device tree is preserved across a kexec with
either of the two kexec syscalls.

FYI: This was the previous attempt that didn't work with the older kexec syscall: https://lore.kernel.org/lkml/4afde78d-e138-9eee-50e0-dbd32f4dcfe0@linux.ibm.com/T/#m158630d214837e41858b03d4b025e6f96cb8f251

Regards,
    Stefan

Stefan Berger (2):
   powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log
   tpm: of: If available Use linux,sml-log to get the log and its size

  arch/powerpc/kernel/prom_init.c |  8 ++------
  drivers/char/tpm/eventlog/of.c  | 36 ++++++++++-----------------------
  2 files changed, 13 insertions(+), 31 deletions(-)

Stefan Berger [off-list ref] writes:

As we discussed via chat, doing it this way sucks the full content of
the log back into Open Firmware. 

That relies on OF handling such big properties, and also means more
memory will be consumed, which can cause problems early in boot.

A better solution is to explicitly add the log to the FDT in the
flattening phase.

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
 
 allOf:
   - $ref: tpm-common.yaml#

diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..616604707c95 100644
--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml

@@ -25,6 +25,11 @@ properties:
       base address of reserved memory allocated for firmware event log
     $ref: /schemas/types.yaml#/definitions/uint64
 
+  linux,sml-log:
+    description:
+      Content of firmware event log
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
   linux,sml-size:
     description:
       size of reserved memory allocated for firmware event log

@@ -53,15 +58,22 @@ dependentRequired:
   linux,sml-base: ['linux,sml-size']
   linux,sml-size: ['linux,sml-base']
 
-# must only have either memory-region or linux,sml-base
+# must only have either memory-region or linux,sml-base/size or linux,sml-log
 # as well as either resets or reset-gpios
 dependentSchemas:
   memory-region:
     properties:
       linux,sml-base: false
+      linux,sml-log: false
   linux,sml-base:
     properties:
       memory-region: false
+      linux,sml-log: false
+  linux,sml-log:
+    properties:
+      memory-region: false
+      linux,sml-base: false
+      linux,sml-size: false
   resets:
     properties:
       reset-gpios: false

cheers

Stefan Berger [off-list ref] writes:

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

It would be good to mention that powernv platforms (sometimes called
Open Power or bare metal) still use linux,sml-base/size, via skiboot.

cheers

On Wed, Mar 06, 2024 at 10:55:11AM -0500, Stefan Berger wrote:

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Those two properties are documented, but linux,sml-log is not, nor can I
find patches on the list documenting it.
There should be a patch adding this to tmp-common.yaml.

Cheers,
Conor.

On 3/7/24 05:42, Michael Ellerman wrote:

Stefan Berger [off-list ref] writes:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

It would be good to mention that powernv platforms (sometimes called
Open Power or bare metal) still use linux,sml-base/size, via skiboot.

Will do in v2.

On 3/7/24 05:41, Michael Ellerman wrote:

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(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index e67effdba85c..41268c30de4c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
  
  	reserve_mem(base, size);
  
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
-
-	prom_debug("sml base     = 0x%llx\n", base);
+	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
+		     (void *)base, size);

As we discussed via chat, doing it this way sucks the full content of
the log back into Open Firmware.

That relies on OF handling such big properties, and also means more
memory will be consumed, which can cause problems early in boot.

A better solution is to explicitly add the log to the FDT in the
flattening phase.

Done.

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

See my proposal below.

I have been working with this patch here now and it passes the following test:

make dt_binding_check dtbs_check DT_SCHEMA_FILES=tpm/ibm,vtpm.yaml

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

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

 allOf:
   - $ref: tpm-common.yaml#

@@ -102,3 +106,21 @@ examples:
             linux,sml-size = <0xbce10200>;
         };
     };
+  - |
+    soc {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tpm@30000003 {
+            compatible = "IBM,vtpm";
+            device_type = "IBM,vtpm";
+            reg = <0x30000003>;
+            interrupts = <0xa0003 0x0>;
+            ibm,#dma-address-cells = <0x2>;
+            ibm,#dma-size-cells = <0x2>;
+            ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
+            ibm,my-drc-index = <0x30000003>;
+            ibm,loc-code = "U8286.41A.10082DV-V3-C3";
+            linux,sml-log = <00 00 00 00 03 00 00>;
+        };
+    };

diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..591c48f8cb74 100644
--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml

@@ -30,6 +30,11 @@ properties:
       size of reserved memory allocated for firmware event log
     $ref: /schemas/types.yaml#/definitions/uint32

+  linux,sml-log:
+    description:
+      firmware event log
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
   memory-region:
     description: reserved memory allocated for firmware event log
     maxItems: 1

Is my patch missing something?

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

Looks pretty good other than that.

BR, Jarkko

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

BR, Jarkko

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

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.

So, I see only description of the problem but nothing how it gets
addressed.

BR, Jarkko

On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:

On 3/7/24 05:41, Michael Ellerman wrote:

quoted

Stefan Berger [off-list ref] writes:

Can you provide a more complete description here please as to what the
different between this and the other property? If I was populating a DT
I would have absolutely no idea whether or not to use this or the other
property, nor how to go about actually populating it.
The "log" in your example doesn't look like an actual log of any sort,
but I know nothing about TPMs so I'll take your word for it that that's
what a TPM log looks like.

+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
   memory-region:
     description: reserved memory allocated for firmware event log
     maxItems: 1


Is my patch missing something?

I think you also need the dependantSchema stuff you had in your original
snippet that makes the linux,* properties mutually exclusive with
memory-region (or at least something like that).

Please make sure you CC the DT maintainers and list on the v2 and Lukas
Wunner too.

Thanks,
Conor.

On 3/7/24 15:39, Conor Dooley wrote:

On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:

quoted

On 3/7/24 05:41, Michael Ellerman wrote:

quoted

Stefan Berger [off-list ref] writes:

quoted

quoted

diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml

b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..591c48f8cb74 100644

--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml

@@ -30,6 +30,11 @@ properties:
        size of reserved memory allocated for firmware event log
      $ref: /schemas/types.yaml#/definitions/uint32

+  linux,sml-log:
+    description:
+      firmware event log

Can you provide a more complete description here please as to what the
different between this and the other property? If I was populating a DT
I would have absolutely no idea whether or not to use this or the other
property, nor how to go about actually populating it.
The "log" in your example doesn't look like an actual log of any sort,
but I know nothing about TPMs so I'll take your word for it that that's
what a TPM log looks like.

In the example I cannot give a log but only a part of it. The log is in binary format and in case of TPM 2.0 starts with a header followed by log entries about what was measured. I don't think it's necessary to even give the full log header here. You do need some TPM specific knowledge about the 'firmware even log'.


The existing properties are described like this:

  linux,sml-base:
    description:
      base address of reserved memory allocated for firmware event log
    $ref: /schemas/types.yaml#/definitions/uint64

  linux,sml-size:
    description:
      size of reserved memory allocated for firmware event log
    $ref: /schemas/types.yaml#/definitions/uint32

Would this describe the new property 'better' by prefixing it with 'embedded'?

  linux,sml-log:
    description:
      embedded firmware event log
    $ref: /schemas/types.yaml#/definitions/uint8-array

quoted

+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
    memory-region:
      description: reserved memory allocated for firmware event log
      maxItems: 1


Is my patch missing something?

I think you also need the dependantSchema stuff you had in your original
snippet that makes the linux,* properties mutually exclusive with
memory-region (or at least something like that).

I modified my new example now like this:

...
            ibm,loc-code = "U9080.HEX.134CA08-V7-C3";
            linux,sml-log = <00 00 00 00 03 00 00>;
            linux,sml-size = <0xbce10200>;   <-- added

The check fails like this:

# make dt_binding_check dtbs_check DT_SCHEMA_FILES=tpm/ibm,vtpm.yaml
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTEX    Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dts
  DTC_CHK Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb
/root/linux/Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb: tpm@30000003: 'linux,sml-base' is a dependency of 'linux,sml-size'
        from schema $id: http://devicetree.org/schemas/tpm/tpm-common.yaml#
/root/linux/Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb: tpm@30000003: 'linux,sml-base' is a dependency of 'linux,sml-size'
        from schema $id: http://devicetree.org/schemas/tpm/ibm,vtpm.yaml#
/root/linux/Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb: tpm@30000003: Unevaluated properties are not allowed ('interrupts', 'linux,sml-log', 'linux,sml-size' were unexpected)
        from schema $id: http://devicetree.org/schemas/tpm/ibm,vtpm.yaml#



When I modify the existing example like this:

            ibm,loc-code = "U8286.41A.10082DV-V3-C3";
            linux,sml-base = <0xc60e 0x0>;
            linux,sml-size = <0xbce10200>;
            linux,sml-log = <00 00 00 00 03 00 00>;   <- added

The check fails like this:

# make dt_binding_check dtbs_check DT_SCHEMA_FILES=tpm/ibm,vtpm.yaml
  LINT    Documentation/devicetree/bindings
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTEX    Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dts
  DTC_CHK Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb
/root/linux/Documentation/devicetree/bindings/tpm/ibm,vtpm.example.dtb: tpm@30000003: More than one condition true in oneOf schema:
        {'$filename': '/root/linux/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml',
         '$id': 'http://devicetree.org/schemas/tpm/ibm,vtpm.yaml#',
         '$schema': 'http://devicetree.org/meta-schemas/core.yaml#',
         'allOf': [{'$ref': 'tpm-common.yaml#'}],
         'oneOf': [{'required': ['linux,sml-base', 'linux,sml-size']},
                   {'required': ['linux,sml-log']}],
         'patternProperties': {'pinctrl-[0-9]+': True},
         'properties': {'$nodename': True,
                        'bootph-all': True,
                        'bootph-pre-ram': True,
                        'bootph-pre-sram': True,
                        'bootph-some-ram': True,
                        'bootph-verify': True,
                        'compatible': {'items': [{'enum': ['IBM,vtpm',
                                                           'IBM,vtpm20']}],
                                       'maxItems': 1,
                                       'minItems': 1,
                                       'type': 'array'},
                        'device_type': {'items': [{'enum': ['IBM,vtpm',

'IBM,vtpm20']}],
                                        'maxItems': 1,
                                        'minItems': 1,
                                        'type': 'array'},
                        'ibm,#dma-address-cells': {'$ref': '/schemas/types.yaml#/definitions/uint32-array'},
                        'ibm,#dma-size-cells': {'$ref': '/schemas/types.yaml#/definitions/uint32-array'},
                        'ibm,loc-code': {'$ref': '/schemas/types.yaml#/definitions/string'},
                        'ibm,my-dma-window': {'$ref': '/schemas/types.yaml#/definitions/uint32-array',
                                              'items': {'maxItems': 5,
                                                        'minItems': 5},
                                              'maxItems': 1,
                                              'type': 'array'},
                        'ibm,my-drc-index': {'$ref': '/schemas/types.yaml#/definitions/uint32'},
                        'phandle': True,
                        'pinctrl-names': True,
                        'reg': {'maxItems': 1, 'minItems': 1},
                        'secure-status': True,
                        'status': True},
         'required': ['compatible',
                      'device_type',
                      'reg',
                      'interrupts',
                      'ibm,#dma-address-cells',
                      'ibm,#dma-size-cells',
                      'ibm,my-dma-window',
                      'ibm,my-drc-index',
                      'ibm,loc-code'],
         'select': {'properties': {'compatible': {'contains': {'enum': ['IBM,vtpm',

'IBM,vtpm20']}}},
                    'required': ['compatible']},
         'title': 'IBM Virtual Trusted Platform Module (vTPM)',
         'type': 'object',
         'unevaluatedProperties': False}
        from schema $id: http://devicetree.org/schemas/tpm/ibm,vtpm.yaml#


It errors out on bad examples, which is good.

Please make sure you CC the DT maintainers and list on the v2 and Lukas
Wunner too.

Yes, I have them already cc'ed here.

Thanks,
Conor.

On Thu, Mar 07, 2024 at 04:15:01PM -0500, Stefan Berger wrote:

On 3/7/24 15:39, Conor Dooley wrote:

quoted

On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:

quoted

On 3/7/24 05:41, Michael Ellerman wrote:

quoted

Stefan Berger [off-list ref] writes:

quoted

quoted

diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml

b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..591c48f8cb74 100644

--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml

@@ -30,6 +30,11 @@ properties:
        size of reserved memory allocated for firmware event log
      $ref: /schemas/types.yaml#/definitions/uint32

+  linux,sml-log:
+    description:
+      firmware event log

Can you provide a more complete description here please as to what the
different between this and the other property? If I was populating a DT
I would have absolutely no idea whether or not to use this or the other
property, nor how to go about actually populating it.
The "log" in your example doesn't look like an actual log of any sort,
but I know nothing about TPMs so I'll take your word for it that that's
what a TPM log looks like.

In the example I cannot give a log but only a part of it. The log is in
binary format and in case of TPM 2.0 starts with a header followed by log
entries about what was measured. I don't think it's necessary to even give
the full log header here. You do need some TPM specific knowledge about the
'firmware even log'.


The existing properties are described like this:

  linux,sml-base:
    description:
      base address of reserved memory allocated for firmware event log
    $ref: /schemas/types.yaml#/definitions/uint64

  linux,sml-size:
    description:
      size of reserved memory allocated for firmware event log
    $ref: /schemas/types.yaml#/definitions/uint32

Would this describe the new property 'better' by prefixing it with
'embedded'?

IMO, no that's not any better. Spell it out so that someone who doesn't
know his arse from his elbow when it comes to tpm immediately knows that
this means the entire tpm log is inside the dtb. The paragraph you wrote
above gives more information about what this property is populated with
than the property description does.

  linux,sml-log:
    description:
      embedded firmware event log
    $ref: /schemas/types.yaml#/definitions/uint8-array

quoted

quoted

+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
    memory-region:
      description: reserved memory allocated for firmware event log
      maxItems: 1


Is my patch missing something?

I think you also need the dependantSchema stuff you had in your original
snippet that makes the linux,* properties mutually exclusive with
memory-region (or at least something like that).

I modified my new example now like this:
...
            ibm,loc-code = "U9080.HEX.134CA08-V7-C3";
            linux,sml-log = <00 00 00 00 03 00 00>;
            linux,sml-size = <0xbce10200>;   <-- added

            ibm,loc-code = "U8286.41A.10082DV-V3-C3";
            linux,sml-base = <0xc60e 0x0>;
            linux,sml-size = <0xbce10200>;
            linux,sml-log = <00 00 00 00 03 00 00>;   <- added

It errors out on bad examples, which is good.

Aye, that is covered by your new oneOf for this one binding. The
dependantSchema bit in tpm-common.yaml enforces it for all tpm devices.
It also covers the memory-region property being mutually exclusive with
the linux,sml-{base,size} properties so I think you need to extend that
to also cover linux,sml-lof property.

quoted

Please make sure you CC the DT maintainers and list on the v2 and Lukas
Wunner too.

Yes, I have them already cc'ed here.

To: Conor Dooley <conor@kernel.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>, linux-integrity@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, conor.dooley@microchip.com, nayna@linux.ibm.com, Lukas Wunner <lukas@wunner.de>, linux-kernel@vger.kernel.org, jarkko@kernel.org,
        rnsastry@linux.ibm.com, peterhuewe@gmx.de, viparash@in.ibm.com

You have Lukas, one of the three DT maintainers and not the list as far
as I can see. Correct me please if I am wrong.

Thanks,
Conor.

On Wed, Mar 06, 2024 at 11:08:20AM -0500, Stefan Berger wrote:

On 3/6/24 10:55, Stefan Berger wrote:

quoted

This series resolves an issue on PowerVM and KVM on Power where the memory
the TPM log was held in may become inaccessible or corrupted after a kexec
soft reboot. The solution on these two platforms is to store the whole log
in the device tree because the device tree is preserved across a kexec with
either of the two kexec syscalls.

FYI: This was the previous attempt that didn't work with the older kexec
syscall: https://lore.kernel.org/lkml/4afde78d-e138-9eee-50e0-dbd32f4dcfe0@linux.ibm.com/T/#m158630d214837e41858b03d4b025e6f96cb8f251

Doesn't everyone else still need that? Is powerpc the only ones that 
care about the old kexec syscall?

Rob

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

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(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index e67effdba85c..41268c30de4c 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c

@@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
 
 	reserve_mem(base, size);
 
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
-	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
-
-	prom_debug("sml base     = 0x%llx\n", base);
+	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
+		     (void *)base, size);

As we discussed via chat, doing it this way sucks the full content of
the log back into Open Firmware. 

That relies on OF handling such big properties, and also means more
memory will be consumed, which can cause problems early in boot.

A better solution is to explicitly add the log to the FDT in the
flattening phase.

Why can't you just use /reserved-memory here? That should be preserved 
from one kernel entry to the next.

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

Why is this Linux specific?

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

On Wed, Mar 06, 2024 at 11:08:20AM -0500, Stefan Berger wrote:

quoted

On 3/6/24 10:55, Stefan Berger wrote:

quoted

This series resolves an issue on PowerVM and KVM on Power where the memory
the TPM log was held in may become inaccessible or corrupted after a kexec
soft reboot. The solution on these two platforms is to store the whole log
in the device tree because the device tree is preserved across a kexec with
either of the two kexec syscalls.

FYI: This was the previous attempt that didn't work with the older kexec
syscall: https://lore.kernel.org/lkml/4afde78d-e138-9eee-50e0-dbd32f4dcfe0@linux.ibm.com/T/#m158630d214837e41858b03d4b025e6f96cb8f251

Doesn't everyone else still need that? Is powerpc the only ones that

Are you referring to the old series with 'that' ? I more or less had to abandon it because it wouldn't solve the problem for the old kexec syscall, so that's why I am embedding the whole log now in the devicetree since the DT is properly carried across the kexec soft reboot on PowerVM and KVM for Power. Maybe other platforms will (have to) follow, but I don't know.

care about the old kexec syscall?

Rob

On 3/7/24 15:00, Jarkko Sakkinen wrote:

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

quoted

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

Ideally, yes. v3 will have 3 patches and all 3 of them will have to be backported *together* and not applied otherwise if any one of them fails. Can this be 'guaranteed'?

BR, Jarkko

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

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

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.

https://elixir.bootlin.com/linux/latest/source/arch/powerpc/kernel/prom_init.c#L1959

2) There is an example in the ibm,vtpm.yaml file that has both of these
and the test case still passes the check when the two entries above are removed. I will post v2 with the changes to the DT bindings for linux,sml-log including an example for linux,sml-log. [The test cases fail, as expected, when an additional property is added, such as when linux,sml-base is added when linux,sml-log is there or linux,sml-log is added when linux,sml-base is there.]

quoted

  
  allOf:
    - $ref: tpm-common.yaml#

diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
index 3c1241b2a43f..616604707c95 100644
--- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
+++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml

@@ -25,6 +25,11 @@ properties:
        base address of reserved memory allocated for firmware event log
      $ref: /schemas/types.yaml#/definitions/uint64
  
+  linux,sml-log:

Why is this Linux specific?

I am not sure about the criteria when to prefix with 'linux,' and when not to. In this case I did it because it is created by Linux and because of existing linux,sml-base and linux,sml-size.

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

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. There's already support for describing the TPM log 
that way anyways. The only reasoning I can see writing out a node for 
that is harder than just adding a property, but that's not a great 
argument IMO.

2) There is an example in the ibm,vtpm.yaml file that has both of these
and the test case still passes the check when the two entries above are
removed. I will post v2 with the changes to the DT bindings for
linux,sml-log including an example for linux,sml-log. [The test cases fail,
as expected, when an additional property is added, such as when
linux,sml-base is added when linux,sml-log is there or linux,sml-log is
added when linux,sml-base is there.]

Sure, removing a required property is never going to break the DT 
checks. What would break is a client (OS) version that only understands 
linux,sml-base and can no longer get the log assuming getting the log 
itself was required. 

Rob

On 3/8/24 15:57, Rob Herring wrote:

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

I am not familiar with /reserved-memory and whether it is supported on the targeted platforms.

On Fri Mar 8, 2024 at 2:17 PM EET, Stefan Berger wrote:

On 3/7/24 15:00, Jarkko Sakkinen wrote:

quoted

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

quoted

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

Ideally, yes. v3 will have 3 patches and all 3 of them will have to be 
backported *together* and not applied otherwise if any one of them 
fails. Can this be 'guaranteed'?

All of them will end up to stable if the following conditions hold:

- All have a fixes tag.
- All have "Cc: stable@vger.kernel.org".
- We agree in the review process that they are all legit fixes.

BR, Jarkko

Rob Herring [off-list ref] writes:

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.

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.

cheers

Stefan Berger [off-list ref] writes:

On 3/7/24 15:00, Jarkko Sakkinen wrote:

quoted

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

quoted

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

Ideally, yes. v3 will have 3 patches and all 3 of them will have to be 
backported *together* and not applied otherwise if any one of them 
fails. Can this be 'guaranteed'?

You can use Depends-on: <previous commit SHA> to indicate the relationship.

cheers

On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote:

Stefan Berger [off-list ref] writes:

quoted

On 3/7/24 15:00, Jarkko Sakkinen wrote:

quoted

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

quoted

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

Ideally, yes. v3 will have 3 patches and all 3 of them will have to be 
backported *together* and not applied otherwise if any one of them 
fails. Can this be 'guaranteed'?

You can use Depends-on: <previous commit SHA> to indicate the relationship.

cheers

Thanks, I've missed depends-on tag.

Stefan, please add also "Cc: stable@vger.kernel.org" just to make sure
that I don't forget to add it.

Right, and since these are so small scoped commits, and bug fixes in
particular, it is also possible to do PR during the release cycle.

BR, Jarkko

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

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.)

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.

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?

Rob

On 3/12/24 11:50, Jarkko Sakkinen wrote:

On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote:

quoted

Stefan Berger [off-list ref] writes:

quoted

On 3/7/24 15:00, Jarkko Sakkinen wrote:

quoted

On Thu Mar 7, 2024 at 9:57 PM EET, Jarkko Sakkinen wrote:

quoted

in short summary: s/Use/use/

On Wed Mar 6, 2024 at 5:55 PM EET, Stefan Berger wrote:

quoted

If linux,sml-log is available use it to get the TPM log rather than the
pointer found in linux,sml-base. This resolves an issue on PowerVM and KVM
on Power where after a kexec the memory pointed to by linux,sml-base may
have been corrupted. Also, linux,sml-log has replaced linux,sml-base and
linux,sml-size on these two platforms.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

So shouldn't this have a fixed tag, or not?

In English: do we want this to be backported to stable kernel releases or not?

Ideally, yes. v3 will have 3 patches and all 3 of them will have to be
backported *together* and not applied otherwise if any one of them
fails. Can this be 'guaranteed'?

You can use Depends-on: <previous commit SHA> to indicate the relationship.

cheers

Thanks, I've missed depends-on tag.

Stefan, please add also "Cc: stable@vger.kernel.org" just to make sure
that I don't forget to add it.

Yeah, once we know whether this is the way forward or not... I posted v2 as RFC to figure this out.

v2's 2/3 patch will only apply to 6.8. To avoid any inconsistencies between code and bindings we cannot even go further back with this series (IFF it's the way forward at all). So I am inclined to remove the Fixes tags. I also find little under Documentation about the Depends-on tag and what it's supposed to be formatted like -- a commit hash of 1/3 appearing in 2/3 for example? The commit hash is not stable at this point so I couldn't created it.

Right, and since these are so small scoped commits, and bug fixes in
particular, it is also possible to do PR during the release cycle.

BR, Jarkko

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