From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BC47C1A1DF5 for ; Fri, 5 Aug 2016 11:31:14 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D4FC80B20; Fri, 5 Aug 2016 18:31:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-116.phx2.redhat.com [10.3.116.116]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u75IVBSN032019; Fri, 5 Aug 2016 14:31:12 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org References: <1470408838-1020-1-git-send-email-ard.biesheuvel@linaro.org> Cc: leif.lindholm@linaro.org From: Laszlo Ersek Message-ID: <0b7b8c2f-c680-9afa-f24d-ed15c603decf@redhat.com> Date: Fri, 5 Aug 2016 20:31:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1470408838-1020-1-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 05 Aug 2016 18:31:14 +0000 (UTC) Subject: Re: [PATCH 1/2] ArmVirtPkg/PrePi: use correct callee saved regs 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: Fri, 05 Aug 2016 18:31:15 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 08/05/16 16:53, Ard Biesheuvel wrote: > Both the ARM and the AARCH64 versions of the PrePi code (shared between > ArmVirtQemuKernel and ArmVirtXen) 'preserve' values across a function > call using registers that are not in fact callee saved. So fix that. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S | 24 ++++++++++---------- > ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S | 10 ++++---- > 2 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S > index 68049d5df2bf..e61f5df12e89 100644 > --- a/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S > +++ b/ArmVirtPkg/PrePi/AArch64/ModuleEntryPoint.S > @@ -71,7 +71,7 @@ ASM_PFX(_ModuleEntryPoint): > // Get ID of this CPU in Multicore system > bl ASM_PFX(ArmReadMpidr) > // Keep a copy of the MpId register value > - mov x10, x0 > + mov x20, x0 > > // Check if we can install the stack at the top of the System Memory or if we need > // to install the stacks at the bottom of the Firmware Device (case the FD is located > @@ -113,39 +113,39 @@ _SetupStack: > // Because the 'push' instruction is equivalent to 'stmdb' (decrement before), we need to increment > // one to the top of the stack. We check if incrementing one does not overflow (case of DRAM at the > // top of the memory space) > - adds x11, x1, #1 > + adds x21, x1, #1 > b.cs _SetupOverflowStack > > _SetupAlignedStack: > - mov x1, x11 > + mov x1, x21 > b _GetBaseUefiMemory > > _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 x21, x21, x1 > + sub x1, x1, x21 > > _GetBaseUefiMemory: > // Calculate the Base of the UEFI Memory > - sub x11, x1, x4 > + sub x21, x1, x4 > > _GetStackBase: > // r1 = The top of the Mpcore Stacks > // Stack for the primary core = PrimaryCoreStack > LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), x2) > - sub x12, x1, x2 > + sub x22, 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 > - sub x12, x12, x1 > + sub x22, x22, x1 > > // x12 = The base of the MpCore Stacks (primary stack & secondary stacks) > - mov x0, x12 > + mov x0, x22 > mov x1, x10 > //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize, SecondaryStackSize) > LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), x2) > @@ -159,9 +159,9 @@ _GetStackBase: > bne _PrepareArguments > > _PrepareArguments: > - mov x0, x10 > - mov x1, x11 > - mov x2, x12 > + mov x0, x20 > + mov x1, x21 > + mov x2, x22 > > // Move sec startup address into a data register > // Ensure we're jumping to FV version of the code (not boot remapped alias) > diff --git a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S > index 441db36857de..3215c7d55876 100644 > --- a/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S > +++ b/ArmVirtPkg/PrePi/Arm/ModuleEntryPoint.S > @@ -154,17 +154,17 @@ _GetStackBase: > // r1 = The top of the Mpcore Stacks > // Stack for the primary core = PrimaryCoreStack > LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), r2) > - sub r12, r1, r2 > + sub r9, 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 > - sub r12, r12, r1 > + sub r9, r9, r1 > > - // r12 = The base of the MpCore Stacks (primary stack & secondary stacks) > - mov r0, r12 > + // r9 = The base of the MpCore Stacks (primary stack & secondary stacks) > + mov r0, r9 > mov r1, r10 > //ArmPlatformStackSet(StackBase, MpId, PrimaryStackSize, SecondaryStackSize) > LoadConstantToReg (FixedPcdGet32(PcdCPUCorePrimaryStackSize), r2) > @@ -180,7 +180,7 @@ _GetStackBase: > _PrepareArguments: > mov r0, r10 > mov r1, r11 > - mov r2, r12 > + mov r2, r9 > > // Move sec startup address into a data register > // Ensure we're jumping to FV version of the code (not boot remapped alias) > Callee saved, caller saved... Bah! :) Acked-by: Laszlo Ersek