Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size with linux,sml-log

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4578 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]