Received-SPF: pass (google.com: domain of linux-kernel+bounces-96209-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 7 Mar 2024 20:39:51 +0000
From: Conor Dooley <conor@kernel.org>
To: Stefan Berger <stefanb@linux.ibm.com>
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
Subject: Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size
 with linux,sml-log
Message-ID: <20240307-cytoplasm-compare-6656aae737ac@spud>
References: <20240306155511.974517-1-stefanb@linux.ibm.com>
 <20240306155511.974517-2-stefanb@linux.ibm.com>
 <87jzmenx2c.fsf@mail.lhotse>
 <768fc5f1-3919-477e-a8e6-16a7e8536add@linux.ibm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="IDMXI+tGPlolqedq"
Content-Disposition: inline
In-Reply-To: <768fc5f1-3919-477e-a8e6-16a7e8536add@linux.ibm.com>


--IDMXI+tGPlolqedq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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:

> >=20
> > Also adding the new linux,sml-log property should be accompanied by a
> > change to the device tree binding.
>=20
>=20
> See my proposal below.
>=20
> >=20
> > The syntax is not very obvious to me, but possibly something like?
> >=20
> > diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml b/Docu=
mentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> > index 50a3fd31241c..cd75037948bc 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#
> > diff --git a/Documentation/devicetree/bindings/tpm/tpm-common.yaml b/Do=
cumentation/devicetree/bindings/tpm/tpm-common.yaml
> > index 3c1241b2a43f..616604707c95 100644
> > --- a/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > +++ b/Documentation/devicetree/bindings/tpm/tpm-common.yaml
> > @@ -25,6 +25,11 @@ properties:
> >         base address of reserved memory allocated for firmware event log
> >       $ref: /schemas/types.yaml#/definitions/uint64
> > +  linux,sml-log:
> > +    description:
> > +      Content of firmware event log
> > +    $ref: /schemas/types.yaml#/definitions/uint8-array
> > +
> >     linux,sml-size:
> >       description:
> >         size of reserved memory allocated for firmware event log
> > @@ -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
> >=20
> >=20
>=20
> I have been working with this patch here now and it passes the following
> test:
>=20
> make dt_binding_check dtbs_check DT_SCHEMA_FILES=3Dtpm/ibm,vtpm.yaml
>=20
>=20
> diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> index 50a3fd31241c..cacf6c3082de 100644
> --- a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> +++ b/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml
> @@ -74,8 +74,12 @@ required:
>    - ibm,my-dma-window
>    - ibm,my-drc-index
>    - ibm,loc-code
> -  - linux,sml-base
> -  - linux,sml-size
> +oneOf:
> +  - required:
> +      - linux,sml-base
> +      - linux,sml-size
> +  - required:
> +      - linux,sml-log
>=20
>  allOf:
>    - $ref: tpm-common.yaml#
> @@ -102,3 +106,21 @@ examples:
>              linux,sml-size =3D <0xbce10200>;
>          };
>      };
> +  - |
> +    soc {
> +        #address-cells =3D <1>;
> +        #size-cells =3D <0>;
> +
> +        tpm@30000003 {
> +            compatible =3D "IBM,vtpm";
> +            device_type =3D "IBM,vtpm";
> +            reg =3D <0x30000003>;
> +            interrupts =3D <0xa0003 0x0>;
> +            ibm,#dma-address-cells =3D <0x2>;
> +            ibm,#dma-size-cells =3D <0x2>;
> +            ibm,my-dma-window =3D <0x10000003 0x0 0x0 0x0 0x10000000>;
> +            ibm,my-drc-index =3D <0x30000003>;
> +            ibm,loc-code =3D "U8286.41A.10082DV-V3-C3";
> +            linux,sml-log =3D <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..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
>=20
> +  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.

> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +
>    memory-region:
>      description: reserved memory allocated for firmware event log
>      maxItems: 1
>=20
>=20
> 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).

Please make sure you CC the DT maintainers and list on the v2 and Lukas
Wunner too.

Thanks,
Conor.

--IDMXI+tGPlolqedq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZeomFgAKCRB4tDGHoIJi
0rDjAQDCS+FWUSIjoa3pw2hIV6BHFi7ckp38kgze/iTx08jC2wD/Y6NcNqHRNxyH
uqytmYK1UqfYlV6C3HtkWQilNytz6QA=
=5EVs
-----END PGP SIGNATURE-----

--IDMXI+tGPlolqedq--