UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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.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>
---
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);
}
--
2.12.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53344): https://edk2.groups.io/g/devel/message/53344
Mute This Topic: https://groups.io/mt/69838817/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.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> > --- > 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----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.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> > > --- > > 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao > A > Sent: Friday, January 17, 2020 2:57 PM > To: devel@edk2.groups.io > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael > D <michael.d.kinney@intel.com> > Subject: [edk2-devel] [PATCH v1] 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; > > 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.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> > --- > 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); > } > -- > 2.12.0.windows.1 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53351): https://edk2.groups.io/g/devel/message/53351 Mute This Topic: https://groups.io/mt/69838817/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.