* [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 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
* 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
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