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

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

I'm not familiar with BSP/AP switch code either. I just happen to see code comments
mentioned  switching BSP/AP. So I think it would be better to initialize cpu 0 the same 
way as others. No matter what, as what the field name implies, ApTopOfStack should
never be used to store stack base address.

I'll try to find if any test cases for the BSP/AP switch were developed before. If non,
I think it's not easy for me to write one. If it's OK, I prefer to check in current fix and
test what you concern later. I think nobody uses this feature.

I'll send v3 as what you suggested. Thanks a lot for all your comments.

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, January 04, 2018 8:22 PM
> 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>
> Subject: 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.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