Received-SPF: pass (google.com: domain of linux-kernel+bounces-96291-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 7 Mar 2024 15:52:14 -0600
From: Rob Herring <robh@kernel.org>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Stefan Berger <stefanb@linux.ibm.com>, 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
Subject: Re: [PATCH 1/2] powerpc/prom_init: Replace linux,sml-base/sml-size
 with linux,sml-log
Message-ID: <20240307215214.GB3110385-robh@kernel.org>
References: <20240306155511.974517-1-stefanb@linux.ibm.com>
 <20240306155511.974517-2-stefanb@linux.ibm.com>
 <87jzmenx2c.fsf@mail.lhotse>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87jzmenx2c.fsf@mail.lhotse>

On Thu, Mar 07, 2024 at 09:41:31PM +1100, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> 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@linux.ibm.com>
> > ---
> >  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.
> 

Why can't you just use /reserved-memory here? That should be preserved 
from one kernel entry to the next.


> Also adding the new linux,sml-log property should be accompanied by a
> change to the device tree binding.
> 
> 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

Dropping required properties is an ABI break. If you drop them, an older 
OS version won't work.

>  
>  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:

Why is this Linux specific?

> +    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
> 
> 
> cheers