* [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure @ 2020-04-22 8:52 Dong, Eric 2020-04-22 8:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs Dong, Eric 0 siblings, 1 reply; 6+ messages in thread From: Dong, Eric @ 2020-04-22 8:52 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 This patch serial used to fix an ASSERT issue. Because AP can't find the CpuMpData through IDT, it raised the ASSERT. V2: 1. Enhance code comments. 2. Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check. Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Chandana Kumar <chandana.c.kumar@intel.com> Eric Dong (2): UefiCpuPkg/MpInitLib: Restore IDT context for APs. UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI. UefiCpuPkg/Library/MpInitLib/MpLib.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) -- 2.23.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. 2020-04-22 8:52 [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure Dong, Eric @ 2020-04-22 8:52 ` Dong, Eric 0 siblings, 0 replies; 6+ messages in thread From: Dong, Eric @ 2020-04-22 8:52 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 This patch fixes an assertion because AP can't find the CpuMpData. When AP is waken up through Init-Sipi-Sipi, AP's IDT should be restored to pre-allocated buffer so AP can get the CpuMpData through the IDT base address. Current code already has logic to handle this when CpuMpData-> InitFlag is ApInitConfig but misses the logic when CpuMpData->InitFlag is ApInitReconfig. This patch fixes this gap. Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Chandana Kumar <chandana.c.kumar@intel.com> Signed-off-by: Eric Dong <eric.dong@intel.com> --- V2: Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check. UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 64a4c3546e..2e87aa1f06 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -692,6 +692,16 @@ ApWakeupFunction ( // RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE); } else { + if (CpuMpData->InitFlag == ApInitReconfig) { + // + // Initialize AP volatile registers in ApInitReconfig path. + // ApInitReconfig happens when: + // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. + // 2. AP is initialized in DXE phase. + // + RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); + } + // // The CPU driver might not flush TLB for APs on spot after updating // page attributes. AP in mwait loop mode needs to take care of it when -- 2.23.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure. @ 2020-04-22 1:34 Dong, Eric 2020-04-22 1:34 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs Dong, Eric 0 siblings, 1 reply; 6+ messages in thread From: Dong, Eric @ 2020-04-22 1:34 UTC (permalink / raw) To: devel REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 This patch serial used to fix an ASSERT issue. Because AP can't find the CpuMpData through IDT, it raised the ASSERT. Eric Dong (2): UefiCpuPkg/MpInitLib: Restore IDT context for APs. UefiCpuPkg/MpInitLib: Avoid ApInitReconfig in PEI. UefiCpuPkg/Library/MpInitLib/MpLib.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) -- 2.23.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. 2020-04-22 1:34 [PATCH 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure Dong, Eric @ 2020-04-22 1:34 ` Dong, Eric 2020-04-22 7:54 ` Ni, Ray 0 siblings, 1 reply; 6+ messages in thread From: Dong, Eric @ 2020-04-22 1:34 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 This patch used to fix a ASSERT because AP can't find the CpuMpData. When AP been waked up through Init-Sipi-Sipi, the IDT should been restore to preallcated buffer to make it can get the CpuMpData through IDT base address. Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, AP will be wake up through Init-Sipi-Sipi. Current code already has logic to handle ApInitConfig, but miss the handler for ApInitReconfig. This patch fixes this gap. Cc: Ray Ni <ray.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Chandana Kumar <chandana.c.kumar@intel.com> Signed-off-by: Eric Dong <eric.dong@intel.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 64a4c3546e..ac7f92fd48 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -664,8 +664,9 @@ ApWakeupFunction ( BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); // // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment, - // to initialize AP in InitConfig path. - // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs. + // to initialize AP in InitConfig/ApInitReconfig path. + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a + // different IDT shared by all APs. // RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack); @@ -673,6 +674,16 @@ ApWakeupFunction ( InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting); } else { + if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData->ApLoopMode != ApInHltLoop)) { + // + // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment, + // to initialize AP in InitConfig/ApInitReconfig path. + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a + // different IDT shared by all APs. + // + RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); + } + // // Execute AP function if AP is ready // -- 2.23.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. 2020-04-22 1:34 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs Dong, Eric @ 2020-04-22 7:54 ` Ni, Ray 2020-04-22 8:22 ` Dong, Eric 0 siblings, 1 reply; 6+ messages in thread From: Ni, Ray @ 2020-04-22 7:54 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Chandana C Please check my comments in below. > -----Original Message----- > From: Dong, Eric <eric.dong@intel.com> > Sent: Wednesday, April 22, 2020 9:35 AM > To: devel@edk2.groups.io > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, > Chandana C <chandana.c.kumar@intel.com> > Subject: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 > > > > This patch used to fix a ASSERT because AP can't find the CpuMpData. 1. This patch fixes an assertion because AP can't find the CpuMpData. > > > When AP been waked up through Init-Sipi-Sipi, the IDT should > been restore to preallcated buffer to make it can get the > CpuMpData through IDT base address. > Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, > AP will be wake up through Init-Sipi-Sipi. Current code already > has logic to handle ApInitConfig, but miss the handler for > ApInitReconfig. This patch fixes this gap. 2. When AP is waken up through Init-Sipi-Sipi, AP's IDT should be restored to pre-allocated buffer so AP can get the CpuMpData through the IDT base address. Current code already has logic to handle this when CpuMpData-> InitFlag is ApInitConfig but misses the logic when CpuMpData->InitFlag is ApInitReconfig. This patch fixes this gap. > > Cc: Ray Ni <ray.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Chandana Kumar <chandana.c.kumar@intel.com> > Signed-off-by: Eric Dong <eric.dong@intel.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 64a4c3546e..ac7f92fd48 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -664,8 +664,9 @@ ApWakeupFunction ( > BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); > > // > > // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP > environment, > > - // to initialize AP in InitConfig path. > > - // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters > points to a different IDT shared by all APs. > > + // to initialize AP in InitConfig/ApInitReconfig path. > > + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters > points to a > > + // different IDT shared by all APs. > > // > > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, > FALSE); > > InitializeApData (CpuMpData, ProcessorNumber, BistData, > ApTopOfStack); > > @@ -673,6 +674,16 @@ ApWakeupFunction ( > > > InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo- > >NumApsExecuting); > > } else { > > + if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData- > >ApLoopMode != ApInHltLoop)) { > > + // > > + // CpuMpData->CpuData[0].VolatileRegisters is initialized based on > BSP environment, > > + // to initialize AP in InitConfig/ApInitReconfig path. 3. Initialize AP volatile registers in ApInitReconfig path. ApInitReconfig happens when: 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. 2. AP is initialized in DXE phase. > > + // NOTE: IDTR.BASE stored in CpuMpData- > >CpuData[0].VolatileRegisters points to a > > + // different IDT shared by all APs. > > + // > > + RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, > FALSE); 4. Is it possible to combine this function call with the restoration code below? if (CpuMpData->InitFlag == ApInitReconfig) { // // ApInitReconfig happens when: // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. // 2. AP is initialized in DXE phase. // In either case, use the volatile registers value derived from BSP. // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a // different IDT shared by all APs. // RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); } else { if (CpuMpData->ApLoopMode == ApInHltLoop) { // // Restore AP's volatile registers saved before AP is halted // RestoreVolatileRegisters (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE); } else { // // The CPU driver might not flush TLB for APs on spot after updating // page attributes. AP in mwait loop mode needs to take care of it when // woken up. // CpuFlushTlb (); } } > > + } > > + > > // > > // Execute AP function if AP is ready > > // > > -- > 2.23.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. 2020-04-22 7:54 ` Ni, Ray @ 2020-04-22 8:22 ` Dong, Eric 2020-04-22 11:26 ` Ni, Ray 0 siblings, 1 reply; 6+ messages in thread From: Dong, Eric @ 2020-04-22 8:22 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Chandana C > -----Original Message----- > From: Ni, Ray > Sent: Wednesday, April 22, 2020 3:54 PM > To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io > Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C > <chandana.c.kumar@intel.com> > Subject: RE: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. > > Please check my comments in below. > > > -----Original Message----- > > From: Dong, Eric <eric.dong@intel.com> > > Sent: Wednesday, April 22, 2020 9:35 AM > > To: devel@edk2.groups.io > > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; > > Kumar, Chandana C <chandana.c.kumar@intel.com> > > Subject: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2683 > > > > > > > > This patch used to fix a ASSERT because AP can't find the CpuMpData. > > 1. This patch fixes an assertion because AP can't find the CpuMpData. > > > > > > > When AP been waked up through Init-Sipi-Sipi, the IDT should been > > restore to preallcated buffer to make it can get the CpuMpData through > > IDT base address. > > Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, AP > > will be wake up through Init-Sipi-Sipi. Current code already has logic > > to handle ApInitConfig, but miss the handler for ApInitReconfig. This > > patch fixes this gap. > > 2. When AP is waken up through Init-Sipi-Sipi, AP's IDT should be restored to > pre-allocated buffer so AP can get the CpuMpData through the IDT base > address. > Current code already has logic to handle this when CpuMpData-> InitFlag is > ApInitConfig but misses the logic when CpuMpData->InitFlag is > ApInitReconfig. > This patch fixes this gap. > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Chandana Kumar <chandana.c.kumar@intel.com> > > Signed-off-by: Eric Dong <eric.dong@intel.com> > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 64a4c3546e..ac7f92fd48 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -664,8 +664,9 @@ ApWakeupFunction ( > > BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); > > > > // > > > > // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP > > environment, > > > > - // to initialize AP in InitConfig path. > > > > - // NOTE: IDTR.BASE stored in CpuMpData- > >CpuData[0].VolatileRegisters > > points to a different IDT shared by all APs. > > > > + // to initialize AP in InitConfig/ApInitReconfig path. > > > > + // NOTE: IDTR.BASE stored in CpuMpData- > >CpuData[0].VolatileRegisters > > points to a > > > > + // different IDT shared by all APs. > > > > // > > > > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, > > FALSE); > > > > InitializeApData (CpuMpData, ProcessorNumber, BistData, > > ApTopOfStack); > > > > @@ -673,6 +674,16 @@ ApWakeupFunction ( > > > > > > InterlockedDecrement ((UINT32 *) &CpuMpData- > >MpCpuExchangeInfo- > > >NumApsExecuting); > > > > } else { > > > > + if ((CpuMpData->InitFlag == ApInitReconfig) && (CpuMpData- > > >ApLoopMode != ApInHltLoop)) { > > > > + // > > > > + // CpuMpData->CpuData[0].VolatileRegisters is initialized based on > > BSP environment, > > > > + // to initialize AP in InitConfig/ApInitReconfig path. > > 3. > Initialize AP volatile registers in ApInitReconfig path. > ApInitReconfig happens when: > 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. > 2. AP is initialized in DXE phase. > > > > > + // NOTE: IDTR.BASE stored in CpuMpData- > > >CpuData[0].VolatileRegisters points to a > > > > + // different IDT shared by all APs. > > > > + // > > > > + RestoreVolatileRegisters (&CpuMpData- > >CpuData[0].VolatileRegisters, > > FALSE); > > 4. Is it possible to combine this function call with the restoration code below? > New logic changes code flow when CpuMpData->InitFlag == ApInitReconfig. In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb, but new code only does CpuFlushTlb. Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi? I prefer to not change this code flow. Thanks, Eric > if (CpuMpData->InitFlag == ApInitReconfig) { > // > // ApInitReconfig happens when: > // 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. > // 2. AP is initialized in DXE phase. > // In either case, use the volatile registers value derived from BSP. > // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters > points to a > // different IDT shared by all APs. > // > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, > FALSE); > } else { > if (CpuMpData->ApLoopMode == ApInHltLoop) { > // > // Restore AP's volatile registers saved before AP is halted > // > RestoreVolatileRegisters (&CpuMpData- > >CpuData[ProcessorNumber].VolatileRegisters, TRUE); > } else { > // > // The CPU driver might not flush TLB for APs on spot after updating > // page attributes. AP in mwait loop mode needs to take care of it when > // woken up. > // > CpuFlushTlb (); > } > } > > > > + } > > > > + > > > > // > > > > // Execute AP function if AP is ready > > > > // > > > > -- > > 2.23.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. 2020-04-22 8:22 ` Dong, Eric @ 2020-04-22 11:26 ` Ni, Ray 0 siblings, 0 replies; 6+ messages in thread From: Ni, Ray @ 2020-04-22 11:26 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Chandana C > > New logic changes code flow when CpuMpData->InitFlag == ApInitReconfig. > In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb, but > new code only does CpuFlushTlb. > Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi? I prefer to not > change this code flow. In ApInitReconfig path, CPU is waken up from INIT-SIPI-SIPI, which means the CR3 is set in waking up vector. CpuFlushTlb() is used here to invalidate the page table cache, which is similar to setting CR3. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-22 11:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-22 8:52 [PATCH v2 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure Dong, Eric 2020-04-22 8:52 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs Dong, Eric -- strict thread matches above, loose matches on Subject: below -- 2020-04-22 1:34 [PATCH 0/2] UefiCpuPkg/MpInitLib: Fix ASSERT in AP procedure Dong, Eric 2020-04-22 1:34 ` [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs Dong, Eric 2020-04-22 7:54 ` Ni, Ray 2020-04-22 8:22 ` Dong, Eric 2020-04-22 11:26 ` Ni, Ray
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox