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

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

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