Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Wednesday, July 11, 2018 11:12 PM > To: Dong, Eric <[email protected]>; Yao, Jiewen <[email protected]>; > Fan Jeff <[email protected]>; [email protected] > Cc: Ni, Ruiyu <[email protected]> > Subject: Re: [edk2] 答复: [Patch] UefiCpuPkg/MpInitLib: Optimize get > processor number performance. > > On 07/11/18 13:31, Dong, Eric wrote: > > Hi Jiewen, > > > > I checked the code, found in the AP function (ApWakeupFunction), it > updated the GDT value with the saved GDT value from BSP. So I think we can't > use GDT in this case. Right? > > > > // > > // Sync BSP's Control registers to APs > > // > > RestoreVolatileRegisters > > (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); > > Side remark: please remember that this particular chunk of code is subject to > change; due to the reviewed (but not yet committed) patch from Ray: > > [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP > > [email protected]">http://mid.mail-archive.com/[email protected] > > Said patch has *not* been committed yet, and *only* because Ray himself > has push rights, but he is currently away. So nobody has picked up the patch > yet. > > I suggest that, before we do anything else for MpInitLib, we commit Ray's > patch first. > > Eric, do you agree? > > If so, can you push the patch, or do you want me to do it? I'm glad to do it > if > you prefer. >
Agree, just push Ray's patch: SHA-1: c563077a380437c114aba4c95be65eb963ebc1f3 Let's continue. > Thanks, > Laszlo > > > >> -----Original Message----- > >> From: Yao, Jiewen > >> Sent: Wednesday, July 11, 2018 3:45 PM > >> To: Dong, Eric <[email protected]>; 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 > >> 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
