> -----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.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> > > --- > > 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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53354): https://edk2.groups.io/g/devel/message/53354 Mute This Topic: https://groups.io/mt/69838817/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
