Hi I believe using stack pointer is not a robust way if the stack guard feature is not enabled. Stack pointer may overflow.
Can we use GDT? Each AP has its own GDT. Thank you Yao Jiewen > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of Dong, > Eric > Sent: Monday, July 9, 2018 2:13 PM > To: Dong, Eric <[email protected]>; Laszlo Ersek <[email protected]>; Fan > Jeff <[email protected]>; [email protected] > Cc: Ni, Ruiyu <[email protected]> > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor > number performance. > > Hi Laszlo, > > I have created https://bugzilla.tianocore.org/show_bug.cgi?id=1002 to request > to add AsmReadEsp() / AsmReadRsp(). > > Thanks, > Eric > > > -----Original Message----- > > From: edk2-devel [mailto:[email protected]] On Behalf Of > > Dong, Eric > > Sent: Monday, July 9, 2018 11:04 AM > > To: Laszlo Ersek <[email protected]>; Fan Jeff <[email protected]>; > > [email protected] > > Cc: Ni, Ruiyu <[email protected]> > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > processor number performance. > > > > Hi Laszlo, > > > > > -----Original Message----- > > > From: Laszlo Ersek [mailto:[email protected]] > > > Sent: Thursday, July 5, 2018 9:04 PM > > > To: Fan Jeff <[email protected]>; Dong, Eric > > > <[email protected]>; [email protected] > > > Cc: Ni, Ruiyu <[email protected]> > > > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > > > processor number performance. > > > > > > Hi Jeff, > > > > > > On 07/04/18 11:39, Fan Jeff wrote: > > > > Eric, > > > > > > > > Current implementation does not call GetApicid() many times, Please > > > correct you commit message. Your fix is to improve the performance > > > against the current implementation. > > > > > > I think the original commit message does make sense. Without the > > > patch, > > > GetProcessorNumber() may call GetApicId() up to TotalProcessorNumber > > > times. With the patch, even if we skip the stack range search, > > > GetProcessorNumber() will call GetApicId() just once. > > > > > > [...] > > > > > > Some more questions below, for the patch: > > > > > > > 发件人: Eric Dong <[email protected]> > > > > 发送时间: Wednesday, July 4, 2018 4:37:36 PM > > > > 收件人: [email protected] > > > > 抄送: Ruiyu Ni; Jeff Fan; Laszlo Ersek > > > > 主题: [Patch] UefiCpuPkg/MpInitLib: Optimize get processor number > > > performance. > > > > > > > > Current function has low performance because it calls GetApicId many > > > > times. > > > > > > > > New logic first try to base on the stack range used by AP to find > > > > the processor number. If this solution failed, then call GetApicId > > > > once and base on this value to search the processor. > > > > > > > > Cc: Ruiyu Ni <[email protected]> > > > > Cc: Jeff Fan <[email protected]> > > > > Cc: Laszlo Ersek <[email protected]> > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Eric Dong <[email protected]> > > > > --- > > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 > ++++++++++++++++++++++--- > > > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > index eb2765910c..abd65bee1a 100644 > > > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > > @@ -418,7 +418,8 @@ ApInitializeSync ( } > > > > > > > > /** > > > > - Find the current Processor number by APIC ID. > > > > + First try to find the current Processor number by stack address, > > > > + if it failed, then base on APIC ID. > > > > > > > > @param[in] CpuMpData Pointer to PEI CPU MP Data > > > > @param[out] ProcessorNumber Return the pocessor number found > > > > @@ -435,16 +436,34 @@ GetProcessorNumber ( > > > > UINTN TotalProcessorNumber; > > > > UINTN Index; > > > > CPU_INFO_IN_HOB *CpuInfoInHob; > > > > + UINT32 CurrentApicId; > > > > > > > > + TotalProcessorNumber = CpuMpData->CpuCount; > > > > CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData- > > > >CpuInfoInHob; > > > > > > > > - TotalProcessorNumber = CpuMpData->CpuCount; > > > > + // > > > > + // First try to base on current stack address to find the AP index. > > > > + // &TotalProcessorNumber value located in the stack range. > > > > + // > > > > for (Index = 0; Index < TotalProcessorNumber; Index ++) { > > > > - if (CpuInfoInHob[Index].ApicId == GetApicId ()) { > > > > + if ((CpuInfoInHob[Index].ApTopOfStack > (UINTN) > > > (&TotalProcessorNumber)) && > > > > + (CpuInfoInHob[Index].ApTopOfStack - > > > > + CpuMpData->CpuApStackSize < (UINTN) (&TotalProcessorNumber))) { > > > > *ProcessorNumber = Index; > > > > return EFI_SUCCESS; > > > > } > > > > } > > > > > > (1) If I understand correctly, ApTopOfStack is the exclusive end > > > (highest > > > address) of the AP stack, so any local variable is supposed to start > > > strictly below it (the stack grows down). This seems to justify the > > > ">" relational operator, in the first subcondition; OK. > > > > > > However, what guarantees that the TotalProcessorNumber local variable > > > is not located exactly at the (inclusive) base of the AP stack? IOW, why > > > is "<" > > > correct, in the second subcondition, rather than "<="? > > > > > > > [Eric] TotalProcessorNumber is the first local variable in this function, > > also > > exist other local variables in this function, so I just use "<" here. > > > > > > > > (2) I'm generally unhappy about taking the address of local variables, > > > in order to determine stack location in C language. Instead, I think > > > we should have > > > AsmReadEsp() / AsmReadRsp() functions -- we used to have > > > AsmReadSp() for Itanium. Please see the following sub-thread, where > > > Jordan originally suggested AsmReadEsp() / AsmReadRsp(): > > > > > > http://mid.mail- > > > archive.com/151056410867.15809.659701894226687543@jljusten-skl > > > > > > http://mid.mail- > > > archive.com/151059627258.20614.16505766191415005802@jljusten-skl > > > > > > Should I file a Feature Request for BaseLib, about adding AsmReadEsp() > > > / AsmReadRsp()? > > > > > > I'm not suggesting that we block this patch with that feature request, > > > but perhaps we should block the *next* patch. > > > > > > > [Eric] Yes, I tries to use the function you suggested but we don't find it, > > so I > > use local variable here. I agree with your suggest that we should add this > > API > > for later usage. I will follow up to add this new API and update this patch > > to V2. > > > > > > > > For the present patch, I'll follow up with test results separately. > > > > > > Thanks, > > > Laszlo > > > > > > > + > > > > + // > > > > + // If can't base on stack to find the AP index, use the APIC ID. > > > > + // > > > > + CurrentApicId = GetApicId (); > > > > + for (Index = 0; Index < TotalProcessorNumber; Index ++) { > > > > + if (CpuInfoInHob[Index].ApicId == CurrentApicId) { > > > > + *ProcessorNumber = Index; > > > > + return EFI_SUCCESS; > > > > + } > > > > + } > > > > + > > > > return EFI_NOT_FOUND; > > > > } > > > > > > > > -- > > > > 2.15.0.windows.1 > > > > > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > [email protected] > > > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
