Received-SPF: pass (google.com: domain of linux-kernel+bounces-100331-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <cbe526b1-b044-48ac-896e-6037a85171ea@linux.ibm.com>
Date: Tue, 12 Mar 2024 10:12:41 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v2 2/3] dt-bindings: tpm: Add linux,sml-log to
 ibm,vtpm.yaml
Content-Language: en-US
To: Lukas Wunner <lukas@wunner.de>
Cc: mpe@ellerman.id.au, linux-integrity@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        jarkko@kernel.org, rnsastry@linux.ibm.com, peterhuewe@gmx.de,
        viparash@in.ibm.com, devicetree@vger.kernel.org, jsnitsel@redhat.com,
        Nayna Jain <nayna@linux.ibm.com>
References: <20240311132030.1103122-1-stefanb@linux.ibm.com>
 <20240311132030.1103122-3-stefanb@linux.ibm.com> <ZfA4TZspY7oOQ4vz@wunner.de>
From: Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <ZfA4TZspY7oOQ4vz@wunner.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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