From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 7A5A61A1E3A for ; Mon, 24 Oct 2016 07:42:50 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id c78so107998606wme.0 for ; Mon, 24 Oct 2016 07:42:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=DDoKk4B6kyru0RVI7tg8SWdMUUiP5hYMDymJysY50Tk=; b=YKrJVQPLyyZmcNrB2+os6/ISYZfKgxUeEyZb9/tzCZeKkrHofGAOXLJ1XV58h/duea vV1A3E1mz5WSS76xuMF8h4HqX74UvemkiQAjHU30JidmfbvFP5L8qNRtvADti/PO80fk nSB7MAXkF61gAZo3a36TX1Fdnr6wEwwv4G8OY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=DDoKk4B6kyru0RVI7tg8SWdMUUiP5hYMDymJysY50Tk=; b=jnaDwE5YqXt0X3b88hQRxrP3YDQU26m8ZtDiQ+fGR8aWaiEUVSMo9eeNHGYjtH7cWI Qn+4kwZL5hAyPMWZDZM0NDH6nqFrLTBggLsoNLypDZthjQuiHLQeFR54YJb/3SURgjsR r+rWyyqIQqqeeRWNW48Gh8IgFGOLy3B1VsWIZoNtwUG+TeUdStHxl6MuFany6ZjuBZhX +XX0IdaSUBFlcNHPxMJOERfZ5NKpmb1WKtXK0oZMEgEt8fvid2kX8IZQdYUFS3fDdhKO Ir6isvv6FLBS23dTkMbhpzoNBFG/w0xhxUgheR0nh1/CQgGKBJHyMw2rdJTwVS2+X+UT +w5A== X-Gm-Message-State: AA6/9RnOjKBLLpwqBv631lPDKtM7ssNV6JJktZpviIsOfspU9IMlp6zwxjC6fadSKpK+J+5K X-Received: by 10.28.208.206 with SMTP id h197mr21864283wmg.14.1477320168110; Mon, 24 Oct 2016 07:42:48 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id x127sm15071457wmd.21.2016.10.24.07.42.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 24 Oct 2016 07:42:47 -0700 (PDT) Date: Mon, 24 Oct 2016 15:42:45 +0100 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, lersek@redhat.com, heyi.guo@Linaro.org, ryan.harkin@linaro.org Message-ID: <20161024144245.GR3471@bivouac.eciton.net> References: <1477299443-9324-1-git-send-email-ard.biesheuvel@linaro.org> <1477299443-9324-2-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1477299443-9324-2-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 2/2] ArmPlatformPkg/PrePi: avoid global variable write to mSystemMemoryEnd 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: Mon, 24 Oct 2016 14:42:50 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 Since this is already a clear improvement: Reviewed-by: Leif Lindholm > --- > > 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 > > -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 > > -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 >