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

["Ni, Ray" <ray.ni@intel.com>]

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