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

[PATCH v3 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.

V3:
1. Remove invalid save Volatile Registers process. Refine restore
   Volatile Registers process.

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 | 47 ++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 13 deletions(-)

-- 
2.23.0.windows.1

[PATCH v3 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>
---
V3:
  Remove invalid save volatile registers process. Refine restore
  volatile registers process.

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

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

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 64a4c3546e..7fd757b428 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -686,18 +686,31 @@ ApWakeupFunction (
         WAKEUP_AP_SIGNAL,
         0
         );
-      if (CpuMpData->ApLoopMode == ApInHltLoop) {
-        //
-        // Restore AP's volatile registers saved
-        //
-        RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
-      } else {
+
+      if (CpuMpData->InitFlag == ApInitReconfig) {
         //
-        // 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
-        // woken up.
+        // 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. 
+        // In either case, use the volatile registers value derived from BSP.
+        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
+        //   different IDT shared by all APs.
         //
-        CpuFlushTlb ();
+        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+      }  else {
+        if (CpuMpData->ApLoopMode == ApInHltLoop) {
+          //
+          // Restore AP's volatile registers saved before AP is halted
+          //
+          RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
+        } else {
+          //
+          // 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
+          // woken up.
+          //
+          CpuFlushTlb ();
+        }
       }
 
       if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
@@ -1780,7 +1793,6 @@ MpInitLibInitialize (
       InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
       CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
       CpuMpData->CpuData[Index].ApFunction = 0;
-      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
     }
   }
 
-- 
2.23.0.windows.1

[PATCH v3 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: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 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 7fd757b428..64528fd1e7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1820,7 +1820,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
@@ -1828,7 +1835,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

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

[Ni, Ray]

Send to correct mail address: devel@edk2.groups.io

> -----Original Message-----
> From: Ni, Ray
> Sent: Sunday, April 26, 2020 11:46 AM
> To: Dong, Eric <eric.dong@intel.com>; \x16devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C <chandana.c.kumar@intel.com>
> Subject: RE: [PATCH v3 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> > -----Original Message-----
> > From: Dong, Eric <eric.dong@intel.com>
> > Sent: Friday, April 24, 2020 4:47 PM
> > To: \x16devel@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 v3 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure
> >
> > 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.
> >
> >
> >
> > V3:
> >
> > 1. Remove invalid save Volatile Registers process. Refine restore
> >
> >    Volatile Registers process.
> >
> >
> >
> > 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 | 47 ++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 13 deletions(-)
> >
> > --
> > 2.23.0.windows.1

Re: [edk2-devel] [PATCH v3 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure

[Laszlo Ersek]

On 04/24/20 10:47, Dong, Eric wrote:
> 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.
> 
> V3:
> 1. Remove invalid save Volatile Registers process. Refine restore
>    Volatile Registers process.
> 
> 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 | 47 ++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 13 deletions(-)
> 

I plan to regression-test this series with OVMF tomorrow. Thanks for
your patience!

Laszlo

Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Laszlo Ersek]

Hi Eric,

(1) on the email address list of this patch set, there is a strange item:

00000000  16 64 65 76 65 6c 40 65  64 6b 32 2e 67 72 6f 75  |.devel@edk2.grou|
00000010  70 73 2e 69 6f                                    |ps.io|

The first character is U+0016, which seems to stand for SYNCHRONOUS IDLE <https://en.wikipedia.org/wiki/Synchronous_Idle>. I think it must have been a typo.

(The set was otherwise posted correctly to the list -- the correct list address is present, too.)

Pointing this out just for future postings.

On 04/24/20 10:47, Dong, Eric wrote:
> 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>
> ---
> V3:
>   Remove invalid save volatile registers process. Refine restore
>   volatile registers process.
> 
> V2: 
>   Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check. 
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 +++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..7fd757b428 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -686,18 +686,31 @@ ApWakeupFunction (
>          WAKEUP_AP_SIGNAL,
>          0
>          );
> -      if (CpuMpData->ApLoopMode == ApInHltLoop) {
> -        //
> -        // Restore AP's volatile registers saved
> -        //
> -        RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> -      } else {
> +
> +      if (CpuMpData->InitFlag == ApInitReconfig) {
>          //
> -        // 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
> -        // woken up.
> +        // 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. 

(2) git-am complains that the line above has a trailing space character. Please remove it before you push the set.

> +        // In either case, use the volatile registers value derived from BSP.
> +        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
> +        //   different IDT shared by all APs.
>          //
> -        CpuFlushTlb ();
> +        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> +      }  else {
> +        if (CpuMpData->ApLoopMode == ApInHltLoop) {
> +          //
> +          // Restore AP's volatile registers saved before AP is halted
> +          //
> +          RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> +        } else {
> +          //
> +          // 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
> +          // woken up.
> +          //
> +          CpuFlushTlb ();
> +        }
>        }
>  
>        if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == CpuStateReady) {
> @@ -1780,7 +1793,6 @@ MpInitLibInitialize (
>        InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
>        CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
>        CpuMpData->CpuData[Index].ApFunction = 0;
> -      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
>      }
>    }
>  
> 

For the full series:

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

(For the testing, I used OVMF, with/without SMM, and normal boot and S3. With SMM, I also tested CPU hotplug. OVMF doesn't support "microcode patching", so maybe it didn't make sense to perform these regression-tests. But, "these patches should be harmless regarding <whatever>", are famous last words... So I rather wanted to regression-test them, under my use case.)

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Dong, Eric]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 29, 2020 6:59 PM
> To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT
> context for APs.
> 
> Hi Eric,
> 
> (1) on the email address list of this patch set, there is a strange item:
> 
> 00000000  16 64 65 76 65 6c 40 65  64 6b 32 2e 67 72 6f 75  |.devel@edk2.grou|
> 00000010  70 73 2e 69 6f                                    |ps.io|
> 
> The first character is U+0016, which seems to stand for SYNCHRONOUS IDLE
> <https://en.wikipedia.org/wiki/Synchronous_Idle>. I think it must have been
> a typo.
> 

I copied the mail address from outlook, maybe some invisible characters caused this error.
 
> (The set was otherwise posted correctly to the list -- the correct list address is
> present, too.)
> 
> Pointing this out just for future postings.
> 
> On 04/24/20 10:47, Dong, Eric wrote:
> > 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>
> > ---
> > V3:
> >   Remove invalid save volatile registers process. Refine restore
> >   volatile registers process.
> >
> > V2:
> >   Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop
> check.
> >
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 34
> > +++++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 64a4c3546e..7fd757b428 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -686,18 +686,31 @@ ApWakeupFunction (
> >          WAKEUP_AP_SIGNAL,
> >          0
> >          );
> > -      if (CpuMpData->ApLoopMode == ApInHltLoop) {
> > -        //
> > -        // Restore AP's volatile registers saved
> > -        //
> > -        RestoreVolatileRegisters (&CpuMpData-
> >CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> > -      } else {
> > +
> > +      if (CpuMpData->InitFlag == ApInitReconfig) {
> >          //
> > -        // 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
> > -        // woken up.
> > +        // 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.
> 
> (2) git-am complains that the line above has a trailing space character. Please
> remove it before you push the set.

Got it. Will remove it when merge the code.

> 
> > +        // In either case, use the volatile registers value derived from BSP.
> > +        // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters points to a
> > +        //   different IDT shared by all APs.
> >          //
> > -        CpuFlushTlb ();
> > +        RestoreVolatileRegisters (&CpuMpData-
> >CpuData[0].VolatileRegisters, FALSE);
> > +      }  else {
> > +        if (CpuMpData->ApLoopMode == ApInHltLoop) {
> > +          //
> > +          // Restore AP's volatile registers saved before AP is halted
> > +          //
> > +          RestoreVolatileRegisters (&CpuMpData-
> >CpuData[ProcessorNumber].VolatileRegisters, TRUE);
> > +        } else {
> > +          //
> > +          // 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
> > +          // woken up.
> > +          //
> > +          CpuFlushTlb ();
> > +        }
> >        }
> >
> >        if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) ==
> > CpuStateReady) { @@ -1780,7 +1793,6 @@ MpInitLibInitialize (
> >        InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
> >        CpuMpData->CpuData[Index].CpuHealthy =
> (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
> >        CpuMpData->CpuData[Index].ApFunction = 0;
> > -      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters,
> &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
> >      }
> >    }
> >
> >
> 
> For the full series:
> 
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> (For the testing, I used OVMF, with/without SMM, and normal boot and S3.
> With SMM, I also tested CPU hotplug. OVMF doesn't support "microcode
> patching", so maybe it didn't make sense to perform these regression-tests.
> But, "these patches should be harmless regarding <whatever>", are famous
> last words... So I rather wanted to regression-test them, under my use case.)

Thanks for your test! Very appreciate your supports.

> 
> Thanks,
> Laszlo