[PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure

[PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 

This patch serial used to fix an ASSERT issue. Because AP can't find 
the CpuMpData through IDT, it raised the ASSERT.

V2:
1. Enhance code comments.
2. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>

Eric Dong (2):
  UefiCpuPkg/MpInitLib: Restore IDT context for APs.
  UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.23.0.windows.1

[PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683

This patch fixes an assertion because AP can't find the CpuMpData.
When AP is waken up through Init-Sipi-Sipi, AP's IDT should
be restored to pre-allocated buffer so AP can get the CpuMpData
through the IDT base address.
Current code already has logic to handle this when CpuMpData->
InitFlag is ApInitConfig but misses the logic
when CpuMpData->InitFlag is ApInitReconfig.
This patch fixes this gap.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
V2:
Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 64a4c3546e..2e87aa1f06 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -692,6 +692,16 @@ ApWakeupFunction (
         //
         RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
       } else {
+        if (CpuMpData->InitFlag == ApInitReconfig) {
+          //
+          // Initialize AP volatile registers in ApInitReconfig path.
+          // ApInitReconfig happens when:
+          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
+          // 2. AP is initialized in DXE phase.
+          //
+          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+        }
+
         //
         // The CPU driver might not flush TLB for APs on spot after updating
         // page attributes. AP in mwait loop mode needs to take care of it when
-- 
2.23.0.windows.1

[PATCH 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

[Dong, Eric]

From: "Dong, Eric" <eric.dong@intel.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683

In PEI phase, AP already been waked up through ApInitConfig,
so it can directly wake up it through change wakup buffer
instead of use ApInitReconfig flag. It can save some time.

Change code to only use ApInitReconfig flag in DXE phase
which must need to update the wake up buffer.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
V2:
Enhance the code comments.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 2e87aa1f06..6d3a0ccc72 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1818,7 +1818,14 @@ MpInitLibInitialize (
   // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
   //
   if (CpuMpData->CpuCount > 1) {
-    CpuMpData->InitFlag = ApInitReconfig;
+    if (OldCpuMpData != NULL) {
+      //
+      // Only needs to use this flag for DXE phase to update the wake up
+      // buffer. Wakeup buffer allocated in PEI phase is no longer valid
+      // in DXE.
+      //
+      CpuMpData->InitFlag = ApInitReconfig;
+    }
     WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     //
     // Wait for all APs finished initialization
@@ -1826,7 +1833,9 @@ MpInitLibInitialize (
     while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
       CpuPause ();
     }
-    CpuMpData->InitFlag = ApInitDone;
+    if (OldCpuMpData != NULL) {
+      CpuMpData->InitFlag = ApInitDone;
+    }
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
       SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
     }
-- 
2.23.0.windows.1

[PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Dong, Eric]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683

This patch fixes an assertion because AP can't find the CpuMpData.
When AP is waken up through Init-Sipi-Sipi, AP's IDT should
be restored to pre-allocated buffer so AP can get the CpuMpData
through the IDT base address.
Current code already has logic to handle this when CpuMpData->
InitFlag is ApInitConfig but misses the logic
when CpuMpData->InitFlag is ApInitReconfig.
This patch fixes this gap.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
V2:
1. Enhance code comments.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 64a4c3546e..2e87aa1f06 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -692,6 +692,16 @@ ApWakeupFunction (
         //
         RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
       } else {
+        if (CpuMpData->InitFlag == ApInitReconfig) {
+          //
+          // Initialize AP volatile registers in ApInitReconfig path.
+          // ApInitReconfig happens when:
+          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
+          // 2. AP is initialized in DXE phase.
+          //
+          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+        }
+
         //
         // The CPU driver might not flush TLB for APs on spot after updating
         // page attributes. AP in mwait loop mode needs to take care of it when
-- 
2.23.0.windows.1

Re: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Ni, Ray]

Eric,
It's natural to use the volatile registers value derived from BSP in ApInitReconfig path.
So I still prefer to use the code I suggested in the review comment to the v1 patch.

We can remove the below line that specially for ApInitReconfig path in MpInitLbInitialize().
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c#L1783:
      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));

Thanks,
Ray
> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Wednesday, April 22, 2020 5:01 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>
> Subject: [PATCH v2 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> This patch fixes an assertion because AP can't find the CpuMpData.
> When AP is waken up through Init-Sipi-Sipi, AP's IDT should
> be restored to pre-allocated buffer so AP can get the CpuMpData
> through the IDT base address.
> Current code already has logic to handle this when CpuMpData->
> InitFlag is ApInitConfig but misses the logic
> when CpuMpData->InitFlag is ApInitReconfig.
> This patch fixes this gap.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> 
> V2:
> 
> 1. Enhance code comments.
> 
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..2e87aa1f06 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -692,6 +692,16 @@ ApWakeupFunction (
>          //
> 
>          RestoreVolatileRegisters (&CpuMpData-
> >CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> 
>        } else {
> 
> +        if (CpuMpData->InitFlag == ApInitReconfig) {
> 
> +          //
> 
> +          // Initialize AP volatile registers in ApInitReconfig path.
> 
> +          // ApInitReconfig happens when:
> 
> +          // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase.
> 
> +          // 2. AP is initialized in DXE phase.
> 
> +          //
> 
> +          RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> 
> +        }
> 
> +
> 
>          //
> 
>          // The CPU driver might not flush TLB for APs on spot after updating
> 
>          // page attributes. AP in mwait loop mode needs to take care of it
> when
> 
> --
> 2.23.0.windows.1