Re: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, January 17, 2020 7:35 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A; Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D
> Subject: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized
> 'InitFlag' field
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2474
> 
> Previous commit d786a17232:
> UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
> 
> Removed the below assignments for the 'InitFlag' field of CPU_MP_DATA
> structure in function MpInitLibInitialize() when APs are waken up to do
> some initialize sync:
> 
> CpuMpData->InitFlag  = ApInitReconfig;
> ...
> CpuMpData->InitFlag = ApInitDone;
> 
> The above commit mistakenly assumed the 'InitFlag' field will have a value
> of 'ApInitDone' when the APs have been successfully waken up before. And
> since there is no explicit comparision for the 'InitFlag' field with the
> 'ApInitReconfig' value. The commit removed those assignments.
> 
> However, under some cases (e.g. when variable OldCpuMpData is not NULL,
> which means function CollectProcessorCount() will not be called), removing
> the above assignments will left the 'InitFlag' field being uninitialized
> with a value of 0, which is a invalid value for the type of 'InitFlag'
> (AP_INIT_STATE).
> 
> It may potentially cause the WakeUpAP() function to run some unnecessary
> codes when the APs have been successfully waken up before:
> 
>   if (CpuMpData->WakeUpByInitSipiSipi ||
>       CpuMpData->InitFlag   != ApInitDone) {
>     ResetVectorRequired = TRUE;
>     AllocateResetVector (CpuMpData);
>     FillExchangeInfoData (CpuMpData);
>     SaveLocalApicTimerSetting (CpuMpData);
>   }
> 
> This commit will address the above-mentioned issue.
> 
> Test done:
> * OS boot on a real platform with multi processors
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>


Since there are only comments and commit message changes compared with V1 patch,
I keep the R-b tag from Ray. Please help to raise if there is concern with this.

Best Regards,
Hao Wu


> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..855d37ba3e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1775,11 +1775,15 @@ MpInitLibInitialize (
>    // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
>    //
>    if (CpuMpData->CpuCount > 1) {
> +    CpuMpData->InitFlag = ApInitReconfig;
>      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> +    //
> +    // Wait for all APs finished initialization
> +    //
>      while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
>        CpuPause ();
>      }
> -
> +    CpuMpData->InitFlag = ApInitDone;
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
>      }
> --
> 2.12.0.windows.1