Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to ibm,vtpm.yaml

[Stefan Berger <stefanb@linux.ibm.com>]

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