| Message ID | 20240311132030.1103122-3-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Preserve TPM log across kexec | expand |
On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote: > Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to > the properties. Either this property is required or both linux,sml-base and > linux,sml-size are required. Add a test case for verification. > > Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation") The Fixes tag is confusing. The patch won't even apply cleanly to the v4.10 commit referenced here as the conversion to yaml happened only recently with v6.8. Why is the Fixes tag necessary in the first place? Same question for the other patches in the series. This looks like feature work rather than a fix. Not sure whether it satisfies the "obviously correct" rule per Documentation/process/stable-kernel-rules.rst. > --- 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 I assume that either these two or the new "linux,sml-log" property are (still) required? If so, a quick grep through the bindings (e.g. auxdisplay/img,ascii-lcd.yaml) shows that the following might work: required: - ... oneOf: - required: - linux,sml-base - required: - linux,sml-log > --- 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: > + Content of firmware event log Please add one or two sentences of context so that readers don't need to use git blame + git log to find out what this is for. (Mention at least that the property may be used to pass the log to a kexec kernel.) > -# 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 Could you add "linux,sml-size: false" to "memory-region" as well while at it for consistency? Thanks, Lukas
On 3/12/24 07:11, Lukas Wunner wrote: > On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote: >> Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to >> the properties. Either this property is required or both linux,sml-base and >> linux,sml-size are required. Add a test case for verification. >> >> Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation") > > The Fixes tag is confusing. The patch won't even apply cleanly to the > v4.10 commit referenced here as the conversion to yaml happened only > recently with v6.8. Then that's as far back (6.8) as the series may be applied. I put the Fixes tag on the first appearance of sml-base/sml-size since for kexec this was never correct. > > Why is the Fixes tag necessary in the first place? Same question for > the other patches in the series. This looks like feature work rather > than a fix. Not sure whether it satisfies the "obviously correct" > rule per Documentation/process/stable-kernel-rules.rst. It is a fix for the interaction of the TPM firmware log with kexec. The sml-base buffer pointer was never protected across a kexec. > > >> --- 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 > > I assume that either these two or the new "linux,sml-log" property > are (still) required? If so, a quick grep through the bindings > (e.g. auxdisplay/img,ascii-lcd.yaml) shows that the following > might work: > > required: > - ... > > oneOf: > - required: > - linux,sml-base > - required: > - linux,sml-log > You're right, they need to be here since examples could now omit sml-base or sml-log. I added them. Thanks. > >> --- 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: >> + Content of firmware event log > > Please add one or two sentences of context so that readers don't > need to use git blame + git log to find out what this is for. > (Mention at least that the property may be used to pass the log > to a kexec kernel.) Ok, will do: Content of firmware event log embedded in device tree to be safely carried across a kexec soft reboot. > > >> -# 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 > > Could you add "linux,sml-size: false" to "memory-region" as well > while at it for consistency? Done. Thanks. Stefan > > Thanks, > > Lukas
On Tue Mar 12, 2024 at 1:11 PM EET, Lukas Wunner wrote: > On Mon, Mar 11, 2024 at 09:20:29AM -0400, Stefan Berger wrote: > > Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to > > the properties. Either this property is required or both linux,sml-base and > > linux,sml-size are required. Add a test case for verification. > > > > Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation") > > The Fixes tag is confusing. The patch won't even apply cleanly to the > v4.10 commit referenced here as the conversion to yaml happened only > recently with v6.8. > > Why is the Fixes tag necessary in the first place? Same question for > the other patches in the series. This looks like feature work rather > than a fix. Not sure whether it satisfies the "obviously correct" > rule per Documentation/process/stable-kernel-rules.rst. I'm not yet sure whether these are bug fixes and or improvements because I did not fully understand the scenario where TPM corrupts the event log so that part reminds to be seen. Probably once I fully understand what is going on, it is possible to argue on that. BR, Jarkko
diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml index 50a3fd31241c..8885ef3b7638 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# @@ -102,3 +100,21 @@ examples: linux,sml-size = <0xbce10200>; }; }; + - | + soc { + #address-cells = <1>; + #size-cells = <0>; + + tpm@30000003 { + compatible = "IBM,vtpm20"; + 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 = "U9080.HEX.134CA08-V7-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..f6f0b551268c 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: + Content of firmware event log + $ref: /schemas/types.yaml#/definitions/uint8-array + memory-region: description: reserved memory allocated for firmware event log maxItems: 1 @@ -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
Add linux,sml-log, which carries the firmware TPM log in a uint8-array, to the properties. Either this property is required or both linux,sml-base and linux,sml-size are required. Add a test case for verification. Fixes: 82003e0487fb ("Documentation: tpm: add the IBM Virtual TPM device tree binding documentation") Cc: Lukas Wunner <lukas@wunner.de> Cc: Nayna Jain <nayna@linux.ibm.com> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- .../devicetree/bindings/tpm/ibm,vtpm.yaml | 20 +++++++++++++++++-- .../devicetree/bindings/tpm/tpm-common.yaml | 14 ++++++++++++- 2 files changed, 31 insertions(+), 3 deletions(-)