[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cbe526b1-b044-48ac-896e-6037a85171ea@linux.ibm.com>
Date: Tue, 12 Mar 2024 10:12:41 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: mpe@...erman.id.au, linux-integrity@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
jarkko@...nel.org, rnsastry@...ux.ibm.com, peterhuewe@....de,
viparash@...ibm.com, devicetree@...r.kernel.org, jsnitsel@...hat.com,
Nayna Jain <nayna@...ux.ibm.com>
Subject: Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to
ibm,vtpm.yaml
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
Powered by blists - more mailing lists