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);
Thanks,
Eric
> -----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)))
> > > > > + CpuMpData->{
> > > > > *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
