* [PATCH v2 1/6] MdeModulePkg/Core/Pei: Add interface for assembly based TemporaryRamSupport
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
@ 2019-04-10 8:39 ` Jordan Justen
2019-04-10 8:39 ` [PATCH v2 2/6] MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration Jordan Justen
` (6 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:39 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/6] MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
2019-04-10 8:39 ` [PATCH v2 1/6] MdeModulePkg/Core/Pei: Add interface for assembly based TemporaryRamSupport Jordan Justen
@ 2019-04-10 8:39 ` Jordan Justen
2019-04-10 8:39 ` [PATCH v2 3/6] MdeModulePkg/Core/Pei: Add ARM " Jordan Justen
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:39 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Leif Lindholm, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 3/6] MdeModulePkg/Core/Pei: Add ARM assembly for TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
2019-04-10 8:39 ` [PATCH v2 1/6] MdeModulePkg/Core/Pei: Add interface for assembly based TemporaryRamSupport Jordan Justen
2019-04-10 8:39 ` [PATCH v2 2/6] MdeModulePkg/Core/Pei: Add AARCH64 assembly for TemporaryRamMigration Jordan Justen
@ 2019-04-10 8:39 ` Jordan Justen
2019-04-10 8:39 ` [PATCH v2 4/6] MdeModulePkg/Core/Pei: Add IA32 " Jordan Justen
` (4 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:39 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Leif Lindholm, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 4/6] MdeModulePkg/Core/Pei: Add IA32 assembly for TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (2 preceding siblings ...)
2019-04-10 8:39 ` [PATCH v2 3/6] MdeModulePkg/Core/Pei: Add ARM " Jordan Justen
@ 2019-04-10 8:39 ` Jordan Justen
2019-04-10 8:39 ` [PATCH v2 5/6] MdeModulePkg/Core/Pei: Add X64 " Jordan Justen
` (3 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:39 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 5/6] MdeModulePkg/Core/Pei: Add X64 assembly for TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (3 preceding siblings ...)
2019-04-10 8:39 ` [PATCH v2 4/6] MdeModulePkg/Core/Pei: Add IA32 " Jordan Justen
@ 2019-04-10 8:39 ` Jordan Justen
2019-04-10 8:40 ` [PATCH v2 6/6] MdeModulePkg/Core/Pei: Use code path for assembly based TemporaryRamSupport Jordan Justen
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:39 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
Some compilers may optimize register usage in ways that are
incompatible with the TemporaryRamSupport PPI. Using assembly code to
call the TemporaryRamMigration function prevents this issue.
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 6/6] MdeModulePkg/Core/Pei: Use code path for assembly based TemporaryRamSupport
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (4 preceding siblings ...)
2019-04-10 8:39 ` [PATCH v2 5/6] MdeModulePkg/Core/Pei: Add X64 " Jordan Justen
@ 2019-04-10 8:40 ` Jordan Justen
2019-04-10 16:41 ` [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Ard Biesheuvel
2019-04-10 17:26 ` Laszlo Ersek
7 siblings, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 8:40 UTC (permalink / raw)
To: devel; +Cc: Jordan Justen, Jian J Wang, Hao Wu, Ray Ni, Star Zeng
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (5 preceding siblings ...)
2019-04-10 8:40 ` [PATCH v2 6/6] MdeModulePkg/Core/Pei: Use code path for assembly based TemporaryRamSupport Jordan Justen
@ 2019-04-10 16:41 ` Ard Biesheuvel
2019-04-10 18:28 ` Laszlo Ersek
2019-04-10 18:54 ` Jordan Justen
2019-04-10 17:26 ` Laszlo Ersek
7 siblings, 2 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2019-04-10 16:41 UTC (permalink / raw)
To: edk2-devel-groups-io, Jordan Justen
Cc: Liu Yu, Ray Ni, Andrew Fish, Laszlo Ersek, Leif Lindholm,
Michael D Kinney
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
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
2019-04-10 16:41 ` [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Ard Biesheuvel
@ 2019-04-10 18:28 ` Laszlo Ersek
2019-04-10 18:31 ` Ard Biesheuvel
2019-04-10 18:54 ` Jordan Justen
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-04-10 18:28 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io, Jordan Justen
Cc: Liu Yu, Ray Ni, Andrew Fish, Leif Lindholm, Michael D Kinney
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
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
2019-04-10 18:28 ` Laszlo Ersek
@ 2019-04-10 18:31 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2019-04-10 18:31 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-groups-io, Jordan Justen, Liu Yu, Ray Ni, Andrew Fish,
Leif Lindholm, Michael D Kinney
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
2019-04-10 16:41 ` [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Ard Biesheuvel
2019-04-10 18:28 ` Laszlo Ersek
@ 2019-04-10 18:54 ` Jordan Justen
1 sibling, 0 replies; 12+ messages in thread
From: Jordan Justen @ 2019-04-10 18:54 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel-groups-io
Cc: Liu Yu, Ray Ni, Andrew Fish, Laszlo Ersek, Leif Lindholm,
Michael D Kinney
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration
2019-04-10 8:39 [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
` (6 preceding siblings ...)
2019-04-10 16:41 ` [edk2-devel] [PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration Ard Biesheuvel
@ 2019-04-10 17:26 ` Laszlo Ersek
7 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-04-10 17:26 UTC (permalink / raw)
To: devel, jordan.l.justen
Cc: Liu Yu, Ray Ni, Andrew Fish, Leif Lindholm, Michael D Kinney
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread