From: Laszlo Ersek <lersek@redhat.com>
To: Jordan Justen <jordan.l.justen@intel.com>, edk2-devel@lists.01.org
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Anthony Perard <anthony.perard@citrix.com>,
Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
Date: Mon, 18 Feb 2019 13:58:17 +0100 [thread overview]
Message-ID: <9dfa970a-8f43-6e1f-cb58-22dd6f26eee6@redhat.com> (raw)
In-Reply-To: <20190218041141.21363-6-jordan.l.justen@intel.com>
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 <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> ---
> 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.<BR>
> + Copyright (c) 2008 - 2019, Intel Corporation. All rights reserved.<BR>
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>
> 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 <lersek@redhat.com>
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
>
>
next prev parent reply other threads:[~2019-02-18 12:58 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
2019-02-18 4:11 ` [PATCH 01/10] EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter Jordan Justen
2019-02-18 4:11 ` [PATCH 02/10] EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram Jordan Justen
2019-02-18 4:11 ` [PATCH 03/10] EmulatorPkg/Sec: Replace assembly temp-ram support with C code Jordan Justen
2019-02-18 4:11 ` [PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function Jordan Justen
2019-02-18 4:11 ` [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations Jordan Justen
2019-02-18 12:58 ` Laszlo Ersek [this message]
2019-02-18 4:11 ` [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration Jordan Justen
2019-02-18 7:53 ` Ard Biesheuvel
2019-02-18 9:08 ` Jordan Justen
2019-02-18 9:32 ` Ard Biesheuvel
2019-02-18 13:01 ` Laszlo Ersek
2019-02-19 22:50 ` Brian J. Johnson
2019-02-19 23:58 ` Jordan Justen
2019-02-20 8:52 ` Jordan Justen
2019-02-20 8:59 ` Ard Biesheuvel
2019-02-18 4:11 ` [PATCH 07/10] MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram migration Jordan Justen
2019-02-18 4:11 ` [PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration Jordan Justen
2019-02-18 4:11 ` [PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration Jordan Justen
2019-02-18 4:11 ` [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration Jordan Justen
2019-02-18 13:15 ` Laszlo Ersek
2019-02-19 2:46 ` [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Ni, Ray
2019-02-19 13:25 ` Gao, Liming
2019-02-20 13:27 ` Ni, Ray
2019-02-20 17:43 ` Jordan Justen
2019-02-21 0:15 ` Ni, Ray
2019-02-21 1:03 ` Jordan Justen
2019-02-21 4:43 ` Ni, Ray
2019-02-19 19:27 ` Jordan Justen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9dfa970a-8f43-6e1f-cb58-22dd6f26eee6@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox