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

[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

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: [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 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 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

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.