* [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI
@ 2019-03-02 12:11 Ray Ni
2019-03-02 21:45 ` Jordan Justen
0 siblings, 1 reply; 5+ messages in thread
From: Ray Ni @ 2019-03-02 12:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Andrew Fish
TemporaryRamSupport PPI is called by PeiCore to migrate temporary
memory from permanent memory. But the implementation assumes
ebp/rbp contains the original esp/rsp value when migrating which
is not always true the compiler optimization is turned on.
A real boot failure is seen using GCC5.
In fact, below commit in year 2013 enhanced PeiCore to migrate
the memory without calling TemporaryRamSupport PPI.
SHA-1: 0f9ebb321638e9142ab3bdcc19000c49bb83b9ba
* Add support for PI1.2.1 TempRam Done PPI.
So this patch removes TemporaryRamSupport PPI implementation from
EmulatorPkg/Sec module to fix the boot failure when using GCC5.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
---
EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 ------------------------------
EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 -----------------------------
EmulatorPkg/Sec/Ia32/TempRam.c | 65 --------------------
EmulatorPkg/Sec/Sec.c | 61 +------------------
EmulatorPkg/Sec/Sec.h | 14 +----
EmulatorPkg/Sec/Sec.inf | 15 +----
EmulatorPkg/Sec/X64/SwitchRam.S | 72 ----------------------
EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ------------------------
8 files changed, 5 insertions(+), 487 deletions(-)
delete mode 100644 EmulatorPkg/Sec/Ia32/SwitchRam.S
delete mode 100644 EmulatorPkg/Sec/Ia32/SwitchRam.asm
delete mode 100644 EmulatorPkg/Sec/Ia32/TempRam.c
delete mode 100644 EmulatorPkg/Sec/X64/SwitchRam.S
delete mode 100644 EmulatorPkg/Sec/X64/SwitchRam.asm
diff --git a/EmulatorPkg/Sec/Ia32/SwitchRam.S b/EmulatorPkg/Sec/Ia32/SwitchRam.S
deleted file mode 100644
index 39304daef1..0000000000
--- a/EmulatorPkg/Sec/Ia32/SwitchRam.S
+++ /dev/null
@@ -1,95 +0,0 @@
-#------------------------------------------------------------------------------
-#
-# Copyright (c) 2007, Intel Corporation. All rights reserved.<BR>
-# This program and the accompanying materials
-# are licensed and made available under the terms and conditions of the BSD License
-# which accompanies this distribution. The full text of the license may be found at
-# http:#opensource.org/licenses/bsd-license.php
-#
-# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-# Module Name:
-#
-# Stack.asm
-#
-# Abstract:
-#
-# Switch the stack from temporary memory to permenent memory.
-#
-#------------------------------------------------------------------------------
-
- .text
-
-
-//------------------------------------------------------------------------------
-// VOID
-// EFIAPI
-// SecSwitchStack (
-// UINT32 TemporaryMemoryBase,
-// UINT32 PermenentMemoryBase
-// )//
-//------------------------------------------------------------------------------
-ASM_GLOBAL ASM_PFX(SecSwitchStack)
-ASM_PFX(SecSwitchStack):
-#
-# Save three register: eax, ebx, ecx
-#
- push %eax
- push %ebx
- push %ecx
- push %edx
-
-#
-# !!CAUTION!! this function address's is pushed into stack after
-# migration of whole temporary memory, so need save it to permenent
-# memory at first!
-#
-
- movl 20(%esp), %ebx # Save the first parameter
- movl 24(%esp), %ecx # Save the second parameter
-
-#
-# Save this function's return address into permenent memory at first.
-# Then, Fixup the esp point to permenent memory
-#
-
- movl %esp, %eax
- subl %ebx, %eax
- addl %ecx, %eax
- movl (%esp), %edx # copy pushed register's value to permenent memory
- movl %edx, (%eax)
- movl 4(%esp), %edx
- movl %edx, 4(%eax)
- movl 8(%esp), %edx
- movl %edx, 8(%eax)
- movl 12(%esp), %edx
- movl %edx, 12(%eax)
- movl 16(%esp), %edx
- movl %edx, 16(%eax)
- movl %eax, %esp # From now, esp is pointed to permenent memory
-
-#
-# Fixup the ebp point to permenent memory
-#
-#ifndef __APPLE__
- movl %ebp, %eax
- subl %ebx, %eax
- addl %ecx, %eax
- movl %eax, %ebp # From now, ebp is pointed to permenent memory
-
-#
-# Fixup callee's ebp point for PeiDispatch
-#
- movl (%ebp), %eax
- subl %ebx, %eax
- addl %ecx, %eax
- movl %eax, (%ebp) # From now, Temporary's PPI caller's stack is in permenent memory
-#endif
-
- pop %edx
- pop %ecx
- pop %ebx
- pop %eax
- ret
-
diff --git a/EmulatorPkg/Sec/Ia32/SwitchRam.asm b/EmulatorPkg/Sec/Ia32/SwitchRam.asm
deleted file mode 100644
index 731ee0ffdb..0000000000
--- a/EmulatorPkg/Sec/Ia32/SwitchRam.asm
+++ /dev/null
@@ -1,94 +0,0 @@
-;------------------------------------------------------------------------------
-;
-; Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.<BR>
-; This program and the accompanying materials
-; are licensed and made available under the terms and conditions of the BSD License
-; which accompanies this distribution. The full text of the license may be found at
-; http://opensource.org/licenses/bsd-license.php
-;
-; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-;
-; Module Name:
-;
-; Stack.asm
-;
-; Abstract:
-;
-; Switch the stack from temporary memory to permenent memory.
-;
-;------------------------------------------------------------------------------
-
- .586p
- .model flat,C
- .code
-
-;------------------------------------------------------------------------------
-; VOID
-; EFIAPI
-; SecSwitchStack (
-; UINT32 TemporaryMemoryBase,
-; UINT32 PermenentMemoryBase
-; );
-;------------------------------------------------------------------------------
-SecSwitchStack PROC
- ;
- ; Save three register: eax, ebx, ecx
- ;
- push eax
- push ebx
- push ecx
- push edx
-
- ;
- ; !!CAUTION!! this function address's is pushed into stack after
- ; migration of whole temporary memory, so need save it to permenent
- ; memory at first!
- ;
-
- mov ebx, [esp + 20] ; Save the first parameter
- mov ecx, [esp + 24] ; Save the second parameter
-
- ;
- ; Save this function's return address into permenent memory at first.
- ; Then, Fixup the esp point to permenent memory
- ;
- mov eax, esp
- sub eax, ebx
- add eax, ecx
- mov edx, dword ptr [esp] ; copy pushed register's value to permenent memory
- mov dword ptr [eax], edx
- mov edx, dword ptr [esp + 4]
- mov dword ptr [eax + 4], edx
- mov edx, dword ptr [esp + 8]
- mov dword ptr [eax + 8], edx
- mov edx, dword ptr [esp + 12]
- mov dword ptr [eax + 12], edx
- mov edx, dword ptr [esp + 16] ; Update this function's return address into permenent memory
- mov dword ptr [eax + 16], edx
- mov esp, eax ; From now, esp is pointed to permenent memory
-
- ;
- ; Fixup the ebp point to permenent memory
- ;
- mov eax, ebp
- sub eax, ebx
- add eax, ecx
- mov ebp, eax ; From now, ebp is pointed to permenent memory
-
- ;
- ; Fixup callee's ebp point for PeiDispatch
- ;
- mov eax, dword ptr [ebp]
- sub eax, ebx
- add eax, ecx
- mov dword ptr [ebp], eax ; From now, Temporary's PPI caller's stack is in permenent memory
-
- pop edx
- pop ecx
- pop ebx
- pop eax
- ret
-SecSwitchStack ENDP
-
- END
diff --git a/EmulatorPkg/Sec/Ia32/TempRam.c b/EmulatorPkg/Sec/Ia32/TempRam.c
deleted file mode 100644
index 591354970b..0000000000
--- a/EmulatorPkg/Sec/Ia32/TempRam.c
+++ /dev/null
@@ -1,65 +0,0 @@
-/*++ @file
- Temp RAM PPI
-
-Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
-This program and the accompanying materials
-are licensed and made available under the terms and conditions of the BSD License
-which accompanies this distribution. The full text of the license may be found at
-http://opensource.org/licenses/bsd-license.php
-
-THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-
-**/
-
-#include <PiPei.h>
-#include <Library/DebugLib.h>
-#include <Library/BaseMemoryLib.h>
-
-#include <Ppi/TemporaryRamSupport.h>
-
-VOID
-EFIAPI
-SecSwitchStack (
- UINT32 TemporaryMemoryBase,
- UINT32 PermenentMemoryBase
- );
-
-
-EFI_STATUS
-EFIAPI
-SecTemporaryRamSupport (
- IN CONST EFI_PEI_SERVICES **PeiServices,
- IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
- IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
- IN UINTN CopySize
- )
-{
- //
- // Migrate the whole temporary memory to permanent memory.
- //
- CopyMem (
- (VOID*)(UINTN)PermanentMemoryBase,
- (VOID*)(UINTN)TemporaryMemoryBase,
- CopySize
- );
-
- //
- // SecSwitchStack function must be invoked after the memory migration
- // immediately, also we need fixup the stack change caused by new call into
- // permanent memory.
- //
- SecSwitchStack ((UINT32) TemporaryMemoryBase, (UINT32) PermanentMemoryBase);
-
- //
- // We need *not* fix the return address because currently,
- // The PeiCore is executed in flash.
- //
-
- //
- // Simulate to invalid temporary memory, terminate temporary memory
- //
- //ZeroMem ((VOID*)(UINTN)TemporaryMemoryBase, CopySize);
-
- return EFI_SUCCESS;
-}
diff --git a/EmulatorPkg/Sec/Sec.c b/EmulatorPkg/Sec/Sec.c
index 4132e9d9b7..84bd54f6c6 100644
--- a/EmulatorPkg/Sec/Sec.c
+++ b/EmulatorPkg/Sec/Sec.c
@@ -4,6 +4,7 @@
The OS application will call the SEC with the PEI Entry Point API.
Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -17,22 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include "Sec.h"
-
-EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI mSecTemporaryRamSupportPpi = {
- SecTemporaryRamSupport
-};
-
-
-EFI_PEI_PPI_DESCRIPTOR gPrivateDispatchTable[] = {
- {
- EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
- &gEfiTemporaryRamSupportPpiGuid,
- &mSecTemporaryRamSupportPpi
- }
-};
-
-
-
/**
The entry point of PE/COFF Image for the PEI Core, that has been hijacked by this
SEC that sits on top of an OS application. So the entry and exit of this module
@@ -77,54 +62,12 @@ _ModuleEntryPoint (
EFI_PEI_FILE_HANDLE FileHandle;
VOID *PeCoffImage;
EFI_PEI_CORE_ENTRY_POINT EntryPoint;
- EFI_PEI_PPI_DESCRIPTOR *Ppi;
- EFI_PEI_PPI_DESCRIPTOR *SecPpiList;
- UINTN SecReseveredMemorySize;
- UINTN Index;
EMU_MAGIC_PAGE()->PpiList = PpiList;
ProcessLibraryConstructorList ();
DEBUG ((EFI_D_ERROR, "SEC Has Started\n"));
- //
- // Add Our PPIs to the list
- //
- SecReseveredMemorySize = sizeof (gPrivateDispatchTable);
- for (Ppi = PpiList, Index = 1; ; Ppi++, Index++) {
- SecReseveredMemorySize += sizeof (EFI_PEI_PPI_DESCRIPTOR);
-
- if ((Ppi->Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) == EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) {
- // Since we are appending, need to clear out privious list terminator.
- Ppi->Flags &= ~EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
- break;
- }
- }
-
- // Keep everything on a good alignment
- SecReseveredMemorySize = ALIGN_VALUE (SecReseveredMemorySize, CPU_STACK_ALIGNMENT);
-
-#if 0
- // Tell the PEI Core to not use our buffer in temp RAM
- SecPpiList = (EFI_PEI_PPI_DESCRIPTOR *)SecCoreData->PeiTemporaryRamBase;
- SecCoreData->PeiTemporaryRamBase = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase + SecReseveredMemorySize);
- SecCoreData->PeiTemporaryRamSize -= SecReseveredMemorySize;
-#else
- {
- //
- // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
- // or I don't understand temp RAM correctly?
- //
- EFI_PEI_PPI_DESCRIPTOR PpiArray[10];
-
- SecPpiList = &PpiArray[0];
- ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
- }
-#endif
- // Copy existing list, and append our entries.
- CopyMem (SecPpiList, PpiList, sizeof (EFI_PEI_PPI_DESCRIPTOR) * Index);
- CopyMem (&SecPpiList[Index], gPrivateDispatchTable, sizeof (gPrivateDispatchTable));
-
// Find PEI Core and transfer control
VolumeHandle = (EFI_PEI_FV_HANDLE)(UINTN)SecCoreData->BootFirmwareVolumeBase;
FileHandle = NULL;
@@ -138,7 +81,7 @@ _ModuleEntryPoint (
ASSERT_EFI_ERROR (Status);
// Transfer control to PEI Core
- EntryPoint (SecCoreData, SecPpiList);
+ EntryPoint (SecCoreData, PpiList);
// PEI Core never returns
ASSERT (FALSE);
diff --git a/EmulatorPkg/Sec/Sec.h b/EmulatorPkg/Sec/Sec.h
index c5781201eb..8f60de4405 100644
--- a/EmulatorPkg/Sec/Sec.h
+++ b/EmulatorPkg/Sec/Sec.h
@@ -4,6 +4,7 @@
The OS application will call the SEC with the PEI Entry Point API.
Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
+Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -25,9 +26,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/PeCoffGetEntryPointLib.h>
#include <Library/BaseMemoryLib.h>
-#include <Ppi/TemporaryRamSupport.h>
-
-
//
// I think this shold be defined in a MdePkg include file?
//
@@ -37,15 +35,5 @@ ProcessLibraryConstructorList (
VOID
);
-EFI_STATUS
-EFIAPI
-SecTemporaryRamSupport (
- IN CONST EFI_PEI_SERVICES **PeiServices,
- IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
- IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
- IN UINTN CopySize
- );
-
-
#endif
diff --git a/EmulatorPkg/Sec/Sec.inf b/EmulatorPkg/Sec/Sec.inf
index d253fd724e..8691c86dde 100644
--- a/EmulatorPkg/Sec/Sec.inf
+++ b/EmulatorPkg/Sec/Sec.inf
@@ -3,7 +3,7 @@
#
# Main executable file of Unix Emulator that loads PEI core after initialization finished.
# Portions copyright (c) 2011, Apple Inc. All rights reserved.<BR>
-# Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
#
# This program and the accompanying materials
# are licensed and made available under the terms and conditions of the BSD License
@@ -25,15 +25,7 @@ [Defines]
[Sources]
Sec.c
-
-[Sources.X64]
- X64/SwitchRam.asm
- X64/SwitchRam.S
-
-[Sources.IA32]
- Ia32/TempRam.c
- Ia32/SwitchRam.asm
- Ia32/SwitchRam.S
+ Sec.h
[Packages]
MdePkg/MdePkg.dec
@@ -46,8 +38,5 @@ [LibraryClasses]
PpiListLib
BaseMemoryLib
-[Ppis]
- gEfiTemporaryRamSupportPpiGuid
-
[Pcd]
gEmulatorPkgTokenSpaceGuid.PcdPeiServicesTablePage
diff --git a/EmulatorPkg/Sec/X64/SwitchRam.S b/EmulatorPkg/Sec/X64/SwitchRam.S
deleted file mode 100644
index 9ed1f911e7..0000000000
--- a/EmulatorPkg/Sec/X64/SwitchRam.S
+++ /dev/null
@@ -1,72 +0,0 @@
-#------------------------------------------------------------------------------
-#
-# Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
-# Portitions copyright (c) 2011, Apple Inc. All rights reserved.
-# This program and the accompanying materials
-# are licensed and made available under the terms and conditions of the BSD License
-# which accompanies this distribution. The full text of the license may be found at
-# http://opensource.org/licenses/bsd-license.php.
-#
-# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-#
-#------------------------------------------------------------------------------
-
-
-
-// EFI_STATUS
-// EFIAPI
-// SecTemporaryRamSupport (
-// IN CONST EFI_PEI_SERVICES **PeiServices, // %rcx
-// IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, // %rdx
-// IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, // %r8
-// IN UINTN CopySize // %r9
-// )
-//
-ASM_GLOBAL ASM_PFX(SecTemporaryRamSupport)
-ASM_PFX(SecTemporaryRamSupport):
- // Adjust callers %rbp to account for stack move
- subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
- addq %r8, %rbp // add in permanent base to offset
-
- pushq %rbp // stack frame is for the debugger
- movq %rsp, %rbp
-
- pushq %rdx // Save TemporaryMemoryBase
- pushq %r8 // Save PermanentMemoryBase
- pushq %r9 // Save CopySize
-
- //
- // Copy all of temp RAM to permanent memory, including stack
- //
- // CopyMem (PermanentMemoryBase, TemporaryMemoryBase, CopySize);
- // %rcx, %rdx, %r8
- movq %r8, %rcx // Shift arguments
- movq %r9, %r8
- subq $0x28, %rsp // Allocate register spill area & 16-byte align stack
- call ASM_PFX(CopyMem)
- // Temp mem stack now copied to permanent location. %esp still in temp memory
- addq $0x28, %rsp
-
- popq %r9 // CopySize (old stack)
- popq %r8 // PermanentMemoryBase (old stack)
- popq %rdx // TemporaryMemoryBase (old stack)
-
- movq %rsp, %rcx // Move to new stack
- subq %rdx, %rcx // Calc offset of stack in Temp Memory
- addq %r8, %rcx // Calc PermanentMemoryBase address
- movq %rcx, %rsp // Update stack
- // Stack now points to permanent memory
-
- // ZeroMem (TemporaryMemoryBase /* rcx */, CopySize /* rdx */);
- movq %rdx, %rcx
- movq %r9, %rdx
- subq $0x28, %rsp // Allocate register spill area & 16-byte align stack
- call ASM_PFX(ZeroMem)
- addq $0x28, %rsp
-
- // This data comes off the NEW stack
- popq %rbp
- ret
-
-
diff --git a/EmulatorPkg/Sec/X64/SwitchRam.asm b/EmulatorPkg/Sec/X64/SwitchRam.asm
deleted file mode 100644
index d1a7b943fd..0000000000
--- a/EmulatorPkg/Sec/X64/SwitchRam.asm
+++ /dev/null
@@ -1,76 +0,0 @@
-;------------------------------------------------------------------------------
-;
-; Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
-; Portitions copyright (c) 2011, Apple Inc. All rights reserved.
-; This program and the accompanying materials
-; are licensed and made available under the terms and conditions of the BSD License
-; which accompanies this distribution. The full text of the license may be found at
-; http://opensource.org/licenses/bsd-license.php.
-;
-; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-;
-;------------------------------------------------------------------------------
-
-EXTERN CopyMem:PROC
-EXTERN ZeroMem:PROC
-
- .code
-
-;------------------------------------------------------------------------------
-; EFI_STATUS
-; EFIAPI
-; SecTemporaryRamSupport (
-; IN CONST EFI_PEI_SERVICES **PeiServices, // %rcx
-; IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, // %rdx
-; IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, // %r8
-; IN UINTN CopySize // %r9
-; )
-;------------------------------------------------------------------------------
-SecTemporaryRamSupport PROC
- ; Adjust callers %rbp to account for stack move
- sub rbp, rdx ; Calc offset of %rbp in Temp Memory
- add rbp, r8 ; add in permanent base to offset
-
- push rbp ; stack frame is for the debugger
- mov rbp, rsp
-
- push rdx ; Save TemporaryMemoryBase
- push r8 ; Save PermanentMemoryBase
- push r9 ; Save CopySize
-
- ;
- ; Copy all of temp RAM to permanent memory, including stack
- ;
- ; CopyMem (PermanentMemoryBase, TemporaryMemoryBase, CopySize);
- ; %rcx, %rdx, %r8
- mov rcx, r8 ; Shift arguments
- mov r8, r9
- sub rsp, 028h ; Allocate register spill area & 16-byte align stack
- call CopyMem
- ; Temp mem stack now copied to permanent location. %esp still in temp memory
- add rsp, 028h
-
- pop r9 ; CopySize (old stack)
- pop r8 ; PermanentMemoryBase (old stack)
- pop rdx ; TemporaryMemoryBase (old stack)
-
- mov rcx, rsp ; Move to new stack
- sub rcx, rdx ; Calc offset of stack in Temp Memory
- add rcx, r8 ; Calc PermanentMemoryBase address
- mov rsp, rcx ; Update stack
- ; Stack now points to permanent memory
-
- ; ZeroMem (TemporaryMemoryBase /* rcx */, CopySize /* rdx */);
- mov rcx, rdx
- mov rdx, r9
- sub rsp, 028h ; Allocate register spill area & 16-byte align stack
- call ZeroMem
- add rsp, 028h
-
- ; This data comes off the NEW stack
- pop rbp
- ret
-SecTemporaryRamSupport ENDP
-
- END
--
2.20.1.windows.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI
2019-03-02 12:11 [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI Ray Ni
@ 2019-03-02 21:45 ` Jordan Justen
2019-03-05 2:37 ` Ni, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Jordan Justen @ 2019-03-02 21:45 UTC (permalink / raw)
To: Ray Ni, edk2-devel
On 2019-03-02 04:11:11, Ray Ni wrote:
> TemporaryRamSupport PPI is called by PeiCore to migrate temporary
> memory from permanent memory. But the implementation assumes
> ebp/rbp contains the original esp/rsp value when migrating which
> is not always true the compiler optimization is turned on.
> A real boot failure is seen using GCC5.
> In fact, below commit in year 2013 enhanced PeiCore to migrate
> the memory without calling TemporaryRamSupport PPI.
> SHA-1: 0f9ebb321638e9142ab3bdcc19000c49bb83b9ba
> * Add support for PI1.2.1 TempRam Done PPI.
>
> So this patch removes TemporaryRamSupport PPI implementation from
> EmulatorPkg/Sec module to fix the boot failure when using GCC5.
I don't think we should just hide the known bug with the
TemporaryRamSupport PPI implementation in the PEI Core.
I would agree that we should make this change after addressing that.
-Jordan
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> ---
> EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 ------------------------------
> EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 -----------------------------
> EmulatorPkg/Sec/Ia32/TempRam.c | 65 --------------------
> EmulatorPkg/Sec/Sec.c | 61 +------------------
> EmulatorPkg/Sec/Sec.h | 14 +----
> EmulatorPkg/Sec/Sec.inf | 15 +----
> EmulatorPkg/Sec/X64/SwitchRam.S | 72 ----------------------
> EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ------------------------
> 8 files changed, 5 insertions(+), 487 deletions(-)
> delete mode 100644 EmulatorPkg/Sec/Ia32/SwitchRam.S
> delete mode 100644 EmulatorPkg/Sec/Ia32/SwitchRam.asm
> delete mode 100644 EmulatorPkg/Sec/Ia32/TempRam.c
> delete mode 100644 EmulatorPkg/Sec/X64/SwitchRam.S
> delete mode 100644 EmulatorPkg/Sec/X64/SwitchRam.asm
>
> diff --git a/EmulatorPkg/Sec/Ia32/SwitchRam.S b/EmulatorPkg/Sec/Ia32/SwitchRam.S
> deleted file mode 100644
> index 39304daef1..0000000000
> --- a/EmulatorPkg/Sec/Ia32/SwitchRam.S
> +++ /dev/null
> @@ -1,95 +0,0 @@
> -#------------------------------------------------------------------------------
> -#
> -# Copyright (c) 2007, Intel Corporation. All rights reserved.<BR>
> -# This program and the accompanying materials
> -# are licensed and made available under the terms and conditions of the BSD License
> -# which accompanies this distribution. The full text of the license may be found at
> -# http:#opensource.org/licenses/bsd-license.php
> -#
> -# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -# Module Name:
> -#
> -# Stack.asm
> -#
> -# Abstract:
> -#
> -# Switch the stack from temporary memory to permenent memory.
> -#
> -#------------------------------------------------------------------------------
> -
> - .text
> -
> -
> -//------------------------------------------------------------------------------
> -// VOID
> -// EFIAPI
> -// SecSwitchStack (
> -// UINT32 TemporaryMemoryBase,
> -// UINT32 PermenentMemoryBase
> -// )//
> -//------------------------------------------------------------------------------
> -ASM_GLOBAL ASM_PFX(SecSwitchStack)
> -ASM_PFX(SecSwitchStack):
> -#
> -# Save three register: eax, ebx, ecx
> -#
> - push %eax
> - push %ebx
> - push %ecx
> - push %edx
> -
> -#
> -# !!CAUTION!! this function address's is pushed into stack after
> -# migration of whole temporary memory, so need save it to permenent
> -# memory at first!
> -#
> -
> - movl 20(%esp), %ebx # Save the first parameter
> - movl 24(%esp), %ecx # Save the second parameter
> -
> -#
> -# Save this function's return address into permenent memory at first.
> -# Then, Fixup the esp point to permenent memory
> -#
> -
> - movl %esp, %eax
> - subl %ebx, %eax
> - addl %ecx, %eax
> - movl (%esp), %edx # copy pushed register's value to permenent memory
> - movl %edx, (%eax)
> - movl 4(%esp), %edx
> - movl %edx, 4(%eax)
> - movl 8(%esp), %edx
> - movl %edx, 8(%eax)
> - movl 12(%esp), %edx
> - movl %edx, 12(%eax)
> - movl 16(%esp), %edx
> - movl %edx, 16(%eax)
> - movl %eax, %esp # From now, esp is pointed to permenent memory
> -
> -#
> -# Fixup the ebp point to permenent memory
> -#
> -#ifndef __APPLE__
> - movl %ebp, %eax
> - subl %ebx, %eax
> - addl %ecx, %eax
> - movl %eax, %ebp # From now, ebp is pointed to permenent memory
> -
> -#
> -# Fixup callee's ebp point for PeiDispatch
> -#
> - movl (%ebp), %eax
> - subl %ebx, %eax
> - addl %ecx, %eax
> - movl %eax, (%ebp) # From now, Temporary's PPI caller's stack is in permenent memory
> -#endif
> -
> - pop %edx
> - pop %ecx
> - pop %ebx
> - pop %eax
> - ret
> -
> diff --git a/EmulatorPkg/Sec/Ia32/SwitchRam.asm b/EmulatorPkg/Sec/Ia32/SwitchRam.asm
> deleted file mode 100644
> index 731ee0ffdb..0000000000
> --- a/EmulatorPkg/Sec/Ia32/SwitchRam.asm
> +++ /dev/null
> @@ -1,94 +0,0 @@
> -;------------------------------------------------------------------------------
> -;
> -; Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.<BR>
> -; This program and the accompanying materials
> -; are licensed and made available under the terms and conditions of the BSD License
> -; which accompanies this distribution. The full text of the license may be found at
> -; http://opensource.org/licenses/bsd-license.php
> -;
> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -;
> -; Module Name:
> -;
> -; Stack.asm
> -;
> -; Abstract:
> -;
> -; Switch the stack from temporary memory to permenent memory.
> -;
> -;------------------------------------------------------------------------------
> -
> - .586p
> - .model flat,C
> - .code
> -
> -;------------------------------------------------------------------------------
> -; VOID
> -; EFIAPI
> -; SecSwitchStack (
> -; UINT32 TemporaryMemoryBase,
> -; UINT32 PermenentMemoryBase
> -; );
> -;------------------------------------------------------------------------------
> -SecSwitchStack PROC
> - ;
> - ; Save three register: eax, ebx, ecx
> - ;
> - push eax
> - push ebx
> - push ecx
> - push edx
> -
> - ;
> - ; !!CAUTION!! this function address's is pushed into stack after
> - ; migration of whole temporary memory, so need save it to permenent
> - ; memory at first!
> - ;
> -
> - mov ebx, [esp + 20] ; Save the first parameter
> - mov ecx, [esp + 24] ; Save the second parameter
> -
> - ;
> - ; Save this function's return address into permenent memory at first.
> - ; Then, Fixup the esp point to permenent memory
> - ;
> - mov eax, esp
> - sub eax, ebx
> - add eax, ecx
> - mov edx, dword ptr [esp] ; copy pushed register's value to permenent memory
> - mov dword ptr [eax], edx
> - mov edx, dword ptr [esp + 4]
> - mov dword ptr [eax + 4], edx
> - mov edx, dword ptr [esp + 8]
> - mov dword ptr [eax + 8], edx
> - mov edx, dword ptr [esp + 12]
> - mov dword ptr [eax + 12], edx
> - mov edx, dword ptr [esp + 16] ; Update this function's return address into permenent memory
> - mov dword ptr [eax + 16], edx
> - mov esp, eax ; From now, esp is pointed to permenent memory
> -
> - ;
> - ; Fixup the ebp point to permenent memory
> - ;
> - mov eax, ebp
> - sub eax, ebx
> - add eax, ecx
> - mov ebp, eax ; From now, ebp is pointed to permenent memory
> -
> - ;
> - ; Fixup callee's ebp point for PeiDispatch
> - ;
> - mov eax, dword ptr [ebp]
> - sub eax, ebx
> - add eax, ecx
> - mov dword ptr [ebp], eax ; From now, Temporary's PPI caller's stack is in permenent memory
> -
> - pop edx
> - pop ecx
> - pop ebx
> - pop eax
> - ret
> -SecSwitchStack ENDP
> -
> - END
> diff --git a/EmulatorPkg/Sec/Ia32/TempRam.c b/EmulatorPkg/Sec/Ia32/TempRam.c
> deleted file mode 100644
> index 591354970b..0000000000
> --- a/EmulatorPkg/Sec/Ia32/TempRam.c
> +++ /dev/null
> @@ -1,65 +0,0 @@
> -/*++ @file
> - Temp RAM PPI
> -
> -Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
> -This program and the accompanying materials
> -are licensed and made available under the terms and conditions of the BSD License
> -which accompanies this distribution. The full text of the license may be found at
> -http://opensource.org/licenses/bsd-license.php
> -
> -THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -
> -**/
> -
> -#include <PiPei.h>
> -#include <Library/DebugLib.h>
> -#include <Library/BaseMemoryLib.h>
> -
> -#include <Ppi/TemporaryRamSupport.h>
> -
> -VOID
> -EFIAPI
> -SecSwitchStack (
> - UINT32 TemporaryMemoryBase,
> - UINT32 PermenentMemoryBase
> - );
> -
> -
> -EFI_STATUS
> -EFIAPI
> -SecTemporaryRamSupport (
> - IN CONST EFI_PEI_SERVICES **PeiServices,
> - IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
> - IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
> - IN UINTN CopySize
> - )
> -{
> - //
> - // Migrate the whole temporary memory to permanent memory.
> - //
> - CopyMem (
> - (VOID*)(UINTN)PermanentMemoryBase,
> - (VOID*)(UINTN)TemporaryMemoryBase,
> - CopySize
> - );
> -
> - //
> - // SecSwitchStack function must be invoked after the memory migration
> - // immediately, also we need fixup the stack change caused by new call into
> - // permanent memory.
> - //
> - SecSwitchStack ((UINT32) TemporaryMemoryBase, (UINT32) PermanentMemoryBase);
> -
> - //
> - // We need *not* fix the return address because currently,
> - // The PeiCore is executed in flash.
> - //
> -
> - //
> - // Simulate to invalid temporary memory, terminate temporary memory
> - //
> - //ZeroMem ((VOID*)(UINTN)TemporaryMemoryBase, CopySize);
> -
> - return EFI_SUCCESS;
> -}
> diff --git a/EmulatorPkg/Sec/Sec.c b/EmulatorPkg/Sec/Sec.c
> index 4132e9d9b7..84bd54f6c6 100644
> --- a/EmulatorPkg/Sec/Sec.c
> +++ b/EmulatorPkg/Sec/Sec.c
> @@ -4,6 +4,7 @@
> The OS application will call the SEC with the PEI Entry Point API.
>
> Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD License
> which accompanies this distribution. The full text of the license may be found at
> @@ -17,22 +18,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include "Sec.h"
>
>
> -
> -EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI mSecTemporaryRamSupportPpi = {
> - SecTemporaryRamSupport
> -};
> -
> -
> -EFI_PEI_PPI_DESCRIPTOR gPrivateDispatchTable[] = {
> - {
> - EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> - &gEfiTemporaryRamSupportPpiGuid,
> - &mSecTemporaryRamSupportPpi
> - }
> -};
> -
> -
> -
> /**
> The entry point of PE/COFF Image for the PEI Core, that has been hijacked by this
> SEC that sits on top of an OS application. So the entry and exit of this module
> @@ -77,54 +62,12 @@ _ModuleEntryPoint (
> EFI_PEI_FILE_HANDLE FileHandle;
> VOID *PeCoffImage;
> EFI_PEI_CORE_ENTRY_POINT EntryPoint;
> - EFI_PEI_PPI_DESCRIPTOR *Ppi;
> - EFI_PEI_PPI_DESCRIPTOR *SecPpiList;
> - UINTN SecReseveredMemorySize;
> - UINTN Index;
>
> EMU_MAGIC_PAGE()->PpiList = PpiList;
> ProcessLibraryConstructorList ();
>
> DEBUG ((EFI_D_ERROR, "SEC Has Started\n"));
>
> - //
> - // Add Our PPIs to the list
> - //
> - SecReseveredMemorySize = sizeof (gPrivateDispatchTable);
> - for (Ppi = PpiList, Index = 1; ; Ppi++, Index++) {
> - SecReseveredMemorySize += sizeof (EFI_PEI_PPI_DESCRIPTOR);
> -
> - if ((Ppi->Flags & EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) == EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST) {
> - // Since we are appending, need to clear out privious list terminator.
> - Ppi->Flags &= ~EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
> - break;
> - }
> - }
> -
> - // Keep everything on a good alignment
> - SecReseveredMemorySize = ALIGN_VALUE (SecReseveredMemorySize, CPU_STACK_ALIGNMENT);
> -
> -#if 0
> - // Tell the PEI Core to not use our buffer in temp RAM
> - SecPpiList = (EFI_PEI_PPI_DESCRIPTOR *)SecCoreData->PeiTemporaryRamBase;
> - SecCoreData->PeiTemporaryRamBase = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase + SecReseveredMemorySize);
> - SecCoreData->PeiTemporaryRamSize -= SecReseveredMemorySize;
> -#else
> - {
> - //
> - // When I subtrack from SecCoreData->PeiTemporaryRamBase PEI Core crashes? Either there is a bug
> - // or I don't understand temp RAM correctly?
> - //
> - EFI_PEI_PPI_DESCRIPTOR PpiArray[10];
> -
> - SecPpiList = &PpiArray[0];
> - ASSERT (sizeof (PpiArray) >= SecReseveredMemorySize);
> - }
> -#endif
> - // Copy existing list, and append our entries.
> - CopyMem (SecPpiList, PpiList, sizeof (EFI_PEI_PPI_DESCRIPTOR) * Index);
> - CopyMem (&SecPpiList[Index], gPrivateDispatchTable, sizeof (gPrivateDispatchTable));
> -
> // Find PEI Core and transfer control
> VolumeHandle = (EFI_PEI_FV_HANDLE)(UINTN)SecCoreData->BootFirmwareVolumeBase;
> FileHandle = NULL;
> @@ -138,7 +81,7 @@ _ModuleEntryPoint (
> ASSERT_EFI_ERROR (Status);
>
> // Transfer control to PEI Core
> - EntryPoint (SecCoreData, SecPpiList);
> + EntryPoint (SecCoreData, PpiList);
>
> // PEI Core never returns
> ASSERT (FALSE);
> diff --git a/EmulatorPkg/Sec/Sec.h b/EmulatorPkg/Sec/Sec.h
> index c5781201eb..8f60de4405 100644
> --- a/EmulatorPkg/Sec/Sec.h
> +++ b/EmulatorPkg/Sec/Sec.h
> @@ -4,6 +4,7 @@
> The OS application will call the SEC with the PEI Entry Point API.
>
> Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
> +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD License
> which accompanies this distribution. The full text of the license may be found at
> @@ -25,9 +26,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/PeCoffGetEntryPointLib.h>
> #include <Library/BaseMemoryLib.h>
>
> -#include <Ppi/TemporaryRamSupport.h>
> -
> -
> //
> // I think this shold be defined in a MdePkg include file?
> //
> @@ -37,15 +35,5 @@ ProcessLibraryConstructorList (
> VOID
> );
>
> -EFI_STATUS
> -EFIAPI
> -SecTemporaryRamSupport (
> - IN CONST EFI_PEI_SERVICES **PeiServices,
> - IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
> - IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
> - IN UINTN CopySize
> - );
> -
> -
> #endif
>
> diff --git a/EmulatorPkg/Sec/Sec.inf b/EmulatorPkg/Sec/Sec.inf
> index d253fd724e..8691c86dde 100644
> --- a/EmulatorPkg/Sec/Sec.inf
> +++ b/EmulatorPkg/Sec/Sec.inf
> @@ -3,7 +3,7 @@
> #
> # Main executable file of Unix Emulator that loads PEI core after initialization finished.
> # Portions copyright (c) 2011, Apple Inc. All rights reserved.<BR>
> -# Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
> #
> # This program and the accompanying materials
> # are licensed and made available under the terms and conditions of the BSD License
> @@ -25,15 +25,7 @@ [Defines]
>
> [Sources]
> Sec.c
> -
> -[Sources.X64]
> - X64/SwitchRam.asm
> - X64/SwitchRam.S
> -
> -[Sources.IA32]
> - Ia32/TempRam.c
> - Ia32/SwitchRam.asm
> - Ia32/SwitchRam.S
> + Sec.h
>
> [Packages]
> MdePkg/MdePkg.dec
> @@ -46,8 +38,5 @@ [LibraryClasses]
> PpiListLib
> BaseMemoryLib
>
> -[Ppis]
> - gEfiTemporaryRamSupportPpiGuid
> -
> [Pcd]
> gEmulatorPkgTokenSpaceGuid.PcdPeiServicesTablePage
> diff --git a/EmulatorPkg/Sec/X64/SwitchRam.S b/EmulatorPkg/Sec/X64/SwitchRam.S
> deleted file mode 100644
> index 9ed1f911e7..0000000000
> --- a/EmulatorPkg/Sec/X64/SwitchRam.S
> +++ /dev/null
> @@ -1,72 +0,0 @@
> -#------------------------------------------------------------------------------
> -#
> -# Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
> -# Portitions copyright (c) 2011, Apple Inc. All rights reserved.
> -# This program and the accompanying materials
> -# are licensed and made available under the terms and conditions of the BSD License
> -# which accompanies this distribution. The full text of the license may be found at
> -# http://opensource.org/licenses/bsd-license.php.
> -#
> -# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -#
> -#------------------------------------------------------------------------------
> -
> -
> -
> -// EFI_STATUS
> -// EFIAPI
> -// SecTemporaryRamSupport (
> -// IN CONST EFI_PEI_SERVICES **PeiServices, // %rcx
> -// IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, // %rdx
> -// IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, // %r8
> -// IN UINTN CopySize // %r9
> -// )
> -//
> -ASM_GLOBAL ASM_PFX(SecTemporaryRamSupport)
> -ASM_PFX(SecTemporaryRamSupport):
> - // Adjust callers %rbp to account for stack move
> - subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
> - addq %r8, %rbp // add in permanent base to offset
> -
> - pushq %rbp // stack frame is for the debugger
> - movq %rsp, %rbp
> -
> - pushq %rdx // Save TemporaryMemoryBase
> - pushq %r8 // Save PermanentMemoryBase
> - pushq %r9 // Save CopySize
> -
> - //
> - // Copy all of temp RAM to permanent memory, including stack
> - //
> - // CopyMem (PermanentMemoryBase, TemporaryMemoryBase, CopySize);
> - // %rcx, %rdx, %r8
> - movq %r8, %rcx // Shift arguments
> - movq %r9, %r8
> - subq $0x28, %rsp // Allocate register spill area & 16-byte align stack
> - call ASM_PFX(CopyMem)
> - // Temp mem stack now copied to permanent location. %esp still in temp memory
> - addq $0x28, %rsp
> -
> - popq %r9 // CopySize (old stack)
> - popq %r8 // PermanentMemoryBase (old stack)
> - popq %rdx // TemporaryMemoryBase (old stack)
> -
> - movq %rsp, %rcx // Move to new stack
> - subq %rdx, %rcx // Calc offset of stack in Temp Memory
> - addq %r8, %rcx // Calc PermanentMemoryBase address
> - movq %rcx, %rsp // Update stack
> - // Stack now points to permanent memory
> -
> - // ZeroMem (TemporaryMemoryBase /* rcx */, CopySize /* rdx */);
> - movq %rdx, %rcx
> - movq %r9, %rdx
> - subq $0x28, %rsp // Allocate register spill area & 16-byte align stack
> - call ASM_PFX(ZeroMem)
> - addq $0x28, %rsp
> -
> - // This data comes off the NEW stack
> - popq %rbp
> - ret
> -
> -
> diff --git a/EmulatorPkg/Sec/X64/SwitchRam.asm b/EmulatorPkg/Sec/X64/SwitchRam.asm
> deleted file mode 100644
> index d1a7b943fd..0000000000
> --- a/EmulatorPkg/Sec/X64/SwitchRam.asm
> +++ /dev/null
> @@ -1,76 +0,0 @@
> -;------------------------------------------------------------------------------
> -;
> -; Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.<BR>
> -; Portitions copyright (c) 2011, Apple Inc. All rights reserved.
> -; This program and the accompanying materials
> -; are licensed and made available under the terms and conditions of the BSD License
> -; which accompanies this distribution. The full text of the license may be found at
> -; http://opensource.org/licenses/bsd-license.php.
> -;
> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> -;
> -;------------------------------------------------------------------------------
> -
> -EXTERN CopyMem:PROC
> -EXTERN ZeroMem:PROC
> -
> - .code
> -
> -;------------------------------------------------------------------------------
> -; EFI_STATUS
> -; EFIAPI
> -; SecTemporaryRamSupport (
> -; IN CONST EFI_PEI_SERVICES **PeiServices, // %rcx
> -; IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase, // %rdx
> -; IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase, // %r8
> -; IN UINTN CopySize // %r9
> -; )
> -;------------------------------------------------------------------------------
> -SecTemporaryRamSupport PROC
> - ; Adjust callers %rbp to account for stack move
> - sub rbp, rdx ; Calc offset of %rbp in Temp Memory
> - add rbp, r8 ; add in permanent base to offset
> -
> - push rbp ; stack frame is for the debugger
> - mov rbp, rsp
> -
> - push rdx ; Save TemporaryMemoryBase
> - push r8 ; Save PermanentMemoryBase
> - push r9 ; Save CopySize
> -
> - ;
> - ; Copy all of temp RAM to permanent memory, including stack
> - ;
> - ; CopyMem (PermanentMemoryBase, TemporaryMemoryBase, CopySize);
> - ; %rcx, %rdx, %r8
> - mov rcx, r8 ; Shift arguments
> - mov r8, r9
> - sub rsp, 028h ; Allocate register spill area & 16-byte align stack
> - call CopyMem
> - ; Temp mem stack now copied to permanent location. %esp still in temp memory
> - add rsp, 028h
> -
> - pop r9 ; CopySize (old stack)
> - pop r8 ; PermanentMemoryBase (old stack)
> - pop rdx ; TemporaryMemoryBase (old stack)
> -
> - mov rcx, rsp ; Move to new stack
> - sub rcx, rdx ; Calc offset of stack in Temp Memory
> - add rcx, r8 ; Calc PermanentMemoryBase address
> - mov rsp, rcx ; Update stack
> - ; Stack now points to permanent memory
> -
> - ; ZeroMem (TemporaryMemoryBase /* rcx */, CopySize /* rdx */);
> - mov rcx, rdx
> - mov rdx, r9
> - sub rsp, 028h ; Allocate register spill area & 16-byte align stack
> - call ZeroMem
> - add rsp, 028h
> -
> - ; This data comes off the NEW stack
> - pop rbp
> - ret
> -SecTemporaryRamSupport ENDP
> -
> - END
> --
> 2.20.1.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI
2019-03-02 21:45 ` Jordan Justen
@ 2019-03-05 2:37 ` Ni, Ray
2019-03-05 21:48 ` Jordan Justen
0 siblings, 1 reply; 5+ messages in thread
From: Ni, Ray @ 2019-03-05 2:37 UTC (permalink / raw)
To: Justen, Jordan L, edk2-devel@lists.01.org
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Jordan
> Justen
> Sent: Sunday, March 3, 2019 5:45 AM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] EmulatorPkg/Sec: Don't install
> TemporaryRamSupport PPI
>
> On 2019-03-02 04:11:11, Ray Ni wrote:
> >
> > So this patch removes TemporaryRamSupport PPI implementation from
> > EmulatorPkg/Sec module to fix the boot failure when using GCC5.
>
> I don't think we should just hide the known bug with the
> TemporaryRamSupport PPI implementation in the PEI Core.
>
> I would agree that we should make this change after addressing that.
Jordan,
I have a goal to replace Nt32 with EmulatorPkg before Q2 stable tag release.
I understand we need more discussions on how to fix the PeiCore bug.
So I don't want this blocks the Nt32 deletion.
And in my opinion, the TemporaryRamSupport PPI that requires being called
from assembly code is a design bug.
I suggest to change the PI spec instead of changing PeiCore by introducing
more assembly code to hide this design bug.
Thanks,
Ray
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI
2019-03-05 2:37 ` Ni, Ray
@ 2019-03-05 21:48 ` Jordan Justen
2019-03-06 2:23 ` Ni, Ray
0 siblings, 1 reply; 5+ messages in thread
From: Jordan Justen @ 2019-03-05 21:48 UTC (permalink / raw)
To: Ni, Ray, edk2-devel@lists.01.org
Cc: Andrew Fish, Laszlo Ersek, Leif Lindholm, Michael D Kinney
On 2019-03-04 18:37:03, Ni, Ray wrote:
>
> > -----Original Message-----
> > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Jordan
> > Justen
> > Sent: Sunday, March 3, 2019 5:45 AM
> > To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH] EmulatorPkg/Sec: Don't install
> > TemporaryRamSupport PPI
> >
> > On 2019-03-02 04:11:11, Ray Ni wrote:
> > >
> > > So this patch removes TemporaryRamSupport PPI implementation from
> > > EmulatorPkg/Sec module to fix the boot failure when using GCC5.
> >
> > I don't think we should just hide the known bug with the
> > TemporaryRamSupport PPI implementation in the PEI Core.
> >
> > I would agree that we should make this change after addressing that.
>
> Jordan,
> I have a goal to replace Nt32 with EmulatorPkg before Q2 stable tag release.
> I understand we need more discussions on how to fix the PeiCore bug.
> So I don't want this blocks the Nt32 deletion.
Given the choice of fixing a known bug in PEI Core relating to the PI
spec TemporaryRamSupport PPI, or deleting Nt32, I would choose fixing
the bug.
But, why not fix the bug, and then make this change?
> And in my opinion, the TemporaryRamSupport PPI that requires being called
> from assembly code is a design bug.
I agree. It's unfortunate, but it is part of the PI spec, and unlikely
to be changed very soon.
Even if a new PPI is added, I would guess that the old PPI might
remain in the PI spec. Maybe it would be marked as deprecated at some
point and then removed later?
I am no fan adding assembly language unnecessarily. But, I can't see a
way around it in this case.
> I suggest to change the PI spec instead of changing PeiCore by introducing
> more assembly code to hide this design bug.
I did prototype a TemporaryRamSupport2 PPI under:
https://github.com/jljusten/edk2/tree/temp-ram-support2
My prototype still used assembly, as I don't think the
TemporaryRamSupport PPI would be quickly removed from the PI spec.
(And, therefore, I think edk2's PEI Core should support it without
known bugs.)
But, I think the new TemporaryRamSupport2 PPI should be possible to
safely implement in C code for both the PEI Core and in the PPI
implementation.
-Jordan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI
2019-03-05 21:48 ` Jordan Justen
@ 2019-03-06 2:23 ` Ni, Ray
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ray @ 2019-03-06 2:23 UTC (permalink / raw)
To: edk2-devel@lists.01.org, Kinney, Michael D, Gao, Liming,
Wang, Jian J, Ard Biesheuvel, 'leif.lindholm@linaro.org'
Cc: Laszlo Ersek, Justen, Jordan L
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Jordan
> Justen
> Sent: Wednesday, March 6, 2019 5:48 AM
> To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: Re: [edk2] [PATCH] EmulatorPkg/Sec: Don't install
> TemporaryRamSupport PPI
>
> On 2019-03-04 18:37:03, Ni, Ray wrote:
> >
> > > -----Original Message-----
> > > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of
> > > Jordan Justen
> > > Sent: Sunday, March 3, 2019 5:45 AM
> > > To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [PATCH] EmulatorPkg/Sec: Don't install
> > > TemporaryRamSupport PPI
> > >
> > > On 2019-03-02 04:11:11, Ray Ni wrote:
> > > >
> > > > So this patch removes TemporaryRamSupport PPI implementation from
> > > > EmulatorPkg/Sec module to fix the boot failure when using GCC5.
> > >
> > > I don't think we should just hide the known bug with the
> > > TemporaryRamSupport PPI implementation in the PEI Core.
> > >
> > > I would agree that we should make this change after addressing that.
> >
> > Jordan,
> > I have a goal to replace Nt32 with EmulatorPkg before Q2 stable tag release.
> > I understand we need more discussions on how to fix the PeiCore bug.
> > So I don't want this blocks the Nt32 deletion.
>
> Given the choice of fixing a known bug in PEI Core relating to the PI spec
> TemporaryRamSupport PPI, or deleting Nt32, I would choose fixing the bug.
>
> But, why not fix the bug, and then make this change?
>
Mike, Liming, Jian,
Can you please review Jordan's patch to fix the code calling TemporaryRamSupport PPI
in PeiCore with some assembly code?
Personally I don't like the assembly code.
But you are the experts and maintainers, I am ok if you give a R-b.
Ard, Leif,
Jordan is fixing a bug in PeiCore but only fixing x86. I am not sure whether ARM code
also contains such TemporaryRamSupport PPI issue.
Thanks,
Ray
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-06 2:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-02 12:11 [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI Ray Ni
2019-03-02 21:45 ` Jordan Justen
2019-03-05 2:37 ` Ni, Ray
2019-03-05 21:48 ` Jordan Justen
2019-03-06 2:23 ` Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox