Re: [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd

[Ryan Harkin <ryan.harkin@linaro.org>]

On 24 October 2016 at 09:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The global variable mSystemMemoryEnd is initialized by PrePi only if
> it has not been initialized by ArmPlatformPeiBootAction(). This allows
> platforms executing under, e.g., ARM Trusted Firmware to dynamically
> reserve a window at the top of memory that will be used by the secure
> firmware.
>
> However, PrePi is a SEC module, and writing to a global variable
> violates the SEC constraints, since SEC and PEI may execute from NOR
> flash.
>
> So instead, initialize mSystemMemoryEnd statically. This will ensure
> it holds the correct value for all implementations where the value
> is not overridden, but still allows it to be overridden during the
> call to ArmPlatformPeiBootAction().
>
> Note that this patch also fixes a latent bug on 32-bit platforms where
> a value of mSystemMemoryEnd exceeding 4 GB would be truncated to 32-bits
> rather than limited to (4 GB - 1)
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

I tested this successfully on FVP Foundation & AEMv8 models, Juno
R0/1/2 and TC2.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

> ---
>
> Build tested only.
>
>  ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S | 14 -------------
>  ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S     | 20 +++++--------------
>  ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.asm   | 21 ++++++--------------
>  ArmPlatformPkg/PrePi/PrePi.c                    |  3 +++
>  4 files changed, 14 insertions(+), 44 deletions(-)
>
> diff --git a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> index d0530a874726..a81709d5d12d 100644
> --- a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -13,8 +13,6 @@
>
>  #include <AsmMacroIoLibV8.h>
>
> -ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
> -
>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -31,16 +29,6 @@ _SetSVCMode:
>  _SystemMemoryEndInit:
>    ldr   x1, mSystemMemoryEnd
>
> -  // Is mSystemMemoryEnd initialized?
> -  cmp   x1, #0
> -  bne   _SetupStackPosition
> -
> -  MOV64 (x1, FixedPcdGet64(PcdSystemMemoryBase) + FixedPcdGet64(PcdSystemMemorySize) - 1)
> -
> -  // Update the global variable
> -  adr   x2, mSystemMemoryEnd
> -  str   x1, [x2]
> -
>  _SetupStackPosition:
>    // r1 = SystemMemoryTop
>
> @@ -129,5 +117,3 @@ _PrepareArguments:
>
>  _NeverReturn:
>    b _NeverReturn
> -
> -ASM_PFX(mSystemMemoryEnd):    .8byte 0
> diff --git a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> index 39030da5f2c3..212cab62d44b 100644
> --- a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -15,8 +15,6 @@
>
>  #include <Chipset/ArmV7.h>
>
> -GCC_ASM_EXPORT(mSystemMemoryEnd)
> -
>  ASM_FUNC(_ModuleEntryPoint)
>    // Do early platform specific actions
>    bl    ASM_PFX(ArmPlatformPeiBootAction)
> @@ -35,17 +33,11 @@ _SetSVCMode:
>  // to install the stacks at the bottom of the Firmware Device (case the FD is located
>  // at the top of the DRAM)
>  _SystemMemoryEndInit:
> -  ldr   r1, mSystemMemoryEnd
> -
> -  // Is mSystemMemoryEnd initialized?
> -  cmp   r1, #0
> -  bne   _SetupStackPosition
> -
> -  MOV32 (r1, FixedPcdGet32(PcdSystemMemoryBase) + FixedPcdGet32(PcdSystemMemorySize) - 1)
> -
> -  // Update the global variable
> -  adr   r2, mSystemMemoryEnd
> -  str   r1, [r2]
> +  ADRL  (r1, mSystemMemoryEnd)
> +  ldrd  r2, r3, [r1]
> +  teq   r3, #0
> +  moveq r1, r2
> +  mvnne r1, #0
>
>  _SetupStackPosition:
>    // r1 = SystemMemoryTop
> @@ -136,5 +128,3 @@ _PrepareArguments:
>
>  _NeverReturn:
>    b _NeverReturn
> -
> -ASM_PFX(mSystemMemoryEnd):  .8byte 0
> diff --git a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.asm b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.asm
> index 023339841f75..1e9daf563bb6 100644
> --- a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.asm
> +++ b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.asm
> @@ -21,15 +21,14 @@
>    IMPORT  ArmReadMpidr
>    IMPORT  ArmPlatformPeiBootAction
>    IMPORT  ArmPlatformStackSet
> +  IMPORT  mSystemMemoryEnd
>
>    EXPORT  _ModuleEntryPoint
> -  EXPORT  mSystemMemoryEnd
>
>    PRESERVE8
>    AREA    PrePiCoreEntryPoint, CODE, READONLY
>
>  StartupAddr        DCD      CEntryPoint
> -mSystemMemoryEnd   DCQ      0
>
>  _ModuleEntryPoint
>    // Do early platform specific actions
> @@ -49,19 +48,11 @@ _SetSVCMode
>  // to install the stacks at the bottom of the Firmware Device (case the FD is located
>  // at the top of the DRAM)
>  _SystemMemoryEndInit
> -  ldr   r1, mSystemMemoryEnd
> -
> -  // Is mSystemMemoryEnd initialized?
> -  cmp   r1, #0
> -  bne   _SetupStackPosition
> -
> -  mov32 r1, FixedPcdGet32(PcdSystemMemoryBase)
> -  mov32 r2, FixedPcdGet32(PcdSystemMemorySize)
> -  sub   r2, r2, #1
> -  add   r1, r1, r2
> -  // Update the global variable
> -  adr   r2, mSystemMemoryEnd
> -  str   r1, [r2]
> +  mov32 r1, mSystemMemoryEnd
> +  ldrd  r2, r3, [r1]
> +  teq   r3, #0
> +  moveq r1, r2
> +  mvnne r1, #0
>
>  _SetupStackPosition
>    // r1 = SystemMemoryTop
> diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
> index 36928c65a73b..e548ccace097 100644
> --- a/ArmPlatformPkg/PrePi/PrePi.c
> +++ b/ArmPlatformPkg/PrePi/PrePi.c
> @@ -32,6 +32,9 @@
>  #define IS_XIP() (((UINT64)FixedPcdGet64 (PcdFdBaseAddress) > mSystemMemoryEnd) || \
>                    ((FixedPcdGet64 (PcdFdBaseAddress) + FixedPcdGet32 (PcdFdSize)) < FixedPcdGet64 (PcdSystemMemoryBase)))
>
> +UINT64 mSystemMemoryEnd = FixedPcdGet64(PcdSystemMemoryBase) +
> +                          FixedPcdGet64(PcdSystemMemorySize) - 1;
> +
>  EFI_STATUS
>  EFIAPI
>  ExtractGuidedSectionLibConstructor (
> --
> 2.7.4
>