Re: [PATCH 22/26] ArmPlatformPkg/PrePi: switch to ASM_FUNC() asm macro

From: Michael Zimmermann <sigmaepsilon92@gmail.com>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	 "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Laszlo Ersek <lersek@redhat.com>, "Cohen, Eugene" <eugene@hp.com>
Subject: Re: [PATCH 22/26] ArmPlatformPkg/PrePi: switch to ASM_FUNC() asm macro
Date: Wed, 31 Aug 2016 06:33:06 +0200	[thread overview]
Message-ID: <CAN9vWDLNp0MNCq3Cuv4pQMy2w3L7KS02-EEfnweWOpGmEfZ+VQ@mail.gmail.com> (raw)
In-Reply-To: <20160811083817.GQ31760@bivouac.eciton.net>

reviewed should mean tested ;)

took me some time to find out why my system currently doesn't boot anymore
but here's the fix for this commit:

diff --git a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
index b7127ce..39030da 100644
--- a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
+++ b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
@@ -101,7 +101,7 @@ _GetStackBase:
   sub   r10, r1, r2

   // Stack for the secondary core = Number of Cores - 1
-  MOV32 (r0, (FixedPcdGet32(PcdCoreCount) - 1) *
FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
+  MOV32 (r1, (FixedPcdGet32(PcdCoreCount) - 1) *
FixedPcdGet32(PcdCPUCoreSecondaryStackSize))^M
   sub   r10, r10, r1

   // r10 = The base of the MpCore Stacks (primary stack & secondary stacks)

On Thu, Aug 11, 2016 at 10:38 AM, Leif Lindholm <leif.lindholm@linaro.org>
wrote:

> On Wed, Aug 10, 2016 at 05:17:58PM +0200, Ard Biesheuvel wrote:
> > Annotate functions with ASM_FUNC() so that they are emitted into
> > separate sections.
>
> Also replacing LoadConstantToReg. Add that to commit message and:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S | 49
> ++++++-------------
> >  ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S     | 50
> ++++++--------------
> >  2 files changed, 29 insertions(+), 70 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> > index 9538c70a237c..d0530a874726 100644
> > --- a/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> > +++ b/ArmPlatformPkg/PrePi/AArch64/ModuleEntryPoint.S
> > @@ -12,24 +12,10 @@
> >  //
> >
> >  #include <AsmMacroIoLibV8.h>
> > -#include <Base.h>
> > -#include <Library/PcdLib.h>
> > -#include <AutoGen.h>
> > -
> > -.text
> > -.align 3
> > -
> > -GCC_ASM_IMPORT(ArmPlatformIsPrimaryCore)
> > -GCC_ASM_IMPORT(ArmReadMpidr)
> > -GCC_ASM_IMPORT(ArmPlatformPeiBootAction)
> > -GCC_ASM_IMPORT(ArmPlatformStackSet)
> > -GCC_ASM_EXPORT(_ModuleEntryPoint)
> > -ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
> >
> > -StartupAddr:                  .8byte ASM_PFX(CEntryPoint)
> > -ASM_PFX(mSystemMemoryEnd):    .8byte 0
> > +ASM_GLOBAL ASM_PFX(mSystemMemoryEnd)
> >
> > -ASM_PFX(_ModuleEntryPoint):
> > +ASM_FUNC(_ModuleEntryPoint)
> >    // Do early platform specific actions
> >    bl    ASM_PFX(ArmPlatformPeiBootAction)
> >
> > @@ -49,10 +35,8 @@ _SystemMemoryEndInit:
> >    cmp   x1, #0
> >    bne   _SetupStackPosition
> >
> > -  LoadConstantToReg (FixedPcdGet64(PcdSystemMemoryBase), x1)
> > -  LoadConstantToReg (FixedPcdGet64(PcdSystemMemorySize), x2)
> > -  sub   x2, x2, #1
> > -  add   x1, x1, x2
> > +  MOV64 (x1, FixedPcdGet64(PcdSystemMemoryBase) + FixedPcdGet64(PcdSystemMemorySize)
> - 1)
> > +
> >    // Update the global variable
> >    adr   x2, mSystemMemoryEnd
> >    str   x1, [x2]
> > @@ -61,13 +45,13 @@ _SetupStackPosition:
> >    // r1 = SystemMemoryTop
> >
> >    // Calculate Top of the Firmware Device
> > -  LoadConstantToReg (FixedPcdGet64(PcdFdBaseAddress), x2)
> > -  LoadConstantToReg (FixedPcdGet32(PcdFdSize), x3)
> > +  MOV64 (x2, FixedPcdGet64(PcdFdBaseAddress))
> > +  MOV32 (x3, FixedPcdGet32(PcdFdSize) - 1)
> >    sub   x3, x3, #1
> >    add   x3, x3, x2      // x3 = FdTop = PcdFdBaseAddress + PcdFdSize
> >
> >    // UEFI Memory Size (stacks are allocated in this region)
> > -  LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryUefiRegionSize), x4)
> > +  MOV32 (x4, FixedPcdGet32(PcdSystemMemoryUefiRegionSize))
> >
> >    //
> >    // Reserve the memory for the UEFI region (contain stacks on its top)
> > @@ -98,9 +82,7 @@ _SetupAlignedStack:
> >  _SetupOverflowStack:
> >    // Case memory at the top of the address space. Ensure the top of the
> stack is EFI_PAGE_SIZE
> >    // aligned (4KB)
> > -  LoadConstantToReg (EFI_PAGE_MASK, x11)
> > -  and   x11, x11, x1
> > -  sub   x1, x1, x11
> > +  and   x1, x1, ~EFI_PAGE_MASK
> >
> >  _GetBaseUefiMemory:
> >    // Calculate the Base of the UEFI Memory
> > @@ -109,22 +91,19 @@ _GetBaseUefiMemory:
> >  _GetStackBase:
> >    // r1 = The top of the Mpcore Stacks
> >    // Stack for the primary core = PrimaryCoreStack
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), x2)
> > +  MOV32 (x2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> >    sub   x12, x1, x2
> >
> >    // Stack for the secondary core = Number of Cores - 1
> > -  LoadConstantToReg (FixedPcdGet32(PcdCoreCount), x0)
> > -  sub   x0, x0, #1
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCoreSecondaryStackSize), x1)
> > -  mul   x1, x1, x0
> > +  MOV32 (x1, (FixedPcdGet32(PcdCoreCount) - 1) * FixedPcdGet32(
> PcdCPUCoreSecondaryStackSize))
> >    sub   x12, x12, x1
> >
> >    // x12 = The base of the MpCore Stacks (primary stack & secondary
> stacks)
> >    mov   x0, x12
> >    mov   x1, x10
> >    //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize,
> SecondaryStackSize)
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), x2)
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCoreSecondaryStackSize), x3)
> > +  MOV32 (x2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> > +  MOV32 (x3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> >    bl    ASM_PFX(ArmPlatformStackSet)
> >
> >    // Is it the Primary Core ?
> > @@ -140,7 +119,7 @@ _PrepareArguments:
> >
> >    // Move sec startup address into a data register
> >    // Ensure we're jumping to FV version of the code (not boot remapped
> alias)
> > -  ldr   x4, StartupAddr
> > +  ldr   x4, =ASM_PFX(CEntryPoint)
> >
> >    // Jump to PrePiCore C code
> >    //    x0 = MpId
> > @@ -150,3 +129,5 @@ _PrepareArguments:
> >
> >  _NeverReturn:
> >    b _NeverReturn
> > +
> > +ASM_PFX(mSystemMemoryEnd):    .8byte 0
> > diff --git a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> > index 1311efc5cb2c..b7127ce9fb4c 100644
> > --- a/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> > +++ b/ArmPlatformPkg/PrePi/Arm/ModuleEntryPoint.S
> > @@ -12,28 +12,12 @@
> >  //
> >
> >  #include <AsmMacroIoLib.h>
> > -#include <Base.h>
> > -#include <Library/PcdLib.h>
> > -#include <AutoGen.h>
> >
> >  #include <Chipset/ArmV7.h>
> >
> > -.text
> > -.align 3
> > -
> > -GCC_ASM_IMPORT(CEntryPoint)
> > -GCC_ASM_IMPORT(ArmPlatformIsPrimaryCore)
> > -GCC_ASM_IMPORT(ArmReadMpidr)
> > -GCC_ASM_IMPORT(ArmPlatformPeiBootAction)
> > -GCC_ASM_IMPORT(ArmPlatformStackSet)
> > -GCC_ASM_EXPORT(_ModuleEntryPoint)
> >  GCC_ASM_EXPORT(mSystemMemoryEnd)
> >
> > -StartupAddr:       .word  CEntryPoint
> > -mSystemMemoryEnd:  .8byte 0
> > -
> > -
> > -ASM_PFX(_ModuleEntryPoint):
> > +ASM_FUNC(_ModuleEntryPoint)
> >    // Do early platform specific actions
> >    bl    ASM_PFX(ArmPlatformPeiBootAction)
> >
> > @@ -57,10 +41,8 @@ _SystemMemoryEndInit:
> >    cmp   r1, #0
> >    bne   _SetupStackPosition
> >
> > -  LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryBase), r1)
> > -  LoadConstantToReg (FixedPcdGet32(PcdSystemMemorySize), r2)
> > -  sub   r2, r2, #1
> > -  add   r1, r1, r2
> > +  MOV32 (r1, FixedPcdGet32(PcdSystemMemoryBase) + FixedPcdGet32(PcdSystemMemorySize)
> - 1)
> > +
> >    // Update the global variable
> >    adr   r2, mSystemMemoryEnd
> >    str   r1, [r2]
> > @@ -69,13 +51,12 @@ _SetupStackPosition:
> >    // r1 = SystemMemoryTop
> >
> >    // Calculate Top of the Firmware Device
> > -  LoadConstantToReg (FixedPcdGet32(PcdFdBaseAddress), r2)
> > -  LoadConstantToReg (FixedPcdGet32(PcdFdSize), r3)
> > -  sub   r3, r3, #1
> > +  MOV32 (r2, FixedPcdGet32(PcdFdBaseAddress))
> > +  MOV32 (r3, FixedPcdGet32(PcdFdSize) - 1)
> >    add   r3, r3, r2      // r3 = FdTop = PcdFdBaseAddress + PcdFdSize
> >
> >    // UEFI Memory Size (stacks are allocated in this region)
> > -  LoadConstantToReg (FixedPcdGet32(PcdSystemMemoryUefiRegionSize), r4)
> > +  MOV32 (r4, FixedPcdGet32(PcdSystemMemoryUefiRegionSize))
> >
> >    //
> >    // Reserve the memory for the UEFI region (contain stacks on its top)
> > @@ -106,9 +87,8 @@ _SetupAlignedStack:
> >  _SetupOverflowStack:
> >    // Case memory at the top of the address space. Ensure the top of the
> stack is EFI_PAGE_SIZE
> >    // aligned (4KB)
> > -  LoadConstantToReg (EFI_PAGE_MASK, r9)
> > -  and   r9, r9, r1
> > -  sub   r1, r1, r9
> > +  MOV32 (r9, ~EFI_PAGE_MASK & 0xFFFFFFFF)
> > +  and   r1, r1, r9
> >
> >  _GetBaseUefiMemory:
> >    // Calculate the Base of the UEFI Memory
> > @@ -117,22 +97,19 @@ _GetBaseUefiMemory:
> >  _GetStackBase:
> >    // r1 = The top of the Mpcore Stacks
> >    // Stack for the primary core = PrimaryCoreStack
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), r2)
> > +  MOV32 (r2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> >    sub   r10, r1, r2
> >
> >    // Stack for the secondary core = Number of Cores - 1
> > -  LoadConstantToReg (FixedPcdGet32(PcdCoreCount), r0)
> > -  sub   r0, r0, #1
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCoreSecondaryStackSize), r1)
> > -  mul   r1, r1, r0
> > +  MOV32 (r0, (FixedPcdGet32(PcdCoreCount) - 1) * FixedPcdGet32(
> PcdCPUCoreSecondaryStackSize))
> >    sub   r10, r10, r1
> >
> >    // r10 = The base of the MpCore Stacks (primary stack & secondary
> stacks)
> >    mov   r0, r10
> >    mov   r1, r8
> >    //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize,
> SecondaryStackSize)
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), r2)
> > -  LoadConstantToReg (FixedPcdGet32(PcdCPUCoreSecondaryStackSize), r3)
> > +  MOV32 (r2, FixedPcdGet32(PcdCPUCorePrimaryStackSize))
> > +  MOV32 (r3, FixedPcdGet32(PcdCPUCoreSecondaryStackSize))
> >    bl    ASM_PFX(ArmPlatformStackSet)
> >
> >    // Is it the Primary Core ?
> > @@ -149,7 +126,7 @@ _PrepareArguments:
> >
> >    // Move sec startup address into a data register
> >    // Ensure we're jumping to FV version of the code (not boot remapped
> alias)
> > -  ldr   r4, StartupAddr
> > +  ldr   r4, =ASM_PFX(CEntryPoint)
> >
> >    // Jump to PrePiCore C code
> >    //    r0 = MpId
> > @@ -160,3 +137,4 @@ _PrepareArguments:
> >  _NeverReturn:
> >    b _NeverReturn
> >
> > +ASM_PFX(mSystemMemoryEnd):  .8byte 0
> > --
> > 2.7.4
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>

