Hi Jeff, Jordan, On Thu, 2014-09-11 at 07:36 +0000, Fan, Jeff wrote: > 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. [Chen] yes. APs did not call 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? [Chen] I use the StartCorePkg to test Mp service. which is located at https://svn.code.sf.net/p/edk2-startcore/code/StartCorePkg I found that after APs run a period of time, they failed assert at MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c: (163) I don't know why. but if I got rid of frequent lock requests. at Ap-loop. it can largely reduce the chance of assertion. > > 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. [Chen] Yes, I think if the StarupThisAP() issues can be solved. then we can add the implementation of StartupALlAps(). ;) > > 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. [Chen] OK. > > 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)); [Chen] I Think the CommonStack is used for StartupApStackLess. if AP has switched to its' own stack. the common stack should free. right? > 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. [Chen] Ok, I will learn them ASAP. ;) Thanks. Chen > > > 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
