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

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

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