[PATCH v3 0/2] Fix wrong address set as Stack Guard for APs

[PATCH v3 0/2] Fix wrong address set as Stack Guard for APs

[Jian J Wang]

> v3 changes:
> a. Split the patch into two patch files.
> b. Pass MpServiceProtocol test cases in PI SCT.

> v2 changes:
> a. Use each AP's ApTopOfStack to get the stack base address instead of
>    cpu0's ApTopOfStack which is actually set incorrectly before.
> b. Fix cpu0's ApTopOfStack initialization.
> c. Fix wrong debug print format.

The reason is that DXE part initialization will reuse the stack allocated
at PEI phase, if MP was initialized before. Some code added to check this
situation and use stack base address saved in HOB passed from PEI.

Jian J Wang (2):
  UefiCpuPkg/MpInitLib: fix incorrect stack base init for cpu0
  UefiCpuPkg/MpInitLib: fix wrong address set as Stack Guard for APs

 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++++++++++++++++++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c    |  2 +-
 2 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.15.1.windows.2

[PATCH v3 1/2] UefiCpuPkg/MpInitLib: fix incorrect stack top init for cpu0

[Jian J Wang]

> v3 changes:
> a. Split the patch into two patch files.
> b. Pass MpServiceProtocol test cases in PI SCT.

> v2 changes:
> a. Use each AP's ApTopOfStack to get the stack base address instead of
>    cpu0's ApTopOfStack which is actually set incorrectly before.
> b. Fix cpu0's ApTopOfStack initialization.
> c. Fix wrong debug print format.

As the name suggests, CpuMpData->CpuInfoInHob[0].ApTopOfStack must be init
to the top of stack. But the MpInitLibInitialize() passed the base address
of stack to InitializeApData(), which is not correct. Although this stack
is not used for BSP, it's should be fixed in case of misunderstanding and
future possible code changes.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 0c2058a7b0..1bfab8467b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1498,7 +1498,7 @@ MpInitLibInitialize (
   //
   // Set BSP basic information
   //
-  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);
+  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
   //
   // Save assembly code information
   //
-- 
2.15.1.windows.2

[PATCH v3 2/2] UefiCpuPkg/MpInitLib: fix wrong address set as Stack Guard for APs

[Jian J Wang]

> v3 changes:
> a. Split the patch into two patch files.
> b. Pass MpServiceProtocol test cases in PI SCT.

> v2 changes:
> a. Use each AP's ApTopOfStack to get the stack base address instead of
>    cpu0's ApTopOfStack which is actually set incorrectly before.
> b. Fix cpu0's ApTopOfStack initialization.
> c. Fix wrong debug print format.

The reason is that DXE part initialization will reuse the stack allocated
at PEI phase, if MP was initialized before. Some code added to check this
situation and use stack base address saved in HOB passed from PEI.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 40c1bf407a..e832c16eca 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -295,6 +295,7 @@ InitMpGlobalData (
   UINTN                               Index;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR     MemDesc;
   UINTN                               StackBase;
+  CPU_INFO_IN_HOB                     *CpuInfoInHob;
 
   SaveCpuMpData (CpuMpData);
 
@@ -314,8 +315,21 @@ InitMpGlobalData (
       ASSERT (FALSE);
     }
 
+    //
+    // DXE will reuse stack allocated for APs at PEI phase if it's available.
+    // Let's check it here.
+    //
+    // Note: BSP's stack guard is set at DxeIpl phase. But for the sake of
+    // BSP/AP exchange, stack guard for ApTopOfStack of cpu 0 will still be
+    // set here.
+    //
+    CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
     for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
-      StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
+      if (CpuInfoInHob != NULL && CpuInfoInHob[Index].ApTopOfStack != 0) {
+        StackBase = CpuInfoInHob[Index].ApTopOfStack - CpuMpData->CpuApStackSize;
+      } else {
+        StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
+      }
 
       Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
       ASSERT_EFI_ERROR (Status);
@@ -326,6 +340,9 @@ InitMpGlobalData (
                       MemDesc.Attributes | EFI_MEMORY_RP
                       );
       ASSERT_EFI_ERROR (Status);
+
+      DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
+              (UINT64)StackBase, (UINT64)Index));
     }
   }
 
-- 
2.15.1.windows.2

Re: [PATCH v3 0/2] Fix wrong address set as Stack Guard for APs

[Laszlo Ersek]

On 01/08/18 06:39, Jian J Wang wrote:
>> v3 changes:
>> a. Split the patch into two patch files.
>> b. Pass MpServiceProtocol test cases in PI SCT.
> 
>> v2 changes:
>> a. Use each AP's ApTopOfStack to get the stack base address instead of
>>    cpu0's ApTopOfStack which is actually set incorrectly before.
>> b. Fix cpu0's ApTopOfStack initialization.
>> c. Fix wrong debug print format.
> 
> The reason is that DXE part initialization will reuse the stack allocated
> at PEI phase, if MP was initialized before. Some code added to check this
> situation and use stack base address saved in HOB passed from PEI.
> 
> Jian J Wang (2):
>   UefiCpuPkg/MpInitLib: fix incorrect stack base init for cpu0
>   UefiCpuPkg/MpInitLib: fix wrong address set as Stack Guard for APs
> 
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++++++++++++++++++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    |  2 +-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 

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

Thanks!
Laszlo

Re: [PATCH v3 0/2] Fix wrong address set as Stack Guard for APs

[Dong, Eric]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J Wang
Sent: Monday, January 8, 2018 1:40 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v3 0/2] Fix wrong address set as Stack Guard for APs

> v3 changes:
> a. Split the patch into two patch files.
> b. Pass MpServiceProtocol test cases in PI SCT.

> v2 changes:
> a. Use each AP's ApTopOfStack to get the stack base address instead of
>    cpu0's ApTopOfStack which is actually set incorrectly before.
> b. Fix cpu0's ApTopOfStack initialization.
> c. Fix wrong debug print format.

The reason is that DXE part initialization will reuse the stack allocated at PEI phase, if MP was initialized before. Some code added to check this situation and use stack base address saved in HOB passed from PEI.

Jian J Wang (2):
  UefiCpuPkg/MpInitLib: fix incorrect stack base init for cpu0
  UefiCpuPkg/MpInitLib: fix wrong address set as Stack Guard for APs

 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++++++++++++++++++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c    |  2 +-
 2 files changed, 19 insertions(+), 2 deletions(-)

--
2.15.1.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel