I added my comments in [Jeff]. -----Original Message----- From: Jordan Justen [mailto:[email protected]] Sent: Thursday, September 11, 2014 3:21 PM To: Fan, Jeff Cc: Chen Fan; [email protected] Subject: Re: [RFC v2 00/15] Introduce Mp Service protocol to UefiCpuPkg
On Wed, Sep 10, 2014 at 11:56 PM, Fan, Jeff <[email protected]> wrote: > Chen, > > Thanks your patches, this is very good start for UefiCpuPkg to add PI MP > services. > > My questions as below for your V2 patch. > What means " do not call anything EFI API from APs."? Does it mean APs will > not call any BS services? > If yes, this is correct. APs routine cannot invoke any BS services. > > Why did you get rid of sending IPI to wake up APs? Do you encounter any issue > with it? > From your patch, I don't know why StarupThisAP () cannot work correctly. > Could you send me your AP routine test code and test procedure? I think that a startup IPI is used once, but not again after the APs have been woken once. At least, that was my feedback previously. [Jeff] We observed that polling a semaphore in memory for all APs may lead the bad boot performance on real platform. So, normally we place APs in one hlt-loop and sending start IPI to wake up them. Sure, semaphore is also one of our methods to wake up AP. > The following are my comments on your MP service implementation for > UefiCpuPkg. > 1. There is no implementation of StartupAllAps (). It's required for PI MP > service from PI Spec. > 2. There is no implementation of SwitchBsp(). It's not required from PI > spec. So, you could write one SwitchBsp function and return EFI_UNSUPPORTED > simply instead of NULL function pointer in MP service structure. > 3. Why free AP stack at end of code InitializeMpSupport()? I think AP stack > should be used by APs during whole POST phase. > FreePages (mCommonStack, EFI_SIZE_TO_PAGES (AP_STACK_SIZE)); 4. > There is no .ASM implementation, we need to check-in code in > UefiCpuPkg to support both MSFT and GCC. Maybe .nasm is good start to > support both at one time for such new assembly code. :-) The startup routine is in ApStartup.c, so it should work with all toolchains. (The NASM source helped construct the ApStartup.c code.) I told Chen I would provide the MpAsm.asm files. I haven't done that yet... [Jeff] I mean MpAsm.asm files. Why not to write MpAsm.nasm files only? > 5. Please follow EDKII coding style document to add function headers > and refine your code. Please refer to > https://sourceforge.net/projects/edk2/files/General%20Documentation/ED > K%20II%20C%20Coding%20Standards%20Specification.pdf/download > You might want to point out a coding style concern you noticed for Chen. [Jeff] Yes. To let the code meet EDKII check-in criteria. > BTW: What platform do you use for testing? I think OVMF. [Jeff] Ovmf is good. Thanks! > Best Regards, > Jeff > > -----Original Message----- > From: Chen Fan [mailto:[email protected]] > Sent: Wednesday, September 10, 2014 6:22 PM > To: [email protected] > Cc: Jordan Justen; Fan, Jeff > Subject: [RFC v2 00/15] Introduce Mp Service protocol to UefiCpuPkg > > This series patchset try to implement Mp Service protocol in UefiCpuPkg, > Jordan had implemented the startup APs code, and I try to add more > initialization code to let all APs work up, this Mp Service protocol's > implementation used EmulatorPkg/MpService for reference. > this patches works on my github: > https://github.com/ChenFanFnst/edk2/tree/cpu-mp-service > > V1-V2: > 1. do not call anything EFI API from APs. > 2. add AP busy-wait for task assignment from BSP and get rid of > IPI sent mechanism. > > NOTE: the last patch implemented StartupThisAP() could not run correctly. > the Ap run loop will cause IoRead32() assert in > MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c: (163) > I can't find the root reason. please help me. > > TODO: > 1) StartupThisAP/StartAllAps: when Timeout expires before AP returns > from Procedure, constrainedly terminate the executed Procedure. > 2) Support SwitchBSP(). (Or it is unnecessary) > > Any Comment is welcome. > > Chen Fan (9): > UefiCpuPkg/CpuDxe: Introduce AsmApDoneWithCommonStack > UefiCpuPkg/CpuDxe: Waiting for Aps initialization done > UefiCpuPkg/CpuDxe: Switch Ap CommonStack with NewStack > UefiCpuPkg/CpuDxe: install Mp Service protocol > UefiCpuPkg/CpuDxe: implement Mp Protocol: GetNumberOfProcessors() > UefiCpuPkg/CpuDxe: implement Mp Protocol: WhoAmI() > UefiCpuPkg/CpuDxe: implement Mp Services: GetProcessorInfo() > UefiCpuPkg/CpuDxe: implement Mp Protocol: EnableDisableAP() > UefiCpuPkg/CpuDxe: implement Mp Protocol: StartupThisAP() > > Jordan Justen (6): > UefiCpuPkg/CpuDxe: Add no-op InitializeMpSupport > UefiCpuPkg/CpuDxe: Add ApEntryPointInC > UefiCpuPkg/CpuDxe: Add stackless assembly AP entry points > UefiCpuPkg/CpuDxe: Move GDT structures into CpuGdt.h > UefiCpuPkg/CpuDxe: Add StartApsStackless routine > UefiCpuPkg/CpuDxe: Startup APs > > UefiCpuPkg/CpuDxe/ApStartup.asm | 111 ++++++ > UefiCpuPkg/CpuDxe/ApStartup.c | 189 ++++++++++ > UefiCpuPkg/CpuDxe/CpuDxe.c | 3 + > UefiCpuPkg/CpuDxe/CpuDxe.inf | 7 + > UefiCpuPkg/CpuDxe/CpuGdt.c | 52 +-- > UefiCpuPkg/CpuDxe/CpuGdt.h | 72 ++++ > UefiCpuPkg/CpuDxe/CpuMp.c | 756 > ++++++++++++++++++++++++++++++++++++++++ > UefiCpuPkg/CpuDxe/CpuMp.h | 152 ++++++++ > UefiCpuPkg/CpuDxe/Ia32/MpAsm.S | 114 ++++++ > UefiCpuPkg/CpuDxe/X64/MpAsm.S | 125 +++++++ > 10 files changed, 1530 insertions(+), 51 deletions(-) create mode > 100644 UefiCpuPkg/CpuDxe/ApStartup.asm create mode 100644 > UefiCpuPkg/CpuDxe/ApStartup.c create mode 100644 > UefiCpuPkg/CpuDxe/CpuGdt.h create mode 100644 > UefiCpuPkg/CpuDxe/CpuMp.c create mode 100644 > UefiCpuPkg/CpuDxe/CpuMp.h create mode 100644 > UefiCpuPkg/CpuDxe/Ia32/MpAsm.S create mode 100644 > UefiCpuPkg/CpuDxe/X64/MpAsm.S > > -- > 1.9.3 > ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
