[PATCH v2 0/5] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[PATCH v2 0/5] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[Jeff Fan]

Allocate safe AP stack under 4GB and make sure BSP wait till all APs running in
safe code.  

v2:
  1. Update #1 to address the comments in
       https://lists.01.org/pipermail/edk2-devel/2016-November/005136.html
  2. Update #2 to address the comments in
       https://lists.01.org/pipermail/edk2-devel/2016-November/005137.html
  3. Update #3 to address the comments in
       https://lists.01.org/pipermail/edk2-devel/2016-November/005138.html
  4. Add #4 to fix getting target C-State bug.
  5. Add #5 to remove ret instruction.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>

Jeff Fan (5):
  UefiCpuPkg/DxeMpLib: Get safe AP loop handler from global variable
  UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB
  UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code
  UefiCpuPkg/DxeMpLib: Fix bug when getting target C-State from eax
  UefiCpuPkg/DxeMpLib: Remove unnecessary ret instruction

 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 29 ++++++++++++++++++++++----
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 16 ++++++++++----
 UefiCpuPkg/Library/MpInitLib/MpLib.h           |  4 +++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  8 ++++---
 4 files changed, 45 insertions(+), 12 deletions(-)

-- 
2.9.3.windows.2

[PATCH v2 1/5] UefiCpuPkg/DxeMpLib: Get safe AP loop handler from global variable

[Jeff Fan]

AP loop function is already saved into global variable, needn't to get it from
AP function parameter.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7f3900b..a0d5eeb 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -244,7 +244,7 @@ RelocateApLoop (
 
   CpuMpData    = GetCpuMpData ();
   MwaitSupport = IsMwaitSupport ();
-  AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) Buffer;
+  AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
   AsmRelocateApLoopFunc (MwaitSupport, CpuMpData->ApTargetCState, CpuMpData->PmCodeSegment);
   //
   // It should never reach here
@@ -273,7 +273,7 @@ MpInitChangeApLoopCallback (
   CpuMpData->SaveRestoreFlag = TRUE;
   CpuMpData->PmCodeSegment = GetProtectedModeCS ();
   CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
-  WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, mReservedApLoopFunc);
+  WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
   DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
 }
 
-- 
2.9.3.windows.2

[PATCH v2 2/5] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[Jeff Fan]

For long mode DXE, we will disable paging on AP to protected mode to execute AP
safe loop code in reserved memory range under 4GB. But we forget to allocate
stack for AP under 4GB and AP still are using original AP stack. If original AP
stack is larger than 4GB, it cannot be used after AP is transferred to protected
mode. Besides MwaitSupport == TRUE, AP stack is still required during phase of
disabling paging in long mode DXE.

MMoreover, even though AP stack is always under 4GB (a) in Ia32 DXE and (b) with
this patch, after transferring to protected mode from X64 DXE, AP stack
(in BootServiceData) maybe crashed by OS after Exit Boot Service event.

This fix is to allocate reserved memory range under 4GB together with AP safe
loop code. APs will switch to new stack in safe loop code.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 19 +++++++++++++++++--
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 13 ++++++++++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h           |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  3 ++-
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index a0d5eeb..5a3b02c 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -18,6 +18,7 @@
 #include <Library/UefiBootServicesTableLib.h>
 
 #define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
+#define  AP_SAFE_STACK_SIZE    128
 
 CPU_MP_DATA      *mCpuMpData = NULL;
 EFI_EVENT        mCheckAllApsEvent = NULL;
@@ -25,6 +26,7 @@ EFI_EVENT        mMpInitExitBootServicesEvent = NULL;
 EFI_EVENT        mLegacyBootEvent = NULL;
 volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
 VOID             *mReservedApLoopFunc = NULL;
+UINTN            mReservedTopOfApStack;
 
 /**
   Get the pointer to CPU MP Data structure.
@@ -241,11 +243,18 @@ RelocateApLoop (
   CPU_MP_DATA            *CpuMpData;
   BOOLEAN                MwaitSupport;
   ASM_RELOCATE_AP_LOOP   AsmRelocateApLoopFunc;
+  UINTN                  ProcessorNumber;
 
+  MpInitLibWhoAmI (&ProcessorNumber); 
   CpuMpData    = GetCpuMpData ();
   MwaitSupport = IsMwaitSupport ();
   AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
-  AsmRelocateApLoopFunc (MwaitSupport, CpuMpData->ApTargetCState, CpuMpData->PmCodeSegment);
+  AsmRelocateApLoopFunc (
+    MwaitSupport,
+    CpuMpData->ApTargetCState,
+    CpuMpData->PmCodeSegment,
+    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE
+    );
   //
   // It should never reach here
   //
@@ -289,6 +298,7 @@ InitMpGlobalData (
 {
   EFI_STATUS                 Status;
   EFI_PHYSICAL_ADDRESS       Address;
+  UINTN                      ApSafeBufferSize;
 
   SaveCpuMpData (CpuMpData);
 
@@ -307,16 +317,21 @@ InitMpGlobalData (
   // Allocating it in advance since memory services are not available in
   // Exit Boot Services callback function.
   //
+  ApSafeBufferSize  = CpuMpData->AddressMap.RelocateApLoopFuncSize;
+  ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
+
   Address = BASE_4GB - 1;
   Status  = gBS->AllocatePages (
                    AllocateMaxAddress,
                    EfiReservedMemoryType,
-                   EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)),
+                   EFI_SIZE_TO_PAGES (ApSafeBufferSize),
                    &Address
                    );
   ASSERT_EFI_ERROR (Status);
   mReservedApLoopFunc = (VOID *) (UINTN) Address;
   ASSERT (mReservedApLoopFunc != NULL);
+  mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
+  ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
   CopyMem (
     mReservedApLoopFunc,
     CpuMpData->AddressMap.RelocateApLoopFuncAddress,
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 9067f78..7ab136b 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -215,19 +215,26 @@ CProcedureInvoke:
 RendezvousFunnelProcEnd:
 
 ;-------------------------------------------------------------------------------------
-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
 ;-------------------------------------------------------------------------------------
 global ASM_PFX(AsmRelocateApLoop)
 ASM_PFX(AsmRelocateApLoop):
 AsmRelocateApLoopStart:
-    cmp        byte [esp + 4], 1
+    mov        eax, esp
+    mov        esp, [eax + 16]     ; TopOfApStack
+    push       dword [eax]         ; push return address for stack trace
+    push       ebp
+    mov        ebp, esp
+    mov        ebx, [eax + 8]      ; ApTargetCState
+    mov        ecx, [eax + 4]      ; MwaitSupport
+    cmp        cl,  1              ; Check mwait-monitor support
     jnz        HltLoop
 MwaitLoop:
     mov        eax, esp
     xor        ecx, ecx
     xor        edx, edx
     monitor
-    mov        eax, [esp + 8]    ; Mwait Cx, Target C-State per eax[7:4]
+    mov        eax, ebx            ; Mwait Cx, Target C-State per eax[7:4]
     shl        eax, 4
     mwait
     jmp        MwaitLoop
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index f73a469..e6dea18 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -250,7 +250,8 @@ VOID
 (EFIAPI * ASM_RELOCATE_AP_LOOP) (
   IN BOOLEAN                 MwaitSupport,
   IN UINTN                   ApTargetCState,
-  IN UINTN                   PmCodeSegment
+  IN UINTN                   PmCodeSegment,
+  IN UINTN                   TopOfApStack
   );
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index e7e7d80..7869970 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -222,11 +222,12 @@ CProcedureInvoke:
 RendezvousFunnelProcEnd:
 
 ;-------------------------------------------------------------------------------------
-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
 ;-------------------------------------------------------------------------------------
 global ASM_PFX(AsmRelocateApLoop)
 ASM_PFX(AsmRelocateApLoop):
 AsmRelocateApLoopStart:
+    mov        rsp, r9
     push       rcx
     push       rdx
 
-- 
2.9.3.windows.2

[PATCH v2 3/5] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code

[Jeff Fan]

Add one semaphore to make sure BSP to wait till all APs run in AP safe loop
code.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 8 +++++++-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 +++-
 UefiCpuPkg/Library/MpInitLib/MpLib.h           | 3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 +++-
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 5a3b02c..8f5074b 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -27,6 +27,7 @@ EFI_EVENT        mLegacyBootEvent = NULL;
 volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
 VOID             *mReservedApLoopFunc = NULL;
 UINTN            mReservedTopOfApStack;
+volatile UINT32  mNumberToFinish = 0;
 
 /**
   Get the pointer to CPU MP Data structure.
@@ -253,7 +254,8 @@ RelocateApLoop (
     MwaitSupport,
     CpuMpData->ApTargetCState,
     CpuMpData->PmCodeSegment,
-    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE
+    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
+    (UINTN) &mNumberToFinish
     );
   //
   // It should never reach here
@@ -282,7 +284,11 @@ MpInitChangeApLoopCallback (
   CpuMpData->SaveRestoreFlag = TRUE;
   CpuMpData->PmCodeSegment = GetProtectedModeCS ();
   CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
+  mNumberToFinish = CpuMpData->CpuCount - 1;
   WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
+  while (mNumberToFinish > 0) {
+    CpuPause ();
+  }
   DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
 }
 
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 7ab136b..a48f0bc 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -215,7 +215,7 @@ CProcedureInvoke:
 RendezvousFunnelProcEnd:
 
 ;-------------------------------------------------------------------------------------
-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
 ;-------------------------------------------------------------------------------------
 global ASM_PFX(AsmRelocateApLoop)
 ASM_PFX(AsmRelocateApLoop):
@@ -227,6 +227,8 @@ AsmRelocateApLoopStart:
     mov        ebp, esp
     mov        ebx, [eax + 8]      ; ApTargetCState
     mov        ecx, [eax + 4]      ; MwaitSupport
+    mov        eax, [eax + 20]     ; CountTofinish
+    lock dec   dword [eax]         ; (*CountTofinish)--
     cmp        cl,  1              ; Check mwait-monitor support
     jnz        HltLoop
 MwaitLoop:
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index e6dea18..49305ad 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -251,7 +251,8 @@ VOID
   IN BOOLEAN                 MwaitSupport,
   IN UINTN                   ApTargetCState,
   IN UINTN                   PmCodeSegment,
-  IN UINTN                   TopOfApStack
+  IN UINTN                   TopOfApStack,
+  IN UINTN                   NumberToFinish
   );
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 7869970..f8f4712 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -222,11 +222,13 @@ CProcedureInvoke:
 RendezvousFunnelProcEnd:
 
 ;-------------------------------------------------------------------------------------
-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
 ;-------------------------------------------------------------------------------------
 global ASM_PFX(AsmRelocateApLoop)
 ASM_PFX(AsmRelocateApLoop):
 AsmRelocateApLoopStart:
+    mov        rax, [rsp + 40]   ; CountTofinish
+    lock dec   dword [rax]       ; (*CountTofinish)--
     mov        rsp, r9
     push       rcx
     push       rdx
-- 
2.9.3.windows.2

[PATCH v2 4/5] UefiCpuPkg/DxeMpLib: Fix bug when getting target C-State from eax

[Jeff Fan]

AP will get target C-State from eax[7:4]. We do shift in ebx firstly before set
to eax. It will lead ebx is correct in the next time.

The fix is to set ebx to eax firstly and does shift in eax. Thus, ebx could keep
original value.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index f8f4712..34c07a6 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -266,8 +266,8 @@ MwaitLoop:
     xor        ecx, ecx           ; ecx = 0
     xor        edx, edx           ; edx = 0
     monitor
-    shl        ebx, 4
     mov        eax, ebx           ; Mwait Cx, Target C-State per eax[7:4]
+    shl        eax, 4
     mwait
     jmp        MwaitLoop
 HltLoop:
-- 
2.9.3.windows.2

[PATCH v2 5/5] UefiCpuPkg/DxeMpLib: Remove unnecessary ret instruction

[Jeff Fan]

Reported-by: Laszlo Ersek <lersek@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Fan <jeff.fan@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 1 -
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index a48f0bc..52363e6 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -244,7 +244,6 @@ HltLoop:
     cli
     hlt
     jmp        HltLoop
-    ret
 AsmRelocateApLoopEnd:
 
 ;-------------------------------------------------------------------------------------
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 34c07a6..fa54d01 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -274,7 +274,6 @@ HltLoop:
     cli
     hlt
     jmp        HltLoop
-    ret
 BITS 64
 AsmRelocateApLoopEnd:
 
-- 
2.9.3.windows.2

Re: [PATCH v2 1/5] UefiCpuPkg/DxeMpLib: Get safe AP loop handler from global variable

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> AP loop function is already saved into global variable, needn't to get it from
> AP function parameter.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7f3900b..a0d5eeb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -244,7 +244,7 @@ RelocateApLoop (
>  
>    CpuMpData    = GetCpuMpData ();
>    MwaitSupport = IsMwaitSupport ();
> -  AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) Buffer;
> +  AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
>    AsmRelocateApLoopFunc (MwaitSupport, CpuMpData->ApTargetCState, CpuMpData->PmCodeSegment);
>    //
>    // It should never reach here
> @@ -273,7 +273,7 @@ MpInitChangeApLoopCallback (
>    CpuMpData->SaveRestoreFlag = TRUE;
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> -  WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, mReservedApLoopFunc);
> +  WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
>    DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
>  }
>  
> 

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

Re: [PATCH v2 2/5] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> For long mode DXE, we will disable paging on AP to protected mode to execute AP
> safe loop code in reserved memory range under 4GB. But we forget to allocate
> stack for AP under 4GB and AP still are using original AP stack. If original AP
> stack is larger than 4GB, it cannot be used after AP is transferred to protected
> mode. Besides MwaitSupport == TRUE, AP stack is still required during phase of
> disabling paging in long mode DXE.
> 
> MMoreover, even though AP stack is always under 4GB (a) in Ia32 DXE and (b) with

Small typo in "MMoreover", can be fixed up before pushing.

Reviewed-by: Laszlo Ersek <lersek@redht.com>

Thanks
Laszlo

> this patch, after transferring to protected mode from X64 DXE, AP stack
> (in BootServiceData) maybe crashed by OS after Exit Boot Service event.
> 
> This fix is to allocate reserved memory range under 4GB together with AP safe
> loop code. APs will switch to new stack in safe loop code.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 19 +++++++++++++++++--
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 13 ++++++++++---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           |  3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  3 ++-
>  4 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a0d5eeb..5a3b02c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -18,6 +18,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
> +#define  AP_SAFE_STACK_SIZE    128
>  
>  CPU_MP_DATA      *mCpuMpData = NULL;
>  EFI_EVENT        mCheckAllApsEvent = NULL;
> @@ -25,6 +26,7 @@ EFI_EVENT        mMpInitExitBootServicesEvent = NULL;
>  EFI_EVENT        mLegacyBootEvent = NULL;
>  volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
>  VOID             *mReservedApLoopFunc = NULL;
> +UINTN            mReservedTopOfApStack;
>  
>  /**
>    Get the pointer to CPU MP Data structure.
> @@ -241,11 +243,18 @@ RelocateApLoop (
>    CPU_MP_DATA            *CpuMpData;
>    BOOLEAN                MwaitSupport;
>    ASM_RELOCATE_AP_LOOP   AsmRelocateApLoopFunc;
> +  UINTN                  ProcessorNumber;
>  
> +  MpInitLibWhoAmI (&ProcessorNumber); 
>    CpuMpData    = GetCpuMpData ();
>    MwaitSupport = IsMwaitSupport ();
>    AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
> -  AsmRelocateApLoopFunc (MwaitSupport, CpuMpData->ApTargetCState, CpuMpData->PmCodeSegment);
> +  AsmRelocateApLoopFunc (
> +    MwaitSupport,
> +    CpuMpData->ApTargetCState,
> +    CpuMpData->PmCodeSegment,
> +    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE
> +    );
>    //
>    // It should never reach here
>    //
> @@ -289,6 +298,7 @@ InitMpGlobalData (
>  {
>    EFI_STATUS                 Status;
>    EFI_PHYSICAL_ADDRESS       Address;
> +  UINTN                      ApSafeBufferSize;
>  
>    SaveCpuMpData (CpuMpData);
>  
> @@ -307,16 +317,21 @@ InitMpGlobalData (
>    // Allocating it in advance since memory services are not available in
>    // Exit Boot Services callback function.
>    //
> +  ApSafeBufferSize  = CpuMpData->AddressMap.RelocateApLoopFuncSize;
> +  ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> +
>    Address = BASE_4GB - 1;
>    Status  = gBS->AllocatePages (
>                     AllocateMaxAddress,
>                     EfiReservedMemoryType,
> -                   EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)),
> +                   EFI_SIZE_TO_PAGES (ApSafeBufferSize),
>                     &Address
>                     );
>    ASSERT_EFI_ERROR (Status);
>    mReservedApLoopFunc = (VOID *) (UINTN) Address;
>    ASSERT (mReservedApLoopFunc != NULL);
> +  mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
> +  ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
>    CopyMem (
>      mReservedApLoopFunc,
>      CpuMpData->AddressMap.RelocateApLoopFuncAddress,
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 9067f78..7ab136b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -215,19 +215,26 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
>  AsmRelocateApLoopStart:
> -    cmp        byte [esp + 4], 1
> +    mov        eax, esp
> +    mov        esp, [eax + 16]     ; TopOfApStack
> +    push       dword [eax]         ; push return address for stack trace
> +    push       ebp
> +    mov        ebp, esp
> +    mov        ebx, [eax + 8]      ; ApTargetCState
> +    mov        ecx, [eax + 4]      ; MwaitSupport
> +    cmp        cl,  1              ; Check mwait-monitor support
>      jnz        HltLoop
>  MwaitLoop:
>      mov        eax, esp
>      xor        ecx, ecx
>      xor        edx, edx
>      monitor
> -    mov        eax, [esp + 8]    ; Mwait Cx, Target C-State per eax[7:4]
> +    mov        eax, ebx            ; Mwait Cx, Target C-State per eax[7:4]
>      shl        eax, 4
>      mwait
>      jmp        MwaitLoop
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index f73a469..e6dea18 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -250,7 +250,8 @@ VOID
>  (EFIAPI * ASM_RELOCATE_AP_LOOP) (
>    IN BOOLEAN                 MwaitSupport,
>    IN UINTN                   ApTargetCState,
> -  IN UINTN                   PmCodeSegment
> +  IN UINTN                   PmCodeSegment,
> +  IN UINTN                   TopOfApStack
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index e7e7d80..7869970 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -222,11 +222,12 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
>  AsmRelocateApLoopStart:
> +    mov        rsp, r9
>      push       rcx
>      push       rdx
>  
> 

Re: [PATCH v2 3/5] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> Add one semaphore to make sure BSP to wait till all APs run in AP safe loop
> code.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 8 +++++++-
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 +++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           | 3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 +++-
>  4 files changed, 15 insertions(+), 4 deletions(-)

Looks good to me.

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

Thanks
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 5a3b02c..8f5074b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -27,6 +27,7 @@ EFI_EVENT        mLegacyBootEvent = NULL;
>  volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
>  VOID             *mReservedApLoopFunc = NULL;
>  UINTN            mReservedTopOfApStack;
> +volatile UINT32  mNumberToFinish = 0;
>  
>  /**
>    Get the pointer to CPU MP Data structure.
> @@ -253,7 +254,8 @@ RelocateApLoop (
>      MwaitSupport,
>      CpuMpData->ApTargetCState,
>      CpuMpData->PmCodeSegment,
> -    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE
> +    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
> +    (UINTN) &mNumberToFinish
>      );
>    //
>    // It should never reach here
> @@ -282,7 +284,11 @@ MpInitChangeApLoopCallback (
>    CpuMpData->SaveRestoreFlag = TRUE;
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> +  mNumberToFinish = CpuMpData->CpuCount - 1;
>    WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> +  while (mNumberToFinish > 0) {
> +    CpuPause ();
> +  }
>    DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
>  }
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 7ab136b..a48f0bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -215,7 +215,7 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
> @@ -227,6 +227,8 @@ AsmRelocateApLoopStart:
>      mov        ebp, esp
>      mov        ebx, [eax + 8]      ; ApTargetCState
>      mov        ecx, [eax + 4]      ; MwaitSupport
> +    mov        eax, [eax + 20]     ; CountTofinish
> +    lock dec   dword [eax]         ; (*CountTofinish)--
>      cmp        cl,  1              ; Check mwait-monitor support
>      jnz        HltLoop
>  MwaitLoop:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index e6dea18..49305ad 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -251,7 +251,8 @@ VOID
>    IN BOOLEAN                 MwaitSupport,
>    IN UINTN                   ApTargetCState,
>    IN UINTN                   PmCodeSegment,
> -  IN UINTN                   TopOfApStack
> +  IN UINTN                   TopOfApStack,
> +  IN UINTN                   NumberToFinish
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 7869970..f8f4712 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -222,11 +222,13 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
>  AsmRelocateApLoopStart:
> +    mov        rax, [rsp + 40]   ; CountTofinish
> +    lock dec   dword [rax]       ; (*CountTofinish)--
>      mov        rsp, r9
>      push       rcx
>      push       rdx
> 

Re: [PATCH v2 4/5] UefiCpuPkg/DxeMpLib: Fix bug when getting target C-State from eax

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> AP will get target C-State from eax[7:4]. We do shift in ebx firstly before set
> to eax. It will lead ebx is correct in the next time.

Please replace the word "correct" with "incorrect" in the commit message
-- without the patch, ebx is wrong at the second and further iterations.

With that word changed:

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

Thanks!
Laszlo

> 
> The fix is to set ebx to eax firstly and does shift in eax. Thus, ebx could keep
> original value.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index f8f4712..34c07a6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -266,8 +266,8 @@ MwaitLoop:
>      xor        ecx, ecx           ; ecx = 0
>      xor        edx, edx           ; edx = 0
>      monitor
> -    shl        ebx, 4
>      mov        eax, ebx           ; Mwait Cx, Target C-State per eax[7:4]
> +    shl        eax, 4
>      mwait
>      jmp        MwaitLoop
>  HltLoop:
> 

Re: [PATCH v2 5/5] UefiCpuPkg/DxeMpLib: Remove unnecessary ret instruction

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 1 -
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index a48f0bc..52363e6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -244,7 +244,6 @@ HltLoop:
>      cli
>      hlt
>      jmp        HltLoop
> -    ret
>  AsmRelocateApLoopEnd:
>  
>  ;-------------------------------------------------------------------------------------
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 34c07a6..fa54d01 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -274,7 +274,6 @@ HltLoop:
>      cli
>      hlt
>      jmp        HltLoop
> -    ret
>  BITS 64
>  AsmRelocateApLoopEnd:
>  
> 

Right, thank you for removing the superfluous RET from the Ia32 code as
well, my report covered the X64 case only, and I noticed Ia32 only later.

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

Please hold off committing the series just yet, I'd like to test it as
well. I'll report back with results.

Thanks!
Laszlo

Re: [PATCH v2 0/5] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB

[Laszlo Ersek]

On 11/25/16 07:03, Jeff Fan wrote:
> Allocate safe AP stack under 4GB and make sure BSP wait till all APs running in
> safe code.  
> 
> v2:
>   1. Update #1 to address the comments in
>        https://lists.01.org/pipermail/edk2-devel/2016-November/005136.html
>   2. Update #2 to address the comments in
>        https://lists.01.org/pipermail/edk2-devel/2016-November/005137.html
>   3. Update #3 to address the comments in
>        https://lists.01.org/pipermail/edk2-devel/2016-November/005138.html
>   4. Add #4 to fix getting target C-State bug.
>   5. Add #5 to remove ret instruction.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> 
> Jeff Fan (5):
>   UefiCpuPkg/DxeMpLib: Get safe AP loop handler from global variable
>   UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB
>   UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code
>   UefiCpuPkg/DxeMpLib: Fix bug when getting target C-State from eax
>   UefiCpuPkg/DxeMpLib: Remove unnecessary ret instruction
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 29 ++++++++++++++++++++++----
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 16 ++++++++++----
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           |  4 +++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  8 ++++---
>  4 files changed, 45 insertions(+), 12 deletions(-)
> 

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