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

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

[Wu, Hao A]

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;

Under some cases (e.g. when variable OldCpuMpData is not NULL, which means
function CollectProcessorCount() will not be called), this 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>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 6ec9b172b8..17e19395f2 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1775,11 +1775,12 @@ 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);
     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

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> A
> Sent: Friday, January 17, 2020 2:57 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [edk2-devel] [PATCH v1] 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;
> 
> Under some cases (e.g. when variable OldCpuMpData is not NULL, which
> means
> function CollectProcessorCount() will not be called), this 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>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..17e19395f2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1775,11 +1775,12 @@ 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);
>      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
> 
> 
> 

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

[Laszlo Ersek]

On 01/17/20 07:56, Hao A Wu wrote:
> 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;
> 
> Under some cases (e.g. when variable OldCpuMpData is not NULL, which means
> function CollectProcessorCount() will not be called), this 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>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..17e19395f2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1775,11 +1775,12 @@ 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);
>      while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
>        CpuPause ();
>      }
> -
> +    CpuMpData->InitFlag = ApInitDone;
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
>      }
> 

It looks reasonable to me, but I was away while patch

"UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches"

was being reviewed, so I can't really say.

Can you explain (in the commit message) *why* commit d786a17232 removed
these InitFlag assignments? I've now read the commit message on
d786a17232, and it's not obvious to me.


Also, it would be nice to reinstate the following comment:

//
// Wait for all APs finish initialization
//

just before the "while" statement.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, January 17, 2020 4:42 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Ni, Ray; Kinney, Michael D
> Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible
> uninitialized 'InitFlag' field
> 
> On 01/17/20 07:56, Hao A Wu wrote:
> > 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;
> >
> > Under some cases (e.g. when variable OldCpuMpData is not NULL, which
> means
> > function CollectProcessorCount() will not be called), this 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>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 6ec9b172b8..17e19395f2 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -1775,11 +1775,12 @@ 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);
> >      while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
> >        CpuPause ();
> >      }
> > -
> > +    CpuMpData->InitFlag = ApInitDone;
> >      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> >        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
> >      }
> >
> 
> It looks reasonable to me, but I was away while patch
> 
> "UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches"
> 
> was being reviewed, so I can't really say.
> 
> Can you explain (in the commit message) *why* commit d786a17232
> removed
> these InitFlag assignments? I've now read the commit message on
> d786a17232, and it's not obvious to me.
> 

Sure.
I will update the commit message to add the information for such removal.

> 
> Also, it would be nice to reinstate the following comment:
> 
> //
> // Wait for all APs finish initialization
> //
> 
> just before the "while" statement.


Yes, I will add back this comment in the next version of patch.

Best Regards,
Hao Wu


> 
> Thanks
> Laszlo
> 
> 
>