I see. Thanks a lot! Regards, Jian
> -----Original Message----- > From: Chaganty, Rangasai V > Sent: Friday, January 05, 2018 10:55 AM > To: Fan Jeff <vanjeff_...@hotmail.com>; Wang, Jian J > <jian.j.w...@intel.com>; Laszlo Ersek <ler...@redhat.com>; edk2- > de...@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com> > Subject: RE: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address > set as Stack Guard > > APIC_BASE MSR 1BH (BIT8) should tell us if the executing thread is BSP or not. > This MSR is defined in SDM. > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fan > Jeff > Sent: Thursday, January 04, 2018 6:50 PM > To: Wang, Jian J <jian.j.w...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com> > Subject: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Sorry, Dump the APICID, not CPUID. > > Jeff > > 发件人: Fan Jeff<mailto:vanjeff_...@hotmail.com> > 发送时间: 2018年1月5日 10:48 > 收件人: Wang, Jian J<mailto:jian.j.w...@intel.com>; Laszlo > Ersek<mailto:ler...@redhat.com>; edk2-devel@lists.01.org<mailto:edk2- > de...@lists.01.org> > 抄送: Yao, Jiewen<mailto:jiewen....@intel.com>; Dong, > Eric<mailto:eric.d...@intel.com> > 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to > know if the switch is successfully. > > Jeff > > > From: Wang, Jian J <jian.j.w...@intel.com> > Sent: Friday, January 5, 2018 9:57:31 AM > To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org > Cc: Yao, Jiewen; Dong, Eric > Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Hi Jeff, > > Do you think the change is OK? Do you know how to test switching BSP? > > Regards, > Jian > > From: Fan Jeff [mailto:vanjeff_...@hotmail.com] > Sent: Friday, January 05, 2018 9:40 AM > To: Laszlo Ersek <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com>; > edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com> > Subject: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set > as Stack Guard > > Laszlo, > > Firstly, SwitchBSP() is one service of MP defined in PI spec. > > For real case, I think multiple socket system(with different processor > stepping) > may use this service for purpose. > > Thanks! > Jeff > > 发件人: Laszlo Ersek<mailto:ler...@redhat.com> > 发送时间: 2018年1月4日 20:21 > 收件人: Wang, Jian J<mailto:jian.j.w...@intel.com>; edk2- > de...@lists.01.org<mailto:edk2-devel@lists.01.org> > 抄送: Yao, Jiewen<mailto:jiewen....@intel.com>; Dong, > Eric<mailto:eric.d...@intel.com> > 主题: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as > Stack Guard > > On 01/04/18 02:45, Wang, Jian J wrote: > > A correction: although BSP doesn't need it, we still need to > > initialize its ApTopOfStack correctly because the MP service supports > > BSP/AP exchange. So I think the line 1501 should be changed to > > > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + > > CpuMpData->CpuApStackSize); > > > > instead of > > > > InitializeApData (CpuMpData, 0, 0, NULL); > > Hmm... Although I don't immediately see all possible consequences of such a > change, it looks like a correct fix. > > Unfortunately, I don't know of any code that actually exercises the BSP/AP > exchange service. I think Intel must have access to some client code like > this, > because I vaguely recall some patches over time that improved BSP/AP > exchange. > > If you modify the InitializeApData() call in question like suggested above, > can > you perhaps locate that client code, and test the change with it? > > Thanks! > Laszlo > > > >> -----Original Message----- > >> From: Wang, Jian J > >> Sent: Thursday, January 04, 2018 9:09 AM > >> To: Wang, Jian J > >> <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Laszlo Ersek > >> <ler...@redhat.com<mailto:ler...@redhat.com>>; > >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >> Cc: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; > >> Dong, Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>> > >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >> address set as Stack Guard > >> > >> Laszlo, > >> > >> More explanations: > >> > >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] > >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is > >> initialized to the bottom of the stack (line 1501) but AP's > >> ApTopOfStack is correctly initialized (line 598). Although my > >> calculation is correct, I think it'd be better to use AP's > >> ApTopOfStack directly. From this perspective, you're right. > >> > >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP > >> doesn't need it anyway. > >> > >> Regards, > >> Jian > >> > >> > >>> -----Original Message----- > >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf > >>> Of > >> Wang, > >>> Jian J > >>> Sent: Thursday, January 04, 2018 8:42 AM > >>> To: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; > >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>> Cc: Yao, Jiewen <jiewen....@intel.com<mailto:jiewen....@intel.com>>; > >>> Dong, Eric <eric.d...@intel.com<mailto:eric.d...@intel.com>> > >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >>> address set as Stack Guard > >>> > >>> Laszlo, > >>> > >>> I revisited code of MpInitLib. I found that > >>> CPU_INFO_IN_HOB.ApTopOfStack was assigned to CpuMpData->Buffer in > >>> MpInitLibInitialize() > >>> > >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > >>> > >>> but in > >>> > >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) > >>> * > >>> CpuMpData->CpuApStackSize; > >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > >>> ApTopOfStack); > >>> > >>> Since InitMpGlobalData() is called just after first situation, my patch is > correct. > >>> > >>> I think the problem here is that ApTopOfStack initialized at line > >>> 1501 is not correct. > >>> > >>> > >>> Regards, > >>> Jian > >>> > >>> > >>>> -----Original Message----- > >>>> From: Laszlo Ersek [mailto:ler...@redhat.com] > >>>> Sent: Thursday, January 04, 2018 1:33 AM > >>>> To: Wang, Jian J > >>>> <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; > >>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>>> Cc: Yao, Jiewen > >>>> <jiewen....@intel.com<mailto:jiewen....@intel.com>>; Dong, Eric > >>>> <eric.d...@intel.com<mailto:eric.d...@intel.com>>; > >>>> Jeff Fan <vanjeff_...@hotmail.com<mailto:vanjeff_...@hotmail.com>> > >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base > >>>> address > >> set > >>>> as Stack Guard > >>>> > >>>> (CC Jeff) > >>>> > >>>> Sorry about the delay. > >>>> > >>>> I have some light comments below; I expect at least a few of them > >>>> to be incorrect :) > >>>> > >>>> On 12/29/17 09:36, Jian J Wang wrote: > >>>>> The reason is that DXE part initialization will reuse the stack > >>>>> allocated at PEI phase, if MP was initialized before. Some code > >>>>> added to check this situation and use stack base address saved in HOB > passed from PEI. > >>>>> > >>>>> Cc: Jiewen Yao <jiewen....@intel.com<mailto:jiewen....@intel.com>> > >>>>> Cc: Eric Dong <eric.d...@intel.com<mailto:eric.d...@intel.com>> > >>>>> Cc: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Jian J Wang > >>>>> <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>> > >>>>> --- > >>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++++++++++++++-- > >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> index 40c1bf407a..05484c9ff3 100644 > >>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > >>>>> @@ -295,6 +295,7 @@ InitMpGlobalData ( > >>>>> UINTN Index; > >>>>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > >>>>> UINTN StackBase; > >>>>> + CPU_INFO_IN_HOB *CpuInfoInHob; > >>>>> > >>>>> SaveCpuMpData (CpuMpData); > >>>>> > >>>>> @@ -314,9 +315,18 @@ InitMpGlobalData ( > >>>>> ASSERT (FALSE); > >>>>> } > >>>>> > >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- > >>> CpuApStackSize; > >>>>> + // > >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's > >>>>> available. > >>>>> + // Let's check it here. > >>>>> + // > >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > >>>>> CpuInfoInHob; > >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { > >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; > >>>>> + } else { > >>>>> + StackBase = CpuMpData->Buffer; > >>>>> + } > >>>> > >>>> So, if the HOB is not found, then StackBase is set okay. > >>>> > >>>> However, I'm unsure about the other case. The > >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the > >>>> stack (highest address, and the stack grows down); however the loop > >>>> below > >>>> *increments* StackBase. Given the incrementing nature of the loop, > >>>> shouldn't we first calculate the actual base (= lowest address) > >>>> from the CPU_INFO_IN_HOB.ApTopOfStack field? > >>>> > >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob > >>>> field points to an *array* of CPU_INFO_IN_HOB structures. > >>>> Therefore, for any given processor #N, we should not calculate the > >>>> stack base as > >>>> > >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- > >>>>> CpuApStackSize > >>>> > >>>> instead we should calculate the stack base as something like: > >>>> > >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- > >>> CpuApStackSize > >>>> > >>>> See > >>>> - the InitializeApData() function, > >>>> - and its call site in the ApWakeupFunction() function. > >>>> > >>>> (To my surprise, I personally modified InitializeApData() earlier, > >>>> in commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP > >>>> stack addresses", 2016-11-17) -- I've totally forgotten about that > >>>> by now!) > >>>> > >>>> What do you think? > >>>> > >>>>> > >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> > >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( > >>>>> MemDesc.Attributes | EFI_MEMORY_RP > >>>>> ); > >>>>> ASSERT_EFI_ERROR (Status); > >>>>> + > >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", > >>>> StackBase, Index)); > >>>> > >>>> StackBase has type UINTN, and so it should not be printed with %x. > >>>> It should be cast to (UINT64), and then printed with %Lx. > >>>> > >>>> Similarly, Index has type UINTN. It should not be printed with %d. > >>>> It should be cast to (UINT64) and printed with %Lu. > >>>> > >>>> > >>>>> + StackBase += CpuMpData->CpuApStackSize; > >>>> > >>>> Again, I don't think the simple increment applies when the > >>>> CpuMpData->CpuInfoInHob array exists. > >>>> > >>>>> } > >>>>> } > >>>>> > >>>>> > >>>> > >>>> Thanks, > >>>> Laszlo > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > >>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> > https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
