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 <[email protected]>
---
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 <[email protected]>
---
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/[email protected]/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 <[email protected]> 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 <[email protected]>
> ---
> 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
Stefan Berger <[email protected]> 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 <[email protected]>
> ---
> 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 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 <[email protected]>
> ---
> 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/7/24 05:42, Michael Ellerman wrote:
> Stefan Berger <[email protected]> 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.
On 3/7/24 05:41, Michael Ellerman wrote:
> Stefan Berger <[email protected]> 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 <[email protected]>
>> ---
>> 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?
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 <[email protected]>
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
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 <[email protected]>
>
> 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.
>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> 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
On Thu, Mar 07, 2024 at 10:11:03AM -0500, Stefan Berger wrote:
> On 3/7/24 05:41, Michael Ellerman wrote:
> > Stefan Berger <[email protected]> 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.
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 <[email protected]> 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.
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 <[email protected]> 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 <[email protected]>
Cc: Michael Ellerman <[email protected]>, [email protected], [email protected], [email protected], [email protected], Lukas Wunner <[email protected]>, [email protected], [email protected],
[email protected], [email protected], [email protected]
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:
> > 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/[email protected]/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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
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/[email protected]/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:
>> 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 <[email protected]>
>>
>> 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:
>> Stefan Berger <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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.
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 <[email protected]>
> >>
> >> 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: [email protected]".
- We agree in the review process that they are all legit fixes.
BR, Jarkko
Rob Herring <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > > > ---
>> > > > 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
Stefan Berger <[email protected]> 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 <[email protected]>
>>>
>>> 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 <[email protected]> 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 <[email protected]>
> >>>
> >>> 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: [email protected]" 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> > > > ---
> >> > > > 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
On 3/12/24 11:50, Jarkko Sakkinen wrote:
> On Tue Mar 12, 2024 at 12:35 PM EET, Michael Ellerman wrote:
>> Stefan Berger <[email protected]> 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 <[email protected]>
>>>>>
>>>>> 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: [email protected]" 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:
>> Rob Herring <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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