[PATCH 0/2] Preserve TPM log across kexec

[PATCH 0/2] Preserve TPM log across kexec

[Stefan Berger]

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

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

[Stefan Berger]

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

[PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Stefan Berger]

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

Re: [PATCH 0/2] Preserve TPM log across kexec

[Stefan Berger]

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

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

[Michael Ellerman]

Stefan Berger <stefanb@linux.ibm.com> writes:
> 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.

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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Michael Ellerman]

Stefan Berger <stefanb@linux.ibm.com> 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

> 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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3296 bytes --]

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Stefan Berger]

On 3/7/24 05:42, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> 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.

Will do in v2.

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

[Stefan Berger]

On 3/7/24 05:41, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> writes:
>> 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.

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

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?

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

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?

> ---
>  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;
>  

Looks pretty good other than that.

BR, Jarkko

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

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:
> > 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

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

[Jarkko Sakkinen]

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.

>
> 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");

BR, Jarkko

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5497 bytes --]

On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
> On 3/7/24 05:41, Michael Ellerman wrote:
> > Stefan Berger <stefanb@linux.ibm.com> writes:

> > 
> > Also adding the new linux,sml-log property should be accompanied by a
> > change to the device tree binding.
> 
> 
> See my proposal below.
> 
> > 
> > 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
> > 
> > 
> 
> 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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Stefan Berger]

On 3/7/24 15:39, Conor Dooley wrote:
> On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
>> On 3/7/24 05:41, Michael Ellerman wrote:
>>> Stefan Berger <stefanb@linux.ibm.com> writes:
> 
>>>
>> 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


> 
>> +    $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.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4578 bytes --]

On Thu, Mar 07, 2024 at 04:15:01PM -0500, Stefan Berger wrote:
> 
> 
> On 3/7/24 15:39, Conor Dooley wrote:
> > On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
> > > On 3/7/24 05:41, Michael Ellerman wrote:
> > > > Stefan Berger <stefanb@linux.ibm.com> writes:
> > 
> > > > 
> > > 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
> 
> 
> > 
> > > +    $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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/2] Preserve TPM log across kexec

[Rob Herring]

On Wed, Mar 06, 2024 at 11:08:20AM -0500, Stefan Berger wrote:
> 
> 
> 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

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

Rob

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

[Rob Herring]

On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> writes:
> > 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.


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

>  
>  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?

> +    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

Re: [PATCH 0/2] Preserve TPM log across kexec

[Stefan Berger]

On 3/7/24 16:42, Rob Herring wrote:
> On Wed, Mar 06, 2024 at 11:08:20AM -0500, Stefan Berger wrote:
>>
>>
>> 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
> 
> 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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Stefan Berger]

On 3/7/24 15:00, Jarkko Sakkinen wrote:
> 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:
>>> 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

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

[Stefan Berger]

On 3/7/24 16:52, Rob Herring wrote:
> On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
>> Stefan Berger <stefanb@linux.ibm.com> writes:
>>> 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(-)
>>>

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

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

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

[Rob Herring]

On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:
> 
> 
> On 3/7/24 16:52, Rob Herring wrote:
> > On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
> > > Stefan Berger <stefanb@linux.ibm.com> writes:
> > > > 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(-)
> > > > 
> 
> > 
> > 
> > > 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

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

[Stefan Berger]

On 3/8/24 15:57, Rob Herring wrote:
> On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:
>>
>>
>> On 3/7/24 16:52, Rob Herring wrote:
>>> On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
>>>> Stefan Berger <stefanb@linux.ibm.com> writes:
>>>>> 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(-)
>>>>>
>>
>>>
>>>
>>>> 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.

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

On Fri Mar 8, 2024 at 2:17 PM EET, Stefan Berger wrote:
>
>
> On 3/7/24 15:00, Jarkko Sakkinen wrote:
> > 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:
> >>> 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

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

[Michael Ellerman]

Rob Herring <robh@kernel.org> writes:
> On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:
>> On 3/7/24 16:52, Rob Herring wrote:
>> > On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
>> > > Stefan Berger <stefanb@linux.ibm.com> writes:
>> > > > 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(-)
>> > > > 
>> 
>> > 
>> > 
>> > > 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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Michael Ellerman]

Stefan Berger <stefanb@linux.ibm.com> writes:
> On 3/7/24 15:00, Jarkko Sakkinen wrote:
>> 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:
>>>> 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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Jarkko Sakkinen]

On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> writes:
> > On 3/7/24 15:00, Jarkko Sakkinen wrote:
> >> 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:
> >>>> 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

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

[Rob Herring]

On Tue, Mar 12, 2024 at 09:32:50PM +1100, Michael Ellerman wrote:
> Rob Herring <robh@kernel.org> writes:
> > On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:
> >> On 3/7/24 16:52, Rob Herring wrote:
> >> > On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
> >> > > Stefan Berger <stefanb@linux.ibm.com> writes:
> >> > > > 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(-)
> >> > > > 
> >> 
> >> > 
> >> > 
> >> > > 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

Re: [PATCH 2/2] tpm: of: If available Use linux,sml-log to get the log and its size

[Stefan Berger]

On 3/12/24 11:50, Jarkko Sakkinen wrote:
> On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote:
>> Stefan Berger <stefanb@linux.ibm.com> writes:
>>> On 3/7/24 15:00, Jarkko Sakkinen wrote:
>>>> 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:
>>>>>> 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

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

[Stefan Berger]

On 3/12/24 12:22, Rob Herring wrote:
> On Tue, Mar 12, 2024 at 09:32:50PM +1100, Michael Ellerman wrote:
>> Rob Herring <robh@kernel.org> writes:
>>> On Fri, Mar 08, 2024 at 07:23:35AM -0500, Stefan Berger wrote:
>>>> On 3/7/24 16:52, Rob Herring wrote:
>>>>> On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
>>>>>> Stefan Berger <stefanb@linux.ibm.com> writes:
>>>>>>> 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(-)
>>>>>>>
>>>>
>>>>>
>>>>>
>>>>>> 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.

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