[edk2-devel] [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 at redhat.com]
> Sent: Friday, January 17, 2020 8:04 PM
> To: Wu, Hao A; devel at 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Michael D Kinney <michael.d.kinney at intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu at intel.com>
> > Reviewed-by: Ray Ni <ray.ni at 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 at redhat.com>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53384): https://edk2.groups.io/g/devel/message/53384
Mute This Topic: https://groups.io/mt/69841061/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-