Jordan, Usually, 32KB is used for AP stack size. One PCD could be used to update its value by platform.
Allocating memory region for the supported CPUs number (specified by one PCD) is OK. But I think pre-allocating and re-org the memory region per real APs number is some complex. If platform really cares the memory occupied size, it could reduce supported CPU count by this PCD. Chen's current solution is also OK to allocate memory for real APs number. Thanks! Jeff -----Original Message----- From: Jordan Justen [mailto:[email protected]] Sent: Thursday, September 25, 2014 2:16 PM To: Chen, Fan; Fan, Jeff Cc: [email protected]; Andrew Fish Subject: Re: [RFC v3 08/19] UefiCpuPkg/CpuDxe: Waiting for Aps initialization done On 2014-09-24 20:16:51, Chen, Fan wrote: > On Wed, 2014-09-24 at 14:56 -0700, Jordan Justen wrote: > > On 2014-09-17 20:12:16, Chen Fan wrote: > > > BSP should detect the AP COUNT to know the AP presence. > > > So in AP initialization code, AP increased the COUNT, and BSP > > > waited 100ms to get the COUNT. then BSP could allocate stack > > > spaces for the APs. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Signed-off-by: Chen Fan <[email protected]> > > > --- > > > UefiCpuPkg/CpuDxe/CpuMp.c | 19 +++++++++++++++++++ > > > UefiCpuPkg/CpuDxe/Ia32/MpAsm.S | 1 + > > > UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm | 2 ++ > > > UefiCpuPkg/CpuDxe/X64/MpAsm.S | 1 + > > > UefiCpuPkg/CpuDxe/X64/MpAsm.nasm | 2 ++ > > > > It looks like you modify the assembly code in patches > > 08/19 UefiCpuPkg/CpuDxe: Waiting for Aps initialization done > > 09/19 UefiCpuPkg/CpuDxe: Switch Ap CommonStack with NewStack > > > > I think that this should not be required. Instead, I think you can > > call out to the previously defined assembly functions from C code to > > accomplish what you need. > > > > For switching stacks, how about: > > 1. BaseLib SwitchStack > > 2. Call MpAsm AsmApDoneWithCommonStack > Hi Jordan, > > For SwitchStack, because of the NewStack spaces must be allocated by > BSP, so BSP must need to know the AP count at first. but at this time, > AP should run-loop without a lock after executing start-up code, so > All APs will be synced to call SwitchStack with the same stack to save > return address and parameters. I think it will cause confusion. > and I introduced function AsmApSwitchStack() that at first locked the > cpu bus to avoid sync. > How do you think? This is a good point. I think firmware usually takes the easy way out on this and defines a PCD for the max number of processors supported. Maybe for today 32 or 64 would be a good default value. Then the BSP can allocate a stack for this amount up front and the APs can claim a portion of the buffer as they start up. If allocate pages is used, then the BSP can free unused pages at the end. (Or, the BSP can put the APs back to sleep and free the whole region.) This is what I would suggest. A more complicated design could involve the APs and BSPs sync'ing via a lock and the BSP noticing that an AP needs a stack allocated. Jeff, Do you know a good stack size per AP? I looked in the PI spec, and I didn't find a spec'd size. Thanks, -Jordan > > > > For AsmApDoneWithCommonStack, I updated my branch to include the > > prototype in CpuMp.h. > > > > Things like incrementing the AP count variable can all be handled in > > C code using a lock as needed. > > > > Thanks, > > > > -Jordan > > > > > 5 files changed, 25 insertions(+) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c > > > index f732ae5..536f998 100644 > > > --- a/UefiCpuPkg/CpuDxe/CpuMp.c > > > +++ b/UefiCpuPkg/CpuDxe/CpuMp.c > > > @@ -18,6 +18,9 @@ > > > VOID *mCommonStack = 0; > > > VOID *mTopOfApCommonStack = 0; > > > > > > +UINTN mNumberOfProcessors; > > > +BOOLEAN volatile mAllApsInitFinished = FALSE; > > > + > > > > > > /** > > > Application Processor C code entry point @@ -32,6 +35,9 @@ > > > ApEntryPointInC ( > > > /* Ap initialization */ > > > AsmApDoneWithCommonStack (); > > > > > > + /* Wait for all Aps complete to initialization */ while > > > + (!mAllApsInitFinished); > > > + > > > CpuDeadLoop (); > > > } > > > > > > @@ -51,8 +57,21 @@ InitializeMpSupport ( > > > return; > > > } > > > > > > + mNumberOfProcessors = 1; > > > + > > > StartApsStackless (AsmApEntryPoint); > > > > > > + if (mNumberOfProcessors == 1) { > > > + goto EXIT; > > > + } > > > + > > > + DEBUG ((DEBUG_ERROR, "Detect CPU count: %d\n", > > > + mNumberOfProcessors)); > > > + > > > + mAllApsInitFinished = TRUE; > > > + > > > + CpuDeadLoop (); > > > + > > > +EXIT: > > > mTopOfApCommonStack = NULL; > > > FreePages (mCommonStack, EFI_SIZE_TO_PAGES (SIZE_64KB)); > > > mCommonStack = NULL; > > > diff --git a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.S > > > b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.S index 2b3d298..bc42c38 100644 > > > --- a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.S > > > +++ b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.S > > > @@ -33,6 +33,7 @@ lock btsl $0, ApStackLock > > > pause > > > jc AsmApEntryPointAcquireLock > > > > > > + incl ASM_PFX(mNumberOfProcessors) > > > movl (ASM_PFX(mTopOfApCommonStack)), %esp > > > call ASM_PFX(ApEntryPointInC) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm > > > b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm > > > index 2ecaa7f..8e0b23f 100644 > > > --- a/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm > > > +++ b/UefiCpuPkg/CpuDxe/Ia32/MpAsm.nasm > > > @@ -13,6 +13,7 @@ > > > > > > extern ASM_PFX(mTopOfApCommonStack) extern > > > ASM_PFX(ApEntryPointInC) > > > +extern ASM_PFX(mNumberOfProcessors) > > > > > > ; > > > ; point to the external interrupt vector table > > > @@ -35,6 +36,7 @@ lock bts dword [ApStackLock], 0 > > > pause > > > jc AsmApEntryPointAcquireLock > > > > > > + inc dword [ASM_PFX(mNumberOfProcessors)] > > > mov esp, [ASM_PFX(mTopOfApCommonStack)] > > > call ASM_PFX(ApEntryPointInC) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/X64/MpAsm.S > > > b/UefiCpuPkg/CpuDxe/X64/MpAsm.S index cfad551..bf488d3 100644 > > > --- a/UefiCpuPkg/CpuDxe/X64/MpAsm.S > > > +++ b/UefiCpuPkg/CpuDxe/X64/MpAsm.S > > > @@ -33,6 +33,7 @@ lock btsl $0, ApStackLock > > > pause > > > jc AsmApEntryPointAcquireLock > > > > > > + incl ASM_PFX(mNumberOfProcessors) > > > movq (ASM_PFX(mTopOfApCommonStack)), %rsp > > > call ASM_PFX(ApEntryPointInC) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm > > > b/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm > > > index 2323443..2de5e9c 100644 > > > --- a/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm > > > +++ b/UefiCpuPkg/CpuDxe/X64/MpAsm.nasm > > > @@ -13,6 +13,7 @@ > > > > > > extern ASM_PFX(mTopOfApCommonStack) extern > > > ASM_PFX(ApEntryPointInC) > > > +extern ASM_PFX(mNumberOfProcessors) > > > > > > ; > > > ; point to the external interrupt vector table > > > @@ -35,6 +36,7 @@ lock bts dword [ApStackLock], 0 > > > pause > > > jc AsmApEntryPointAcquireLock > > > > > > + inc dword [ASM_PFX(mNumberOfProcessors)] > > > mov rsp, [ASM_PFX(mTopOfApCommonStack)] > > > call ASM_PFX(ApEntryPointInC) > > > > > > -- > > > 1.9.3 > > > > ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
