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

[Stefan Berger]

On 3/7/24 05:41, Michael Ellerman wrote:
> Stefan Berger <stefanb@xxxxxxxxxxxxx> writes:
> > linux,sml-base holds the address of a buffer with the TPM log. This
> > buffer may become invalid after a kexec and therefore embed the whole TPM
> > log in linux,sml-log. This helps to protect the log since it is properly
> > carried across a kexec with both of the kexec syscalls.
> >
> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > ---
> >   arch/powerpc/kernel/prom_init.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> > index e67effdba85c..41268c30de4c 100644
> > --- a/arch/powerpc/kernel/prom_init.c
> > +++ b/arch/powerpc/kernel/prom_init.c
> > @@ -1956,12 +1956,8 @@ static void __init prom_instantiate_sml(void)
> >   
> >   	reserve_mem(base, size);
> >   
> > -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> > -		     &base, sizeof(base));
> > -	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> > -		     &size, sizeof(size));
> > -
> > -	prom_debug("sml base     = 0x%llx\n", base);
> > +	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-log",
> > +		     (void *)base, size);
>
> As we discussed via chat, doing it this way sucks the full content of
> the log back into Open Firmware.
>
> That relies on OF handling such big properties, and also means more
> memory will be consumed, which can cause problems early in boot.
>
> A better solution is to explicitly add the log to the FDT in the
> flattening phase.

Done.

> Also adding the new linux,sml-log property should be accompanied by a
> change to the device tree binding.


See my proposal below.

> The syntax is not very obvious to me, but possibly something like?
>
> diff --git a/Documentation/devicetree/bindings/tpm/ibm,vtpm.yaml b/Documentation/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/Documentation/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

I have been working with this patch here now and it passes the following 
test:

make dt_binding_check dtbs_check DT_SCHEMA_FILES=tpm/ibm,vtpm.yaml


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

 allOf:
   - $ref: tpm-common.yaml#
@@ -102,3 +106,21 @@ examples:
             linux,sml-size = <0xbce10200>;
         };
     };
+  - |
+    soc {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tpm@30000003 {
+            compatible = "IBM,vtpm";
+            device_type = "IBM,vtpm";
+            reg = <0x30000003>;
+            interrupts = <0xa0003 0x0>;
+            ibm,#dma-address-cells = <0x2>;
+            ibm,#dma-size-cells = <0x2>;
+            ibm,my-dma-window = <0x10000003 0x0 0x0 0x0 0x10000000>;
+            ibm,my-drc-index = <0x30000003>;
+            ibm,loc-code = "U8286.41A.10082DV-V3-C3";
+            linux,sml-log = <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

+  linux,sml-log:
+    description:
+      firmware event log
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+
   memory-region:
     description: reserved memory allocated for firmware event log
     maxItems: 1


Is my patch missing something?