From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22c.google.com (mail-it0-x22c.google.com [IPv6:2607:f8b0:4001:c0b::22c]) (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 03B681A1E45 for ; Wed, 31 Aug 2016 02:45:33 -0700 (PDT) Received: by mail-it0-x22c.google.com with SMTP id g62so85690880ith.1 for ; Wed, 31 Aug 2016 02:45:32 -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=DG9/1jR+n+pSTJ7FU1PolNa8+d8LGh8wFZKUuaQHXII=; b=PY5eNqPyXFzkePgXKQU3qe/AST6GvmTU8RM6OEjTan/A4G54eqypxzaGIdB3Inwn+/ LXDGQ0xiNFtT6ub4O/+fxOO58E1crxMUwdzJ3mh/8MTG/8/oo5uG4Hug7uyk/kR8sdcN 0xgfCmSf0egM809jKTaz/VbVWi+tmg0Q5N/s0= 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=DG9/1jR+n+pSTJ7FU1PolNa8+d8LGh8wFZKUuaQHXII=; b=VTPQYSirkopE1wfL+z4TEND+J+UFqSTA6c8Gp/7EiCtGnmirksFk02WfrV+wDBAR57 yCrltwlO5k3yJ2g38muZO3giVnL3I4fkqjZU1QoqB1bxWYwP2WjFgQrMGoM4Rajlbgbt Qfl3CXULxez6IAC5tWm3WV+DCOwATB2sUAKTrlA6UO6fMaV4RivyduYeS3klvxOHYc6t ZqNjPmqtAgcNLytExKJvMIxC0KP/7rzCcugFIYYi+pVTr8NqwWx+00zWfZRe1iAOCp56 j6lYb43aHqVBI8oKc/IWpCmFUyNDMInpcwRhoYHvb+UQVr034nB4zTizfzW4h1XvdjbH sdXw== X-Gm-Message-State: AE9vXwNCBM83B2tnosSYJo/sv15ucWaD5s+IilrSpXI2qz/pEG/rjZN7pVuUJxOlJ+4Kgm+01Zckheo1GwORya9v X-Received: by 10.36.214.193 with SMTP id o184mr13232014itg.5.1472636732124; Wed, 31 Aug 2016 02:45:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Wed, 31 Aug 2016 02:45:31 -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, 31 Aug 2016 10:45:31 +0100 Message-ID: To: Michael Zimmermann Cc: Leif Lindholm , "edk2-devel@lists.01.org" , Laszlo Ersek , "Cohen, Eugene" 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, 31 Aug 2016 09:45:33 -0000 Content-Type: text/plain; charset=UTF-8 On 31 August 2016 at 10:43, Michael Zimmermann wrote: > ArmVirtQemuKernel uses ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S which is not > affected by this bug. > Ah, of course. It does highlight why having so many copies of essentially the same code is a bad idea, but it is a difficult pattern to get rid of in Tianocore. > as it turns out, only BeagleBoardPkg uses the generic PrePi code. > Thanks, Ard. > On Wed, Aug 31, 2016 at 11:14 AM, Ard Biesheuvel > wrote: >> >> On 31 August 2016 at 05:33, Michael Zimmermann >> wrote: >> > reviewed should mean tested ;) >> > >> >> Apologies for the breakage. As it turns out, ArmVirtQemuKernel does >> work even with this bug, probably due to the fact that its >> PcdCoreCount == 1 (or simply that it uses the UniCore flavor rather >> than the MPCore flavor of PrePi) >> >> In any case, thanks for tracking this down, and for proposing a fix. >> >> > 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 >> > >> > 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 >> > >> > > >