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

[Laszlo Ersek <lersek@redhat.com>]

On 01/04/18 04:09, Jian J Wang wrote:
>> v2 changes:
>> a. Use each AP's ApTopOfStack to get the stack base address instead of
>>    cpu0's ApTopOfStack which is actually set incorrectly before.
>> b. Fix cpu0's ApTopOfStack initialization.
>> c. Fix wrong debug print format.

The end result of this patch looks fine to me.

However, please split update (b), which affects "MpLib.c", to a separate
patch. The reason is that update (b) addresses a distinct bug.

So in v3,

- patch v3 1/2 should fix the top-of-stack initialization for the BSP,
  with its own commit message (referencing the BSP/AP switch service),

- patch v3 2/2 should do everything else from v2.

I'm ready to R-b such a v3.

Thank you!
Laszlo

> 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 | 19 ++++++++++++++++++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    |  2 +-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 40c1bf407a..e832c16eca 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,8 +315,21 @@ InitMpGlobalData (
>        ASSERT (FALSE);
>      }
>  
> +    //
> +    // DXE will reuse stack allocated for APs at PEI phase if it's available.
> +    // Let's check it here.
> +    //
> +    // Note: BSP's stack guard is set at DxeIpl phase. But for the sake of
> +    // BSP/AP exchange, stack guard for ApTopOfStack of cpu 0 will still be
> +    // set here.
> +    //
> +    CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
>      for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> -      StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> +      if (CpuInfoInHob != NULL && CpuInfoInHob[Index].ApTopOfStack != 0) {
> +        StackBase = CpuInfoInHob[Index].ApTopOfStack - CpuMpData->CpuApStackSize;
> +      } else {
> +        StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> +      }
>  
>        Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc);
>        ASSERT_EFI_ERROR (Status);
> @@ -326,6 +340,9 @@ InitMpGlobalData (
>                        MemDesc.Attributes | EFI_MEMORY_RP
>                        );
>        ASSERT_EFI_ERROR (Status);
> +
> +      DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
> +              (UINT64)StackBase, (UINT64)Index));
>      }
>    }
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 0c2058a7b0..1bfab8467b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1498,7 +1498,7 @@ MpInitLibInitialize (
>    //
>    // Set BSP basic information
>    //
> -  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);
> +  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>    //
>    // Save assembly code information
>    //
>