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 DBE4E20886D9B for ; Mon, 18 Feb 2019 04:58:21 -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 916DB5A49; Mon, 18 Feb 2019 12:58:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-242.rdu2.redhat.com [10.10.120.242]) by smtp.corp.redhat.com (Postfix) with ESMTP id 230165D9C6; Mon, 18 Feb 2019 12:58:17 +0000 (UTC) To: Jordan Justen , edk2-devel@lists.01.org Cc: Ard Biesheuvel , Anthony Perard , Julien Grall References: <20190218041141.21363-1-jordan.l.justen@intel.com> <20190218041141.21363-6-jordan.l.justen@intel.com> From: Laszlo Ersek Message-ID: <9dfa970a-8f43-6e1f-cb58-22dd6f26eee6@redhat.com> Date: Mon, 18 Feb 2019 13:58:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190218041141.21363-6-jordan.l.justen@intel.com> 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.30]); Mon, 18 Feb 2019 12:58:20 +0000 (UTC) Subject: Re: [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 12:58:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/18/19 05:11, Jordan Justen wrote: > Apparently something depends on the heap being above the stack after > TemporaryRamMigration. > > This is the opposite order that OVMF has always used for TempRam > before migration to permanent ram. In 42a83e80f37c (svn r10770), Mike > changed OVMF's TemporaryRamMigration to swap the locations of heap and > stack during the migration. > > Rather than doing the swap during TemporaryRamMigration, this change > causes OVMF to boot with TemporaryRam setup with heap being above the > stack. Then, during TemporaryRamMigration, we can directly copy all of > TemporaryRam. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Anthony Perard > Cc: Julien Grall > --- > OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +- > OvmfPkg/Sec/SecMain.c | 39 +++++++++++++++++----------------- > OvmfPkg/Sec/X64/SecEntry.nasm | 2 +- > 3 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/OvmfPkg/Sec/Ia32/SecEntry.nasm b/OvmfPkg/Sec/Ia32/SecEntry.nasm > index 03501969eb..61917b9eef 100644 > --- a/OvmfPkg/Sec/Ia32/SecEntry.nasm > +++ b/OvmfPkg/Sec/Ia32/SecEntry.nasm > @@ -57,7 +57,7 @@ ASM_PFX(_ModuleEntryPoint): > ; Load temporary RAM stack based on PCDs > ; > %define SEC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + \ > - FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) > + (FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 2)) > mov eax, SEC_TOP_OF_STACK > mov esp, eax > nop > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > index f7fec3d8c0..46ac739862 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 - 2019, Intel Corporation. All rights reserved.
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > This program and the accompanying materials > @@ -779,15 +779,15 @@ SecCoreStartupWithStack ( > #endif > > // > - // |-------------| <-- TopOfCurrentStack > - // | Stack | 32k > // |-------------| > // | Heap | 32k > + // |-------------| <-- TopOfCurrentStack > + // | Stack | 32k > // |-------------| <-- SecCoreData.TemporaryRamBase > // > > ASSERT ((UINTN) (PcdGet32 (PcdOvmfSecPeiTempRamBase) + > - PcdGet32 (PcdOvmfSecPeiTempRamSize)) == > + (PcdGet32 (PcdOvmfSecPeiTempRamSize) / 2)) == > (UINTN) TopOfCurrentStack); > > // > @@ -795,14 +795,17 @@ SecCoreStartupWithStack ( > // > SecCoreData.DataSize = sizeof(EFI_SEC_PEI_HAND_OFF); > > - SecCoreData.TemporaryRamSize = (UINTN) PcdGet32 (PcdOvmfSecPeiTempRamSize); > - SecCoreData.TemporaryRamBase = (VOID*)((UINT8 *)TopOfCurrentStack - SecCoreData.TemporaryRamSize); > + SecCoreData.TemporaryRamBase = > + (VOID*)(UINTN) PcdGet32 (PcdOvmfSecPeiTempRamBase); > + SecCoreData.TemporaryRamSize = (UINTN) PcdGet32 (PcdOvmfSecPeiTempRamSize); > > - SecCoreData.PeiTemporaryRamBase = SecCoreData.TemporaryRamBase; > - SecCoreData.PeiTemporaryRamSize = SecCoreData.TemporaryRamSize >> 1; > + SecCoreData.PeiTemporaryRamBase = > + (UINT8*)(VOID*)(UINTN) PcdGet32 (PcdOvmfSecPeiTempRamBase) + > + ((UINTN) PcdGet32 (PcdOvmfSecPeiTempRamSize) / 2); > + SecCoreData.PeiTemporaryRamSize = PcdGet32 (PcdOvmfSecPeiTempRamSize) / 2; > > - SecCoreData.StackBase = (UINT8 *)SecCoreData.TemporaryRamBase + SecCoreData.PeiTemporaryRamSize; > - SecCoreData.StackSize = SecCoreData.TemporaryRamSize >> 1; > + SecCoreData.StackBase = (VOID*)(UINTN) PcdGet32 (PcdOvmfSecPeiTempRamBase); > + SecCoreData.StackSize = PcdGet32 (PcdOvmfSecPeiTempRamSize) / 2; > > SecCoreData.BootFirmwareVolumeBase = BootFv; > SecCoreData.BootFirmwareVolumeSize = (UINTN) BootFv->FvLength; > @@ -895,10 +898,10 @@ TemporaryRamMigration ( > (UINT64)CopySize > )); > > - OldHeap = (VOID*)(UINTN)TemporaryMemoryBase; > + OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1)); > NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1)); > > - OldStack = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1)); > + OldStack = (VOID*)(UINTN)TemporaryMemoryBase; > NewStack = (VOID*)(UINTN)PermanentMemoryBase; > > DebugAgentContext.HeapMigrateOffset = (UINTN)NewHeap - (UINTN)OldHeap; > @@ -908,15 +911,13 @@ TemporaryRamMigration ( > InitializeDebugAgent (DEBUG_AGENT_INIT_POSTMEM_SEC, (VOID *) &DebugAgentContext, NULL); > > // > - // Migrate Heap > + // Migrate the whole temporary memory to permenent memory. s/permenent/permanent/ > // > - CopyMem (NewHeap, OldHeap, CopySize >> 1); > + CopyMem( > + (VOID*)(UINTN)PermanentMemoryBase, > + (VOID*)(UINTN)TemporaryMemoryBase, > + CopySize); I think the last paren should be on a separate line, in this style. With those updates: Reviewed-by: Laszlo Ersek Thanks Laszlo > > - // > - // Migrate Stack > - // > - CopyMem (NewStack, OldStack, CopySize >> 1); > - > // > // Rebase IDT table in permanent memory > // > diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm > index d76adcffd8..dd603d6eb0 100644 > --- a/OvmfPkg/Sec/X64/SecEntry.nasm > +++ b/OvmfPkg/Sec/X64/SecEntry.nasm > @@ -59,7 +59,7 @@ ASM_PFX(_ModuleEntryPoint): > ; Load temporary RAM stack based on PCDs > ; > %define SEC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + \ > - FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) > + (FixedPcdGet32 (PcdOvmfSecPeiTempRamSize) / 2)) > mov rsp, SEC_TOP_OF_STACK > nop > >