Got it. Many thanks! Regards, Jian
From: Fan Jeff [mailto:vanjeff_...@hotmail.com] Sent: Friday, January 05, 2018 10:50 AM 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-devel@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<mailto:jian.j.w...@intel.com>> Sent: Friday, January 5, 2018 9:57:31 AM To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org<mailto: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<mailto:ler...@redhat.com>>; 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>> 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-devel@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
