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

["Laszlo Ersek" <lersek@redhat.com>]

On 04/22/20 11:01, Dong, Eric wrote:
> 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:
> 1. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check.
> 
>  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);
>      }
> 

Hm. The only feedback I could provide for this patch is a regression
test result. However, seeing how Ray has outstanding questions for this
patch, I think I'll delay my testing to the next version (or do it later
for this version, if Ray is ultimately OK with it).

Thanks
Laszlo