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. Also, it would be nice to reinstate the following comment: // // Wait for all APs finish initialization // just before the "while" statement. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53353): https://edk2.groups.io/g/devel/message/53353 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] -=-=-=-=-=-=-=-=-=-=-=-
