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

[Laszlo Ersek <lersek@redhat.com>]

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