* [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd
@ 2016-10-24 8:57 Ard Biesheuvel
2016-10-24 8:57 ` [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd Ard Biesheuvel
2016-10-24 11:29 ` [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:57 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, lersek, heyi.guo, ryan.harkin, Ard Biesheuvel
Recording the top of SEC visible system memory in a global variable is
not necessary, and violates the constraints of the SEC/PEI environment,
given that it may execute from NOR flash. So remove it.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 6 ------
ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 8 +-------
ArmVirtPkg/PrePi/PrePi.h | 2 --
3 files changed, 1 insertion(+), 15 deletions(-)
diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
index 9c040b17f253..cc8b47e69026 100644
--- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
+++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
@@ -14,8 +14,6 @@
#include <AsmMacroIoLibV8.h>
-ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
-
ASM_FUNC(_ModuleEntryPoint)
//
// We are built as a ET_DYN PIE executable, so we need to process all
@@ -68,8 +66,6 @@ _SetupStackPosition:
ldr x2, PcdGet64 (PcdSystemMemorySize)
sub x2, x2, #1
add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase + PcdSystemMemorySize
- adr x2, mSystemMemoryEnd
- str x1, [x2]
// Calculate Top of the Firmware Device
ldr x2, PcdGet64 (PcdFdBaseAddress)
@@ -151,5 +147,3 @@ _PrepareArguments:
_NeverReturn:
b _NeverReturn
-
-ASM_PFX(mSystemMemoryEnd): .8byte 0
diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
index e03aeefbb003..59028d0a553e 100644
--- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
+++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
@@ -14,8 +14,6 @@
#include <AsmMacroIoLib.h>
-ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
-
ASM_FUNC(_ModuleEntryPoint)
//
// We are built as a ET_DYN PIE executable, so we need to process all
@@ -66,12 +64,10 @@ _SetupStackPosition:
ADRL (r12, PcdGet64 (PcdSystemMemorySize))
ldrd r2, r3, [r12]
- // calculate the top of memory, and record it in mSystemMemoryEnd
+ // calculate the top of memory
adds r2, r2, r1
sub r2, r2, #1
addcs r3, r3, #1
- adr r12, mSystemMemoryEnd
- strd r2, r3, [r12]
// truncate the memory used by UEFI to 4 GB range
teq r3, #0
@@ -159,5 +155,3 @@ _PrepareArguments:
_NeverReturn:
b _NeverReturn
-
-ASM_PFX(mSystemMemoryEnd): .quad 0
diff --git a/ArmVirtPkg/PrePi/PrePi.h b/ArmVirtPkg/PrePi/PrePi.h
index 9b828377adc3..d3189c0b8a6f 100644
--- a/ArmVirtPkg/PrePi/PrePi.h
+++ b/ArmVirtPkg/PrePi/PrePi.h
@@ -29,8 +29,6 @@
#define SerialPrint(txt) SerialPortWrite (txt, AsciiStrLen(txt)+1);
-extern UINT64 mSystemMemoryEnd;
-
RETURN_STATUS
EFIAPI
TimerConstructor (
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd
2016-10-24 8:57 [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Ard Biesheuvel
@ 2016-10-24 8:57 ` Ard Biesheuvel
2016-10-24 12:05 ` Ryan Harkin
2016-10-24 14:42 ` Leif Lindholm
2016-10-24 11:29 ` [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Laszlo Ersek
1 sibling, 2 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:57 UTC (permalink / raw)
To: edk2-devel; +Cc: leif.lindholm, lersek, heyi.guo, ryan.harkin, Ard Biesheuvel
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd
2016-10-24 8:57 [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Ard Biesheuvel
2016-10-24 8:57 ` [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd Ard Biesheuvel
@ 2016-10-24 11:29 ` Laszlo Ersek
1 sibling, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2016-10-24 11:29 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel; +Cc: leif.lindholm, heyi.guo, ryan.harkin
On 10/24/16 10:57, Ard Biesheuvel wrote:
> Recording the top of SEC visible system memory in a global variable is
> not necessary, and violates the constraints of the SEC/PEI environment,
> given that it may execute from NOR flash. So remove it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 6 ------
> ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 8 +-------
> ArmVirtPkg/PrePi/PrePi.h | 2 --
> 3 files changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> index 9c040b17f253..cc8b47e69026 100644
> --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S
> @@ -14,8 +14,6 @@
>
> #include <AsmMacroIoLibV8.h>
>
> -ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
> -
> ASM_FUNC(_ModuleEntryPoint)
> //
> // We are built as a ET_DYN PIE executable, so we need to process all
> @@ -68,8 +66,6 @@ _SetupStackPosition:
> ldr x2, PcdGet64 (PcdSystemMemorySize)
> sub x2, x2, #1
> add x1, x1, x2 // x1 = SystemMemoryTop = PcdSystemMemoryBase + PcdSystemMemorySize
> - adr x2, mSystemMemoryEnd
> - str x1, [x2]
>
> // Calculate Top of the Firmware Device
> ldr x2, PcdGet64 (PcdFdBaseAddress)
> @@ -151,5 +147,3 @@ _PrepareArguments:
>
> _NeverReturn:
> b _NeverReturn
> -
> -ASM_PFX(mSystemMemoryEnd): .8byte 0
> diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> index e03aeefbb003..59028d0a553e 100644
> --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S
> @@ -14,8 +14,6 @@
>
> #include <AsmMacroIoLib.h>
>
> -ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
> -
> ASM_FUNC(_ModuleEntryPoint)
> //
> // We are built as a ET_DYN PIE executable, so we need to process all
> @@ -66,12 +64,10 @@ _SetupStackPosition:
> ADRL (r12, PcdGet64 (PcdSystemMemorySize))
> ldrd r2, r3, [r12]
>
> - // calculate the top of memory, and record it in mSystemMemoryEnd
> + // calculate the top of memory
> adds r2, r2, r1
> sub r2, r2, #1
> addcs r3, r3, #1
> - adr r12, mSystemMemoryEnd
> - strd r2, r3, [r12]
>
> // truncate the memory used by UEFI to 4 GB range
> teq r3, #0
> @@ -159,5 +155,3 @@ _PrepareArguments:
>
> _NeverReturn:
> b _NeverReturn
> -
> -ASM_PFX(mSystemMemoryEnd): .quad 0
> diff --git a/ArmVirtPkg/PrePi/PrePi.h b/ArmVirtPkg/PrePi/PrePi.h
> index 9b828377adc3..d3189c0b8a6f 100644
> --- a/ArmVirtPkg/PrePi/PrePi.h
> +++ b/ArmVirtPkg/PrePi/PrePi.h
> @@ -29,8 +29,6 @@
>
> #define SerialPrint(txt) SerialPortWrite (txt, AsciiStrLen(txt)+1);
>
> -extern UINT64 mSystemMemoryEnd;
> -
> RETURN_STATUS
> EFIAPI
> TimerConstructor (
>
series
Acked-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd
2016-10-24 8:57 ` [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd Ard Biesheuvel
@ 2016-10-24 12:05 ` Ryan Harkin
2016-10-24 12:21 ` Ard Biesheuvel
2016-10-24 14:42 ` Leif Lindholm
1 sibling, 1 reply; 6+ messages in thread
From: Ryan Harkin @ 2016-10-24 12:05 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Leif Lindholm, Laszlo Ersek, Heyi Guo
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd
2016-10-24 12:05 ` Ryan Harkin
@ 2016-10-24 12:21 ` Ard Biesheuvel
0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 12:21 UTC (permalink / raw)
To: Ryan Harkin; +Cc: edk2-devel-01, Leif Lindholm, Laszlo Ersek, Heyi Guo
On 24 October 2016 at 13:05, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> 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>
>
Many thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd
2016-10-24 8:57 ` [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd Ard Biesheuvel
2016-10-24 12:05 ` Ryan Harkin
@ 2016-10-24 14:42 ` Leif Lindholm
1 sibling, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2016-10-24 14:42 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek, heyi.guo, ryan.harkin
On Mon, Oct 24, 2016 at 09:57:23AM +0100, Ard Biesheuvel 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.
So, I agree this is probably why this variable was added to begin
with, but I don't think it necessarily solves a real problem these
days. (There is no guarantee ARM-TF will live at top of RAM to begin
with.)
But the fix for that probably includes killing of SystemMemoryBase and
SystemMemorySize at the same time...
> 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>
Since this is already a clear improvement:
Reviewed-by: Leif Lindholm <leif.lindholm@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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-24 14:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-24 8:57 [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Ard Biesheuvel
2016-10-24 8:57 ` [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd Ard Biesheuvel
2016-10-24 12:05 ` Ryan Harkin
2016-10-24 12:21 ` Ard Biesheuvel
2016-10-24 14:42 ` Leif Lindholm
2016-10-24 11:29 ` [PATCH 1/2] ArmVirtPkg/PrePi: remove mSystemMemoryEnd Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox