* [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 @ 2023-10-25 10:07 Yuanhao Xie 2023-10-25 10:36 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Yuanhao Xie @ 2023-10-25 10:07 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Eric Dong, Rahul Kumar, Tom Lendacky, Yuanhao Xie The purpose is to fix an assertion with applying the following patch series: UefiCpuPkg: Refactor the logic for placing APs in HltLoop. UefiCpuPkg: Refactor the logic for placing APs in Mwait/Runloop. UefiCpuPkg: Create MpHandOff. UefiCpuPkg: ApWakeupFunction directly use CpuMpData. UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence. UefiCpuPkg: Decouple the SEV-ES functionality. The assertion arises from a timing discrepancy between BSP completing its startup signal check and the APs incrementing the FinishedCount. Instead of assertion, use while loop to waits until all the APs have incremented the FinishedCount. Cc: Ray Ni <ray.ni@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 6f1456cfe1..9a6ec5db5c 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -913,8 +913,8 @@ DxeApEntryPoint ( UINTN ProcessorNumber; GetProcessorNumber (CpuMpData, &ProcessorNumber); - InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); + InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); PlaceAPInMwaitLoopOrRunLoop ( CpuMpData->ApLoopMode, CpuMpData->CpuData[ProcessorNumber].StartupApSignal, @@ -2201,7 +2201,12 @@ MpInitLibInitialize ( // looping process there. // SwitchApContext (MpHandOff); - ASSERT (CpuMpData->FinishedCount == (CpuMpData->CpuCount - 1)); + // + // Wait for all APs finished initialization + // + while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { + CpuPause (); + } // // Set Apstate as Idle, otherwise Aps cannot be waken-up again. -- 2.36.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110028): https://edk2.groups.io/g/devel/message/110028 Mute This Topic: https://groups.io/mt/102174874/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 2023-10-25 10:07 [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 Yuanhao Xie @ 2023-10-25 10:36 ` Laszlo Ersek 2023-10-25 11:52 ` Yuanhao Xie 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2023-10-25 10:36 UTC (permalink / raw) To: devel, yuanhao.xie; +Cc: Ray Ni, Eric Dong, Rahul Kumar, Tom Lendacky On 10/25/23 12:07, Yuanhao Xie wrote: > The purpose is to fix an assertion with applying the following patch > series: > > UefiCpuPkg: Refactor the logic for placing APs in HltLoop. > UefiCpuPkg: Refactor the logic for placing APs in Mwait/Runloop. > UefiCpuPkg: Create MpHandOff. > UefiCpuPkg: ApWakeupFunction directly use CpuMpData. > UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence. > UefiCpuPkg: Decouple the SEV-ES functionality. > > The assertion arises from a timing discrepancy between BSP completing > its startup signal check and the APs incrementing the FinishedCount. > > Instead of assertion, use while loop to waits until all the APs have > incremented the FinishedCount. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 6f1456cfe1..9a6ec5db5c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -913,8 +913,8 @@ DxeApEntryPoint ( > UINTN ProcessorNumber; > > GetProcessorNumber (CpuMpData, &ProcessorNumber); > - InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); > + InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > PlaceAPInMwaitLoopOrRunLoop ( > CpuMpData->ApLoopMode, > CpuMpData->CpuData[ProcessorNumber].StartupApSignal, > @@ -2201,7 +2201,12 @@ MpInitLibInitialize ( > // looping process there. > // > SwitchApContext (MpHandOff); > - ASSERT (CpuMpData->FinishedCount == (CpuMpData->CpuCount - 1)); > + // > + // Wait for all APs finished initialization > + // > + while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > + CpuPause (); > + } > > // > // Set Apstate as Idle, otherwise Aps cannot be waken-up again. The second hunk makes sense. SwitchApContext() returns after all APs are "live", but that doesn't guarantee that all APs are also "done" by the time the BSP reaches the FinishedCount check. (1) What is the justification for the first hunk? I understand that we may want to report "finished" from an AP as late as possible. Is that the only (general) reason for the first hunk, or is there a specific reason? Either way, the reason for the first hunk should be documented in the commit message. (2) The subject line should be UefiCpuPkg/MpInitLib: wait for all APs to finish initialization because - we should also state the component name within UefiCpuPkg, - "remove assert" is just a natural language expression of the direct code change, so it's not useful; what's useful is naming the *goal* that we're achieving. (3) I suggest appending Fixes: 964a4f032dcd to the commit message, just above your Signed-off-by. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110034): https://edk2.groups.io/g/devel/message/110034 Mute This Topic: https://groups.io/mt/102174874/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 2023-10-25 10:36 ` Laszlo Ersek @ 2023-10-25 11:52 ` Yuanhao Xie 0 siblings, 0 replies; 3+ messages in thread From: Yuanhao Xie @ 2023-10-25 11:52 UTC (permalink / raw) To: Laszlo Ersek Cc: Ni, Ray, Dong, Eric, Kumar, Rahul R, Tom Lendacky, devel@edk2.groups.io Hi Laszlo, Thanks a lot for the feedbacks and quick response. I updated the commit message, please check v3. The changes includes: -An explanation for the first chunk, which aims to report the incrementation of "finished" for as late as possible. -Renamed the subject line and added the module. -Added "Fixes: 964a4f032dcd" to the commit message. Regards Yuanhao -----Original Message----- From: Laszlo Ersek <lersek@redhat.com> Sent: Wednesday, October 25, 2023 6:37 PM To: devel@edk2.groups.io; Xie, Yuanhao <yuanhao.xie@intel.com> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Tom Lendacky <thomas.lendacky@amd.com> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 On 10/25/23 12:07, Yuanhao Xie wrote: > The purpose is to fix an assertion with applying the following patch > series: > > UefiCpuPkg: Refactor the logic for placing APs in HltLoop. > UefiCpuPkg: Refactor the logic for placing APs in Mwait/Runloop. > UefiCpuPkg: Create MpHandOff. > UefiCpuPkg: ApWakeupFunction directly use CpuMpData. > UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence. > UefiCpuPkg: Decouple the SEV-ES functionality. > > The assertion arises from a timing discrepancy between BSP completing > its startup signal check and the APs incrementing the FinishedCount. > > Instead of assertion, use while loop to waits until all the APs have > incremented the FinishedCount. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 6f1456cfe1..9a6ec5db5c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -913,8 +913,8 @@ DxeApEntryPoint ( > UINTN ProcessorNumber; > > GetProcessorNumber (CpuMpData, &ProcessorNumber); > - InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, > FALSE); > + InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > PlaceAPInMwaitLoopOrRunLoop ( > CpuMpData->ApLoopMode, > CpuMpData->CpuData[ProcessorNumber].StartupApSignal, > @@ -2201,7 +2201,12 @@ MpInitLibInitialize ( > // looping process there. > // > SwitchApContext (MpHandOff); > - ASSERT (CpuMpData->FinishedCount == (CpuMpData->CpuCount - 1)); > + // > + // Wait for all APs finished initialization > + // > + while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > + CpuPause (); > + } > > // > // Set Apstate as Idle, otherwise Aps cannot be waken-up again. The second hunk makes sense. SwitchApContext() returns after all APs are "live", but that doesn't guarantee that all APs are also "done" by the time the BSP reaches the FinishedCount check. (1) What is the justification for the first hunk? I understand that we may want to report "finished" from an AP as late as possible. Is that the only (general) reason for the first hunk, or is there a specific reason? Either way, the reason for the first hunk should be documented in the commit message. (2) The subject line should be UefiCpuPkg/MpInitLib: wait for all APs to finish initialization because - we should also state the component name within UefiCpuPkg, - "remove assert" is just a natural language expression of the direct code change, so it's not useful; what's useful is naming the *goal* that we're achieving. (3) I suggest appending Fixes: 964a4f032dcd to the commit message, just above your Signed-off-by. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110053): https://edk2.groups.io/g/devel/message/110053 Mute This Topic: https://groups.io/mt/102174874/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-25 11:52 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-25 10:07 [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 Yuanhao Xie 2023-10-25 10:36 ` Laszlo Ersek 2023-10-25 11:52 ` Yuanhao Xie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox