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. > 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... > 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/EDK%20II%20C%20Coding%20Standards%20Specification.pdf/download > You might want to point out a coding style concern you noticed for Chen. > BTW: What platform do you use for testing? I think OVMF. > 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
