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

[PATCH v2] 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;

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>
---
 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

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

[Wu, Hao A]

> -----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

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

[Laszlo Ersek]

On 01/17/20 12:35, 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;
> 
> 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>
> ---
>  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);
>      }
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

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

[Wu, Hao A]

Thanks all,

Patch has been pushed via commit 18fcb37598.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, January 17, 2020 8:04 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Ni, Ray; Kinney, Michael D
> Subject: Re: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized
> 'InitFlag' field
> 
> On 01/17/20 12:35, 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;
> >
> > 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>
> > ---
> >  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);
> >      }
> >
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>