| Message ID | 20240306155511.974517-2-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Preserve TPM log across kexec | expand |
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
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?
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
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.
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.
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.
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
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.
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
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.
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
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
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
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");
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(-)