Re: [PATCH] ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI core

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Oct 20, 2017 at 12:23:25PM +0100, Ard Biesheuvel wrote:
> DEBUG builds of PEI code will print a diagnostic message regarding
> the utilization of temporary RAM before switching to permanent RAM.
> For example,
> 
>   Total temporary memory:    16352 bytes.
>     temporary memory stack ever used:       4820 bytes.
>     temporary memory heap used for HobList: 4720 bytes.
> 
> Tracking stack utilization like this requires the stack to be seeded
> with a known magic value, and this needs to occur before entering C
> code, given that it uses the stack. Currently, only Nt32Pkg appears
> to implement this feature, but it is useful nonetheless, so let's
> wire it up for PrePeiCore.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S |  7 +++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 10 ++++++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 10 ++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> index aab5edab0c42..7a33e2754869 100644
> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> @@ -13,6 +13,8 @@
>  
>  #include <AsmMacroIoLibV8.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA55AA55AA5
> +
>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -84,4 +86,9 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, x1
> +  MOV64 (x8, FixedPcdGet64(PcdCPUCoresStackBase))
> +  MOV64 (x9, INIT_CAR_VALUE)
> +0:stp   x9, x9, [x8], #16
> +  cmp   x8, x1
> +  b.lt  0b
>    b     _PrepareArguments
> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> index 14344425ad4c..7342e49bea59 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> @@ -13,6 +13,8 @@
>  
>  #include <AsmMacroIoLib.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA5
> +

Worth moving to a common header somewhere?

Also defined/used in MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.

That file has an explicit comment saying "temporary memory is filled
with this initial value during SEC phase". Should this end have a
corresponding comment saying "checked for during PEI phase"?

/
    Leif

>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -65,6 +67,14 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, r1
> +  MOV32 (r8, FixedPcdGet64(PcdCPUCoresStackBase))
> +  MOV32 (r9, INIT_CAR_VALUE)
> +  mov   r10, r9
> +  mov   r11, r9
> +  mov   r12, r9
> +0:stm   r8!, {r9-r12}
> +  cmp   r8, r1
> +  blt   0b
>    b     _PrepareArguments
>  
>  _NeverReturn:
> diff --git a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> index abea675828df..7455de8aa66e 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> @@ -13,6 +13,8 @@
>  
>  #include <AutoGen.h>
>  
> +#define INIT_CAR_VALUE      0x5AA55AA5
> +
>    INCLUDE AsmMacroIoLib.inc
>  
>    IMPORT  CEntryPoint
> @@ -79,6 +81,14 @@ _PrepareArguments
>  
>  _SetupPrimaryCoreStack
>    mov   sp, r1
> +  mov32 r8, FixedPcdGet64(PcdCPUCoresStackBase)
> +  mov32 r9, INIT_CAR_VALUE
> +  mov   r10, r9
> +  mov   r11, r9
> +  mov   r12, r9
> +0:stm   r8!, {r9-r12}
> +  cmp   r8, r1
> +  blt   0b
>    b     _PrepareArguments
>  
>  _NeverReturn
> -- 
> 2.11.0
>