* [PATCH 01/10] EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
@ 2019-02-18 4:11 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 02/10] EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram Jordan Justen
` (9 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Andrew Fish, Ray Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
---
EmulatorPkg/build.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/EmulatorPkg/build.sh b/EmulatorPkg/build.sh
index 2daaaadcd4..2af6b5f998 100755
--- a/EmulatorPkg/build.sh
+++ b/EmulatorPkg/build.sh
@@ -1,7 +1,7 @@
#!/bin/bash
#
# Copyright (c) 2008 - 2011, Apple Inc. All rights reserved.<BR>
-# Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2010 - 2018, 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
@@ -194,7 +194,7 @@ do
done
PLATFORMFILE=$WORKSPACE/EmulatorPkg/EmulatorPkg.dsc
-BUILD_DIR=$BUILD_OUTPUT_DIR/DEBUG_"$TARGET_TOOLS"
+BUILD_DIR=$BUILD_OUTPUT_DIR/"$BUILDTARGET"_"$TARGET_TOOLS"
BUILD_ROOT_ARCH=$BUILD_DIR/$PROCESSOR
if [[ ! -f `which build` || ! -f `which GenFv` ]];
@@ -224,11 +224,11 @@ if [[ "$RUN_EMULATOR" == "yes" ]]; then
then
# only older versions of Xcode support -ccc-host-tripe, for newer versions
# it is -target
- cp $WORKSPACE/EmulatorPkg/Unix/lldbefi.py $BUILD_OUTPUT_DIR/DEBUG_"$TARGET_TOOLS"/$PROCESSOR
+ cp $WORKSPACE/EmulatorPkg/Unix/lldbefi.py $BUILD_OUTPUT_DIR/"$BUILDTARGET"_"$TARGET_TOOLS"/$PROCESSOR
cd $BUILD_ROOT_ARCH; /usr/bin/lldb --source $WORKSPACE/EmulatorPkg/Unix/lldbinit Host
exit $?
else
- cp $WORKSPACE/EmulatorPkg/Unix/.gdbinit $BUILD_OUTPUT_DIR/DEBUG_"$TARGET_TOOLS"/$PROCESSOR
+ cp $WORKSPACE/EmulatorPkg/Unix/.gdbinit $BUILD_OUTPUT_DIR/"$BUILDTARGET"_"$TARGET_TOOLS"/$PROCESSOR
fi
;;
esac
@@ -261,7 +261,7 @@ if [[ $HOST_TOOLS == $TARGET_TOOLS ]]; then
else
build -p $WORKSPACE/EmulatorPkg/EmulatorPkg.dsc $BUILD_OPTIONS -a $PROCESSOR -b $BUILDTARGET -t $HOST_TOOLS -D BUILD_$ARCH_SIZE -D UNIX_SEC_BUILD -D SKIP_MAIN_BUILD -n 3 modules
build -p $WORKSPACE/EmulatorPkg/EmulatorPkg.dsc $BUILD_OPTIONS -a $PROCESSOR -b $BUILDTARGET -t $TARGET_TOOLS -D BUILD_$ARCH_SIZE $NETWORK_SUPPORT $BUILD_NEW_SHELL $BUILD_FAT -n 3
- cp $BUILD_OUTPUT_DIR/DEBUG_"$HOST_TOOLS"/$PROCESSOR/Host $BUILD_ROOT_ARCH
+ cp $BUILD_OUTPUT_DIR/"$BUILDTARGET"_"$HOST_TOOLS"/$PROCESSOR/Host $BUILD_ROOT_ARCH
fi
exit $?
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 02/10] EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
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 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 03/10] EmulatorPkg/Sec: Replace assembly temp-ram support with C code Jordan Justen
` (8 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Andrew Fish, Ray Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
---
EmulatorPkg/Unix/Host/Host.c | 2 +-
EmulatorPkg/Unix/Host/Host.inf | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/EmulatorPkg/Unix/Host/Host.c b/EmulatorPkg/Unix/Host/Host.c
index 9aba7c854d..2537b0427b 100644
--- a/EmulatorPkg/Unix/Host/Host.c
+++ b/EmulatorPkg/Unix/Host/Host.c
@@ -216,7 +216,7 @@ main (
for (StackPointer = (UINTN*) (UINTN) InitialStackMemory;
StackPointer < (UINTN*)(UINTN)((UINTN) InitialStackMemory + (UINT64) InitialStackMemorySize);
StackPointer ++) {
- *StackPointer = 0x5AA55AA5;
+ *StackPointer = PcdGet32 (PcdInitValueInTempStack);
}
//
diff --git a/EmulatorPkg/Unix/Host/Host.inf b/EmulatorPkg/Unix/Host/Host.inf
index 6db269842e..9cafa7b771 100644
--- a/EmulatorPkg/Unix/Host/Host.inf
+++ b/EmulatorPkg/Unix/Host/Host.inf
@@ -109,6 +109,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
gEmulatorPkgTokenSpaceGuid.PcdEmuFlashNvStorageFtwSpareBase
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
gEmulatorPkgTokenSpaceGuid.PcdPeiServicesTablePage
[FeaturePcd]
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 03/10] EmulatorPkg/Sec: Replace assembly temp-ram support with C code
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 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function Jordan Justen
` (7 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Andrew Fish, Ray Ni
OvmfPkg uses similar code based on SetJump/LongJump.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
---
EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 ------------------------------
EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 -----------------------------
EmulatorPkg/Sec/Ia32/TempRam.c | 65 --------------------
EmulatorPkg/Sec/Sec.c | 64 +++++++++++++++++++-
EmulatorPkg/Sec/Sec.inf | 13 +---
EmulatorPkg/Sec/X64/SwitchRam.S | 72 ----------------------
EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ------------------------
7 files changed, 66 insertions(+), 413 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..bd61e5ac4a 100644
--- a/EmulatorPkg/Sec/Sec.c
+++ b/EmulatorPkg/Sec/Sec.c
@@ -4,6 +4,8 @@
The OS application will call the SEC with the PEI Entry Point API.
Copyright (c) 2011, Apple Inc. All rights reserved.<BR>
+Copyright (c) 2018 - 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,9 +19,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include "Sec.h"
+EFI_STATUS
+EFIAPI
+TemporaryRamMigration (
+ IN CONST EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
+ IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
+ IN UINTN CopySize
+ );
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI mSecTemporaryRamSupportPpi = {
- SecTemporaryRamSupport
+ TemporaryRamMigration
};
@@ -31,6 +41,58 @@ EFI_PEI_PPI_DESCRIPTOR gPrivateDispatchTable[] = {
}
};
+EFI_STATUS
+EFIAPI
+TemporaryRamMigration (
+ IN CONST EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
+ IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
+ IN UINTN CopySize
+ )
+{
+ BASE_LIBRARY_JUMP_BUFFER JumpBuffer;
+ INTN OldToNewStackDelta;
+
+ DEBUG ((DEBUG_INFO,
+ "TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
+ TemporaryMemoryBase,
+ PermanentMemoryBase,
+ (UINT64)CopySize
+ ));
+
+ OldToNewStackDelta = (INTN)PermanentMemoryBase - (INTN)TemporaryMemoryBase;
+
+ CopyMem (
+ (VOID*)(UINTN) PermanentMemoryBase,
+ (VOID*)(UINTN) TemporaryMemoryBase,
+ CopySize
+ );
+
+ //
+ // Use SetJump()/LongJump() to switch to a new stack.
+ //
+ if (SetJump (&JumpBuffer) == 0) {
+#if defined (MDE_CPU_IA32)
+ JumpBuffer.Esp = JumpBuffer.Esp + OldToNewStackDelta;
+ JumpBuffer.Ebp = JumpBuffer.Ebp + OldToNewStackDelta;
+#endif
+#if defined (MDE_CPU_X64)
+ JumpBuffer.Rsp = JumpBuffer.Rsp + OldToNewStackDelta;
+ JumpBuffer.Rbp = JumpBuffer.Rbp + OldToNewStackDelta;
+#endif
+ LongJump (&JumpBuffer, (UINTN)-1);
+ }
+
+ //
+ // Initialize Temporary RAM to a bad value to make sure it will not
+ // be used after migration.
+ //
+ SetMem32 (
+ (VOID*)(UINTN)TemporaryMemoryBase, CopySize,
+ PcdGet32 (PcdInitValueInTempStack));
+
+ return EFI_SUCCESS;
+}
/**
diff --git a/EmulatorPkg/Sec/Sec.inf b/EmulatorPkg/Sec/Sec.inf
index d253fd724e..93c09d248a 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
@@ -26,17 +26,9 @@
[Sources]
Sec.c
-[Sources.X64]
- X64/SwitchRam.asm
- X64/SwitchRam.S
-
-[Sources.IA32]
- Ia32/TempRam.c
- Ia32/SwitchRam.asm
- Ia32/SwitchRam.S
-
[Packages]
MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
EmulatorPkg/EmulatorPkg.dec
[LibraryClasses]
@@ -50,4 +42,5 @@
gEfiTemporaryRamSupportPpiGuid
[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack
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.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (2 preceding siblings ...)
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 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations Jordan Justen
` (6 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Andrew Fish, Ray Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Ray Ni <ray.ni@intel.com>
---
EmulatorPkg/Sec/Sec.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/EmulatorPkg/Sec/Sec.c b/EmulatorPkg/Sec/Sec.c
index bd61e5ac4a..7d5534cb67 100644
--- a/EmulatorPkg/Sec/Sec.c
+++ b/EmulatorPkg/Sec/Sec.c
@@ -41,6 +41,13 @@ EFI_PEI_PPI_DESCRIPTOR gPrivateDispatchTable[] = {
}
};
+#ifdef __GNUC__
+#pragma GCC push_options
+#pragma GCC optimize ("O0")
+#else
+#pragma optimize ("", off)
+#endif
+
EFI_STATUS
EFIAPI
TemporaryRamMigration (
@@ -94,6 +101,11 @@ TemporaryRamMigration (
return EFI_SUCCESS;
}
+#ifdef __GNUC__
+#pragma GCC pop_options
+#else
+#pragma optimize ("", on)
+#endif
/**
The entry point of PE/COFF Image for the PEI Core, that has been hijacked by this
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (3 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function Jordan Justen
@ 2019-02-18 4:11 ` Jordan Justen
2019-02-18 12:58 ` Laszlo Ersek
2019-02-18 4:11 ` [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration Jordan Justen
` (5 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard,
Julien Grall
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.
//
- CopyMem (NewHeap, OldHeap, CopySize >> 1);
+ CopyMem(
+ (VOID*)(UINTN)PermanentMemoryBase,
+ (VOID*)(UINTN)TemporaryMemoryBase,
+ CopySize);
- //
- // 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
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
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
0 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2019-02-18 12:58 UTC (permalink / raw)
To: Jordan Justen, edk2-devel; +Cc: Ard Biesheuvel, Anthony Perard, Julien Grall
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
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (4 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations Jordan Justen
@ 2019-02-18 4:11 ` Jordan Justen
2019-02-18 7:53 ` Ard Biesheuvel
2019-02-18 4:11 ` [PATCH 07/10] MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram migration Jordan Justen
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard,
Julien Grall
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/SecMain.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 46ac739862..86c22a2ac9 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -873,6 +873,13 @@ SecStartupPhase2(
CpuDeadLoop ();
}
+#ifdef __GNUC__
+#pragma GCC push_options
+#pragma GCC optimize ("O0")
+#else
+#pragma optimize ("", off)
+#endif
+
EFI_STATUS
EFIAPI
TemporaryRamMigration (
@@ -946,3 +953,8 @@ TemporaryRamMigration (
return EFI_SUCCESS;
}
+#ifdef __GNUC__
+#pragma GCC pop_options
+#else
+#pragma optimize ("", on)
+#endif
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
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
0 siblings, 1 reply; 29+ messages in thread
From: Ard Biesheuvel @ 2019-02-18 7:53 UTC (permalink / raw)
To: Jordan Justen
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Anthony Perard,
Julien Grall
On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
>
This needs an explanation why optimization needs to be disabled.
> 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/SecMain.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 46ac739862..86c22a2ac9 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -873,6 +873,13 @@ SecStartupPhase2(
> CpuDeadLoop ();
> }
>
> +#ifdef __GNUC__
> +#pragma GCC push_options
> +#pragma GCC optimize ("O0")
> +#else
> +#pragma optimize ("", off)
> +#endif
> +
> EFI_STATUS
> EFIAPI
> TemporaryRamMigration (
> @@ -946,3 +953,8 @@ TemporaryRamMigration (
> return EFI_SUCCESS;
> }
>
> +#ifdef __GNUC__
> +#pragma GCC pop_options
> +#else
> +#pragma optimize ("", on)
> +#endif
I can't tell from the context if this is the end of the file, but if
it is not, aren't you turning on optimization here for non-GCC even if
it was not enabled on the command line to begin with?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-18 7:53 ` Ard Biesheuvel
@ 2019-02-18 9:08 ` Jordan Justen
2019-02-18 9:32 ` Ard Biesheuvel
0 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 9:08 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Anthony Perard,
Julien Grall
On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
> On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >
>
> This needs an explanation why optimization needs to be disabled.
I'm not sure this is required. The reason I added these patches is to
hopefully prevent the compiler from removing the frame pointer. We
adjust the frame pointer in the code, and that is a little sketchy if
the frame pointer isn't being used.
Unfortunately, it can reasonably be argued that the
TemporaryRamSupport PPI definition ultimately makes it unsafe to write
the migration code in C.
I tried reverting both the EmulatorPkg and OvmfPkg patches for
disabling the optimizations, and with my setup there was no impact. I
think there is a good change that we'd be pretty safe to just drop
these two patches to wait and see if someone encounters a situation
that requires it.
Ok, so based on this explanation, do you think I should add info to
the commit message and keep the patches, or just drop them?
Thanks,
-Jordan
>
> > 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/SecMain.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> > index 46ac739862..86c22a2ac9 100644
> > --- a/OvmfPkg/Sec/SecMain.c
> > +++ b/OvmfPkg/Sec/SecMain.c
> > @@ -873,6 +873,13 @@ SecStartupPhase2(
> > CpuDeadLoop ();
> > }
> >
> > +#ifdef __GNUC__
> > +#pragma GCC push_options
> > +#pragma GCC optimize ("O0")
> > +#else
> > +#pragma optimize ("", off)
> > +#endif
> > +
> > EFI_STATUS
> > EFIAPI
> > TemporaryRamMigration (
> > @@ -946,3 +953,8 @@ TemporaryRamMigration (
> > return EFI_SUCCESS;
> > }
> >
> > +#ifdef __GNUC__
> > +#pragma GCC pop_options
> > +#else
> > +#pragma optimize ("", on)
> > +#endif
>
> I can't tell from the context if this is the end of the file, but if
> it is not, aren't you turning on optimization here for non-GCC even if
> it was not enabled on the command line to begin with?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-18 9:08 ` Jordan Justen
@ 2019-02-18 9:32 ` Ard Biesheuvel
2019-02-18 13:01 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-02-18 9:32 UTC (permalink / raw)
To: Jordan Justen
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Anthony Perard,
Julien Grall
On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
>
> On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
> > On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > >
> >
> > This needs an explanation why optimization needs to be disabled.
>
> I'm not sure this is required. The reason I added these patches is to
> hopefully prevent the compiler from removing the frame pointer. We
> adjust the frame pointer in the code, and that is a little sketchy if
> the frame pointer isn't being used.
>
> Unfortunately, it can reasonably be argued that the
> TemporaryRamSupport PPI definition ultimately makes it unsafe to write
> the migration code in C.
>
> I tried reverting both the EmulatorPkg and OvmfPkg patches for
> disabling the optimizations, and with my setup there was no impact. I
> think there is a good change that we'd be pretty safe to just drop
> these two patches to wait and see if someone encounters a situation
> that requires it.
>
> Ok, so based on this explanation, do you think I should add info to
> the commit message and keep the patches, or just drop them?
>
I think 'little sketchy' is an understatement here (as is
setjmp/longjmp in general), but it is the reality we have to deal with
when writing startup code in C. Looking at the code, I agree that the
fact that [re]bp is assigned directly implies that we should not
permit it to be used as a general purpose register, especially when
you throw LTO into the mix, which could produce all kinds of
surprising results when it decides to inline functions being called
from here.
For GCC/Clang, I don't think it is correct to assume that changing the
optimization level will result in -fno-omit-frame-pointer to be set,
so I'd prefer setting that option directly, either via the pragma, or
for the whole file.
For MSVC, I have no idea how to tweak the compiler to force it to emit
frame pointers.
> >
> > > 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/SecMain.c | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> > > index 46ac739862..86c22a2ac9 100644
> > > --- a/OvmfPkg/Sec/SecMain.c
> > > +++ b/OvmfPkg/Sec/SecMain.c
> > > @@ -873,6 +873,13 @@ SecStartupPhase2(
> > > CpuDeadLoop ();
> > > }
> > >
> > > +#ifdef __GNUC__
> > > +#pragma GCC push_options
> > > +#pragma GCC optimize ("O0")
> > > +#else
> > > +#pragma optimize ("", off)
> > > +#endif
> > > +
> > > EFI_STATUS
> > > EFIAPI
> > > TemporaryRamMigration (
> > > @@ -946,3 +953,8 @@ TemporaryRamMigration (
> > > return EFI_SUCCESS;
> > > }
> > >
> > > +#ifdef __GNUC__
> > > +#pragma GCC pop_options
> > > +#else
> > > +#pragma optimize ("", on)
> > > +#endif
> >
> > I can't tell from the context if this is the end of the file, but if
> > it is not, aren't you turning on optimization here for non-GCC even if
> > it was not enabled on the command line to begin with?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-18 9:32 ` Ard Biesheuvel
@ 2019-02-18 13:01 ` Laszlo Ersek
2019-02-19 22:50 ` Brian J. Johnson
2019-02-20 8:52 ` Jordan Justen
2 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2019-02-18 13:01 UTC (permalink / raw)
To: Ard Biesheuvel, Jordan Justen
Cc: edk2-devel@lists.01.org, Anthony Perard, Julien Grall
On 02/18/19 10:32, Ard Biesheuvel wrote:
> On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>
>> On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
>>> On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>>>
>>>
>>> This needs an explanation why optimization needs to be disabled.
>>
>> I'm not sure this is required. The reason I added these patches is to
>> hopefully prevent the compiler from removing the frame pointer. We
>> adjust the frame pointer in the code, and that is a little sketchy if
>> the frame pointer isn't being used.
>>
>> Unfortunately, it can reasonably be argued that the
>> TemporaryRamSupport PPI definition ultimately makes it unsafe to write
>> the migration code in C.
>>
>> I tried reverting both the EmulatorPkg and OvmfPkg patches for
>> disabling the optimizations, and with my setup there was no impact. I
>> think there is a good change that we'd be pretty safe to just drop
>> these two patches to wait and see if someone encounters a situation
>> that requires it.
>>
>> Ok, so based on this explanation, do you think I should add info to
>> the commit message and keep the patches, or just drop them?
>>
>
> I think 'little sketchy' is an understatement here (as is
> setjmp/longjmp in general), but it is the reality we have to deal with
> when writing startup code in C. Looking at the code, I agree that the
> fact that [re]bp is assigned directly implies that we should not
> permit it to be used as a general purpose register, especially when
> you throw LTO into the mix, which could produce all kinds of
> surprising results when it decides to inline functions being called
> from here.
>
> For GCC/Clang, I don't think it is correct to assume that changing the
> optimization level will result in -fno-omit-frame-pointer to be set,
> so I'd prefer setting that option directly, either via the pragma, or
> for the whole file.
>
> For MSVC, I have no idea how to tweak the compiler to force it to emit
> frame pointers.
I think modifying the build options in the INF file would be more readable.
Thanks
Laszlo
>>>
>>>> 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/SecMain.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>>>> index 46ac739862..86c22a2ac9 100644
>>>> --- a/OvmfPkg/Sec/SecMain.c
>>>> +++ b/OvmfPkg/Sec/SecMain.c
>>>> @@ -873,6 +873,13 @@ SecStartupPhase2(
>>>> CpuDeadLoop ();
>>>> }
>>>>
>>>> +#ifdef __GNUC__
>>>> +#pragma GCC push_options
>>>> +#pragma GCC optimize ("O0")
>>>> +#else
>>>> +#pragma optimize ("", off)
>>>> +#endif
>>>> +
>>>> EFI_STATUS
>>>> EFIAPI
>>>> TemporaryRamMigration (
>>>> @@ -946,3 +953,8 @@ TemporaryRamMigration (
>>>> return EFI_SUCCESS;
>>>> }
>>>>
>>>> +#ifdef __GNUC__
>>>> +#pragma GCC pop_options
>>>> +#else
>>>> +#pragma optimize ("", on)
>>>> +#endif
>>>
>>> I can't tell from the context if this is the end of the file, but if
>>> it is not, aren't you turning on optimization here for non-GCC even if
>>> it was not enabled on the command line to begin with?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
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
2 siblings, 1 reply; 29+ messages in thread
From: Brian J. Johnson @ 2019-02-19 22:50 UTC (permalink / raw)
To: Ard Biesheuvel, Jordan Justen
Cc: Anthony Perard, edk2-devel@lists.01.org, Laszlo Ersek
On 2/18/19 3:32 AM, Ard Biesheuvel wrote:
> On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>
>> On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
>>> On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>>>
>>>
>>> This needs an explanation why optimization needs to be disabled.
>>
>> I'm not sure this is required. The reason I added these patches is to
>> hopefully prevent the compiler from removing the frame pointer. We
>> adjust the frame pointer in the code, and that is a little sketchy if
>> the frame pointer isn't being used.
>>
>> Unfortunately, it can reasonably be argued that the
>> TemporaryRamSupport PPI definition ultimately makes it unsafe to write
>> the migration code in C.
>>
>> I tried reverting both the EmulatorPkg and OvmfPkg patches for
>> disabling the optimizations, and with my setup there was no impact. I
>> think there is a good change that we'd be pretty safe to just drop
>> these two patches to wait and see if someone encounters a situation
>> that requires it.
>>
>> Ok, so based on this explanation, do you think I should add info to
>> the commit message and keep the patches, or just drop them?
>>
>
> I think 'little sketchy' is an understatement here (as is
> setjmp/longjmp in general), but it is the reality we have to deal with
> when writing startup code in C. Looking at the code, I agree that the
> fact that [re]bp is assigned directly implies that we should not
> permit it to be used as a general purpose register, especially when
> you throw LTO into the mix, which could produce all kinds of
> surprising results when it decides to inline functions being called
> from here.
>
> For GCC/Clang, I don't think it is correct to assume that changing the
> optimization level will result in -fno-omit-frame-pointer to be set,
> so I'd prefer setting that option directly, either via the pragma, or
> for the whole file.
>
> For MSVC, I have no idea how to tweak the compiler to force it to emit
> frame pointers.
>
https://docs.microsoft.com/en-us/cpp/build/reference/oy-frame-pointer-omission?view=vs-2017
Brian
>
>>>
>>>> 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/SecMain.c | 12 ++++++++++++
>>>> 1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>>>> index 46ac739862..86c22a2ac9 100644
>>>> --- a/OvmfPkg/Sec/SecMain.c
>>>> +++ b/OvmfPkg/Sec/SecMain.c
>>>> @@ -873,6 +873,13 @@ SecStartupPhase2(
>>>> CpuDeadLoop ();
>>>> }
>>>>
>>>> +#ifdef __GNUC__
>>>> +#pragma GCC push_options
>>>> +#pragma GCC optimize ("O0")
>>>> +#else
>>>> +#pragma optimize ("", off)
>>>> +#endif
>>>> +
>>>> EFI_STATUS
>>>> EFIAPI
>>>> TemporaryRamMigration (
>>>> @@ -946,3 +953,8 @@ TemporaryRamMigration (
>>>> return EFI_SUCCESS;
>>>> }
>>>>
>>>> +#ifdef __GNUC__
>>>> +#pragma GCC pop_options
>>>> +#else
>>>> +#pragma optimize ("", on)
>>>> +#endif
>>>
>>> I can't tell from the context if this is the end of the file, but if
>>> it is not, aren't you turning on optimization here for non-GCC even if
>>> it was not enabled on the command line to begin with?
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
--
Brian J. Johnson
Enterprise X86 Lab
Hewlett Packard Enterprise
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-19 22:50 ` Brian J. Johnson
@ 2019-02-19 23:58 ` Jordan Justen
0 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-19 23:58 UTC (permalink / raw)
To: Brian J. Johnson, Ard Biesheuvel
Cc: Anthony Perard, edk2-devel@lists.01.org, Laszlo Ersek
On 2019-02-19 14:50:13, Brian J. Johnson wrote:
> On 2/18/19 3:32 AM, Ard Biesheuvel wrote:
> > On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >>
> >> On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
> >>> On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >>>>
> >>>
> >>> This needs an explanation why optimization needs to be disabled.
> >>
> >> I'm not sure this is required. The reason I added these patches is to
> >> hopefully prevent the compiler from removing the frame pointer. We
> >> adjust the frame pointer in the code, and that is a little sketchy if
> >> the frame pointer isn't being used.
> >>
> >> Unfortunately, it can reasonably be argued that the
> >> TemporaryRamSupport PPI definition ultimately makes it unsafe to write
> >> the migration code in C.
> >>
> >> I tried reverting both the EmulatorPkg and OvmfPkg patches for
> >> disabling the optimizations, and with my setup there was no impact. I
> >> think there is a good change that we'd be pretty safe to just drop
> >> these two patches to wait and see if someone encounters a situation
> >> that requires it.
> >>
> >> Ok, so based on this explanation, do you think I should add info to
> >> the commit message and keep the patches, or just drop them?
> >>
> >
> > I think 'little sketchy' is an understatement here (as is
> > setjmp/longjmp in general), but it is the reality we have to deal with
> > when writing startup code in C. Looking at the code, I agree that the
> > fact that [re]bp is assigned directly implies that we should not
> > permit it to be used as a general purpose register, especially when
> > you throw LTO into the mix, which could produce all kinds of
> > surprising results when it decides to inline functions being called
> > from here.
> >
> > For GCC/Clang, I don't think it is correct to assume that changing the
> > optimization level will result in -fno-omit-frame-pointer to be set,
> > so I'd prefer setting that option directly, either via the pragma, or
> > for the whole file.
> >
> > For MSVC, I have no idea how to tweak the compiler to force it to emit
> > frame pointers.
> >
>
> https://docs.microsoft.com/en-us/cpp/build/reference/oy-frame-pointer-omission?view=vs-2017
Hmm, and based on:
https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=vs-2017
#pragma optimize ("", off)
...includes the "y" option.
This 2nd page seems a little confused, as it documents "y" as
"Generate frame pointers on the program stack", while the 1st page
says "Suppresses creation of frame pointers on the call stack".
I think the "suppress" is more accurate as it makes more sense that
suppressing the frame pointer gives better optimization opportunities.
Anyway, I think that means that `#pragma optimize ("", off)` does what
we want on MSVC to force frame pointers to be used.
-Jordan
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-18 9:32 ` Ard Biesheuvel
2019-02-18 13:01 ` Laszlo Ersek
2019-02-19 22:50 ` Brian J. Johnson
@ 2019-02-20 8:52 ` Jordan Justen
2019-02-20 8:59 ` Ard Biesheuvel
2 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-20 8:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Anthony Perard,
Julien Grall
On 2019-02-18 01:32:53, Ard Biesheuvel wrote:
> On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >
> > On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
> > > On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > > >
> > >
> > > This needs an explanation why optimization needs to be disabled.
> >
> > I'm not sure this is required. The reason I added these patches is to
> > hopefully prevent the compiler from removing the frame pointer. We
> > adjust the frame pointer in the code, and that is a little sketchy if
> > the frame pointer isn't being used.
> >
> > Unfortunately, it can reasonably be argued that the
> > TemporaryRamSupport PPI definition ultimately makes it unsafe to write
> > the migration code in C.
> >
> > I tried reverting both the EmulatorPkg and OvmfPkg patches for
> > disabling the optimizations, and with my setup there was no impact. I
> > think there is a good change that we'd be pretty safe to just drop
> > these two patches to wait and see if someone encounters a situation
> > that requires it.
> >
> > Ok, so based on this explanation, do you think I should add info to
> > the commit message and keep the patches, or just drop them?
> >
>
> I think 'little sketchy' is an understatement here (as is
> setjmp/longjmp in general), but it is the reality we have to deal with
> when writing startup code in C. Looking at the code, I agree that the
> fact that [re]bp is assigned directly implies that we should not
> permit it to be used as a general purpose register, especially when
> you throw LTO into the mix, which could produce all kinds of
> surprising results when it decides to inline functions being called
> from here.
>
> For GCC/Clang, I don't think it is correct to assume that changing the
> optimization level will result in -fno-omit-frame-pointer to be set,
> so I'd prefer setting that option directly, either via the pragma, or
> for the whole file.
Based on: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
It appears that -O0 will not have -fomit-frame-pointer, since that is
added in -O1.
For both gcc and MSVC, I think we could be more targeted:
#ifdef __GNUC__
#pragma GCC push_options
#pragma GCC optimize ("no-omit-frame-pointer")
#else
#pragma optimize ("y", off)
#endif
Do you prefer this version?
-Jordan
> For MSVC, I have no idea how to tweak the compiler to force it to emit
> frame pointers.
>
>
> > >
> > > > 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/SecMain.c | 12 ++++++++++++
> > > > 1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> > > > index 46ac739862..86c22a2ac9 100644
> > > > --- a/OvmfPkg/Sec/SecMain.c
> > > > +++ b/OvmfPkg/Sec/SecMain.c
> > > > @@ -873,6 +873,13 @@ SecStartupPhase2(
> > > > CpuDeadLoop ();
> > > > }
> > > >
> > > > +#ifdef __GNUC__
> > > > +#pragma GCC push_options
> > > > +#pragma GCC optimize ("O0")
> > > > +#else
> > > > +#pragma optimize ("", off)
> > > > +#endif
> > > > +
> > > > EFI_STATUS
> > > > EFIAPI
> > > > TemporaryRamMigration (
> > > > @@ -946,3 +953,8 @@ TemporaryRamMigration (
> > > > return EFI_SUCCESS;
> > > > }
> > > >
> > > > +#ifdef __GNUC__
> > > > +#pragma GCC pop_options
> > > > +#else
> > > > +#pragma optimize ("", on)
> > > > +#endif
> > >
> > > I can't tell from the context if this is the end of the file, but if
> > > it is not, aren't you turning on optimization here for non-GCC even if
> > > it was not enabled on the command line to begin with?
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
2019-02-20 8:52 ` Jordan Justen
@ 2019-02-20 8:59 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2019-02-20 8:59 UTC (permalink / raw)
To: Jordan Justen
Cc: edk2-devel@lists.01.org, Laszlo Ersek, Anthony Perard,
Julien Grall
On Wed, 20 Feb 2019 at 09:52, Jordan Justen <jordan.l.justen@intel.com> wrote:
>
> On 2019-02-18 01:32:53, Ard Biesheuvel wrote:
> > On Mon, 18 Feb 2019 at 10:08, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > >
> > > On 2019-02-17 23:53:01, Ard Biesheuvel wrote:
> > > > On Mon, 18 Feb 2019 at 05:12, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > > > >
> > > >
> > > > This needs an explanation why optimization needs to be disabled.
> > >
> > > I'm not sure this is required. The reason I added these patches is to
> > > hopefully prevent the compiler from removing the frame pointer. We
> > > adjust the frame pointer in the code, and that is a little sketchy if
> > > the frame pointer isn't being used.
> > >
> > > Unfortunately, it can reasonably be argued that the
> > > TemporaryRamSupport PPI definition ultimately makes it unsafe to write
> > > the migration code in C.
> > >
> > > I tried reverting both the EmulatorPkg and OvmfPkg patches for
> > > disabling the optimizations, and with my setup there was no impact. I
> > > think there is a good change that we'd be pretty safe to just drop
> > > these two patches to wait and see if someone encounters a situation
> > > that requires it.
> > >
> > > Ok, so based on this explanation, do you think I should add info to
> > > the commit message and keep the patches, or just drop them?
> > >
> >
> > I think 'little sketchy' is an understatement here (as is
> > setjmp/longjmp in general), but it is the reality we have to deal with
> > when writing startup code in C. Looking at the code, I agree that the
> > fact that [re]bp is assigned directly implies that we should not
> > permit it to be used as a general purpose register, especially when
> > you throw LTO into the mix, which could produce all kinds of
> > surprising results when it decides to inline functions being called
> > from here.
> >
> > For GCC/Clang, I don't think it is correct to assume that changing the
> > optimization level will result in -fno-omit-frame-pointer to be set,
> > so I'd prefer setting that option directly, either via the pragma, or
> > for the whole file.
>
> Based on: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
>
> It appears that -O0 will not have -fomit-frame-pointer, since that is
> added in -O1.
>
For current versions of GCC, perhaps. But what about older versions?
What about future versions? What about Clang?
> For both gcc and MSVC, I think we could be more targeted:
>
> #ifdef __GNUC__
> #pragma GCC push_options
> #pragma GCC optimize ("no-omit-frame-pointer")
> #else
> #pragma optimize ("y", off)
> #endif
>
> Do you prefer this version?
>
Assuming that "y" affects frame pointer generation, yes.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 07/10] MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram migration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (5 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration Jordan Justen
@ 2019-02-18 4:11 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration Jordan Justen
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
There is potential problem with PEI Core's usage of the
TemporaryRamMigration PPI. When the TemporaryRamMigration function is
called, it returns to C based code after changing the stack to the new
permanent memory copy of the stack. But, the C compiler may have
stored pointers to addresses on the old temporary RAM stack. Even
though the stack is copied to a new permanent memory location, it is
not possible to adjust all pointers that the C compiler may have added
within the stack data.
For this reason, it is only safe to return to assembly code after
calling TemporaryRamMigration. The assembly code can make sure the old
temporary RAM stack is not used before calling a new C function. When
the new function is called, it will use the new permanent memory
stack, so it is safe to use C code again.
This code adds new function named PeiTemporaryRamMigration which can
be implemented in assembly code as described above.
PeiTemporaryRamMigration must call the TemporaryRamMigration function,
and then calls a new C PeiTemporaryRamMigrated function. This
guanantees PeiTemporaryRamMigrated will only use the permanent memory
stack.
For now, this patch should have no effect, since it still uses C to
implement PeiTemporaryRamMigration. But, PeiTemporaryRamMigration can
then be changed into an assembly code function to fix the issue
described above.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++++++-------
.../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
4 files changed, 143 insertions(+), 21 deletions(-)
create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index 68670f43e0..8e3fa161dd 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -688,6 +688,7 @@ PeiCheckAndSwitchStack (
EFI_PHYSICAL_ADDRESS TempBase2;
UINTN TempSize2;
UINTN Index;
+ PEI_CORE_TEMPORARY_RAM_TRANSITION TempRamTransitionData;
PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
@@ -822,30 +823,20 @@ PeiCheckAndSwitchStack (
Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private - StackOffset);
}
- //
- // Temporary Ram Support PPI is provided by platform, it will copy
- // temporary memory to permanent memory and do stack switching.
- // After invoking Temporary Ram Support PPI, the following code's
- // stack is in permanent memory.
- //
- TemporaryRamSupportPpi->TemporaryRamMigration (
- PeiServices,
- TemporaryRamBase,
- (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize),
- TemporaryRamSize
- );
-
- //
- // Migrate memory pages allocated in pre-memory phase.
- // It could not be called before calling TemporaryRamSupportPpi->TemporaryRamMigration()
- // as the migrated memory pages may be overridden by TemporaryRamSupportPpi->TemporaryRamMigration().
- //
- MigrateMemoryPages (Private, TRUE);
+ TempRamTransitionData.TemporaryRamMigration =
+ TemporaryRamSupportPpi->TemporaryRamMigration;
+ TempRamTransitionData.PeiServices = PeiServices;
+ TempRamTransitionData.TemporaryMemoryBase = TemporaryRamBase;
+ TempRamTransitionData.PermanentMemoryBase =
+ (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize);
+ TempRamTransitionData.CopySize = TemporaryRamSize;
+ TempRamTransitionData.Private = Private;
+ TempRamTransitionData.SecCoreData = SecCoreData;
//
- // Entry PEI Phase 2
+ // Migrate Temporary RAM and enter PEI Phase 2
//
- PeiCore (SecCoreData, NULL, Private);
+ PeiTemporaryRamMigration(&TempRamTransitionData);
} else {
//
// Migrate memory pages allocated in pre-memory phase.
@@ -958,6 +949,32 @@ PeiCheckAndSwitchStack (
}
}
+VOID
+EFIAPI
+PeiTemporaryRamMigrated (
+ IN VOID *CallbackContext
+ )
+{
+ PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData =
+ (PEI_CORE_TEMPORARY_RAM_TRANSITION*)CallbackContext;
+
+ //
+ // Migrate memory pages allocated in pre-memory phase.
+ // It could not be called before calling TemporaryRamSupportPpi->TemporaryRamMigration()
+ // as the migrated memory pages may be overridden by TemporaryRamSupportPpi->TemporaryRamMigration().
+ //
+ MigrateMemoryPages (TempRamTransitionData->Private, TRUE);
+
+ //
+ // Entry PEI Phase 2
+ //
+ PeiCore (
+ TempRamTransitionData->SecCoreData,
+ NULL,
+ TempRamTransitionData->Private
+ );
+}
+
/**
Conduct PEIM dispatch.
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c b/MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
new file mode 100644
index 0000000000..9e9d0854ed
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
@@ -0,0 +1,52 @@
+/** @file
+ EFI PEI Core temporary RAM migration
+
+Copyright (c) 2018, 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.
+
+**/
+
+#include "PeiMain.h"
+
+/**
+ To call the TemporaryRamMigration PPI, we might not be able to rely
+ on C code's handling of the stack. In these cases we use an assembly
+ function to make sure the old stack is not used after the
+ TemporaryRamMigration PPI is used.
+
+ After calling the TemporaryRamMigration PPI, this function calls
+ PeiTemporaryRamMigrated.
+
+ This C based function provides an implementation that may work for
+ some architectures.
+
+ @param TempRamTransitionData
+**/
+VOID
+EFIAPI
+PeiTemporaryRamMigration (
+ IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+ )
+{
+ //
+ // Temporary Ram Support PPI is provided by platform, it will copy
+ // temporary memory to permanent memory and do stack switching.
+ // After invoking Temporary Ram Support PPI, the following code's
+ // stack is in permanent memory.
+ //
+ TempRamTransitionData->TemporaryRamMigration (
+ TempRamTransitionData->PeiServices,
+ TempRamTransitionData->TemporaryMemoryBase,
+ TempRamTransitionData->PermanentMemoryBase,
+ TempRamTransitionData->CopySize
+ );
+
+ PeiTemporaryRamMigrated(TempRamTransitionData);
+}
diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index a61da73fd8..2d48f8d5d1 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -1871,4 +1871,56 @@ PeiReinitializeFv (
IN PEI_CORE_INSTANCE *PrivateData
);
+#pragma pack(1)
+typedef struct {
+ /**
+ These fields are used by PeiTemporaryRamMigration to call the
+ TemporaryRamMigration PPI.
+ **/
+ TEMPORARY_RAM_MIGRATION TemporaryRamMigration;
+ CONST EFI_PEI_SERVICES **PeiServices;
+ EFI_PHYSICAL_ADDRESS TemporaryMemoryBase;
+ EFI_PHYSICAL_ADDRESS PermanentMemoryBase;
+ UINTN CopySize;
+
+ /**
+ These fields are used by PeiTemporaryRamMigrated.
+ **/
+ PEI_CORE_INSTANCE *Private;
+ CONST EFI_SEC_PEI_HAND_OFF *SecCoreData;
+} PEI_CORE_TEMPORARY_RAM_TRANSITION;
+#pragma pack()
+
+/**
+ To call the TemporaryRamMigration PPI, we might not be able to rely
+ on C code's handling of the stack. In these cases we use an assembly
+ function to make sure the old stack is not used after the
+ TemporaryRamMigration PPI is used.
+
+ After calling the TemporaryRamMigration PPI, this function calls
+ PeiTemporaryRamMigrated.
+
+ @param TempRamTransitionData
+**/
+VOID
+EFIAPI
+PeiTemporaryRamMigration (
+ IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+ );
+
+/**
+ After PeiTemporaryRamMigration has called the TemporaryRamMigration
+ PPI, it will call this C based function to allow PEI to continue
+ after the migration using the new stack in the migrated RAM.
+
+ @param CallbackContext Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ data.
+**/
+VOID
+EFIAPI
+PeiTemporaryRamMigrated (
+ IN VOID *CallbackContext
+ );
+
+
#endif
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 5bab2aab8c..94b26b3572 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -45,6 +45,7 @@
FwVol/FwVol.c
FwVol/FwVol.h
Dispatcher/Dispatcher.c
+ Dispatcher/TemporaryRamMigration.c
Dependency/Dependency.c
Dependency/Dependency.h
BootMode/BootMode.c
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (6 preceding siblings ...)
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 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration Jordan Justen
` (2 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
Some compilers may optimize register usage in ways that are
incompatible with the TemporaryRamSupport PPI. Using assembly code to
call the TemporaryRamMigration function prevents this issue.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Dispatcher/X64/TemporaryRamMigration.S | 69 +++++++++++++++++
.../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++-
3 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
new file mode 100644
index 0000000000..cd9d902224
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
@@ -0,0 +1,69 @@
+#------------------------------------------------------------------------------
+#
+# Copyright (c) 2018 - 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 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.
+#
+#------------------------------------------------------------------------------
+
+#------------------------------------------------------------------------------
+# VOID
+# EFIAPI
+# PeiTemporaryRamMigration (
+# IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+# );
+#
+# @param[in] RCX Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+#
+# @return None This routine does not return
+#------------------------------------------------------------------------------
+ASM_GLOBAL ASM_PFX(PeiTemporaryRamMigration)
+ASM_PFX(PeiTemporaryRamMigration):
+
+ #
+ # We never return from this call
+ #
+ add $8, %rsp
+
+ #
+ # (rax) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ #
+ mov %rcx, %rax
+
+ #
+ # We'll store the new location of TempRamTransitionData after
+ # migration in rbx. By the X64 calling convention we should
+ # normally save rbx, but we won't be returning to the caller, so
+ # we don't need to save it. By the same rule, the
+ # TemporaryRamMigration PPI call should preserve rbx for us.
+ #
+ mov %rcx, %rbx
+ add 0x18(%rax), %rbx
+ sub 0x10(%rax), %rbx
+
+ #
+ # Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+ # (rcx) PeiServices
+ # (rdx) TemporaryMemoryBase
+ # (r8) PermanentMemoryBase
+ # (r9) CopySize
+ #
+ mov 0x08(%rax), %rcx
+ mov 0x10(%rax), %rdx
+ mov 0x18(%rax), %r8
+ mov 0x20(%rax), %r9
+ callq *(%rax)
+
+ #
+ # Setup parameters and call PeiTemporaryRamMigrated
+ # (rcx) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ #
+ mov %rbx, %rcx
+ call ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
new file mode 100644
index 0000000000..2783b0f403
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
@@ -0,0 +1,75 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2018, 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.
+;
+;------------------------------------------------------------------------------
+
+#include <Base.h>
+
+ SECTION .text
+
+extern ASM_PFX(PeiTemporaryRamMigrated)
+
+;------------------------------------------------------------------------------
+; VOID
+; EFIAPI
+; PeiTemporaryRamMigration (
+; IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+; );
+;
+; @param[in] RCX Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+;
+; @return None This routine does not return
+;------------------------------------------------------------------------------
+global ASM_PFX(PeiTemporaryRamMigration)
+ASM_PFX(PeiTemporaryRamMigration):
+
+ ;
+ ; We never return from this call
+ ;
+ add rsp, 8
+
+ ;
+ ; (rax) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ ;
+ mov rax, rcx
+
+ ;
+ ; We'll store the new location of TempRamTransitionData after
+ ; migration in rbx. By the X64 calling convention we should
+ ; normally save rbx, but we won't be returning to the caller, so
+ ; we don't need to save it. By the same rule, the
+ ; TemporaryRamMigration PPI call should preserve rbx for us.
+ ;
+ mov rbx, rcx
+ add rbx, [rax + 0x18]
+ sub rbx, [rax + 0x10]
+
+ ;
+ ; Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+ ; (rcx) PeiServices
+ ; (rdx) TemporaryMemoryBase
+ ; (r8) PermanentMemoryBase
+ ; (r9) CopySize
+ ;
+ mov rcx, [rax + 0x08]
+ mov rdx, [rax + 0x10]
+ mov r8, [rax + 0x18]
+ mov r9, [rax + 0x20]
+ call [rax + 0x00]
+
+ ;
+ ; Setup parameters and call PeiTemporaryRamMigrated
+ ; (rcx) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ ;
+ mov rcx, rbx
+ call ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 94b26b3572..1646f73385 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -45,7 +45,6 @@
FwVol/FwVol.c
FwVol/FwVol.h
Dispatcher/Dispatcher.c
- Dispatcher/TemporaryRamMigration.c
Dependency/Dependency.c
Dependency/Dependency.h
BootMode/BootMode.c
@@ -53,6 +52,19 @@
PciCfg2/PciCfg2.c
PeiMain.h
+[Sources.Ia32]
+ Dispatcher/TemporaryRamMigration.c
+
+[Sources.X64]
+ Dispatcher/X64/TemporaryRamMigration.nasm
+ Dispatcher/X64/TemporaryRamMigration.S
+
+[Sources.ARM]
+ Dispatcher/TemporaryRamMigration.c
+
+[Sources.AARCH64]
+ Dispatcher/TemporaryRamMigration.c
+
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (7 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration Jordan Justen
@ 2019-02-18 4:11 ` Jordan Justen
2019-02-18 4:11 ` [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration Jordan Justen
2019-02-19 2:46 ` [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Ni, Ray
10 siblings, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
.../Dispatcher/Ia32/TemporaryRamMigration.S | 72 +++++++++++++++++
.../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.inf | 3 +-
3 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
new file mode 100644
index 0000000000..363cadca2a
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
@@ -0,0 +1,72 @@
+#------------------------------------------------------------------------------
+#
+# Copyright (c) 2018 - 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 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.
+#
+#------------------------------------------------------------------------------
+
+#------------------------------------------------------------------------------
+# VOID
+# EFIAPI
+# PeiTemporaryRamMigration (
+# IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+# );
+#
+# @param[in] Stack Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+#
+# @return None This routine does not return
+#------------------------------------------------------------------------------
+ASM_GLOBAL ASM_PFX(PeiTemporaryRamMigration)
+ASM_PFX(PeiTemporaryRamMigration):
+
+ #
+ # We never return from this call
+ #
+ add $4, %esp
+
+ #
+ # (eax) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ #
+ mov (%esp), %eax
+
+ #
+ # We'll store the new location of TempRamTransitionData after
+ # migration in ebx. By the IA32 calling convention we should
+ # normally save ebx, but we won't be returning to the caller, so
+ # we don't need to save it. By the same rule, the
+ # TemporaryRamMigration PPI call should preserve ebx for us.
+ #
+ mov (%esp), %ebx
+ add 0x10(%eax), %ebx
+ sub 0x08(%eax), %ebx
+
+ #
+ # Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+ # 32-bit PeiServices
+ # 64-bit TemporaryMemoryBase
+ # 64-bit PermanentMemoryBase
+ # 32-bit CopySize
+ #
+ pushl 0x18(%eax) # CopySize
+ pushl 0x14(%eax) # PermanentMemoryBase Upper 32-bit
+ pushl 0x10(%eax) # PermanentMemoryBase Lower 32-bit
+ pushl 0x0c(%eax) # TemporaryMemoryBase Upper 32-bit
+ pushl 0x08(%eax) # TemporaryMemoryBase Lower 32-bit
+ pushl 0x04(%eax) # PeiServices
+ calll *(%eax)
+ sub 0x18, %esp
+
+ #
+ # Setup parameters and call PeiTemporaryRamMigrated
+ # 32-bit Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ #
+ push %ebx
+ call ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
new file mode 100644
index 0000000000..e200a3631f
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
@@ -0,0 +1,78 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2018 - 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 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 <Base.h>
+
+ SECTION .text
+
+extern ASM_PFX(PeiTemporaryRamMigrated)
+
+;------------------------------------------------------------------------------
+; VOID
+; EFIAPI
+; PeiTemporaryRamMigration (
+; IN PEI_CORE_TEMPORARY_RAM_TRANSITION *TempRamTransitionData
+; );
+;
+; @param[in] Stack Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+;
+; @return None This routine does not return
+;------------------------------------------------------------------------------
+global ASM_PFX(PeiTemporaryRamMigration)
+ASM_PFX(PeiTemporaryRamMigration):
+
+ ;
+ ; We never return from this call
+ ;
+ add esp, 4
+
+ ;
+ ; (eax) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ ;
+ mov eax, [esp]
+
+ ;
+ ; We'll store the new location of TempRamTransitionData after
+ ; migration in ebx. By the IA32 calling convention we should
+ ; normally save ebx, but we won't be returning to the caller, so
+ ; we don't need to save it. By the same rule, the
+ ; TemporaryRamMigration PPI call should preserve ebx for us.
+ ;
+ mov ebx, [esp]
+ add ebx, [eax + 0x10]
+ sub ebx, [eax + 0x08]
+
+ ;
+ ; Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+ ; 32-bit PeiServices
+ ; 64-bit TemporaryMemoryBase
+ ; 64-bit PermanentMemoryBase
+ ; 32-bit CopySize
+ ;
+ push DWORD [eax + 0x18] ; CopySize
+ push DWORD [eax + 0x14] ; PermanentMemoryBase Upper 32-bit
+ push DWORD [eax + 0x10] ; PermanentMemoryBase Lower 32-bit
+ push DWORD [eax + 0x0c] ; TemporaryMemoryBase Upper 32-bit
+ push DWORD [eax + 0x08] ; TemporaryMemoryBase Lower 32-bit
+ push DWORD [eax + 0x04] ; PeiServices
+ call DWORD [eax + 0x00]
+ sub esp, 0x18
+
+ ;
+ ; Setup parameters and call PeiTemporaryRamMigrated
+ ; 32-bit Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+ ;
+ push ebx
+ call ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 1646f73385..505544d4e8 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -53,7 +53,8 @@
PeiMain.h
[Sources.Ia32]
- Dispatcher/TemporaryRamMigration.c
+ Dispatcher/Ia32/TemporaryRamMigration.nasm
+ Dispatcher/Ia32/TemporaryRamMigration.S
[Sources.X64]
Dispatcher/X64/TemporaryRamMigration.nasm
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (8 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration Jordan Justen
@ 2019-02-18 4:11 ` 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
10 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-18 4:11 UTC (permalink / raw)
To: edk2-devel
Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard,
Julien Grall
On some platforms, the TemporaryRamMigration PPI may cause Temporary
RAM to become inaccessible after the RAM is migrated. To emulate this
in OVMF, we should initialize memory to a bad state to make sure it
isn't accidentally being used after TemporaryRamMigration is called.
I tested IA32 booting to shell and X64 booting to OS with this change.
With X64 I also tested S3 suspend/resume.
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/SecMain.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 86c22a2ac9..72946e0eab 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -948,6 +948,14 @@ TemporaryRamMigration (
LongJump (&JumpBuffer, (UINTN)-1);
}
+ //
+ // Initialize Temporary RAM to a bad value to make sure it will not
+ // be used after migration.
+ //
+ SetMem32 (
+ (VOID*)(UINTN)TemporaryMemoryBase, CopySize,
+ PcdGet32 (PcdInitValueInTempStack));
+
SaveAndSetDebugTimerInterrupt (OldStatus);
return EFI_SUCCESS;
--
2.20.0.rc1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
2019-02-18 4:11 ` [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration Jordan Justen
@ 2019-02-18 13:15 ` Laszlo Ersek
0 siblings, 0 replies; 29+ messages in thread
From: Laszlo Ersek @ 2019-02-18 13:15 UTC (permalink / raw)
To: Jordan Justen, edk2-devel; +Cc: Ard Biesheuvel, Anthony Perard, Julien Grall
On 02/18/19 05:11, Jordan Justen wrote:
> On some platforms, the TemporaryRamMigration PPI may cause Temporary
> RAM to become inaccessible after the RAM is migrated. To emulate this
> in OVMF, we should initialize memory to a bad state to make sure it
> isn't accidentally being used after TemporaryRamMigration is called.
>
> I tested IA32 booting to shell and X64 booting to OS with this change.
> With X64 I also tested S3 suspend/resume.
>
> 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/SecMain.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 86c22a2ac9..72946e0eab 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -948,6 +948,14 @@ TemporaryRamMigration (
> LongJump (&JumpBuffer, (UINTN)-1);
> }
>
> + //
> + // Initialize Temporary RAM to a bad value to make sure it will not
> + // be used after migration.
> + //
> + SetMem32 (
> + (VOID*)(UINTN)TemporaryMemoryBase, CopySize,
> + PcdGet32 (PcdInitValueInTempStack));
> +
> SaveAndSetDebugTimerInterrupt (OldStatus);
>
> return EFI_SUCCESS;
>
Assuming the previous PEI Core patches in this series don't change the
order, it looks like the PeiCheckAndSwitchStack() function
[MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c] logs the "stack ever
used" stat first, and calls the TemporaryRamMigration() PPI member second.
Thus, by the time we overwrite the temp RAM in this patch, the
statistics will have been printed in PeiCheckAndSwitchStack(). OK.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-18 4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (9 preceding siblings ...)
2019-02-18 4:11 ` [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration Jordan Justen
@ 2019-02-19 2:46 ` Ni, Ray
2019-02-19 13:25 ` Gao, Liming
2019-02-19 19:27 ` Jordan Justen
10 siblings, 2 replies; 29+ messages in thread
From: Ni, Ray @ 2019-02-19 2:46 UTC (permalink / raw)
To: Justen, Jordan L, edk2-devel@lists.01.org
Cc: Liu Yu, Andrew Fish, Anthony Perard, Ard Biesheuvel, Wu, Hao A,
Wang, Jian J, Julien Grall, Laszlo Ersek, Zeng, Star
Jordan,
I find many real platforms do not implement the temporary ram migration
PPI and rely on the PeiCore migration logic.
So perhaps TemporaryRamMigration PPI was added to help platform to
destroy the temporary RAM (CAR in x86 platform).
But with the introduction of TemporaryRamDone PPI, maybe the
TemporaryRamMigration PPI can be retired.
The logic in PeiCore to call TemporaryRamMigration is just for backward
compatibility.
If that's true, do you still need to enhance PeiCore?
For the Emulator case, I already found without TemporaryRamMigration
the platform can still boot.
Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
the PeiCore migration logic + TemporaryDone PPI?
Thanks,
Ray
> -----Original Message-----
> From: Justen, Jordan L <jordan.l.justen@intel.com>
> Sent: Monday, February 18, 2019 12:12 PM
> To: edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
> Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>
> https://github.com/jljusten/edk2.git temp-ram-support
>
> This series fixes an issue that, while rare, is possible based on the way the
> TemporaryRamSupport PPI is defined along with how it is used by the PEI
> Core.
>
> Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
> this issue.
>
> The details of the issue are described in the commit message of the
> "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> migration" patch.
>
> Along with this, I added a few Temporary RAM patches for EmulatorPkg and
> OvmfPkg.
>
> Cc: Liu Yu <pedroa.liu@outlook.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Jordan Justen (10):
> EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
> EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
> EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> function
> OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> migration
> MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
> MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
> OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
>
> EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
> EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
> EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
> EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
> EmulatorPkg/Sec/Sec.inf | 13 +--
> EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
> EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
> EmulatorPkg/Unix/Host/Host.c | 2 +-
> EmulatorPkg/Unix/Host/Host.inf | 1 +
> EmulatorPkg/build.sh | 10 +-
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> .../Dispatcher/Ia32/TemporaryRamMigration.S | 72 ++++++++++++++
> .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
> .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
> .../Dispatcher/X64/TemporaryRamMigration.S | 69 ++++++++++++++
> .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
> MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
> MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
> OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
> OvmfPkg/Sec/SecMain.c | 59 ++++++++----
> OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
> 21 files changed, 577 insertions(+), 461 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 create mode 100644
> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> create mode 100644
> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> create mode 100644
> MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> create mode 100644
> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> create mode 100644
> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>
> --
> 2.20.0.rc1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
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-19 19:27 ` Jordan Justen
1 sibling, 1 reply; 29+ messages in thread
From: Gao, Liming @ 2019-02-19 13:25 UTC (permalink / raw)
To: Ni, Ray, Justen, Jordan L, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star
Ray:
In PI spec, TEMPORARY_RAM_SUPPORT_PPI is described as an optional PPI that is only required for platforms that may have side effects when both Temporary RAM and Permanent RAM are enabled. If a platform does not have any side effects when both Temporary RAM and Permanent RAM are enabled, and the platform is required to disable the use of Temporary RAM, then EFI_PEI_TEMPORARY_RAM_DONE should be produced. If a platform does not have any side effects when both Temporary RAM and Permanent RAM are enabled, and the platform is not required to disable the use of Temporary RAM, then neither EFI_PEI_TEMPORARY_RAM_DONE nor EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI should be produced.
Now, real platform has no side effect. So, only TempRamDone PPI is produced. For emulator platform, is there any side effect when both Temporary RAM and Permanent RAM are enabled?
Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ray
> Sent: Tuesday, February 19, 2019 10:46 AM
> To: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>
> Jordan,
> I find many real platforms do not implement the temporary ram migration
> PPI and rely on the PeiCore migration logic.
> So perhaps TemporaryRamMigration PPI was added to help platform to
> destroy the temporary RAM (CAR in x86 platform).
> But with the introduction of TemporaryRamDone PPI, maybe the
> TemporaryRamMigration PPI can be retired.
> The logic in PeiCore to call TemporaryRamMigration is just for backward
> compatibility.
> If that's true, do you still need to enhance PeiCore?
>
> For the Emulator case, I already found without TemporaryRamMigration
> the platform can still boot.
>
> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
> the PeiCore migration logic + TemporaryDone PPI?
>
> Thanks,
> Ray
>
> > -----Original Message-----
> > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > Sent: Monday, February 18, 2019 12:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> > <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
> > Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
> > J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
> > <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> >
> > https://github.com/jljusten/edk2.git temp-ram-support
> >
> > This series fixes an issue that, while rare, is possible based on the way the
> > TemporaryRamSupport PPI is defined along with how it is used by the PEI
> > Core.
> >
> > Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
> > this issue.
> >
> > The details of the issue are described in the commit message of the
> > "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > migration" patch.
> >
> > Along with this, I added a few Temporary RAM patches for EmulatorPkg and
> > OvmfPkg.
> >
> > Cc: Liu Yu <pedroa.liu@outlook.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Julien Grall <julien.grall@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> >
> > Jordan Justen (10):
> > EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
> > EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
> > EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> > EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> > function
> > OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> > OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> > MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > migration
> > MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
> > MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
> > OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
> >
> > EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
> > EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
> > EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
> > EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
> > EmulatorPkg/Sec/Sec.inf | 13 +--
> > EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
> > EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
> > EmulatorPkg/Unix/Host/Host.c | 2 +-
> > EmulatorPkg/Unix/Host/Host.inf | 1 +
> > EmulatorPkg/build.sh | 10 +-
> > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> > .../Dispatcher/Ia32/TemporaryRamMigration.S | 72 ++++++++++++++
> > .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
> > .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
> > .../Dispatcher/X64/TemporaryRamMigration.S | 69 ++++++++++++++
> > .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
> > MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
> > MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
> > OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
> > OvmfPkg/Sec/SecMain.c | 59 ++++++++----
> > OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
> > 21 files changed, 577 insertions(+), 461 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 create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> >
> > --
> > 2.20.0.rc1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-19 13:25 ` Gao, Liming
@ 2019-02-20 13:27 ` Ni, Ray
2019-02-20 17:43 ` Jordan Justen
0 siblings, 1 reply; 29+ messages in thread
From: Ni, Ray @ 2019-02-20 13:27 UTC (permalink / raw)
To: Gao, Liming, Justen, Jordan L, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star
On 2/19/2019 9:25 PM, Gao, Liming wrote:
> Ray:
> In PI spec, TEMPORARY_RAM_SUPPORT_PPI is described as an optional PPI that is only required for platforms that may have side effects when both Temporary RAM and Permanent RAM are enabled. If a platform does not have any side effects when both Temporary RAM and Permanent RAM are enabled, and the platform is required to disable the use of Temporary RAM, then EFI_PEI_TEMPORARY_RAM_DONE should be produced. If a platform does not have any side effects when both Temporary RAM and Permanent RAM are enabled, and the platform is not required to disable the use of Temporary RAM, then neither EFI_PEI_TEMPORARY_RAM_DONE nor EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI should be produced.
>
> Now, real platform has no side effect. So, only TempRamDone PPI is produced. For emulator platform, is there any side effect when both Temporary RAM and Permanent RAM are enabled?
>
No side effect when both of T-RAM and P-RAM are enabled.
Which means no side effect when neither of the PPIs is produced.
But for demo purpose, I think producing TemporaryRamDone PPI makes sense.
I will work out patches for EmulatorPkg to produce TemoraryRamDone.
> Thanks
> Liming
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ray
>> Sent: Tuesday, February 19, 2019 10:46 AM
>> To: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org
>> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: Re: [edk2] [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>>
>> Jordan,
>> I find many real platforms do not implement the temporary ram migration
>> PPI and rely on the PeiCore migration logic.
>> So perhaps TemporaryRamMigration PPI was added to help platform to
>> destroy the temporary RAM (CAR in x86 platform).
>> But with the introduction of TemporaryRamDone PPI, maybe the
>> TemporaryRamMigration PPI can be retired.
>> The logic in PeiCore to call TemporaryRamMigration is just for backward
>> compatibility.
>> If that's true, do you still need to enhance PeiCore?
>>
>> For the Emulator case, I already found without TemporaryRamMigration
>> the platform can still boot.
>>
>> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
>> the PeiCore migration logic + TemporaryDone PPI?
>>
>> Thanks,
>> Ray
>>
>>> -----Original Message-----
>>> From: Justen, Jordan L <jordan.l.justen@intel.com>
>>> Sent: Monday, February 18, 2019 12:12 PM
>>> To: edk2-devel@lists.01.org
>>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
>>> <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
>>> Perard <anthony.perard@citrix.com>; Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
>>> J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
>>> <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
>>> <star.zeng@intel.com>
>>> Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>>>
>>> https://github.com/jljusten/edk2.git temp-ram-support
>>>
>>> This series fixes an issue that, while rare, is possible based on the way the
>>> TemporaryRamSupport PPI is defined along with how it is used by the PEI
>>> Core.
>>>
>>> Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
>>> this issue.
>>>
>>> The details of the issue are described in the commit message of the
>>> "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
>>> migration" patch.
>>>
>>> Along with this, I added a few Temporary RAM patches for EmulatorPkg and
>>> OvmfPkg.
>>>
>>> Cc: Liu Yu <pedroa.liu@outlook.com>
>>> Cc: Andrew Fish <afish@apple.com>
>>> Cc: Anthony Perard <anthony.perard@citrix.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Julien Grall <julien.grall@linaro.org>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>>
>>> Jordan Justen (10):
>>> EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
>>> EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
>>> EmulatorPkg/Sec: Replace assembly temp-ram support with C code
>>> EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
>>> function
>>> OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
>>> OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
>>> MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
>>> migration
>>> MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
>>> MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
>>> OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
>>>
>>> EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
>>> EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
>>> EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
>>> EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
>>> EmulatorPkg/Sec/Sec.inf | 13 +--
>>> EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
>>> EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
>>> EmulatorPkg/Unix/Host/Host.c | 2 +-
>>> EmulatorPkg/Unix/Host/Host.inf | 1 +
>>> EmulatorPkg/build.sh | 10 +-
>>> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
>>> .../Dispatcher/Ia32/TemporaryRamMigration.S | 72 ++++++++++++++
>>> .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
>>> .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
>>> .../Dispatcher/X64/TemporaryRamMigration.S | 69 ++++++++++++++
>>> .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
>>> MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
>>> MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
>>> OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
>>> OvmfPkg/Sec/SecMain.c | 59 ++++++++----
>>> OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
>>> 21 files changed, 577 insertions(+), 461 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 create mode 100644
>>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
>>> create mode 100644
>>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>>> create mode 100644
>>> MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
>>> create mode 100644
>>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
>>> create mode 100644
>>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>>>
>>> --
>>> 2.20.0.rc1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-20 13:27 ` Ni, Ray
@ 2019-02-20 17:43 ` Jordan Justen
2019-02-21 0:15 ` Ni, Ray
0 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-20 17:43 UTC (permalink / raw)
To: Gao, Liming, Ni, Ray, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star
On 2019-02-20 05:27:21, Ni, Ray wrote:
> On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > Ray:
> >
> > Now, real platform has no side effect. So, only TempRamDone
> > PPI is produced. For emulator platform, is there any side
> > effect when both Temporary RAM and Permanent RAM are enabled?
> >
>
> No side effect when both of T-RAM and P-RAM are enabled.
> Which means no side effect when neither of the PPIs is produced.
> But for demo purpose, I think producing TemporaryRamDone PPI makes sense.
In addition to being a demo/sample, it also provides a check that no
modules are accessing temp-ram after temp-ram should no longer be
used.
> I will work out patches for EmulatorPkg to produce TemoraryRamDone.
I think we should first fix TemporaryRamSupport usage. Otherwise, we
are just hiding the bug.
Have you tried these patches to verify that they fix the issue in your
setup? Have you taken a look at the patches to see what problem is
being fixed?
-Jordan
>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni, Ray
> >> Sent: Tuesday, February 19, 2019 10:46 AM
> >> To: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org
> >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> >> <star.zeng@intel.com>
> >> Subject: Re: [edk2] [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> >>
> >> Jordan,
> >> I find many real platforms do not implement the temporary ram migration
> >> PPI and rely on the PeiCore migration logic.
> >> So perhaps TemporaryRamMigration PPI was added to help platform to
> >> destroy the temporary RAM (CAR in x86 platform).
> >> But with the introduction of TemporaryRamDone PPI, maybe the
> >> TemporaryRamMigration PPI can be retired.
> >> The logic in PeiCore to call TemporaryRamMigration is just for backward
> >> compatibility.
> >> If that's true, do you still need to enhance PeiCore?
> >>
> >> For the Emulator case, I already found without TemporaryRamMigration
> >> the platform can still boot.
> >>
> >> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
> >> the PeiCore migration logic + TemporaryDone PPI?
> >>
> >> Thanks,
> >> Ray
> >>
> >>> -----Original Message-----
> >>> From: Justen, Jordan L <jordan.l.justen@intel.com>
> >>> Sent: Monday, February 18, 2019 12:12 PM
> >>> To: edk2-devel@lists.01.org
> >>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> >>> <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
> >>> Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> >>> <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
> >>> J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
> >>> <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> >>> <star.zeng@intel.com>
> >>> Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> >>>
> >>> https://github.com/jljusten/edk2.git temp-ram-support
> >>>
> >>> This series fixes an issue that, while rare, is possible based on the way the
> >>> TemporaryRamSupport PPI is defined along with how it is used by the PEI
> >>> Core.
> >>>
> >>> Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
> >>> this issue.
> >>>
> >>> The details of the issue are described in the commit message of the
> >>> "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> >>> migration" patch.
> >>>
> >>> Along with this, I added a few Temporary RAM patches for EmulatorPkg and
> >>> OvmfPkg.
> >>>
> >>> Cc: Liu Yu <pedroa.liu@outlook.com>
> >>> Cc: Andrew Fish <afish@apple.com>
> >>> Cc: Anthony Perard <anthony.perard@citrix.com>
> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Cc: Hao Wu <hao.a.wu@intel.com>
> >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>> Cc: Julien Grall <julien.grall@linaro.org>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Cc: Ray Ni <ray.ni@intel.com>
> >>> Cc: Star Zeng <star.zeng@intel.com>
> >>>
> >>> Jordan Justen (10):
> >>> EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
> >>> EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
> >>> EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> >>> EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> >>> function
> >>> OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> >>> OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> >>> MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> >>> migration
> >>> MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
> >>> MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
> >>> OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
> >>>
> >>> EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
> >>> EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
> >>> EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
> >>> EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
> >>> EmulatorPkg/Sec/Sec.inf | 13 +--
> >>> EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
> >>> EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
> >>> EmulatorPkg/Unix/Host/Host.c | 2 +-
> >>> EmulatorPkg/Unix/Host/Host.inf | 1 +
> >>> EmulatorPkg/build.sh | 10 +-
> >>> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> >>> .../Dispatcher/Ia32/TemporaryRamMigration.S | 72 ++++++++++++++
> >>> .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
> >>> .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
> >>> .../Dispatcher/X64/TemporaryRamMigration.S | 69 ++++++++++++++
> >>> .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
> >>> MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
> >>> MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
> >>> OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
> >>> OvmfPkg/Sec/SecMain.c | 59 ++++++++----
> >>> OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
> >>> 21 files changed, 577 insertions(+), 461 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 create mode 100644
> >>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> >>> create mode 100644
> >>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> >>> create mode 100644
> >>> MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> >>> create mode 100644
> >>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> >>> create mode 100644
> >>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> >>>
> >>> --
> >>> 2.20.0.rc1
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
>
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-20 17:43 ` Jordan Justen
@ 2019-02-21 0:15 ` Ni, Ray
2019-02-21 1:03 ` Jordan Justen
0 siblings, 1 reply; 29+ messages in thread
From: Ni, Ray @ 2019-02-21 0:15 UTC (permalink / raw)
To: Justen, Jordan L, Gao, Liming, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star
> -----Original Message-----
> From: Justen, Jordan L <jordan.l.justen@intel.com>
> Sent: Thursday, February 21, 2019 1:44 AM
> To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>
> On 2019-02-20 05:27:21, Ni, Ray wrote:
> > On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > > Ray:
> > >
> > > Now, real platform has no side effect. So, only TempRamDone
> > > PPI is produced. For emulator platform, is there any side
> > > effect when both Temporary RAM and Permanent RAM are enabled?
> > >
> >
> > No side effect when both of T-RAM and P-RAM are enabled.
> > Which means no side effect when neither of the PPIs is produced.
> > But for demo purpose, I think producing TemporaryRamDone PPI makes
> sense.
>
> In addition to being a demo/sample, it also provides a check that no modules
> are accessing temp-ram after temp-ram should no longer be used.
>
> > I will work out patches for EmulatorPkg to produce TemoraryRamDone.
>
> I think we should first fix TemporaryRamSupport usage. Otherwise, we are
> just hiding the bug.
>
> Have you tried these patches to verify that they fix the issue in your setup?
> Have you taken a look at the patches to see what problem is being fixed?
I feel the change to PeiCore is a bit complex and introduce additional deps (assembly).
Behavior of ARM and x86 becomes different. (The patch only fixes x86 issue.)
Given there is no real requirement on this, I prefer to not change PeiCore.
>
> -Jordan
>
> >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >> Of Ni, Ray
> > >> Sent: Tuesday, February 19, 2019 10:46 AM
> > >> To: Justen, Jordan L <jordan.l.justen@intel.com>;
> > >> edk2-devel@lists.01.org
> > >> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> > >> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>;
> > >> Zeng, Star <star.zeng@intel.com>
> > >> Subject: Re: [edk2] [PATCH 00/10] Fix PEI Core issue during
> > >> TemporaryRamMigration
> > >>
> > >> Jordan,
> > >> I find many real platforms do not implement the temporary ram
> > >> migration PPI and rely on the PeiCore migration logic.
> > >> So perhaps TemporaryRamMigration PPI was added to help platform to
> > >> destroy the temporary RAM (CAR in x86 platform).
> > >> But with the introduction of TemporaryRamDone PPI, maybe the
> > >> TemporaryRamMigration PPI can be retired.
> > >> The logic in PeiCore to call TemporaryRamMigration is just for
> > >> backward compatibility.
> > >> If that's true, do you still need to enhance PeiCore?
> > >>
> > >> For the Emulator case, I already found without
> > >> TemporaryRamMigration the platform can still boot.
> > >>
> > >> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply
> on
> > >> the PeiCore migration logic + TemporaryDone PPI?
> > >>
> > >> Thanks,
> > >> Ray
> > >>
> > >>> -----Original Message-----
> > >>> From: Justen, Jordan L <jordan.l.justen@intel.com>
> > >>> Sent: Monday, February 18, 2019 12:12 PM
> > >>> To: edk2-devel@lists.01.org
> > >>> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> > >>> <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>;
> Anthony
> > >>> Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> > >>> <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>;
> Wang,
> > >>> Jian J <jian.j.wang@intel.com>; Julien Grall
> > >>> <julien.grall@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Ni,
> > >>> Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > >>> Subject: [PATCH 00/10] Fix PEI Core issue during
> > >>> TemporaryRamMigration
> > >>>
> > >>> https://github.com/jljusten/edk2.git temp-ram-support
> > >>>
> > >>> This series fixes an issue that, while rare, is possible based on
> > >>> the way the TemporaryRamSupport PPI is defined along with how it
> > >>> is used by the PEI Core.
> > >>>
> > >>> Liu Yu reported a boot issue with EmulatorPkg, which I believe was
> > >>> caused by this issue.
> > >>>
> > >>> The details of the issue are described in the commit message of
> > >>> the
> > >>> "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > >>> migration" patch.
> > >>>
> > >>> Along with this, I added a few Temporary RAM patches for
> > >>> EmulatorPkg and OvmfPkg.
> > >>>
> > >>> Cc: Liu Yu <pedroa.liu@outlook.com>
> > >>> Cc: Andrew Fish <afish@apple.com>
> > >>> Cc: Anthony Perard <anthony.perard@citrix.com>
> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >>> Cc: Hao Wu <hao.a.wu@intel.com>
> > >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> > >>> Cc: Julien Grall <julien.grall@linaro.org>
> > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > >>> Cc: Ray Ni <ray.ni@intel.com>
> > >>> Cc: Star Zeng <star.zeng@intel.com>
> > >>>
> > >>> Jordan Justen (10):
> > >>> EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET
> parameter
> > >>> EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-
> ram
> > >>> EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> > >>> EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> > >>> function
> > >>> OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> > >>> OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> > >>> MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > >>> migration
> > >>> MdeModulePkg/Core/Pei: Use assembly for X64
> TemporaryRamMigration
> > >>> MdeModulePkg/Core/Pei: Use assembly for IA32
> TemporaryRamMigration
> > >>> OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
> > >>>
> > >>> EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
> > >>> EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
> > >>> EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
> > >>> EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
> > >>> EmulatorPkg/Sec/Sec.inf | 13 +--
> > >>> EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
> > >>> EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
> > >>> EmulatorPkg/Unix/Host/Host.c | 2 +-
> > >>> EmulatorPkg/Unix/Host/Host.inf | 1 +
> > >>> EmulatorPkg/build.sh | 10 +-
> > >>> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> > >>> .../Dispatcher/Ia32/TemporaryRamMigration.S | 72
> ++++++++++++++
> > >>> .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
> > >>> .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
> > >>> .../Dispatcher/X64/TemporaryRamMigration.S | 69
> ++++++++++++++
> > >>> .../Dispatcher/X64/TemporaryRamMigration.nasm | 75
> +++++++++++++++
> > >>> MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
> > >>> MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
> > >>> OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
> > >>> OvmfPkg/Sec/SecMain.c | 59 ++++++++----
> > >>> OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
> > >>> 21 files changed, 577 insertions(+), 461 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 create mode 100644
> > >>> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> > >>> create mode 100644
> > >>>
> MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> > >>> create mode 100644
> > >>> MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> > >>> create mode 100644
> > >>> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> > >>> create mode 100644
> > >>>
> MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> > >>>
> > >>> --
> > >>> 2.20.0.rc1
> > >>
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> >
> >
> > --
> > Thanks,
> > Ray
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-21 0:15 ` Ni, Ray
@ 2019-02-21 1:03 ` Jordan Justen
2019-02-21 4:43 ` Ni, Ray
0 siblings, 1 reply; 29+ messages in thread
From: Jordan Justen @ 2019-02-21 1:03 UTC (permalink / raw)
To: Gao, Liming, Ni, Ray, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star, Andrew Fish
On 2019-02-20 16:15:59, Ni, Ray wrote:
> > -----Original Message-----
> > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > Sent: Thursday, February 21, 2019 1:44 AM
> > To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> > devel@lists.01.org
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> > <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> > Star <star.zeng@intel.com>
> > Subject: Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> >
> > On 2019-02-20 05:27:21, Ni, Ray wrote:
> > > On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > > > Ray:
> > > >
> > > > Now, real platform has no side effect. So, only TempRamDone
> > > > PPI is produced. For emulator platform, is there any side
> > > > effect when both Temporary RAM and Permanent RAM are enabled?
> > > >
> > >
> > > No side effect when both of T-RAM and P-RAM are enabled.
> > > Which means no side effect when neither of the PPIs is produced.
> > > But for demo purpose, I think producing TemporaryRamDone PPI makes
> > sense.
> >
> > In addition to being a demo/sample, it also provides a check that no modules
> > are accessing temp-ram after temp-ram should no longer be used.
> >
> > > I will work out patches for EmulatorPkg to produce TemoraryRamDone.
> >
> > I think we should first fix TemporaryRamSupport usage. Otherwise, we are
> > just hiding the bug.
> >
> > Have you tried these patches to verify that they fix the issue in your setup?
> > Have you taken a look at the patches to see what problem is being fixed?
>
> I feel the change to PeiCore is a bit complex and introduce additional deps (assembly).
I asked about this last November, and as I recall, Andrew said we can
(and should) add assembly to PEI Core if it is required to meet the
specs.
> Behavior of ARM and x86 becomes different. (The patch only fixes x86 issue.)
Yes. Similar code should be written for ARM, but I don't have
experience with ARM assembly code.
> Given there is no real requirement on this,
Isn't the requirement to be compatible with the PI specification?
It seems that at least you and Liu Yu have encountered a build
environment that produces code for PEI Core that isn't compatible with
the PI specification.
It doesn't seem like the best response to this is to just change the
platform boot path and ignore the bug.
I do agree that after this issue is fixed, we can then consider
changing the platform. The downside to changing it then will be to
leave an untested code path, but at least we won't leave it with a
known bug present.
-Jordan
> I prefer to not change PeiCore.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
2019-02-21 1:03 ` Jordan Justen
@ 2019-02-21 4:43 ` Ni, Ray
0 siblings, 0 replies; 29+ messages in thread
From: Ni, Ray @ 2019-02-21 4:43 UTC (permalink / raw)
To: Justen, Jordan L, Gao, Liming, edk2-devel@lists.01.org
Cc: Wu, Hao A, Anthony Perard, Laszlo Ersek, Zeng, Star, Andrew Fish
> -----Original Message-----
> From: Justen, Jordan L <jordan.l.justen@intel.com>
> Sent: Thursday, February 21, 2019 9:03 AM
> To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star <star.zeng@intel.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
>
> On 2019-02-20 16:15:59, Ni, Ray wrote:
> > > -----Original Message-----
> > > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > > Sent: Thursday, February 21, 2019 1:44 AM
> > > To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > edk2- devel@lists.01.org
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> > > <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> > > Star <star.zeng@intel.com>
> > > Subject: Re: [PATCH 00/10] Fix PEI Core issue during
> > > TemporaryRamMigration
> > >
> > > On 2019-02-20 05:27:21, Ni, Ray wrote:
> > > > On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > > > > Ray:
> > > > >
> > > > > Now, real platform has no side effect. So, only TempRamDone
> > > > > PPI is produced. For emulator platform, is there any side
> > > > > effect when both Temporary RAM and Permanent RAM are
> enabled?
> > > > >
> > > >
> > > > No side effect when both of T-RAM and P-RAM are enabled.
> > > > Which means no side effect when neither of the PPIs is produced.
> > > > But for demo purpose, I think producing TemporaryRamDone PPI
> makes
> > > sense.
> > >
> > > In addition to being a demo/sample, it also provides a check that no
> > > modules are accessing temp-ram after temp-ram should no longer be
> used.
> > >
> > > > I will work out patches for EmulatorPkg to produce TemoraryRamDone.
> > >
> > > I think we should first fix TemporaryRamSupport usage. Otherwise, we
> > > are just hiding the bug.
> > >
> > > Have you tried these patches to verify that they fix the issue in your
> setup?
> > > Have you taken a look at the patches to see what problem is being fixed?
> >
> > I feel the change to PeiCore is a bit complex and introduce additional deps
> (assembly).
>
> I asked about this last November, and as I recall, Andrew said we can (and
> should) add assembly to PEI Core if it is required to meet the specs.
>
> > Behavior of ARM and x86 becomes different. (The patch only fixes x86
> > issue.)
>
> Yes. Similar code should be written for ARM, but I don't have experience
> with ARM assembly code.
>
> > Given there is no real requirement on this,
>
> Isn't the requirement to be compatible with the PI specification?
>
> It seems that at least you and Liu Yu have encountered a build environment
> that produces code for PEI Core that isn't compatible with the PI specification.
>
> It doesn't seem like the best response to this is to just change the platform
> boot path and ignore the bug.
The environment Liu Yu and I encountered is the same Emulator environment.
I think the code change you did to PeiCore is great. It re-wrote the C code in
assembly to make sure the implementation doesn't rely on C compiler's
behavior.
But looking back, if a PI spec interface should depend on assembly
Implementation, I will pursue to change the PI spec interface directly.
Given the fact that no platform is producing this RamMigration PPI,
changing PI spec then changing the PeiCore to use new PPI interface
has no impact.
To be honest, I don't want to introduce complexity to PeiCore or edk2. That's
my only concern.
>
> I do agree that after this issue is fixed, we can then consider changing the
> platform. The downside to changing it then will be to leave an untested code
> path, but at least we won't leave it with a known bug present.
>
> -Jordan
>
> > I prefer to not change PeiCore.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
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-19 19:27 ` Jordan Justen
1 sibling, 0 replies; 29+ messages in thread
From: Jordan Justen @ 2019-02-19 19:27 UTC (permalink / raw)
To: Ni, Ray, edk2-devel@lists.01.org
Cc: Liu Yu, Andrew Fish, Anthony Perard, Ard Biesheuvel, Wu, Hao A,
Wang, Jian J, Julien Grall, Laszlo Ersek, Zeng, Star, Gao, Liming
On 2019-02-18 18:46:24, Ni, Ray wrote:
> Jordan,
> I find many real platforms do not implement the temporary ram migration
> PPI and rely on the PeiCore migration logic.
> So perhaps TemporaryRamMigration PPI was added to help platform to
> destroy the temporary RAM (CAR in x86 platform).
I guess since it is in the PI spec, we can't be sure it is only used
for this case, or that it might not be used in the future.
> But with the introduction of TemporaryRamDone PPI, maybe the
> TemporaryRamMigration PPI can be retired.
> The logic in PeiCore to call TemporaryRamMigration is just for backward
> compatibility.
> If that's true, do you still need to enhance PeiCore?
I checked the PI 1.4 spec, and I didn't see anything indicating that
TemporaryRamSupport PPI is deprecated.
Since it is not deprecated, should we ignore the known issue?
> For the Emulator case, I already found without TemporaryRamMigration
> the platform can still boot.
>
> Does OVMF hard-depend on TemporaryRamMigration? Or it can reply on
> the PeiCore migration logic + TemporaryDone PPI?
This is a good question. If it is true that TemporaryRamSupport is
rarely used, then maybe it is better to have the sample platforms use
the more commonly used path.
Personally, I think we should still address the issue with
TemporaryRamSupport, and leave the question of whether to test
TemporaryRamSupport code paths in the sample platforms as a separate
task.
At the least, I think we should still continue to use TemporaryRamDone
to reset the temp-ram contents to help make sure nothing accidentally
depends on a temp-ram pointer. Unfortunately, this would mean that the
TemporaryRamSupport path is not really being tested, but it might be
the better choise if TemporaryRamSupport is never used in real
platforms.
-Jordan
>
> > -----Original Message-----
> > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > Sent: Monday, February 18, 2019 12:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Liu Yu
> > <pedroa.liu@outlook.com>; Andrew Fish <afish@apple.com>; Anthony
> > Perard <anthony.perard@citrix.com>; Ard Biesheuvel
> > <ard.biesheuvel@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian
> > J <jian.j.wang@intel.com>; Julien Grall <julien.grall@linaro.org>; Laszlo Ersek
> > <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> >
> > https://github.com/jljusten/edk2.git temp-ram-support
> >
> > This series fixes an issue that, while rare, is possible based on the way the
> > TemporaryRamSupport PPI is defined along with how it is used by the PEI
> > Core.
> >
> > Liu Yu reported a boot issue with EmulatorPkg, which I believe was caused by
> > this issue.
> >
> > The details of the issue are described in the commit message of the
> > "MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > migration" patch.
> >
> > Along with this, I added a few Temporary RAM patches for EmulatorPkg and
> > OvmfPkg.
> >
> > Cc: Liu Yu <pedroa.liu@outlook.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Julien Grall <julien.grall@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> >
> > Jordan Justen (10):
> > EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter
> > EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram
> > EmulatorPkg/Sec: Replace assembly temp-ram support with C code
> > EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration
> > function
> > OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations
> > OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration
> > MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram
> > migration
> > MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration
> > MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration
> > OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration
> >
> > EmulatorPkg/Sec/Ia32/SwitchRam.S | 95 -------------------
> > EmulatorPkg/Sec/Ia32/SwitchRam.asm | 94 ------------------
> > EmulatorPkg/Sec/Ia32/TempRam.c | 65 -------------
> > EmulatorPkg/Sec/Sec.c | 76 ++++++++++++++-
> > EmulatorPkg/Sec/Sec.inf | 13 +--
> > EmulatorPkg/Sec/X64/SwitchRam.S | 72 --------------
> > EmulatorPkg/Sec/X64/SwitchRam.asm | 76 ---------------
> > EmulatorPkg/Unix/Host/Host.c | 2 +-
> > EmulatorPkg/Unix/Host/Host.inf | 1 +
> > EmulatorPkg/build.sh | 10 +-
> > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 ++++++++----
> > .../Dispatcher/Ia32/TemporaryRamMigration.S | 72 ++++++++++++++
> > .../Ia32/TemporaryRamMigration.nasm | 78 +++++++++++++++
> > .../Pei/Dispatcher/TemporaryRamMigration.c | 52 ++++++++++
> > .../Dispatcher/X64/TemporaryRamMigration.S | 69 ++++++++++++++
> > .../Dispatcher/X64/TemporaryRamMigration.nasm | 75 +++++++++++++++
> > MdeModulePkg/Core/Pei/PeiMain.h | 52 ++++++++++
> > MdeModulePkg/Core/Pei/PeiMain.inf | 14 +++
> > OvmfPkg/Sec/Ia32/SecEntry.nasm | 2 +-
> > OvmfPkg/Sec/SecMain.c | 59 ++++++++----
> > OvmfPkg/Sec/X64/SecEntry.nasm | 2 +-
> > 21 files changed, 577 insertions(+), 461 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 create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.S
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/TemporaryRamMigration.c
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.S
> > create mode 100644
> > MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> >
> > --
> > 2.20.0.rc1
>
^ permalink raw reply [flat|nested] 29+ messages in thread