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); Regards, Jian > -----Original Message----- > From: Wang, Jian J > Sent: Thursday, January 04, 2018 9:09 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: 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>; edk2-devel@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 > > > > 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>; edk2-devel@lists.01.org > > > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > > > Jeff Fan <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> > > > > Cc: Eric Dong <eric.d...@intel.com> > > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Jian J Wang <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 > > 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
