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. 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. > 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. > 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). > 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
