[PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

https://github.com/jljusten/edk2.git temp-ram-support-v2

https://github.com/jljusten/edk2/commits/temp-ram-support-v2

v2:
 * Add AARCH64 and ARM assembly
 * Drop IA32 and X64 .S source files
 * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
   code based on the stack pointer change before & after
   TemporaryRamSupport->TemporaryRamMigration
 * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
   just complicating the series.

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
"MdeModulePkg/Core/Pei: Add interface for assembly based
TemporaryRamSupport" patch.

Testing:

I tested building and booting in several scenarios:

* OVMF IA32 & X64 on Linux
* ArmVirtPkg AARCH64 & ARM on x86_64 Linux
* EmulatorPkg IA32 & X64 on Linux

Untested:

* My system does not reproduce the issue that Liu Yu reported with
  EmulatorPkg, so I can't say that I have verified that issue.
* Building on windows
* AARCH64/ARM TemporaryRamMigration.asm sources

Cc: Liu Yu <pedroa.liu@outlook.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Jordan Justen (6):
  MdeModulePkg/Core/Pei: Add interface for assembly based
    TemporaryRamSupport
  MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
  MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
  MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
  MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
  MdeModulePkg/Core/Pei: Use code path for assembly based
    TemporaryRamSupport

 .../AArch64/TemporaryRamMigration.S           | 63 +++++++++++++++
 .../AArch64/TemporaryRamMigration.asm         | 63 +++++++++++++++
 .../Dispatcher/Arm/TemporaryRamMigration.S    | 68 ++++++++++++++++
 .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 ++++++++++++++++
 MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +++++++++-----
 .../Ia32/TemporaryRamMigration.nasm           | 77 +++++++++++++++++++
 .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.h               | 52 +++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.inf             | 15 ++++
 9 files changed, 518 insertions(+), 21 deletions(-)
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm

-- 
2.20.1

[PATCH v2 1/6] MdeModulePkg/Core/Pei: Add interface for assembly based TemporaryRamSupport

[Jordan Justen]

There is potential problem with PEI Core's usage of the
TemporaryRamSupport 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 patch add the interface that the assembly function will need. The
PEI_CORE_TEMPORARY_RAM_TRANSITION contains all the data that the
assembly code will need to call the
TemporaryRamSupport->TemporaryRamMigration function, and then the
context that PEI will need after this call when the new C based
PeiTemporaryRamMigrated function is called.

After all assembly code based implementations have been added, PEI
Core will be updated to use the new assembly based code path.

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/PeiMain.h | 52 +++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h
index 0aed4f4685..6522798059 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.h
+++ b/MdeModulePkg/Core/Pei/PeiMain.h
@@ -1865,4 +1865,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
-- 
2.20.1

[PATCH v2 2/6] MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration

[Jordan Justen]

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../AArch64/TemporaryRamMigration.S           | 63 +++++++++++++++++++
 .../AArch64/TemporaryRamMigration.asm         | 63 +++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.inf             |  4 ++
 3 files changed, 130 insertions(+)
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S b/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
new file mode 100644
index 0000000000..e73932daff
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
@@ -0,0 +1,63 @@
+#------------------------------------------------------------------------------
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#------------------------------------------------------------------------------
+
+.text
+.align 5
+
+GCC_ASM_EXPORT(PeiTemporaryRamMigration)
+
+#------------------------------------------------------------------------------
+# VOID
+# EFIAPI
+# PeiTemporaryRamMigration (
+#   IN  PEI_CORE_TEMPORARY_RAM_TRANSITION  *TempRamTransitionData
+#   );
+#
+# @param[in]  x0    Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+#
+# @return     None  This routine does not return
+#------------------------------------------------------------------------------
+ASM_PFX(PeiTemporaryRamMigration):
+
+    #
+    # We store the TempRamTransitionData pointer in x19. By the
+    # AArch64 calling convention we should normally save x19, 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 x19 for us.
+    #
+    mov     x19, x0
+
+    #
+    # Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+    #   (rcx) PeiServices
+    #   (rdx) TemporaryMemoryBase
+    #   (r8)  PermanentMemoryBase
+    #   (r9)  CopySize
+    #
+    ldp     x0, x1, [x19, 0x08]
+    ldp     x2, x3, [x19, 0x18]
+
+    #
+    # (x19) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    #
+    # Adjusted based on stack change made during
+    # TemporaryRamSupport->TemporaryRamMigration call
+    #
+    ldr     x4, [x19]
+    mov     x5, sp
+    sub     x19, x19, x5
+    blr     x4
+    mov     x5, sp
+    add     x19, x19, x5
+
+    #
+    # Setup parameters and call PeiTemporaryRamMigrated
+    #   (x0) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+    #
+    mov     x0, x19
+    bl      ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm b/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
new file mode 100644
index 0000000000..fbffd16c52
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
@@ -0,0 +1,63 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+  EXPORT PeiTemporaryRamMigration
+  AREA PeiCore_LowLevel, CODE, READONLY
+
+;------------------------------------------------------------------------------
+; VOID
+; EFIAPI
+; PeiTemporaryRamMigration (
+;   IN  PEI_CORE_TEMPORARY_RAM_TRANSITION  *TempRamTransitionData
+;   );
+;
+; @param[in]  x0    Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+;
+; @return     None  This routine does not return
+;------------------------------------------------------------------------------
+PeiTemporaryRamMigration
+
+    ;
+    ; We store the TempRamTransitionData pointer in x19. By the
+    ; AArch64 calling convention we should normally save x19, 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 x19 for us.
+    ;
+    mov     x19, x0
+
+    ;
+    ; Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+    ;   (rcx) PeiServices
+    ;   (rdx) TemporaryMemoryBase
+    ;   (r8)  PermanentMemoryBase
+    ;   (r9)  CopySize
+    ;
+    ldp     x0, x1, [x19, 0x08]
+    ldp     x2, x3, [x19, 0x18]
+
+    ;
+    ; (x19) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    ;
+    ; Adjusted based on stack change made during
+    ; TemporaryRamSupport->TemporaryRamMigration call
+    ;
+    ldr     x4, [x19]
+    mov     x5, sp
+    sub     x19, x19, x5
+    blr     x4
+    mov     x5, sp
+    add     x19, x19, x5
+
+    ;
+    ; Setup parameters and call PeiTemporaryRamMigrated
+    ;   (x0) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+    ;
+    mov     x0, x19
+    bl      ASM_PFX(PeiTemporaryRamMigrated)
+
+  END
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index c8a6d5b80b..95ef7bb006 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -47,6 +47,10 @@
   PciCfg2/PciCfg2.c
   PeiMain.h
 
+[Sources.AARCH64]
+  Dispatcher/AArch64/TemporaryRamMigration.asm | MSFT
+  Dispatcher/AArch64/TemporaryRamMigration.S   | GCC
+
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
-- 
2.20.1

[PATCH v2 3/6] MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration

[Jordan Justen]

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 .../Dispatcher/Arm/TemporaryRamMigration.S    | 68 +++++++++++++++++++
 .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 +++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.inf             |  5 ++
 3 files changed, 141 insertions(+)
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S b/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
new file mode 100644
index 0000000000..872bbcf27d
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
@@ -0,0 +1,68 @@
+#------------------------------------------------------------------------------
+#
+# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#------------------------------------------------------------------------------
+
+.text
+.align 5
+
+GCC_ASM_EXPORT(PeiTemporaryRamMigration)
+
+#------------------------------------------------------------------------------
+# VOID
+# EFIAPI
+# PeiTemporaryRamMigration (
+#   IN  PEI_CORE_TEMPORARY_RAM_TRANSITION  *TempRamTransitionData
+#   );
+#
+# @param[in]  r0    Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+#
+# @return     None  This routine does not return
+#------------------------------------------------------------------------------
+ASM_PFX(PeiTemporaryRamMigration):
+
+    #
+    # We store the TempRamTransitionData pointer in r6. By the
+    # Arm calling convention we should normally save r6, 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 r6 for us.
+    #
+    mov     r6, r0
+
+    #
+    # Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+    #   (r0)      PeiServices
+    #   (r2,r3)   TemporaryMemoryBase
+    #   (stack)   PermanentMemoryBase
+    #   (stack)   CopySize
+    #
+    add     r7, r6, #4
+    ldmia   r7, {r0-r5}
+    stmfd   sp!, {r3, r4, r5}
+    mov     r3, r2
+    mov     r2, r1
+
+    #
+    # (r6) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    #
+    # Adjusted based on stack change made during
+    # TemporaryRamSupport->TemporaryRamMigration call
+    #
+    ldr     r4, [r6]
+    mov     r5, sp
+    sub     r6, r6, r5
+    mov     lr, pc
+    bx      r4
+    mov     r0, sp
+    add     r6, r6, r0
+    add     sp, sp, #0xc
+
+    #
+    # Setup parameters and call PeiTemporaryRamMigrated
+    #   (r0) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+    #
+    mov     r0, r6
+    bl      ASM_PFX(PeiTemporaryRamMigrated)
diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm b/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
new file mode 100644
index 0000000000..1cfb0e0dd8
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
@@ -0,0 +1,68 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+  EXPORT PeiTemporaryRamMigration
+  AREA PeiCore_LowLevel, CODE, READONLY
+
+;------------------------------------------------------------------------------
+; VOID
+; EFIAPI
+; PeiTemporaryRamMigration (
+;   IN  PEI_CORE_TEMPORARY_RAM_TRANSITION  *TempRamTransitionData
+;   );
+;
+; @param[in]  r0    Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+;
+; @return     None  This routine does not return
+;------------------------------------------------------------------------------
+PeiTemporaryRamMigration
+
+    ;
+    ; We store the TempRamTransitionData pointer in r6. By the
+    ; Arm calling convention we should normally save r6, 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 r6 for us.
+    ;
+    mov     r6, r0
+
+    ;
+    ; Setup parameters and call TemporaryRamSupport->TemporaryRamMigration
+    ;   (r0)      PeiServices
+    ;   (r2,r3)   TemporaryMemoryBase
+    ;   (stack)   PermanentMemoryBase
+    ;   (stack)   CopySize
+    ;
+    add     r7, r6, #4
+    ldmia   r7, {r0-r5}
+    stmfd   sp!, {r3, r4, r5}
+    mov     r3, r2
+    mov     r2, r1
+
+    ;
+    ; (r6) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    ;
+    ; Adjusted based on stack change made during
+    ; TemporaryRamSupport->TemporaryRamMigration call
+    ;
+    ldr     r4, [r6]
+    mov     r5, sp
+    sub     r6, r6, r5
+    mov     lr, pc
+    bx      r4
+    mov     r0, sp
+    add     r6, r6, r0
+    add     sp, sp, #0xc
+
+    ;
+    ; Setup parameters and call PeiTemporaryRamMigrated
+    ;   (r0) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION
+    ;
+    mov     r0, r6
+    bl      ASM_PFX(PeiTemporaryRamMigrated)
+
+  END
diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf
index 95ef7bb006..1be898bb91 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -51,6 +51,11 @@
   Dispatcher/AArch64/TemporaryRamMigration.asm | MSFT
   Dispatcher/AArch64/TemporaryRamMigration.S   | GCC
 
+[Sources.ARM]
+  Dispatcher/Arm/TemporaryRamMigration.asm | RVCT
+  Dispatcher/Arm/TemporaryRamMigration.asm | MSFT
+  Dispatcher/Arm/TemporaryRamMigration.S   | GCC
+
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
-- 
2.20.1

[PATCH v2 4/6] MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration

[Jordan Justen]

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>
---
 .../Ia32/TemporaryRamMigration.nasm           | 77 +++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.inf             |  3 +
 2 files changed, 80 insertions(+)
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
new file mode 100644
index 0000000000..8014e3cc95
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
@@ -0,0 +1,77 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+#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 store the TempRamTransitionData pointer 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]
+
+    ;
+    ; 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
+
+    ;
+    ; (ebx) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    ;
+    ; Adjusted based on stack change made during
+    ; TemporaryRamSupport->TemporaryRamMigration call
+    ;
+    sub     ebx, esp
+    call    DWORD [eax + 0x00]
+    add     ebx, esp
+    add     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 1be898bb91..918c4a0df8 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -56,6 +56,9 @@
   Dispatcher/Arm/TemporaryRamMigration.asm | MSFT
   Dispatcher/Arm/TemporaryRamMigration.S   | GCC
 
+[Sources.Ia32]
+  Dispatcher/Ia32/TemporaryRamMigration.nasm
+
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
-- 
2.20.1

[PATCH v2 5/6] MdeModulePkg/Core/Pei: Add X64 assembly for 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.

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.nasm | 74 +++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.inf             |  3 +
 2 files changed, 77 insertions(+)
 create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
new file mode 100644
index 0000000000..5de8acdf34
--- /dev/null
+++ b/MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
@@ -0,0 +1,74 @@
+;------------------------------------------------------------------------------
+;
+; Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+#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 store the TempRamTransitionData pointer 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
+
+    ;
+    ; 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]
+
+    ;
+    ; (rbx) Pointer to PEI_CORE_TEMPORARY_RAM_TRANSITION on stack
+    ;
+    ; Adjusted based on stack change made during
+    ; TemporaryRamSupport->TemporaryRamMigration call
+    ;
+    sub     rbx, rsp
+    call    [rax + 0x00]
+    add     rbx, rsp
+
+    ;
+    ; 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 918c4a0df8..fb643a9dd5 100644
--- a/MdeModulePkg/Core/Pei/PeiMain.inf
+++ b/MdeModulePkg/Core/Pei/PeiMain.inf
@@ -59,6 +59,9 @@
 [Sources.Ia32]
   Dispatcher/Ia32/TemporaryRamMigration.nasm
 
+[Sources.X64]
+  Dispatcher/X64/TemporaryRamMigration.nasm
+
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
-- 
2.20.1

[PATCH v2 6/6] MdeModulePkg/Core/Pei: Use code path for assembly based TemporaryRamSupport

[Jordan Justen]

There is potential problem with PEI Core's usage of the
TemporaryRamSupport PPI. The issue is described in the previous patch:

"MdeModulePkg/Core/Pei: Add interface for assembly based TemporaryRamSupport"

Now that assembly paths are available for all supported architectures,
we can make use of the assembly based PEI Core code path.

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 ++++++++++++-------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
index ba2fd0cae1..116124d6f8 100644
--- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
@@ -682,6 +682,7 @@ PeiCheckAndSwitchStack (
   EFI_PHYSICAL_ADDRESS                  TempBase2;
   UINTN                                 TempSize2;
   UINTN                                 Index;
+  PEI_CORE_TEMPORARY_RAM_TRANSITION     TempRamTransitionData;
 
   PeiServices = (CONST EFI_PEI_SERVICES **) &Private->Ps;
 
@@ -816,30 +817,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.
@@ -952,6 +943,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.
 
-- 
2.20.1

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Ard Biesheuvel]

On Wed, 10 Apr 2019 at 01:41, Jordan Justen <jordan.l.justen@intel.com> wrote:
>
> https://github.com/jljusten/edk2.git temp-ram-support-v2
>
> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
>
> v2:
>  * Add AARCH64 and ARM assembly

Hi Jordan,

I'm not sure I'm following the reasoning behind this. Does this fix an
issue we currently have on ARM systems? And how did you build and/or
test OVMF for ARM?


>  * Drop IA32 and X64 .S source files
>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>    code based on the stack pointer change before & after
>    TemporaryRamSupport->TemporaryRamMigration
>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>    just complicating the series.
>
> 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
> "MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport" patch.
>
> Testing:
>
> I tested building and booting in several scenarios:
>
> * OVMF IA32 & X64 on Linux
> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
> * EmulatorPkg IA32 & X64 on Linux
>
> Untested:
>
> * My system does not reproduce the issue that Liu Yu reported with
>   EmulatorPkg, so I can't say that I have verified that issue.
> * Building on windows
> * AARCH64/ARM TemporaryRamMigration.asm sources
>
> Cc: Liu Yu <pedroa.liu@outlook.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>
> Jordan Justen (6):
>   MdeModulePkg/Core/Pei: Add interface for assembly based
>     TemporaryRamSupport
>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Use code path for assembly based
>     TemporaryRamSupport
>
>  .../AArch64/TemporaryRamMigration.S           | 63 +++++++++++++++
>  .../AArch64/TemporaryRamMigration.asm         | 63 +++++++++++++++
>  .../Dispatcher/Arm/TemporaryRamMigration.S    | 68 ++++++++++++++++
>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 ++++++++++++++++
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +++++++++-----
>  .../Ia32/TemporaryRamMigration.nasm           | 77 +++++++++++++++++++
>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++++++++++++++++++
>  MdeModulePkg/Core/Pei/PeiMain.h               | 52 +++++++++++++
>  MdeModulePkg/Core/Pei/PeiMain.inf             | 15 ++++
>  9 files changed, 518 insertions(+), 21 deletions(-)
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>
> --
> 2.20.1
>
>
> 
>

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Laszlo Ersek]

On 04/10/19 10:39, Jordan Justen wrote:
> https://github.com/jljusten/edk2.git temp-ram-support-v2
> 
> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> 
> v2:
>  * Add AARCH64 and ARM assembly
>  * Drop IA32 and X64 .S source files
>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>    code based on the stack pointer change before & after
>    TemporaryRamSupport->TemporaryRamMigration
>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>    just complicating the series.
> 
> 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
> "MdeModulePkg/Core/Pei: Add interface for assembly based
> TemporaryRamSupport" patch.
> 
> Testing:
> 
> I tested building and booting in several scenarios:
> 
> * OVMF IA32 & X64 on Linux

Performed my usual Linux checks on these, as described in
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>.

For building, I used the GCC48 toolchain.

> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux

Performed a normal boot test, on AARCH64 KVM.

For building, I used the GCC5 toolchain (using an oldie 6.1.1
cross-compiler from x86_64).

For the series,

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>


If a nontrivial update is needed for the series, I don't mind retesting,
but only as the last action before pushing. Testing is time consuming
and now I'm asking myself if I should have waited for some feedback to
arrive first. :)

Thanks
Laszlo

> * EmulatorPkg IA32 & X64 on Linux
> 
> Untested:
> 
> * My system does not reproduce the issue that Liu Yu reported with
>   EmulatorPkg, so I can't say that I have verified that issue.
> * Building on windows
> * AARCH64/ARM TemporaryRamMigration.asm sources
> 
> Cc: Liu Yu <pedroa.liu@outlook.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Jordan Justen (6):
>   MdeModulePkg/Core/Pei: Add interface for assembly based
>     TemporaryRamSupport
>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>   MdeModulePkg/Core/Pei: Use code path for assembly based
>     TemporaryRamSupport
> 
>  .../AArch64/TemporaryRamMigration.S           | 63 +++++++++++++++
>  .../AArch64/TemporaryRamMigration.asm         | 63 +++++++++++++++
>  .../Dispatcher/Arm/TemporaryRamMigration.S    | 68 ++++++++++++++++
>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 ++++++++++++++++
>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +++++++++-----
>  .../Ia32/TemporaryRamMigration.nasm           | 77 +++++++++++++++++++
>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++++++++++++++++++
>  MdeModulePkg/Core/Pei/PeiMain.h               | 52 +++++++++++++
>  MdeModulePkg/Core/Pei/PeiMain.inf             | 15 ++++
>  9 files changed, 518 insertions(+), 21 deletions(-)
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
> 

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Laszlo Ersek]

On 04/10/19 18:41, Ard Biesheuvel wrote:
> On Wed, 10 Apr 2019 at 01:41, Jordan Justen <jordan.l.justen@intel.com> wrote:
>>
>> https://github.com/jljusten/edk2.git temp-ram-support-v2
>>
>> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
>>
>> v2:
>>  * Add AARCH64 and ARM assembly
> 
> Hi Jordan,
> 
> I'm not sure I'm following the reasoning behind this. Does this fix an
> issue we currently have on ARM systems?

IIUC, the PEI Core can only be updated to use the "safe path" (= the
assembly path) on IA32/X64 if that path (= the assembly path) *exists*
regardless of architecture.

This is anyway my understanding of the last commit message in the series.

I can't evaluate whether the problem statement, in the first commit
message in the series, would ever turn into an actual issue on
ARM/AARCH64, dependent on toolchain. On IA32/X64, we've seen examples
(although none of those have bit me personally), and AIUI the issue
could theoretically apply to all arches (again, dependent on toolchain).

(Apologies if I've only increased the confusion with this -- but at
least it could help me improve my own understanding.)

> And how did you build and/or test OVMF for ARM?

Hmmm, I'm unsure where this question / implication comes from. AIUI, the
new ARM/AARCH64 assembly is automatically put to use if you run the PEI
Core -- as part of a firmware platform that uses temp RAM migration --
on ARM/AARCH64.

Thanks,
Laszlo

> 
> 
>>  * Drop IA32 and X64 .S source files
>>  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
>>    code based on the stack pointer change before & after
>>    TemporaryRamSupport->TemporaryRamMigration
>>  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
>>    just complicating the series.
>>
>> 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
>> "MdeModulePkg/Core/Pei: Add interface for assembly based
>> TemporaryRamSupport" patch.
>>
>> Testing:
>>
>> I tested building and booting in several scenarios:
>>
>> * OVMF IA32 & X64 on Linux
>> * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
>> * EmulatorPkg IA32 & X64 on Linux
>>
>> Untested:
>>
>> * My system does not reproduce the issue that Liu Yu reported with
>>   EmulatorPkg, so I can't say that I have verified that issue.
>> * Building on windows
>> * AARCH64/ARM TemporaryRamMigration.asm sources
>>
>> Cc: Liu Yu <pedroa.liu@outlook.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>
>> Jordan Justen (6):
>>   MdeModulePkg/Core/Pei: Add interface for assembly based
>>     TemporaryRamSupport
>>   MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
>>   MdeModulePkg/Core/Pei: Use code path for assembly based
>>     TemporaryRamSupport
>>
>>  .../AArch64/TemporaryRamMigration.S           | 63 +++++++++++++++
>>  .../AArch64/TemporaryRamMigration.asm         | 63 +++++++++++++++
>>  .../Dispatcher/Arm/TemporaryRamMigration.S    | 68 ++++++++++++++++
>>  .../Dispatcher/Arm/TemporaryRamMigration.asm  | 68 ++++++++++++++++
>>  MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 59 +++++++++-----
>>  .../Ia32/TemporaryRamMigration.nasm           | 77 +++++++++++++++++++
>>  .../Dispatcher/X64/TemporaryRamMigration.nasm | 74 ++++++++++++++++++
>>  MdeModulePkg/Core/Pei/PeiMain.h               | 52 +++++++++++++
>>  MdeModulePkg/Core/Pei/PeiMain.inf             | 15 ++++
>>  9 files changed, 518 insertions(+), 21 deletions(-)
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.S
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/AArch64/TemporaryRamMigration.asm
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.S
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Arm/TemporaryRamMigration.asm
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/Ia32/TemporaryRamMigration.nasm
>>  create mode 100644 MdeModulePkg/Core/Pei/Dispatcher/X64/TemporaryRamMigration.nasm
>>
>> --
>> 2.20.1
>>
>>
>> 
>>

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Ard Biesheuvel]

On Wed, 10 Apr 2019 at 11:28, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 04/10/19 18:41, Ard Biesheuvel wrote:
> > On Wed, 10 Apr 2019 at 01:41, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >>
> >> https://github.com/jljusten/edk2.git temp-ram-support-v2
> >>
> >> https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> >>
> >> v2:
> >>  * Add AARCH64 and ARM assembly
> >
> > Hi Jordan,
> >
> > I'm not sure I'm following the reasoning behind this. Does this fix an
> > issue we currently have on ARM systems?
>
> IIUC, the PEI Core can only be updated to use the "safe path" (= the
> assembly path) on IA32/X64 if that path (= the assembly path) *exists*
> regardless of architecture.
>
> This is anyway my understanding of the last commit message in the series.
>
> I can't evaluate whether the problem statement, in the first commit
> message in the series, would ever turn into an actual issue on
> ARM/AARCH64, dependent on toolchain. On IA32/X64, we've seen examples
> (although none of those have bit me personally), and AIUI the issue
> could theoretically apply to all arches (again, dependent on toolchain).
>
> (Apologies if I've only increased the confusion with this -- but at
> least it could help me improve my own understanding.)
>
> > And how did you build and/or test OVMF for ARM?
>
> Hmmm, I'm unsure where this question / implication comes from. AIUI, the
> new ARM/AARCH64 assembly is automatically put to use if you run the PEI
> Core -- as part of a firmware platform that uses temp RAM migration --
> on ARM/AARCH64.
>

OK, seems like I'm the one creating confusion here. These are
MdeModulePkg patches not OvmfPkg patches, and I noticed some other
patches by Jordan adding ARM/AARCH64 build support to OVMF.

I will put the assembly patches on my to-review list, but it may take
me a couple of days to get to them.

Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration

[Jordan Justen]

On 2019-04-10 09:41:43, Ard Biesheuvel wrote:
> On Wed, 10 Apr 2019 at 01:41, Jordan Justen <jordan.l.justen@intel.com> wrote:
> >
> > https://github.com/jljusten/edk2.git temp-ram-support-v2
> >
> > https://github.com/jljusten/edk2/commits/temp-ram-support-v2
> >
> > v2:
> >  * Add AARCH64 and ARM assembly
> 
> Hi Jordan,
> 
> I'm not sure I'm following the reasoning behind this.

Did you see the explanation in patch 1 commit message? Could you reply
there with questions, or maybe I should try to expand on that?

> Does this fix an issue we currently have on ARM systems?

Yes, but I don't know of a case where it has been observed on
AARCH64/ARM. Nevertheless, as far as I can tell, a similar issue could
happen because the current implementation relies on non-spec'd
behavior of code gen within a C function. (Not the C calling
convention, but what code does with inside the function between
calls.)

> And how did you build and/or test OVMF for ARM?

I built ArmVirtPkg for AARCH64 and ARM on x86_64 Linux with a
cross-compiler, and ran it with qemu.

-Jordan

> 
> >  * Drop IA32 and X64 .S source files
> >  * Adjust PEI_CORE_TEMPORARY_RAM_TRANSITION pointer in the assembly
> >    code based on the stack pointer change before & after
> >    TemporaryRamSupport->TemporaryRamMigration
> >  * Drop extra cleanup patches for OvmfPkg & EmulatorPkg. These were
> >    just complicating the series.
> >
> > 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
> > "MdeModulePkg/Core/Pei: Add interface for assembly based
> > TemporaryRamSupport" patch.
> >
> > Testing:
> >
> > I tested building and booting in several scenarios:
> >
> > * OVMF IA32 & X64 on Linux
> > * ArmVirtPkg AARCH64 & ARM on x86_64 Linux
> > * EmulatorPkg IA32 & X64 on Linux
> >
> > Untested:
> >
> > * My system does not reproduce the issue that Liu Yu reported with
> >   EmulatorPkg, so I can't say that I have verified that issue.
> > * Building on windows
> > * AARCH64/ARM TemporaryRamMigration.asm sources