From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22a.google.com (mail-it0-x22a.google.com [IPv6:2607:f8b0:4001:c0b::22a]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 46FBB1A1E43 for ; Wed, 7 Sep 2016 04:59:06 -0700 (PDT) Received: by mail-it0-x22a.google.com with SMTP id i184so192312878itf.1 for ; Wed, 07 Sep 2016 04:59:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+Rj3ZVPQIMR2J9JA4//AetU653E1wy5mSEmRPg7fnH4=; b=hQM2+rFj3RrCiL48GefOsOehW+fnJK3Il0dQv5Z1aPvpYor9N6jLeZQvRaikOcU8hM SBq9cZGS5BMVc7U6hoveZE7twJrLP5v2obLMHWqFtXW9PEJq1Y7csrw570Cw1KiyudUn rqg/ECV/54vpqIETn4ERCt4CIHsMMkqjWtzco= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+Rj3ZVPQIMR2J9JA4//AetU653E1wy5mSEmRPg7fnH4=; b=ms+Dz7iFhQMK1a7NdnrPVk3PJLdZLF4suTEdA6p2wFaWSeHwBo9jFooCZkQ/LurBiW xGz+yN8rciupqVe41xg9av1aGBylIkVfN6v5FyxfvszcwSQDxwY9/fzz/tYy6YCNvIRU oEBoSgZ59+/xjzczWz9DreP++y/+pydIf1SIjPI6uan1DOVKSloE5RIaeIN8WRXXnXfb D0K6dQUSrim75dL0IB84QhEQXXGzxk83GpLy1qODqbn92UlsVUAoRzW2zKxmx+tlIeb1 N7y0TomPrlTpygBqxqQ5UfCnes+oHucoYXIk63hvWhcpssnnSxvg4Tl0ehAwO4SOHflz l6FQ== X-Gm-Message-State: AE9vXwN13XgNhH9JxU7G+IRb7c4ms+Q3fdGWAh2OmWSDDYTSs83OutLd78GXEJ9NTLVuBwEJXiNfhDwcV83OvWOX X-Received: by 10.36.214.193 with SMTP id o184mr5923958itg.5.1473249545782; Wed, 07 Sep 2016 04:59:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Wed, 7 Sep 2016 04:59:05 -0700 (PDT) In-Reply-To: References: <1470842282-8415-1-git-send-email-ard.biesheuvel@linaro.org> <1470842282-8415-23-git-send-email-ard.biesheuvel@linaro.org> <20160811083817.GQ31760@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 7 Sep 2016 12:59:05 +0100 Message-ID: To: Ryan Harkin Cc: Michael Zimmermann , Leif Lindholm , "edk2-devel@lists.01.org" , "Cohen, Eugene" , Laszlo Ersek Subject: Re: [PATCH 22/26] ArmPlatformPkg/PrePi: switch to ASM_FUNC() asm macro X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Sep 2016 11:59:06 -0000 Content-Type: text/plain; charset=UTF-8 On 7 September 2016 at 12:10, Ryan Harkin wrote: > On 31 August 2016 at 05:33, Michael Zimmermann wrote: >> reviewed should mean tested ;) >> >> took me some time to find out why my system currently doesn't boot anymore > > I have just bisected my TC2 boot failure down to this patch too. > >> 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) > > And this fix works for me on TC2 also. > May we take that as a tested-by? > Michael/Ard, has one of you already or does one of you intend to > submit the fix for this? > Michael: care to send it as a proper patch, please? > >> >> On Thu, Aug 11, 2016 at 10:38 AM, Leif Lindholm >> 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 >>> >>> > Contributed-under: TianoCore Contribution Agreement 1.0 >>> > Signed-off-by: Ard Biesheuvel >>> > --- >>> > 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 >>> > -#include >>> > -#include >>> > -#include >>> > - >>> > -.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 >>> > -#include >>> > -#include >>> > -#include >>> > >>> > #include >>> > >>> > -.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 >>> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel