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

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

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.wang@intel.com>; 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,
> 
> 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