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
