> -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, April 29, 2020 6:59 PM > To: devel@edk2.groups.io; Dong, Eric <eric.d...@intel.com> > Cc: Ni, Ray <ray...@intel.com>; Kumar, Chandana C > <chandana.c.ku...@intel.com> > Subject: Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT > context for APs. > > 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. >
I copied the mail address from outlook, maybe some invisible characters caused this error. > (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. Got it. Will remove it when merge the code. > > > + // 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 for your test! Very appreciate your supports. > > Thanks, > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58304): https://edk2.groups.io/g/devel/message/58304 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] -=-=-=-=-=-=-=-=-=-=-=-
