What about adding this code to mStartupCodeTemplate and sending the APs there using the MP services?
Regardless of that, I think the C-based-asm would better fit in ApStartup.c. Why not just a cli-hlt loop? Maybe if the MP services were used then the separate counter goes away. How about 0 rather than 0x1234? Can you provide a nasm source block in a comment like in ApStartup.c? -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
