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

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

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

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

-- 
2.23.0.windows.1

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

[Dong, Eric]

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

This patch used to fix a ASSERT because AP can't find the CpuMpData.

When AP been waked up through Init-Sipi-Sipi, the IDT should
been restore to preallcated buffer to make it can get the
CpuMpData through IDT base address.
Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig,
AP will be wake up through Init-Sipi-Sipi. Current code already
has logic to handle ApInitConfig, but miss the handler for
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>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 64a4c3546e..ac7f92fd48 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -664,8 +664,9 @@ ApWakeupFunction (
       BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
       //
       // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
-      //   to initialize AP in InitConfig path.
-      // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
+      //   to initialize AP in InitConfig/ApInitReconfig path.
+      // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
+      //  different IDT shared by all APs.
       //
       RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
       InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
@@ -673,6 +674,16 @@ ApWakeupFunction (
 
       InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
     } else {
+      if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData->ApLoopMode != ApInHltLoop)) {
+        //
+        // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
+        //   to initialize AP in InitConfig/ApInitReconfig path.
+        // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a
+        //   different IDT shared by all APs.
+        //
+        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+      }
+
       //
       // Execute AP function if AP is ready
       //
-- 
2.23.0.windows.1

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

[Dong, Eric]

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>
---
 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 ac7f92fd48..644971b68f 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1819,7 +1819,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. Current been used wake up buffer is allocated in PEI phase
+      // and no long valid at this time.
+      //
+      CpuMpData->InitFlag = ApInitReconfig;
+    }
     WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     //
     // Wait for all APs finished initialization
@@ -1827,7 +1834,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 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.

[Ni, Ray]

Please check my comments in below.

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Wednesday, April 22, 2020 9:35 AM
> 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 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> 
> 
> 
> This patch used to fix a ASSERT because AP can't find the CpuMpData.

1. This patch fixes an assertion because AP can't find the CpuMpData.

> 
> 
> When AP been waked up through Init-Sipi-Sipi, the IDT should
> been restore to preallcated buffer to make it can get the
> CpuMpData through IDT base address.
> Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig,
> AP will be wake up through Init-Sipi-Sipi. Current code already
> has logic to handle ApInitConfig, but miss the handler for
> ApInitReconfig. This patch fixes this gap.

2. 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>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 64a4c3546e..ac7f92fd48 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -664,8 +664,9 @@ ApWakeupFunction (
>        BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> 
>        //
> 
>        // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP
> environment,
> 
> -      //   to initialize AP in InitConfig path.
> 
> -      // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters
> points to a different IDT shared by all APs.
> 
> +      //   to initialize AP in InitConfig/ApInitReconfig path.
> 
> +      // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters
> points to a
> 
> +      //  different IDT shared by all APs.
> 
>        //
> 
>        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> 
>        InitializeApData (CpuMpData, ProcessorNumber, BistData,
> ApTopOfStack);
> 
> @@ -673,6 +674,16 @@ ApWakeupFunction (
> 
> 
>        InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo-
> >NumApsExecuting);
> 
>      } else {
> 
> +      if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData-
> >ApLoopMode != ApInHltLoop)) {
> 
> +        //
> 
> +        // CpuMpData->CpuData[0].VolatileRegisters is initialized based on
> BSP environment,
> 
> +        //   to initialize AP in InitConfig/ApInitReconfig path.

3.
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.

> 
> +        // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters points to a
> 
> +        //   different IDT shared by all APs.
> 
> +        //
> 
> +        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);

4. Is it possible to combine this function call with the restoration code below?

      if (CpuMpData->InitFlag == ApInitReconfig) {
        //
        // 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.
        //
        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 ();
        }
      }
> 
> +      }
> 
> +
> 
>        //
> 
>        // Execute AP function if AP is ready
> 
>        //
> 
> --
> 2.23.0.windows.1

Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI.

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com> with a bit comments update:

"Current been used wake up buffer is allocated in PEI phase and no long valid at this time."
->
"Wakeup buffer allocated in PEI phase is no longer valid in DXE."


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong,
> Eric
> Sent: Wednesday, April 22, 2020 9:35 AM
> 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: [edk2-devel] [PATCH 2/2] UefiCpuPkg/MpInitLib: Avoid
> ApInitReconfig in PEI.
> 
> 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>
> ---
>  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 ac7f92fd48..644971b68f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1819,7 +1819,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. Current been used wake up buffer is allocated in PEI phase
> 
> +      // and no long valid at this time.
> 
> +      //
> 
> +      CpuMpData->InitFlag = ApInitReconfig;
> 
> +    }
> 
>      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> 
>      //
> 
>      // Wait for all APs finished initialization
> 
> @@ -1827,7 +1834,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
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#57774): https://edk2.groups.io/g/devel/message/57774
> Mute This Topic: https://groups.io/mt/73187417/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [ray.ni@intel.com]
> -=-=-=-=-=-=

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

[Dong, Eric]

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, April 22, 2020 3:54 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>
> Subject: RE: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> 
> Please check my comments in below.
> 
> > -----Original Message-----
> > From: Dong, Eric <eric.dong@intel.com>
> > Sent: Wednesday, April 22, 2020 9:35 AM
> > 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 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> >
> >
> >
> > This patch used to fix a ASSERT because AP can't find the CpuMpData.
> 
> 1. This patch fixes an assertion because AP can't find the CpuMpData.
> 
> >
> >
> > When AP been waked up through Init-Sipi-Sipi, the IDT should been
> > restore to preallcated buffer to make it can get the CpuMpData through
> > IDT base address.
> > Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, AP
> > will be wake up through Init-Sipi-Sipi. Current code already has logic
> > to handle ApInitConfig, but miss the handler for ApInitReconfig. This
> > patch fixes this gap.
> 
> 2. 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>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 64a4c3546e..ac7f92fd48 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -664,8 +664,9 @@ ApWakeupFunction (
> >        BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
> >
> >        //
> >
> >        // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP
> > environment,
> >
> > -      //   to initialize AP in InitConfig path.
> >
> > -      // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters
> > points to a different IDT shared by all APs.
> >
> > +      //   to initialize AP in InitConfig/ApInitReconfig path.
> >
> > +      // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters
> > points to a
> >
> > +      //  different IDT shared by all APs.
> >
> >        //
> >
> >        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> > FALSE);
> >
> >        InitializeApData (CpuMpData, ProcessorNumber, BistData,
> > ApTopOfStack);
> >
> > @@ -673,6 +674,16 @@ ApWakeupFunction (
> >
> >
> >        InterlockedDecrement ((UINT32 *) &CpuMpData-
> >MpCpuExchangeInfo-
> > >NumApsExecuting);
> >
> >      } else {
> >
> > +      if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData-
> > >ApLoopMode != ApInHltLoop)) {
> >
> > +        //
> >
> > +        // CpuMpData->CpuData[0].VolatileRegisters is initialized based on
> > BSP environment,
> >
> > +        //   to initialize AP in InitConfig/ApInitReconfig path.
> 
> 3.
> 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.
> 
> >
> > +        // NOTE: IDTR.BASE stored in CpuMpData-
> > >CpuData[0].VolatileRegisters points to a
> >
> > +        //   different IDT shared by all APs.
> >
> > +        //
> >
> > +        RestoreVolatileRegisters (&CpuMpData-
> >CpuData[0].VolatileRegisters,
> > FALSE);
> 
> 4. Is it possible to combine this function call with the restoration code below?
> 

New logic changes code flow when CpuMpData->InitFlag == ApInitReconfig. In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb, but new code only does CpuFlushTlb.
Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi?  I prefer to not change this code flow.

Thanks,
Eric

>       if (CpuMpData->InitFlag == ApInitReconfig) {
>         //
>         // 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.
>         //
>         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 ();
>         }
>       }
> >
> > +      }
> >
> > +
> >
> >        //
> >
> >        // Execute AP function if AP is ready
> >
> >        //
> >
> > --
> > 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

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

[Dong, Eric]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Dong, Eric
> Sent: Wednesday, April 22, 2020 4:23 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT
> context for APs.
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Wednesday, April 22, 2020 3:54 PM
> > To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> > Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C
> > <chandana.c.kumar@intel.com>
> > Subject: RE: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for
> APs.
> >
> > Please check my comments in below.
> >
> > > -----Original Message-----
> > > From: Dong, Eric <eric.dong@intel.com>
> > > Sent: Wednesday, April 22, 2020 9:35 AM
> > > 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 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683
> > >
> > >
> > >
> > > This patch used to fix a ASSERT because AP can't find the CpuMpData.
> >
> > 1. This patch fixes an assertion because AP can't find the CpuMpData.
> >
> > >
> > >
> > > When AP been waked up through Init-Sipi-Sipi, the IDT should been
> > > restore to preallcated buffer to make it can get the CpuMpData
> > > through IDT base address.
> > > Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, AP
> > > will be wake up through Init-Sipi-Sipi. Current code already has
> > > logic to handle ApInitConfig, but miss the handler for
> > > ApInitReconfig. This patch fixes this gap.
> >
> > 2. 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>
> > > ---
> > >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > index 64a4c3546e..ac7f92fd48 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > > @@ -664,8 +664,9 @@ ApWakeupFunction (
> > >        BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof
> > > (UINTN));
> > >
> > >        //
> > >
> > >        // CpuMpData->CpuData[0].VolatileRegisters is initialized
> > > based on BSP environment,
> > >
> > > -      //   to initialize AP in InitConfig path.
> > >
> > > -      // NOTE: IDTR.BASE stored in CpuMpData-
> > >CpuData[0].VolatileRegisters
> > > points to a different IDT shared by all APs.
> > >
> > > +      //   to initialize AP in InitConfig/ApInitReconfig path.
> > >
> > > +      // NOTE: IDTR.BASE stored in CpuMpData-
> > >CpuData[0].VolatileRegisters
> > > points to a
> > >
> > > +      //  different IDT shared by all APs.
> > >
> > >        //
> > >
> > >        RestoreVolatileRegisters
> > > (&CpuMpData->CpuData[0].VolatileRegisters,
> > > FALSE);
> > >
> > >        InitializeApData (CpuMpData, ProcessorNumber, BistData,
> > > ApTopOfStack);
> > >
> > > @@ -673,6 +674,16 @@ ApWakeupFunction (
> > >
> > >
> > >        InterlockedDecrement ((UINT32 *) &CpuMpData-
> > >MpCpuExchangeInfo-
> > > >NumApsExecuting);
> > >
> > >      } else {
> > >
> > > +      if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData-
> > > >ApLoopMode != ApInHltLoop)) {
> > >
> > > +        //
> > >
> > > +        // CpuMpData->CpuData[0].VolatileRegisters is initialized
> > > + based on
> > > BSP environment,
> > >
> > > +        //   to initialize AP in InitConfig/ApInitReconfig path.
> >
> > 3.
> > 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.
> >
> > >
> > > +        // NOTE: IDTR.BASE stored in CpuMpData-
> > > >CpuData[0].VolatileRegisters points to a
> > >
> > > +        //   different IDT shared by all APs.
> > >
> > > +        //
> > >
> > > +        RestoreVolatileRegisters (&CpuMpData-
> > >CpuData[0].VolatileRegisters,
> > > FALSE);
> >
> > 4. Is it possible to combine this function call with the restoration code
> below?
> >
> 

I move the code into the else of " if (CpuMpData->ApLoopMode == ApInHltLoop)", it's truly better than original.
Please see my v2 change.

Thanks,
Eric

> New logic changes code flow when CpuMpData->InitFlag == ApInitReconfig.
> In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb, but
> new code only does CpuFlushTlb.
> Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi?  I prefer to not
> change this code flow.

> 
> Thanks,
> Eric
> 
> >       if (CpuMpData->InitFlag == ApInitReconfig) {
> >         //
> >         // 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.
> >         //
> >         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 ();
> >         }
> >       }
> > >
> > > +      }
> > >
> > > +
> > >
> > >        //
> > >
> > >        // Execute AP function if AP is ready
> > >
> > >        //
> > >
> > > --
> > > 2.23.0.windows.1
> 
> 

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

[Ni, Ray]

> 
> New logic changes code flow when CpuMpData->InitFlag == ApInitReconfig.
> In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb, but
> new code only does CpuFlushTlb.
> Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi?  I prefer to not
> change this code flow.

In ApInitReconfig path, CPU is waken up from INIT-SIPI-SIPI, which means the
CR3 is set in waking up vector. CpuFlushTlb() is used here to invalidate the page
table cache, which is similar to setting CR3.