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

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

I see. Thanks a lot!

Regards,
Jian


> -----Original Message-----
> From: Chaganty, Rangasai V
> Sent: Friday, January 05, 2018 10:55 AM
> To: Fan Jeff <vanjeff_919@hotmail.com>; 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
> 
> APIC_BASE MSR 1BH (BIT8) should tell us if the executing thread is BSP or not.
> This MSR is defined in SDM.
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Fan
> Jeff
> Sent: Thursday, January 04, 2018 6:50 PM
> 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: [edk2] 答复: [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> Sorry, Dump the APICID, not CPUID.
> 
> Jeff
> 
> 发件人: Fan Jeff<mailto:vanjeff_919@hotmail.com>
> 发送时间: 2018年1月5日 10:48
> 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; Laszlo
> Ersek<mailto:lersek@redhat.com>; edk2-devel@lists.01.org<mailto:edk2-
> devel@lists.01.org>
> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong,
> Eric<mailto:eric.dong@intel.com>
> 主题: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> You may use MP->SwitchBSP() to do BSP switch and then dump BSP’s CPUID to
> know if the switch is successfully.
> 
> Jeff
> 
> 
> From: Wang, Jian J <jian.j.wang@intel.com>
> Sent: Friday, January 5, 2018 9:57:31 AM
> To: Fan Jeff; Laszlo Ersek; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Dong, Eric
> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> Hi Jeff,
> 
> Do you think the change is OK? Do you know how to test switching BSP?
> 
> Regards,
> Jian
> 
> From: Fan Jeff [mailto:vanjeff_919@hotmail.com]
> Sent: Friday, January 05, 2018 9:40 AM
> To: Laszlo Ersek <lersek@redhat.com>; 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: 答复: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> Laszlo,
> 
> Firstly, SwitchBSP() is one service of MP defined in PI spec.
> 
> For real case, I think multiple socket system(with different processor stepping)
> may use this service for purpose.
> 
> Thanks!
> Jeff
> 
> 发件人: Laszlo Ersek<mailto:lersek@redhat.com>
> 发送时间: 2018年1月4日 20:21
> 收件人: Wang, Jian J<mailto:jian.j.wang@intel.com>; edk2-
> devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> 抄送: Yao, Jiewen<mailto:jiewen.yao@intel.com>; Dong,
> Eric<mailto:eric.dong@intel.com>
> 主题: 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<mailto:jian.j.wang@intel.com>>; Laszlo Ersek
> >> <lersek@redhat.com<mailto:lersek@redhat.com>>;
> >> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> >> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>;
> >> Dong, Eric <eric.dong@intel.com<mailto: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<mailto:lersek@redhat.com>>;
> >>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> >>> Cc: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>;
> >>> Dong, Eric <eric.dong@intel.com<mailto: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<mailto:jian.j.wang@intel.com>>;
> >>>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> >>>> Cc: Yao, Jiewen
> >>>> <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Dong, Eric
> >>>> <eric.dong@intel.com<mailto:eric.dong@intel.com>>;
> >>>> Jeff Fan <vanjeff_919@hotmail.com<mailto: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<mailto:jiewen.yao@intel.com>>
> >>>>> Cc: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> >>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>> Signed-off-by: Jian J Wang
> >>>>> <jian.j.wang@intel.com<mailto: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<mailto:edk2-devel@lists.01.org>
> >>> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto: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