Jordan, Sending NMI to wake up AP had been discussed later year. It prevents platform to provide own real NMI handler.
For current implementation, after finished AP task, AP will check if it is in Idle state or not. If yes, place itself into Sleep state. On most cast, AP will place itself into Sleep state just after no more than 2 times loop. But to handle such seldom case (improve performance), MP service need to check AP state to decide how to wakeup it. It introduces some complex sync implementation between BSP/Aps. You could refer to my fix on this sync. So, I think this solution is expensive than the benefit we got. Thanks! Jeff -----Original Message----- From: Justen, Jordan L Sent: Wednesday, October 28, 2015 3:51 PM To: Fan, Jeff; Laszlo Ersek; [email protected] Cc: Chen Fan; Kinney, Michael D Subject: RE: [edk2] [PATCH] UefiCpuPkg/CpuDxe: fix ExitBootServices() callback in the presence of SMIs On 2015-10-27 22:24:08, Fan, Jeff wrote: > Jordan, > > Sending Fixed Interrupt IPI to wakeup AP required set EFLAGS.IF bit on > AP from my experience. (Please correct me if I am wrong.) As a result, > system timer interrupt maybe handled by AP instead of BSP. The APs would have to be set up to use a different "nop" interrupt handler table. So, the APs wouldn't run any interrupt handling code. I don't think this would interfere with the BSP getting the interrupts and handling them. Another idea to investigate is sending an NMI to wake the AP. > Most of cases, AP will be in hlt state and require INIT-SIPI-SIPI to > be wakeup and work on many real platform. I don’t think 1S is > reasonable data. (Do you remember where 1S comes from?) See Laszlo's patch "OvmfPkg: increase MP services startup timeout" > Using MP service in my original patch need to take care some MP > service local sync data. That's why I think Laszlo's patch is simpler > and better. I think Laszlo's patch is fine. I think he should prepare it, get it reviewed, and then add it to his SMM series, and then it'll be ready if nothing better comes along. > Another point: Dynamically to change the AP state (CpuSleep and > software loop) makes the code complex. I find some dead lock issue > between BSP/AP, I will send the fix later. Long term, I also suggest > to drop this logic. Anyway, I need others' input on it before dropping > it. Could you give an overview of what your idea would be? I don't agree that the current code is too complex. But, I'm sure there could be bugs. :) -Jordan > -----Original Message----- > From: Justen, Jordan L > Sent: Wednesday, October 28, 2015 6:55 AM > To: Laszlo Ersek; [email protected] > Cc: Chen Fan; Kinney, Michael D; Fan, Jeff > Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: fix ExitBootServices() > callback in the presence of SMIs > > On 2015-10-27 05:20:12, Laszlo Ersek wrote: > > On 10/27/15 07:48, Jordan Justen wrote: > > > What about adding this code to mStartupCodeTemplate and sending > > > the APs there using the MP services? > > > > That was the idea in Jeff's earlier series, but it comes with a > > number of complications. The main one is that the HLT loop is > > entered as a "normal" StartupAllAPs() callback function. That > > necessitates tearing down parts of the environment that the MP > > services implementation builds up carefully at initialization. Other > > parts of that laborously constructed environment are plain irrelevant. > > Hmm. I assumed that starting an AP task should be much lower latency than > running INIT-SIPI-SIPI. Yet, examining the current code, it looks like we > generally call CpuSleep on the AP, and we only currently support resuming > from this state with an INIT-SIPI-SIPI. > > I think the APs could be woken from CPU sleep by an interrupt IPI, which > should be a lot less traumatic than INIT-SIPI-SIPI. This probably would > require a stub interrupt table to be setup for the APs. > > With that in place, we could then send the APs to the cli-hlt loop much > faster than via INIT-SIPI-SIPI. > > (Especially given that apparently this can take up to 1 second?! :) > > Another reason it could be good to work with the code is that we > probably should put the code into a shutdown mode, so if future MP > service requests come in, the MP services code doesn't try to restart > the APs. (Ie, it should just return an immediate error.) > > > Instead, this solution takes manual mode switching, paging, and > > stacks out of the picture, which is a good thing. The INIT-SIPI-SIPI > > sequence places the AP in real mode immediately, which is perfect > > for the minimal stub that we'd like the APs to run. > > I didn't consider the potential problem with leaving paging enabled, but if > we get the processor back into IA32 mode, we can leave the APs running > anywhere in the low 32-bit address space. It would be nice to not to leave > any memory allocated below 1MB if we can avoid it. > > > > Regardless of that, I think the C-based-asm would better fit in > > > ApStartup.c. > > > > I can move the code of course, but to me it doesn't seem overly > > justified. The C-based ASM in this question is only used by the > > ExitBootServices() callback, so (as you can see it in the patch) I > > could keep the template STATIC. If I move it to ApStartup.c, I'll > > have to make it extern (or else do the copying outside of the > > callback). I feel that this isn't overly justified, but if you insist, I > > can do it. > > Ok. This is fine. I am not a fan of the C-base-asm, despite the fact > that I wrote it, and I still think it is better than separate assembly > files. (Having only NASM be used in EDK II might change my mind.) > > I guess I just don't want to see it proliferate too much. :) > > > > Why not just a cli-hlt loop? > > > > Because if the ExitBootServices() callback doesn't wait until all > > APs abort and abandon the MP services idle loop -- or worse, an > > assigned MP services job that is still running -- then the BSP could > > immediately return from ExitBootServices(), and proceed to third party code > > that: > > - overwrites the page that used to host the idle loop, or > > - raises an SMI (via a variable service call for example) and then the > > APs might return to the old idle loop upon seeing RSM. > > > > The synchronization is in place so that the callback returns knowing > > that all APs have abandoned the old idle loop (or whatever job they > > were running). > > We could verify all APs have set CpuStateBusy. By that time it seems > extremely likely that all APs will be running in the runtime safe code. > > -Jordan > > > > > > Maybe if the MP services were used then the separate counter goes > > > away. > > > > I don't want it to go away. :) Plus, as far as I can see in > > StartupAllAPs() [UefiCpuPkg/CpuDxe/CpuMp.c], it doesn't wait for the > > *initiation* of the user procedure in any case. It can wait for the > > completion, but that's not good to us now -- our procedure is an > > infinite loop; it will not complete. All we care about is feedback > > that the old code has been abandoned completely. > > > > > How about 0 rather than 0x1234? > > > > I wanted something that would reflect the little endian encoding in > > a pure hexdump. I don't feel strongly about this though. > > > > > Can you provide a nasm source block in a comment like in ApStartup.c? > > > > Sure. > > > > Please follow up one by one on the points where I disagreed, so I > > can send v2. The two parts that I do feel strongly about are (a) not > > using MP services itself for this, (b) the counter / synchronization. > > > > Thanks > > Laszl > > > > > > > > -Jordan > > > > > > On 2015-10-26 14:33:09, Laszlo Ersek wrote: > > >> Our current callback puts the APs in wait-for-SIPI state, by > > >> sending them an INIT IPI. This works fine as long as the APs are > > >> not woken between > > >> ExitBootServices() and the time the runtime OS boots the APs. > > >> > > >> However, if a platform includes PiSmmCpuDxeSmm, and bases the > > >> runtime UEFI services (like the variable services) on SMM and > > >> SMIs, then two problem scenarios can be experienced: > > >> > > >> (a) On virtual platforms, the APs can be (spuriously?) woken by SMIs, > > >> between ExitBootServices() and the runtime OS's AP boot; for example > > >> by variable service calls. In this case the APs have no code to > > >> continue executing after the RSM instruction, which causes them to > > >> re-set. > > >> > > >> (b) On physical platforms, the APs might not wake from the wait-for-SIPI > > >> state at all, causing performance (or functional) problems with > > >> PiSmmCpuDxeSmm, dependent on the BSP/AP sync mode configured therein. > > >> > > >> Implement the following logic instead: > > >> > > >> - If there is no AP, there's no need for an ExitBootServices() callback. > > >> > > >> - Otherwise, reboot all APs with a minimal real-mode stub. Each AP shall > > >> bump a shared counter atomically at first, then spiral into an infinite > > >> HLT loop. This occurs regardless of what each AP has been doing at that > > >> moment. > > >> > > >> - APs can wake from HLT due to an SMI easily. > > >> > > >> - The BSP in the ExitBootServices() callback will wait until all APs have > > >> moved on to the stub. > > >> > > >> - Since we exit boot services time now with the original MP services > > >> startup code abandoned by all APs, we can even allocate that startup > > >> code in Boot Services Code type memory; only the new stub has to be > > >> AcpiNVS. > > >> > > >> Cc: Chen Fan <[email protected]> > > >> Cc: Jeff Fan <[email protected]> > > >> Cc: Jordan Justen <[email protected]> > > >> Cc: Michael Kinney <[email protected]> > > >> Contributed-under: TianoCore Contribution Agreement 1.0 > > >> Signed-off-by: Laszlo Ersek <[email protected]> > > >> --- > > >> > > >> Notes: > > >> So this would be option (2) from > > >> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/3357/focus=3403>. > > >> With this patch in place, the AP reboots I had seen on TCG are gone. > > >> > > >> UefiCpuPkg/CpuDxe/ApStartup.c | 2 +- > > >> UefiCpuPkg/CpuDxe/CpuMp.c | 84 > > >> +++++++++++++++++++++++++++++++++++-------- > > >> 2 files changed, 71 insertions(+), 15 deletions(-) > > >> > > >> diff --git a/UefiCpuPkg/CpuDxe/ApStartup.c > > >> b/UefiCpuPkg/CpuDxe/ApStartup.c index 78fb26f..b8d736f 100644 > > >> --- a/UefiCpuPkg/CpuDxe/ApStartup.c > > >> +++ b/UefiCpuPkg/CpuDxe/ApStartup.c > > >> @@ -382,7 +382,7 @@ PrepareAPStartupCode ( > > >> StartAddress = BASE_1MB; > > >> Status = gBS->AllocatePages ( > > >> AllocateMaxAddress, > > >> - EfiACPIMemoryNVS, > > >> + EfiBootServicesCode, > > >> EFI_SIZE_TO_PAGES (sizeof (*StartupCode)), > > >> &StartAddress > > >> ); > > >> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c > > >> b/UefiCpuPkg/CpuDxe/CpuMp.c index 04c2f1f..98976a1 100644 > > >> --- a/UefiCpuPkg/CpuDxe/CpuMp.c > > >> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c > > >> @@ -30,6 +30,33 @@ VOID *mApStackStart = 0; volatile BOOLEAN > > >> mAPsAlreadyInitFinished = FALSE; volatile BOOLEAN > > >> mStopCheckAllAPsStatus = TRUE; > > >> > > >> +#pragma pack (1) > > >> +typedef struct { > > >> + UINT8 MovAx; > > >> + UINT16 AxValue; > > >> + UINT8 MovDsAx[2]; > > >> + UINT8 LockedWordCounterIncrement[6]; > > >> + > > >> + UINT8 Cli; > > >> + UINT8 Hlt; > > >> + UINT8 JmpToCli[2]; > > >> + > > >> + UINT16 WordCounter; > > >> +} CPU_AP_HLT_LOOP; > > >> +#pragma pack () > > >> + > > >> +STATIC CONST CPU_AP_HLT_LOOP mCpuApHltLoopTemplate = { > > >> + 0xB8, 0x1234, // mov ax,0x1234 > > >> + { 0x8E, 0xD8 }, // mov ds,ax > > >> + { 0xF0, 0x3E, 0xFF, 0x06, 0x0F, 0x00 }, // lock inc word > > >> +[ds:0xf] > > >> + > > >> + 0xFA, // cli @ offset 0xb > > >> + 0xF4, // hlt > > >> + { 0xEB, 0xFC }, // jmp short 0xb > > >> + > > >> + 0x0000 // WordCounter @ offset 0xf > > >> +}; > > >> + > > >> EFI_MP_SERVICES_PROTOCOL mMpServicesTemplate = { > > >> GetNumberOfProcessors, > > >> GetProcessorInfo, > > >> @@ -1659,11 +1686,31 @@ ExitBootServicesCallback ( > > >> IN VOID *Context > > >> ) > > >> { > > >> + CPU_AP_HLT_LOOP *CpuApHltLoop; > > >> + UINT16 ApCount; > > >> + > > >> + CpuApHltLoop = Context; > > >> + CopyMem (CpuApHltLoop, &mCpuApHltLoopTemplate, sizeof > > >> + mCpuApHltLoopTemplate); > > >> + > > >> // > > >> - // Avoid APs access invalid buff datas which allocated by > > >> BootServices, > > >> - // so we send INIT IPI to APs to let them wait for SIPI state. > > >> + // Point DS via AX to the page where the routine has been allocated. > > >> // > > >> - SendInitIpiAllExcludingSelf (); > > >> + CpuApHltLoop->AxValue = (UINT16)((UINTN)Context >> 4); DEBUG > > >> + ((EFI_D_VERBOSE, "%a: %a: HLT loop routine at %04x:%04x\n", > > >> + gEfiCallerBaseName, __FUNCTION__, CpuApHltLoop->AxValue, > > >> + 0)); > > >> + > > >> + // > > >> + // Force all APs to execute the HLT loop. Wait until they all enter > > >> the code. > > >> + // > > >> + SendInitSipiSipiAllExcludingSelf ((UINT32)(UINTN)Context); > > >> + > > >> + ApCount = (UINT16)(mMpSystemData.NumberOfProcessors - 1); > > >> + while > > >> + (InterlockedCompareExchange16 ( > > >> + &CpuApHltLoop->WordCounter, > > >> + ApCount, > > >> + ApCount) != ApCount) { > > >> + CpuPause(); > > >> + } > > >> } > > >> > > >> /** > > >> @@ -1691,9 +1738,10 @@ InitializeMpSupport ( > > >> VOID > > >> ) > > >> { > > >> - EFI_STATUS Status; > > >> - MTRR_SETTINGS MtrrSettings; > > >> - UINTN Timeout; > > >> + EFI_STATUS Status; > > >> + MTRR_SETTINGS MtrrSettings; > > >> + UINTN Timeout; > > >> + EFI_PHYSICAL_ADDRESS CpuApHltLoopAddress; > > >> > > >> gMaxLogicalProcessorNumber = (UINTN) PcdGet32 > > >> (PcdCpuMaxLogicalProcessorNumber); > > >> if (gMaxLogicalProcessorNumber < 1) { @@ -1795,12 +1843,20 @@ > > >> InitializeMpSupport ( > > >> } > > >> } > > >> > > >> - Status = gBS->CreateEvent ( > > >> - EVT_SIGNAL_EXIT_BOOT_SERVICES, > > >> - TPL_CALLBACK, > > >> - ExitBootServicesCallback, > > >> - NULL, > > >> - &mExitBootServicesEvent > > >> - ); > > >> - ASSERT_EFI_ERROR (Status); > > >> + if (mMpSystemData.NumberOfProcessors > 1) { > > >> + CpuApHltLoopAddress = BASE_1MB - 1; > > >> + Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, > > >> + EFI_SIZE_TO_PAGES (sizeof (CPU_AP_HLT_LOOP)), > > >> + &CpuApHltLoopAddress); > > >> + ASSERT_EFI_ERROR (Status); > > >> + > > >> + Status = gBS->CreateEvent ( > > >> + EVT_SIGNAL_EXIT_BOOT_SERVICES, > > >> + TPL_CALLBACK, > > >> + ExitBootServicesCallback, > > >> + (VOID *)(UINTN)CpuApHltLoopAddress, > > >> + &mExitBootServicesEvent > > >> + ); > > >> + ASSERT_EFI_ERROR (Status); > > >> + } > > >> } > > >> -- > > >> 1.8.3.1 > > >> > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
