Hi Eric, (1) on the email address list of this patch set, there is a strange item:
00000000 16 64 65 76 65 6c 40 65 64 6b 32 2e 67 72 6f 75 |.de...@edk2.grou| 00000010 70 73 2e 69 6f |ps.io| The first character is U+0016, which seems to stand for SYNCHRONOUS IDLE <https://en.wikipedia.org/wiki/Synchronous_Idle>. I think it must have been a typo. (The set was otherwise posted correctly to the list -- the correct list address is present, too.) Pointing this out just for future postings. On 04/24/20 10:47, Dong, Eric wrote: > 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...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Chandana Kumar <chandana.c.ku...@intel.com> > Signed-off-by: Eric Dong <eric.d...@intel.com> > --- > V3: > Remove invalid save volatile registers process. Refine restore > volatile registers process. > > V2: > Enhance code to remove CpuMpData->ApLoopMode == ApInHltLoop check. > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 +++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 64a4c3546e..7fd757b428 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -686,18 +686,31 @@ ApWakeupFunction ( > WAKEUP_AP_SIGNAL, > 0 > ); > - if (CpuMpData->ApLoopMode == ApInHltLoop) { > - // > - // Restore AP's volatile registers saved > - // > - RestoreVolatileRegisters > (&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, TRUE); > - } else { > + > + if (CpuMpData->InitFlag == ApInitReconfig) { > // > - // 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. > + // 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. (2) git-am complains that the line above has a trailing space character. Please remove it before you push the set. > + // 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. > // > - CpuFlushTlb (); > + 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 (); > + } > } > > if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) == > CpuStateReady) { > @@ -1780,7 +1793,6 @@ MpInitLibInitialize ( > InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); > CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == > 0)? TRUE:FALSE; > CpuMpData->CpuData[Index].ApFunction = 0; > - CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, > &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); > } > } > > For the full series: Regression-tested-by: Laszlo Ersek <ler...@redhat.com> (For the testing, I used OVMF, with/without SMM, and normal boot and S3. With SMM, I also tested CPU hotplug. OVMF doesn't support "microcode patching", so maybe it didn't make sense to perform these regression-tests. But, "these patches should be harmless regarding <whatever>", are famous last words... So I rather wanted to regression-test them, under my use case.) Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58301): https://edk2.groups.io/g/devel/message/58301 Mute This Topic: https://groups.io/mt/73237635/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
