From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org 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 1101B21B00DC7 for ; Mon, 13 Nov 2017 04:30:01 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 61EBA49038; Mon, 13 Nov 2017 12:34:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-226.rdu2.redhat.com [10.10.120.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id 202C46D7E4; Mon, 13 Nov 2017 12:34:05 +0000 (UTC) To: Ard Biesheuvel , Jordan Justen Cc: edk2-devel-01 , Ruiyu Ni References: <20171110154908.306-1-lersek@redhat.com> <151043270153.17841.16763408160801933614@jljusten-skl> <151043786891.19895.6326436717816766532@jljusten-skl> <151056410867.15809.659701894226687543@jljusten-skl> From: Laszlo Ersek Message-ID: Date: Mon, 13 Nov 2017 13:34:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 13 Nov 2017 12:34:07 +0000 (UTC) Subject: Re: [PATCH 0/4] OvmfPkg: measure temp stack usage, restore temp RAM to 64KB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Nov 2017 12:30:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/13/17 11:09, Ard Biesheuvel wrote: > On 13 November 2017 at 09:08, Jordan Justen > wrote: >> On 2017-11-12 02:58:37, Ard Biesheuvel wrote: >>> On 11 November 2017 at 22:04, Jordan Justen >>> wrote: >>>> On 2017-11-11 12:38:21, Jordan Justen wrote: >>>>> On 2017-11-10 07:49:04, Laszlo Ersek wrote: >>>>>> The first three patches enable the PEI_CORE to report OVMF's temp >>>>>> SEC/PEI stack and heap usage. >>>>>> >>>>>> - This depends on the new fixed PCD "PcdInitValueInTempStack", >>>>>> recently added for >>>>>> >>>>>> ("INIT_CAR_VALUE should be defined in a central location"). >>>>>> >>>>>> - Ard recently implemented the same in ArmPlatformPkg, for >>>>>> >>>>>> ("measure temp SEC/PEI stack usage"). >>>>>> >>>>>> The last (fourth) patch adapts OVMF to the larger MtrrLib stack >>>>>> demand that originates from commit 2bbd7e2fbd4b >>>>>> ("UefiCpuPkg/MtrrLib: Update algorithm to calculate optimal >>>>>> settings", 2017-09-27). OVMF's temp RAM size is restored to the >>>>>> historical / original 64KB. >>>>>> >>>>>> This work is tracked in >>>>>> ("measure >>>>>> temp SEC/PEI stack usage; grow temp SEC/PEI RAM"), with links to >>>>>> related mailing list discussions. >>>>>> >>>>>> Repo: https://github.com/lersek/edk2.git >>>>>> Branch: temp_ram_tweaks >>>>>> >>>>>> Cc: Ard Biesheuvel >>>>>> Cc: Jordan Justen >>>>>> Cc: Ruiyu Ni >>>>>> >>>>>> Thanks >>>>>> Laszlo >>>>>> >>>>>> Laszlo Ersek (4): >>>>>> OvmfPkg/Sec/Ia32: free up EAX for other uses while setting up >>>>>> the stack >>>>>> OvmfPkg/Sec/Ia32: seed the temporary RAM with >>>>>> PcdInitValueInTempStack >>>>>> OvmfPkg/Sec/X64: seed the temporary RAM with >>>>>> PcdInitValueInTempStack >>>>> >>>>> I'd like to try a different option for these 3. Can you hold off a >>>>> bit before pushing this series? >>>> >>>> I think we should use a C based approach instead, like in the >>>> attached patch. >>>> >>> >>> I'm not sure: having to abuse SetJump () >> >> True, that was annoying. It seems like we could have AsmReadEsp and >> AsmReadRsp in BaseLib since we have AsmReadSp for IPF. >> >>> and having to leave an arbitrary 512 byte window both seem pretty >>> good reasons to stick with assembly. >> >> Also true. I chose 512 because it seemed like more than SetMem32 >> could reasonably need, but also much below the minimum I would expect >> PEI to use. (It seemed that around 4k ended up being used.) >> >>> Is your concern that the stack gets cleared in RELEASE builds as >>> well? >> >> No. I just prefer if we can use C rather than assembly whenever it is >> reasonable. >> > > No discussion there. But in my opinion, anything involving the > absolute value of the stack pointer does not belong in C. > I chose assembly because it seemed much cleaner and simpler to seed the stack before C code actually started using the stack. GCC sometimes plays dirty tricks with laying out local variables on the stack, so addresses of local variables cannot / should not be used for this purpose. (For example, see commit f98f5ec304ec, "UefiCpuPkg: S3Resume2Pei: align return stacks explicitly", 2013-12-13.) BASE_LIBRARY_JUMP_BUFFER didn't occur to me (I've always considered jump buffers opaque to client code). AsmReadSp() is IPF only (the BaseLib class header says so, and the tree agrees). AsmReadEsp() and AsmReadRsp() do not exist in BaseLib, and they would only be implementable for x86. I think platform-dependent library *interfaces* don't belong into BaseLib (in other words, AsmReadSp() is already a mistake in my opinion; we had better not make it worse). I guess I could live with BASE_LIBRARY_JUMP_BUFFER. More specific comments: > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > index f7fec3d8c0..077f7d6563 100644 > --- a/OvmfPkg/Sec/SecMain.c > +++ b/OvmfPkg/Sec/SecMain.c > @@ -1,7 +1,7 @@ > /** @file > Main SEC phase code. Transitions to PEI. > > - Copyright (c) 2008 - 2015, Intel Corporation. All rights reserved.
> + Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > This program and the accompanying materials > @@ -731,6 +731,25 @@ SecCoreStartupWithStack ( > UINT32 Index; > volatile UINT8 *Table; > > + // > + // Fill most of temporary RAM with PcdInitValueInTempStack. We stop > + // filling at the current stack pointer - 512 bytes. > + // > + DEBUG_CODE_BEGIN (); > + BASE_LIBRARY_JUMP_BUFFER JumpBuffer; > + UINTN StackUsed; > + > + SetJump (&JumpBuffer); > +#if defined (MDE_CPU_IA32) > + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Esp; > +#elif defined (MDE_CPU_X64) > + StackUsed = (UINTN)TopOfCurrentStack - JumpBuffer.Rsp; > +#endif > + SetMem32 ((VOID*)(UINTN)PcdGet32 (PcdOvmfSecPeiTempRamBase), > + PcdGet32 (PcdOvmfSecPeiTempRamSize) - StackUsed - 512, > + FixedPcdGet32 (PcdInitValueInTempStack)); (1) SetMem32() is likely problematic in itself; please refer to the following comment -- partly visible in the context of Jordan's patch --, from commit 320b4f084a25 ("OvmfPkg: Sec: force reinit of BaseExtractGuidedSectionLib handler table", 2015-11-30): // // To ensure SMM can't be compromised on S3 resume, we must force re-init of // the BaseExtractGuidedSectionLib. Since this is before library contructors // are called, we must use a loop rather than SetMem. // Thus, we should use a loop and a pointer-to-volatile. (It would likely be slower than the REP STOSD / REP STOSQ.) (2) The indentation of arguments is off. (It doesn't matter if we replace SetMem32() anyway, due to (1).) (3) If we replace SetMem32() with a loop, and (consequently) there's no risk for SetMem32() to bust its own stack / return address, then how should the constant 512 change? Does it become zero? What does GCC guarantee about the value of ESP / RSP at that point, versus the addresses of SecCoreStartupWithStack()'s own local variables? > + DEBUG_CODE_END (); > + > // > // To ensure SMM can't be compromised on S3 resume, we must force re-init of > // the BaseExtractGuidedSectionLib. Since this is before library contructors Thanks Laszlo