Re: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

["Wang, Jian J" <jian.j.wang@intel.com>]

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-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Thursday, January 04, 2018 8:42 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@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:lersek@redhat.com]
> > Sent: Thursday, January 04, 2018 1:33 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Jeff Fan <vanjeff_919@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.yao@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang <jian.j.wang@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