.../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(-)
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(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 28301bb8f0..4714f2d527 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -179,6 +179,11 @@ ProgramStack:
mov esp, dword [edi + CPU_INFO_IN_HOB.ApTopOfStack]
CProcedureInvoke:
+ ;
+ ; Reserve 4 bytes for storing CpuMpData.
+ ; Using sub esp instead of push ebp to avoid overwriting the existed CpuMpData
+ ;
+ 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..a9188eb59f 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,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));
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 +1848,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
@@ -1866,8 +1887,8 @@ MpInitLibInitialize (
// Make sure no memory usage outside of the allocated buffer.
//
ASSERT (
- (CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) ==
- Buffer + BufferSize
+ (CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) <=
+ (UINTN)MpBuffer + BufferSize
);
//
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..322bdd03e6 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -237,11 +237,17 @@ ProgramStack:
mov rsp, qword [rdi + CPU_INFO_IN_HOB.ApTopOfStack]
CProcedureInvoke:
+ ;
+ ; Reserve 8 bytes for storing CpuMpData.
+ ; Using sub rsp instead of push rbp to avoid overwriting existed CpuMpData
+ ;
+ 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 (#92473): https://edk2.groups.io/g/devel/message/92473
Mute This Topic: https://groups.io/mt/93054601/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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); > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#92867): https://edk2.groups.io/g/devel/message/92867 Mute This Topic: https://groups.io/mt/93054601/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> - MpBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize)); > + // > + // Allocate extra ApStackSize to let stack align on ApStackSize bounday > + // > + MpBuffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize)); 1. Above comments are not necessary. Can you please remove them? > ASSERT ( > - (CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) * > MaxLogicalProcessorNumber) == > - Buffer + BufferSize > + (CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) * > MaxLogicalProcessorNumber) <= > + (UINTN)MpBuffer + BufferSize > ); 2. Can you try changing "<=" back to "=="? I agree that "Buffer" should be changed to "MpBuffer" in above check. > - For AP, the structure is just after IDT. > + For AP, the structure is stored in the top of each AP's stack. 3. Can you change it to "For AP, the pointer of CPU_MP_DATA is stored in top of each AP's stack."? > + ; > + ; Reserve 8 bytes for storing CpuMpData. > + ; Using sub rsp instead of push rbp to avoid overwriting existed > CpuMpData 4. Can you explain a bit more in which case the overwritten may happen? I guess the overwritten may happen when the PcdCpuApLoopMode is ApInHltLoop but pointer of CpuMpData is only stored in top of AP stack in first time INIT-SIPI-SIPI. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#92474): https://edk2.groups.io/g/devel/message/92474 Mute This Topic: https://groups.io/mt/93054601/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.