Re: [edk2-devel] [PATCH] UefiCpuPkg: Use Top of each AP's stack to save CpuMpData

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 8/16/22 02:57, Yuanhao Xie via groups.io wrote:
> To remove the dependency of CPU register, 4/8 byte at the top of the stack
> is occupied for CpuMpData. BIST information is also taken care here.
> This modification is only for PEI phase, since in DXE phase CpuMpData is
> accessed via global variable.
> 
> Signed-off-by: Yuanhao <yuanhao.xie@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>   .../Library/MpInitLib/Ia32/MpFuncs.nasm       |  5 +++
>   UefiCpuPkg/Library/MpInitLib/MpLib.c          | 41 ++++++++++++++-----
>   UefiCpuPkg/Library/MpInitLib/MpLib.h          |  8 ++++
>   UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       | 10 +++--
>   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm |  6 +++
>   5 files changed, 56 insertions(+), 14 deletions(-)
> 

> @@ -1796,30 +1806,41 @@ MpInitLibInitialize (
>     AsmGetAddressMap (&AddressMap);
>     GetApResetVectorSize (&AddressMap, &ApResetVectorSizeBelow1Mb, &ApResetVectorSizeAbove1Mb);
>     ApStackSize = PcdGet32 (PcdCpuApStackSize);
> -  ApLoopMode  = GetApLoopMode (&MonitorFilterSize);
> +  //
> +  // ApStackSize must be power of 2
> +  //
> +  ASSERT ((ApStackSize & (ApStackSize - 1)) == 0);
> +  ApLoopMode = GetApLoopMode (&MonitorFilterSize);
>   
>     //
>     // Save BSP's Control registers for APs.
>     //
>     SaveVolatileRegisters (&VolatileRegisters);
>   
> +  //
> +  // Allocate extra ApStackSize to let AP stack align on ApStackSize bounday
> +  //
>     BufferSize  = ApStackSize * MaxLogicalProcessorNumber;
> +  BufferSize += ApStackSize;
>     BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
>     BufferSize += ApResetVectorSizeBelow1Mb;
>     BufferSize  = ALIGN_VALUE (BufferSize, 8);
>     BufferSize += VolatileRegisters.Idtr.Limit + 1;
>     BufferSize += sizeof (CPU_MP_DATA);
>     BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber;
> -  MpBuffer    = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
> +  //
> +  // Allocate extra ApStackSize to let stack align on ApStackSize bounday
> +  //
> +  MpBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));

If you're allocating pages, is all the alignment stuff really necessary? 
The allocated value is going to be page aligned already, right?

Thanks,
Tom

>     ASSERT (MpBuffer != NULL);
>     ZeroMem (MpBuffer, BufferSize);
> -  Buffer = (UINTN)MpBuffer;
> +  Buffer = ALIGN_VALUE ((UINTN)MpBuffer, ApStackSize);
>