Reviewed-by: Ray Ni <ray...@intel.com>
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao....@intel.com>
> Sent: Friday, August 19, 2022 2:17 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao....@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Kumar, Rahul R
> <rahul.r.ku...@intel.com>
> Subject: [Patch V2] UefiCpuPkg: Use Top of each AP's stack to save
> CpuMpData
>
> From: Yuanhao Xie <yuanhao....@intel.com>
>
> 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.
>
> Change-Id: I7564279544622617c322310b4c7028ac0e11a9c4
> Signed-off-by: Yuanhao <yuanhao....@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> ---
> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 8 ++++
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 37 ++++++++++++++-----
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 ++++
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 10 +++--
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 9 +++++
> 5 files changed, 59 insertions(+), 13 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 28301bb8f0..2625d28401 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -179,6 +179,14 @@ ProgramStack:
> mov esp, dword [edi + CPU_INFO_IN_HOB.ApTopOfStack]
>
> CProcedureInvoke:
> + ;
> + ; Reserve 4 bytes for CpuMpData.
> + ; When the AP wakes up again via INIT-SIPI-SIPI, push 0 will cause the
> existing CpuMpData to be overwritten with 0.
> + ; CpuMpData is filled in via InitializeApData() during the first time
> INIT-
> SIPI-SIPI,
> + ; while overwirrten may occurs when under ApInHltLoop but InitFlag is
> not set to ApInitConfig.
> + ; Therefore reservation is implemented by sub esp instead of push 0.
> + ;
> + sub esp, 4
> push ebp ; push BIST data at top of AP stack
> xor ebp, ebp ; clear ebp for call stack trace
> push ebp
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8d1f24370a..a4ce1c6d2e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -571,6 +571,7 @@ InitializeApData (
> {
> CPU_INFO_IN_HOB *CpuInfoInHob;
> MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
> + AP_STACK_DATA *ApStackData;
>
> CpuInfoInHob = (CPU_INFO_IN_HOB
> *)(UINTN)CpuMpData->CpuInfoInHob;
> CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
> @@ -578,6 +579,12 @@ InitializeApData (
> CpuInfoInHob[ProcessorNumber].Health = BistData;
> CpuInfoInHob[ProcessorNumber].ApTopOfStack = ApTopOfStack;
>
> + //
> + // AP_STACK_DATA is stored at the top of AP Stack
> + //
> + ApStackData = (AP_STACK_DATA *)((UINTN)ApTopOfStack - sizeof
> (AP_STACK_DATA));
> + ApStackData->MpData = CpuMpData;
> +
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ?
> TRUE : FALSE;
>
> @@ -623,6 +630,7 @@ ApWakeupFunction (
> CPU_INFO_IN_HOB *CpuInfoInHob;
> UINT64 ApTopOfStack;
> UINTN CurrentApicMode;
> + AP_STACK_DATA *ApStackData;
>
> //
> // AP finished assembly code and begin to execute C code
> @@ -648,7 +656,9 @@ ApWakeupFunction (
> // This is first time AP wakeup, get BIST information from AP stack
> //
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) *
> CpuMpData->CpuApStackSize;
> - BistData = *(UINT32 *)((UINTN)ApTopOfStack - sizeof (UINTN));
> + ApStackData = (AP_STACK_DATA *)((UINTN)ApTopOfStack - sizeof
> (AP_STACK_DATA));
> + BistData = (UINT32)ApStackData->Bist;
> +
> //
> // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP
> environment,
> // to initialize AP in InitConfig path.
> @@ -1796,14 +1806,22 @@ 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);
>
> - BufferSize = ApStackSize * MaxLogicalProcessorNumber;
> + BufferSize = ApStackSize * MaxLogicalProcessorNumber;
> + //
> + // Allocate extra ApStackSize to let AP stack align on ApStackSize bounday
> + //
> + BufferSize += ApStackSize;
> BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
> BufferSize += ApResetVectorSizeBelow1Mb;
> BufferSize = ALIGN_VALUE (BufferSize, 8);
> @@ -1813,13 +1831,13 @@ MpInitLibInitialize (
> MpBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
> ASSERT (MpBuffer != NULL);
> ZeroMem (MpBuffer, BufferSize);
> - Buffer = (UINTN)MpBuffer;
> + Buffer = ALIGN_VALUE ((UINTN)MpBuffer, ApStackSize);
>
> //
> - // The layout of the Buffer is as below:
> + // The layout of the Buffer is as below (lower address on top):
> //
> - // +--------------------+ <-- Buffer
> - // AP Stacks (N)
> + // +--------------------+ <-- Buffer (Pointer of CpuMpData is stored in
> the
> top of each AP's stack.)
> + // AP Stacks (N) (StackTop = (RSP + ApStackSize) &
> ~ApStackSize))
> // +--------------------+ <-- MonitorBuffer
> // AP Monitor Filters (N)
> // +--------------------+ <-- BackupBufferAddr (CpuMpData->BackupBuffer)
> @@ -1827,7 +1845,7 @@ MpInitLibInitialize (
> // +--------------------+
> // Padding
> // +--------------------+ <-- ApIdtBase (8-byte boundary)
> - // AP IDT All APs share one separate IDT. So AP can get
> address of
> CPU_MP_DATA from IDT Base.
> + // AP IDT All APs share one separate IDT.
> // +--------------------+ <-- CpuMpData
> // CPU_MP_DATA
> // +--------------------+ <-- CpuMpData->CpuData
> @@ -1864,10 +1882,11 @@ MpInitLibInitialize (
>
> //
> // Make sure no memory usage outside of the allocated buffer.
> + // (ApStackSize - (Buffer - (UINTN)MpBuffer)) is the redundant caused by
> alignment
> //
> ASSERT (
> (CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) *
> MaxLogicalProcessorNumber) ==
> - Buffer + BufferSize
> + (UINTN)MpBuffer + BufferSize - (ApStackSize - Buffer +
> (UINTN)MpBuffer)
> );
>
> //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 974fb76019..69b621a340 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -302,6 +302,14 @@ struct _CPU_MP_DATA {
> UINT64 GhcbBase;
> };
>
> +//
> +// AP_STACK_DATA is stored at the top of each AP stack.
> +//
> +typedef struct {
> + UINTN Bist;
> + CPU_MP_DATA *MpData;
> +} AP_STACK_DATA;
> +
> #define AP_SAFE_STACK_SIZE 128
> #define AP_RESET_STACK_SIZE AP_SAFE_STACK_SIZE
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 65400b95a2..e732371ddd 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -89,7 +89,7 @@ EnableDebugAgent (
> /**
> Get pointer to CPU MP Data structure.
> For BSP, the pointer is retrieved from HOB.
> - For AP, the structure is just after IDT.
> + For AP, the structure is stored in the top of each AP's stack.
>
> @return The pointer to CPU MP Data structure.
> **/
> @@ -100,15 +100,17 @@ GetCpuMpData (
> {
> CPU_MP_DATA *CpuMpData;
> MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr;
> - IA32_DESCRIPTOR Idtr;
> + UINTN ApTopOfStack;
> + AP_STACK_DATA *ApStackData;
>
> ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
> if (ApicBaseMsr.Bits.BSP == 1) {
> CpuMpData = GetCpuMpDataFromGuidedHob ();
> ASSERT (CpuMpData != NULL);
> } else {
> - AsmReadIdtr (&Idtr);
> - CpuMpData = (CPU_MP_DATA *)(Idtr.Base + Idtr.Limit + 1);
> + ApTopOfStack = ALIGN_VALUE ((UINTN)&ApTopOfStack,
> (UINTN)PcdGet32 (PcdCpuApStackSize));
> + ApStackData = (AP_STACK_DATA *)((UINTN)ApTopOfStack- sizeof
> (AP_STACK_DATA));
> + CpuMpData = (CPU_MP_DATA *)ApStackData->MpData;
> }
>
> return CpuMpData;
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 1daaa72b1e..6751cae6f1 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -237,11 +237,20 @@ ProgramStack:
> mov rsp, qword [rdi + CPU_INFO_IN_HOB.ApTopOfStack]
>
> CProcedureInvoke:
> + ;
> + ; Reserve 8 bytes for CpuMpData.
> + ; When the AP wakes up again via INIT-SIPI-SIPI, push 0 will cause the
> existing CpuMpData to be overwritten with 0.
> + ; CpuMpData is filled in via InitializeApData() during the first time
> INIT-
> SIPI-SIPI,
> + ; while overwirrten may occurs when under ApInHltLoop but InitFlag is
> not set to ApInitConfig.
> + ; Therefore reservation is implemented by sub rsp instead of push 0.
> + ;
> + sub rsp, 8
> push rbp ; Push BIST data at top of AP stack
> xor rbp, rbp ; Clear ebp for call stack trace
> push rbp
> mov rbp, rsp
>
> + push qword 0 ; Push 8 bytes for alignment
> mov rax, qword [esi + MP_CPU_EXCHANGE_INFO_FIELD
> (InitializeFloatingPointUnits)]
> sub rsp, 20h
> call rax ; Call assembly function to initialize FPU
> per UEFI spec
> --
> 2.36.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92801): https://edk2.groups.io/g/devel/message/92801
Mute This Topic: https://groups.io/mt/93119724/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
