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

[Laszlo Ersek <lersek@redhat.com>]

On 01/04/18 02:09, Wang, Jian J wrote:
> 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.

Right, after I sent my email, it occurred to me that your calculation
could actually match the other calculation that originally populates the
CpuInfoInHob[N].ApTopOfStack fields. In other words, the values assigned
could be correct. However, I do think / agree that we shouldn't
duplicate the calculation, instead we should reuse the pre-computed values.

Thanks!
Laszlo

> 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