[PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

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

[PATCH 01/10] EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter

[Jordan Justen]

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

[PATCH 02/10] EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram

[Jordan Justen]

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

[PATCH 03/10] EmulatorPkg/Sec: Replace assembly temp-ram support with C code

[Jordan Justen]

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

[PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function

[Jordan Justen]

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

[PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations

[Jordan Justen]

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

[PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Jordan Justen]

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

[PATCH 07/10] MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram migration

[Jordan Justen]

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

[PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration

[Jordan Justen]

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

[PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration

[Jordan Justen]

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

[PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration

[Jordan Justen]

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

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Ard Biesheuvel]

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?

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Jordan Justen]

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?

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Ard Biesheuvel]

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?

Re: [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations

[Laszlo Ersek]

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
>  
> 

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Laszlo Ersek]

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?

Re: [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration

[Laszlo Ersek]

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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Ni, Ray]

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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Gao, Liming]

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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

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
> 

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Brian J. Johnson]

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

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Jordan Justen]

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

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Jordan Justen]

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?

Re: [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration

[Ard Biesheuvel]

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.

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Ni, Ray]

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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Ni, Ray]

> -----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

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

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.

Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration

[Ni, Ray]

> -----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.