Thanks all, Patch has been pushed via commit 18fcb37598.
Best Regards, Hao Wu > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@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.d...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Signed-off-by: Hao A Wu <hao.a...@intel.com> > > Reviewed-by: Ray Ni <ray...@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 <ler...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
