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

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

On Fri, Nov 03, 2017 at 11:33:52AM +0000, 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 as well.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=748
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

OK, this may sound completely unreasonable, but seeing those
implementations overwrite callee-saved registers without saving them
makes my brain unhappy. (Yes, I know.)

Could they either:
- Have a comment prepended establishing the implicit ABI of which
  registers the caller cannot rely on reusing after return.
  Preferably somewhat echoed at the call site.
- Be rewritten to use only scratch registers?

/
    Leif

> ---
> v2: switch to newly introduced PCD
> 
>  ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S | 6 ++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S     | 8 ++++++++
>  ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm   | 8 ++++++++
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf           | 2 ++
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf          | 2 ++
>  5 files changed, 26 insertions(+)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> index aab5edab0c42..0950fd0c0cdb 100644
> --- a/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/AArch64/PrePeiCoreEntryPoint.S
> @@ -84,4 +84,10 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, x1
> +  MOV64 (x8, FixedPcdGet64 (PcdCPUCoresStackBase))
> +  MOV64 (x9, FixedPcdGet32 (PcdInitValueInTempStack) |\
> +             FixedPcdGet32 (PcdInitValueInTempStack) << 32)
> +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..a491af30a048 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.S
> @@ -65,6 +65,14 @@ _PrepareArguments:
>  
>  _SetupPrimaryCoreStack:
>    mov   sp, r1
> +  MOV32 (r8, FixedPcdGet64 (PcdCPUCoresStackBase))
> +  MOV32 (r9, FixedPcdGet32 (PcdInitValueInTempStack))
> +  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..dc1ad8144492 100644
> --- a/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> +++ b/ArmPlatformPkg/PrePeiCore/Arm/PrePeiCoreEntryPoint.asm
> @@ -79,6 +79,14 @@ _PrepareArguments
>  
>  _SetupPrimaryCoreStack
>    mov   sp, r1
> +  mov32 r8, FixedPcdGet64 (PcdCPUCoresStackBase)
> +  mov32 r9, FixedPcdGet32 (PcdInitValueInTempStack)
> +  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/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> index ecdbccb8d620..8e0456f8dc2a 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> @@ -75,3 +75,5 @@ [FixedPcd]
>    gArmTokenSpaceGuid.PcdGicDistributorBase
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase
>    gArmTokenSpaceGuid.PcdGicSgiIntId
> +
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> index b5d4e389b2a4..ec83cec2d879 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> @@ -69,3 +69,5 @@ [FixedPcd]
>    gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase
>    gArmPlatformTokenSpaceGuid.PcdCPUCorePrimaryStackSize
>    gArmPlatformTokenSpaceGuid.PcdCPUCoreSecondaryStackSize
> +
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
> -- 
> 2.11.0
>