* [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64
@ 2016-11-17 0:17 Laszlo Ersek
2016-11-17 0:17 ` [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Laszlo Ersek @ 2016-11-17 0:17 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jeff Fan
Recent commit 845c5be1fd9b ("UefiCpuPkg/MpInitLib: Program AP stack in
fixed address") has regressed the X64 build of OVMF; the 64-bit build of
CpuMpPei wouldn't boot.
Patches #1 and #2 clean up small nits in MpInitLib that I noticed while
analyzing the code.
Patch #3 fixes the regression.
Patch #4 fixes another bug that is currently masked in all builds of
OVMF (because all builds of OVMF include CpuMpPei), but which
nonetheless exists. I found it by code analysis.
Repo: https://github.com/lersek/edk2.git
Branch: ap_stack_64bit
Cc: Jeff Fan <jeff.fan@intel.com>
Thanks!
Laszlo
Laszlo Ersek (4):
UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency
UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction
UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo
UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 -
UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++++----
UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 +-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 +++-----
5 files changed, 11 insertions(+), 12 deletions(-)
--
2.9.2
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency 2016-11-17 0:17 [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64 Laszlo Ersek @ 2016-11-17 0:17 ` Laszlo Ersek 2016-11-17 0:46 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction Laszlo Ersek ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2016-11-17 0:17 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan Cc: Jeff Fan <jeff.fan@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 - 1 file changed, 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf index 972c9ad7edba..11b230174ec8 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf @@ -51,13 +51,12 @@ [LibraryClasses] MemoryAllocationLib HobLib MtrrLib CpuLib UefiCpuLib UefiBootServicesTableLib - HobLib [Guids] gEfiEventExitBootServicesGuid ## CONSUMES ## Event gEfiEventLegacyBootGuid ## CONSUMES ## Event [Pcd] -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency 2016-11-17 0:17 ` [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Laszlo Ersek @ 2016-11-17 0:46 ` Fan, Jeff 0 siblings, 0 replies; 10+ messages in thread From: Fan, Jeff @ 2016-11-17 0:46 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Reviewed-by: Jeff Fan <jeff.fan@intel.com> -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, November 17, 2016 8:18 AM To: edk2-devel-01 Cc: Fan, Jeff Subject: [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Cc: Jeff Fan <jeff.fan@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 - 1 file changed, 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf index 972c9ad7edba..11b230174ec8 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf @@ -51,13 +51,12 @@ [LibraryClasses] MemoryAllocationLib HobLib MtrrLib CpuLib UefiCpuLib UefiBootServicesTableLib - HobLib [Guids] gEfiEventExitBootServicesGuid ## CONSUMES ## Event gEfiEventLegacyBootGuid ## CONSUMES ## Event [Pcd] -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction 2016-11-17 0:17 [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64 Laszlo Ersek 2016-11-17 0:17 ` [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Laszlo Ersek @ 2016-11-17 0:17 ` Laszlo Ersek 2016-11-17 0:46 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo Laszlo Ersek 2016-11-17 0:17 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses Laszlo Ersek 3 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2016-11-17 0:17 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan At this point, ESI still has the value from EBX. Cc: Jeff Fan <jeff.fan@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 1 - 1 file changed, 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index bfc3ff1f5c7a..6a8794d83b5d 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -122,13 +122,12 @@ LongModeStart: mov esi, ebx lea edi, [esi + InitFlagLocation] cmp qword [edi], 1 ; ApInitConfig jnz GetApicId ; AP init - mov esi, ebx mov edi, esi add edi, LockLocation mov rax, NotVacantFlag TestLock: xchg qword [edi], rax -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction 2016-11-17 0:17 ` [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction Laszlo Ersek @ 2016-11-17 0:46 ` Fan, Jeff 0 siblings, 0 replies; 10+ messages in thread From: Fan, Jeff @ 2016-11-17 0:46 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Reviewed-by: Jeff Fan <jeff.fan@intel.com> -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, November 17, 2016 8:18 AM To: edk2-devel-01 Cc: Fan, Jeff Subject: [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction At this point, ESI still has the value from EBX. Cc: Jeff Fan <jeff.fan@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 1 - 1 file changed, 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index bfc3ff1f5c7a..6a8794d83b5d 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -122,13 +122,12 @@ LongModeStart: mov esi, ebx lea edi, [esi + InitFlagLocation] cmp qword [edi], 1 ; ApInitConfig jnz GetApicId ; AP init - mov esi, ebx mov edi, esi add edi, LockLocation mov rax, NotVacantFlag TestLock: xchg qword [edi], rax -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo 2016-11-17 0:17 [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64 Laszlo Ersek 2016-11-17 0:17 ` [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Laszlo Ersek 2016-11-17 0:17 ` [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction Laszlo Ersek @ 2016-11-17 0:17 ` Laszlo Ersek 2016-11-17 0:56 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses Laszlo Ersek 3 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2016-11-17 0:17 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan The recent patch "UefiCpuPkg/MpInitLib: Program AP stack in fixed address" inadvertently broke the first startup of APs during X64 PEI, because in the TestLock section of the code, it replaced the access to the NumApsExecuting counter with an access to the unrelated InitFlag field. Cc: Jeff Fan <jeff.fan@intel.com> Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index 6a8794d83b5d..138b97312b1d 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -131,13 +131,13 @@ LongModeStart: TestLock: xchg qword [edi], rax cmp rax, NotVacantFlag jz TestLock - lea ecx, [esi + InitFlagLocation] + lea ecx, [esi + NumApsExecutingLocation] inc dword [ecx] mov ebx, [ecx] Releaselock: mov rax, VacantFlag xchg qword [edi], rax -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo 2016-11-17 0:17 ` [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo Laszlo Ersek @ 2016-11-17 0:56 ` Fan, Jeff 0 siblings, 0 replies; 10+ messages in thread From: Fan, Jeff @ 2016-11-17 0:56 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Reviewed-by: Jeff Fan <jeff.fan@intel.com> Thanks! -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, November 17, 2016 8:18 AM To: edk2-devel-01 Cc: Fan, Jeff Subject: [edk2] [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo The recent patch "UefiCpuPkg/MpInitLib: Program AP stack in fixed address" inadvertently broke the first startup of APs during X64 PEI, because in the TestLock section of the code, it replaced the access to the NumApsExecuting counter with an access to the unrelated InitFlag field. Cc: Jeff Fan <jeff.fan@intel.com> Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index 6a8794d83b5d..138b97312b1d 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -131,13 +131,13 @@ LongModeStart: TestLock: xchg qword [edi], rax cmp rax, NotVacantFlag jz TestLock - lea ecx, [esi + InitFlagLocation] + lea ecx, [esi + NumApsExecutingLocation] inc dword [ecx] mov ebx, [ecx] Releaselock: mov rax, VacantFlag xchg qword [edi], rax -- 2.9.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses 2016-11-17 0:17 [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64 Laszlo Ersek ` (2 preceding siblings ...) 2016-11-17 0:17 ` [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo Laszlo Ersek @ 2016-11-17 0:17 ` Laszlo Ersek 2016-11-17 1:18 ` Fan, Jeff 3 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2016-11-17 0:17 UTC (permalink / raw) To: edk2-devel-01; +Cc: Jeff Fan The cached "CPU_INFO_IN_HOB.ApTopOfStack" field currently has type UINT32. This is not ideal because the AP stacks are located within "CpuMpData->Buffer", which is allocated with a plain AllocatePages() call in MpInitLibInitialize(): platform CpuMpPei included PEI RAM > 4GB result -------- ----------------- ------------- ------ Ia32 * n/a good Ia32X64 no n/a BAD Ia32X64 yes n/a good X64 no * BAD X64 yes no good X64 yes yes BAD - If we are on an Ia32X64 or X64 platform that does not include CpuMpPei, then CpuDxe cannot reuse the CPU_INFO_IN_HOB structures preallocated by CpuMpPei (through the CpuInitMpLib GUID HOB), and then AllocatePages() -- invoked first in 64-bit DXE -- could return an address outside of 32-bit address space. - If we are on an X64 platform where the permanent PEI RAM extends above the 32-bit address space, then the same issue can surface even if CpuMpPei is included: even the original allocation of the CPU_INFO_IN_HOB structures, by CpuMpPei, could be satisfied from above 4GB. The original "AP init" branch in "X64/MpFuncs.nasm" correctly considers a 64-bit stack start: the "MP_CPU_EXCHANGE_INFO.StackStart" field has type UINTN, and the code uses QWORD addition and movement to set RSP from it. Adapt the "GetApicId" branch of "X64/MpFuncs.nasm": - change the type of "CPU_INFO_IN_HOB.ApTopOfStack" to UINT64, - remove the explicit truncation to UINT32 in InitializeApData(), - update the "GetNextProcNumber" iteration size to the new size of "CPU_INFO_IN_HOB", - set RSP with a QWORD movement from "CPU_INFO_IN_HOB.ApTopOfStack". Because the same CPU_INFO_IN_HOB structure is used by "Ia32/MpFuncs.nasm", we have to update the "GetNextProcNumber" iteration size there as well. The ESP setting can be preserved as a DWORD movement from the original offset (decimal 12), since our integers are little endian. Cc: Jeff Fan <jeff.fan@intel.com> Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++- UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++++---- UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 +- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 5 ++--- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h index 0ac777a099b1..f73a469ae84f 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h @@ -126,18 +126,20 @@ typedef struct { // // Basic CPU information saved in Guided HOB. // Because the contents will be shard between PEI and DXE, // we need to make sure the each fields offset same in different // architecture. // +#pragma pack (1) typedef struct { UINT32 InitialApicId; UINT32 ApicId; UINT32 Health; - UINT32 ApTopOfStack; + UINT64 ApTopOfStack; } CPU_INFO_IN_HOB; +#pragma pack () // // AP reset code information including code address and size, // this structure will be shared be C code and assembly code. // It is natural aligned by design. // diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 3c2e6d6b89d9..15dbfa1e7d6c 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -430,22 +430,22 @@ CollectProcessorCount ( **/ VOID InitializeApData ( IN OUT CPU_MP_DATA *CpuMpData, IN UINTN ProcessorNumber, IN UINT32 BistData, - IN UINTN ApTopOfStack + IN UINT64 ApTopOfStack ) { CPU_INFO_IN_HOB *CpuInfoInHob; CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId (); CpuInfoInHob[ProcessorNumber].ApicId = GetApicId (); CpuInfoInHob[ProcessorNumber].Health = BistData; - CpuInfoInHob[ProcessorNumber].ApTopOfStack = (UINT32) ApTopOfStack; + CpuInfoInHob[ProcessorNumber].ApTopOfStack = ApTopOfStack; CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE; CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE; if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) { // // Set x2APIC mode if there are any logical processor reporting @@ -477,13 +477,13 @@ ApWakeupFunction ( UINTN ProcessorNumber; EFI_AP_PROCEDURE Procedure; VOID *Parameter; UINT32 BistData; volatile UINT32 *ApStartupSignalBuffer; CPU_INFO_IN_HOB *CpuInfoInHob; - UINTN ApTopOfStack; + UINT64 ApTopOfStack; // // AP finished assembly code and begin to execute C code // CpuMpData = ExchangeInfo->CpuMpData; @@ -497,13 +497,13 @@ ApWakeupFunction ( InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); ProcessorNumber = NumApsExecuting; // // This is first time AP wakeup, get BIST information from AP stack // ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; - BistData = *(UINT32 *) (ApTopOfStack - sizeof (UINTN)); + BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); // // Do some AP initialize sync // ApInitializeSync (CpuMpData); // // Sync BSP's Control registers to APs diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm index 4bfa084c85a9..64e51d87ae24 100644 --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm @@ -178,13 +178,13 @@ GetProcessorNumber: lea eax, [esi + CpuInfoLocation] mov edi, [eax] GetNextProcNumber: cmp [edi], edx ; APIC ID match? jz ProgramStack - add edi, 16 + add edi, 20 inc ebx jmp GetNextProcNumber ProgramStack: mov esp, [edi + 12] diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index 138b97312b1d..aaabb50c5468 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -182,19 +182,18 @@ GetProcessorNumber: lea eax, [esi + CpuInfoLocation] mov edi, [eax] GetNextProcNumber: cmp dword [edi], edx ; APIC ID match? jz ProgramStack - add edi, 16 + add edi, 20 inc ebx jmp GetNextProcNumber ProgramStack: - xor rsp, rsp - mov esp, dword [edi + 12] + mov rsp, qword [edi + 12] CProcedureInvoke: push rbp ; Push BIST data at top of AP stack xor rbp, rbp ; Clear ebp for call stack trace push rbp mov rbp, rsp -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses 2016-11-17 0:17 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses Laszlo Ersek @ 2016-11-17 1:18 ` Fan, Jeff 2016-11-17 10:01 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Fan, Jeff @ 2016-11-17 1:18 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01 Laszlo, We have two solutions to fix stack > 4G issue. 1. Allocate AP stack buffer and all CPU MP data buffer under < 4G at the beginning. 2. Support AP stack buffer and all CPU MP data buffer > 4G as showed in your patch. For 1), it seems not necessary. For 2), besides your patch. We still need to update RelocateApLoop() in DxeMpLib.c to use one separate stack under 4G when paging disabled on long mode DXE. (Currently, we still use AP existing stack after paging disabled) I prefer the 2), please go ahead to check-in this serial of patch. I will create another patch to fix RelocateApLoop() stack issue. Reviewed-by: Jeff Fan <jeff.fan@intel.com> Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, November 17, 2016 8:18 AM To: edk2-devel-01 Cc: Fan, Jeff Subject: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses The cached "CPU_INFO_IN_HOB.ApTopOfStack" field currently has type UINT32. This is not ideal because the AP stacks are located within "CpuMpData->Buffer", which is allocated with a plain AllocatePages() call in MpInitLibInitialize(): platform CpuMpPei included PEI RAM > 4GB result -------- ----------------- ------------- ------ Ia32 * n/a good Ia32X64 no n/a BAD Ia32X64 yes n/a good X64 no * BAD X64 yes no good X64 yes yes BAD - If we are on an Ia32X64 or X64 platform that does not include CpuMpPei, then CpuDxe cannot reuse the CPU_INFO_IN_HOB structures preallocated by CpuMpPei (through the CpuInitMpLib GUID HOB), and then AllocatePages() -- invoked first in 64-bit DXE -- could return an address outside of 32-bit address space. - If we are on an X64 platform where the permanent PEI RAM extends above the 32-bit address space, then the same issue can surface even if CpuMpPei is included: even the original allocation of the CPU_INFO_IN_HOB structures, by CpuMpPei, could be satisfied from above 4GB. The original "AP init" branch in "X64/MpFuncs.nasm" correctly considers a 64-bit stack start: the "MP_CPU_EXCHANGE_INFO.StackStart" field has type UINTN, and the code uses QWORD addition and movement to set RSP from it. Adapt the "GetApicId" branch of "X64/MpFuncs.nasm": - change the type of "CPU_INFO_IN_HOB.ApTopOfStack" to UINT64, - remove the explicit truncation to UINT32 in InitializeApData(), - update the "GetNextProcNumber" iteration size to the new size of "CPU_INFO_IN_HOB", - set RSP with a QWORD movement from "CPU_INFO_IN_HOB.ApTopOfStack". Because the same CPU_INFO_IN_HOB structure is used by "Ia32/MpFuncs.nasm", we have to update the "GetNextProcNumber" iteration size there as well. The ESP setting can be preserved as a DWORD movement from the original offset (decimal 12), since our integers are little endian. Cc: Jeff Fan <jeff.fan@intel.com> Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++- UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++++---- UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 +- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 5 ++--- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h index 0ac777a099b1..f73a469ae84f 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h @@ -126,18 +126,20 @@ typedef struct { // // Basic CPU information saved in Guided HOB. // Because the contents will be shard between PEI and DXE, // we need to make sure the each fields offset same in different // architecture. // +#pragma pack (1) typedef struct { UINT32 InitialApicId; UINT32 ApicId; UINT32 Health; - UINT32 ApTopOfStack; + UINT64 ApTopOfStack; } CPU_INFO_IN_HOB; +#pragma pack () // // AP reset code information including code address and size, // this structure will be shared be C code and assembly code. // It is natural aligned by design. // diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index 3c2e6d6b89d9..15dbfa1e7d6c 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -430,22 +430,22 @@ CollectProcessorCount ( **/ VOID InitializeApData ( IN OUT CPU_MP_DATA *CpuMpData, IN UINTN ProcessorNumber, IN UINT32 BistData, - IN UINTN ApTopOfStack + IN UINT64 ApTopOfStack ) { CPU_INFO_IN_HOB *CpuInfoInHob; CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId (); CpuInfoInHob[ProcessorNumber].ApicId = GetApicId (); CpuInfoInHob[ProcessorNumber].Health = BistData; - CpuInfoInHob[ProcessorNumber].ApTopOfStack = (UINT32) ApTopOfStack; + CpuInfoInHob[ProcessorNumber].ApTopOfStack = ApTopOfStack; CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE; CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE; if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) { // // Set x2APIC mode if there are any logical processor reporting @@ -477,13 +477,13 @@ ApWakeupFunction ( UINTN ProcessorNumber; EFI_AP_PROCEDURE Procedure; VOID *Parameter; UINT32 BistData; volatile UINT32 *ApStartupSignalBuffer; CPU_INFO_IN_HOB *CpuInfoInHob; - UINTN ApTopOfStack; + UINT64 ApTopOfStack; // // AP finished assembly code and begin to execute C code // CpuMpData = ExchangeInfo->CpuMpData; @@ -497,13 +497,13 @@ ApWakeupFunction ( InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); ProcessorNumber = NumApsExecuting; // // This is first time AP wakeup, get BIST information from AP stack // ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; - BistData = *(UINT32 *) (ApTopOfStack - sizeof (UINTN)); + BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); // // Do some AP initialize sync // ApInitializeSync (CpuMpData); // // Sync BSP's Control registers to APs diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm index 4bfa084c85a9..64e51d87ae24 100644 --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm @@ -178,13 +178,13 @@ GetProcessorNumber: lea eax, [esi + CpuInfoLocation] mov edi, [eax] GetNextProcNumber: cmp [edi], edx ; APIC ID match? jz ProgramStack - add edi, 16 + add edi, 20 inc ebx jmp GetNextProcNumber ProgramStack: mov esp, [edi + 12] diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm index 138b97312b1d..aaabb50c5468 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm @@ -182,19 +182,18 @@ GetProcessorNumber: lea eax, [esi + CpuInfoLocation] mov edi, [eax] GetNextProcNumber: cmp dword [edi], edx ; APIC ID match? jz ProgramStack - add edi, 16 + add edi, 20 inc ebx jmp GetNextProcNumber ProgramStack: - xor rsp, rsp - mov esp, dword [edi + 12] + mov rsp, qword [edi + 12] CProcedureInvoke: push rbp ; Push BIST data at top of AP stack xor rbp, rbp ; Clear ebp for call stack trace push rbp mov rbp, rsp -- 2.9.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses 2016-11-17 1:18 ` Fan, Jeff @ 2016-11-17 10:01 ` Laszlo Ersek 0 siblings, 0 replies; 10+ messages in thread From: Laszlo Ersek @ 2016-11-17 10:01 UTC (permalink / raw) To: Fan, Jeff, edk2-devel-01 On 11/17/16 02:18, Fan, Jeff wrote: > Laszlo, > > We have two solutions to fix stack > 4G issue. > 1. Allocate AP stack buffer and all CPU MP data buffer under < 4G at the beginning. > 2. Support AP stack buffer and all CPU MP data buffer > 4G as showed in your patch. > > For 1), it seems not necessary. > For 2), besides your patch. We still need to update RelocateApLoop() in DxeMpLib.c to use one separate stack under 4G when paging disabled on long mode DXE. > (Currently, we still use AP existing stack after paging disabled) > I prefer the 2), please go ahead to check-in this serial of patch. I will create another patch to fix RelocateApLoop() stack issue. > > Reviewed-by: Jeff Fan <jeff.fan@intel.com> > Thank you, series committed as 97d2760429d6..dd3fa0cd72de. Cheers Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, November 17, 2016 8:18 AM > To: edk2-devel-01 > Cc: Fan, Jeff > Subject: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses > > The cached "CPU_INFO_IN_HOB.ApTopOfStack" field currently has type UINT32. > This is not ideal because the AP stacks are located within "CpuMpData->Buffer", which is allocated with a plain AllocatePages() call in MpInitLibInitialize(): > > platform CpuMpPei included PEI RAM > 4GB result > -------- ----------------- ------------- ------ > Ia32 * n/a good > Ia32X64 no n/a BAD > Ia32X64 yes n/a good > X64 no * BAD > X64 yes no good > X64 yes yes BAD > > - If we are on an Ia32X64 or X64 platform that does not include CpuMpPei, > then CpuDxe cannot reuse the CPU_INFO_IN_HOB structures preallocated by > CpuMpPei (through the CpuInitMpLib GUID HOB), and then AllocatePages() > -- invoked first in 64-bit DXE -- could return an address outside of > 32-bit address space. > > - If we are on an X64 platform where the permanent PEI RAM extends above > the 32-bit address space, then the same issue can surface even if > CpuMpPei is included: even the original allocation of the > CPU_INFO_IN_HOB structures, by CpuMpPei, could be satisfied from above > 4GB. > > The original "AP init" branch in "X64/MpFuncs.nasm" correctly considers a 64-bit stack start: the "MP_CPU_EXCHANGE_INFO.StackStart" field has type UINTN, and the code uses QWORD addition and movement to set RSP from it. > > Adapt the "GetApicId" branch of "X64/MpFuncs.nasm": > > - change the type of "CPU_INFO_IN_HOB.ApTopOfStack" to UINT64, > > - remove the explicit truncation to UINT32 in InitializeApData(), > > - update the "GetNextProcNumber" iteration size to the new size of > "CPU_INFO_IN_HOB", > > - set RSP with a QWORD movement from "CPU_INFO_IN_HOB.ApTopOfStack". > > Because the same CPU_INFO_IN_HOB structure is used by "Ia32/MpFuncs.nasm", we have to update the "GetNextProcNumber" iteration size there as well. > The ESP setting can be preserved as a DWORD movement from the original offset (decimal 12), since our integers are little endian. > > Cc: Jeff Fan <jeff.fan@intel.com> > Fixes: 845c5be1fd9bf7edfac4a103dfab70829686978f > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 4 +++- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++++---- > UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 +- UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 5 ++--- > 4 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 0ac777a099b1..f73a469ae84f 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -126,18 +126,20 @@ typedef struct { > // > // Basic CPU information saved in Guided HOB. > // Because the contents will be shard between PEI and DXE, // we need to make sure the each fields offset same in different // architecture. > // > +#pragma pack (1) > typedef struct { > UINT32 InitialApicId; > UINT32 ApicId; > UINT32 Health; > - UINT32 ApTopOfStack; > + UINT64 ApTopOfStack; > } CPU_INFO_IN_HOB; > +#pragma pack () > > // > // AP reset code information including code address and size, // this structure will be shared be C code and assembly code. > // It is natural aligned by design. > // > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 3c2e6d6b89d9..15dbfa1e7d6c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -430,22 +430,22 @@ CollectProcessorCount ( **/ VOID InitializeApData ( > IN OUT CPU_MP_DATA *CpuMpData, > IN UINTN ProcessorNumber, > IN UINT32 BistData, > - IN UINTN ApTopOfStack > + IN UINT64 ApTopOfStack > ) > { > CPU_INFO_IN_HOB *CpuInfoInHob; > > CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId (); > CpuInfoInHob[ProcessorNumber].ApicId = GetApicId (); > CpuInfoInHob[ProcessorNumber].Health = BistData; > - CpuInfoInHob[ProcessorNumber].ApTopOfStack = (UINT32) ApTopOfStack; > + CpuInfoInHob[ProcessorNumber].ApTopOfStack = ApTopOfStack; > > CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE; > CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE; > if (CpuInfoInHob[ProcessorNumber].InitialApicId >= 0xFF) { > // > // Set x2APIC mode if there are any logical processor reporting @@ -477,13 +477,13 @@ ApWakeupFunction ( > UINTN ProcessorNumber; > EFI_AP_PROCEDURE Procedure; > VOID *Parameter; > UINT32 BistData; > volatile UINT32 *ApStartupSignalBuffer; > CPU_INFO_IN_HOB *CpuInfoInHob; > - UINTN ApTopOfStack; > + UINT64 ApTopOfStack; > > // > // AP finished assembly code and begin to execute C code > // > CpuMpData = ExchangeInfo->CpuMpData; > > @@ -497,13 +497,13 @@ ApWakeupFunction ( > InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); > ProcessorNumber = NumApsExecuting; > // > // This is first time AP wakeup, get BIST information from AP stack > // > ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; > - BistData = *(UINT32 *) (ApTopOfStack - sizeof (UINTN)); > + BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); > // > // Do some AP initialize sync > // > ApInitializeSync (CpuMpData); > // > // Sync BSP's Control registers to APs diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 4bfa084c85a9..64e51d87ae24 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -178,13 +178,13 @@ GetProcessorNumber: > lea eax, [esi + CpuInfoLocation] > mov edi, [eax] > > GetNextProcNumber: > cmp [edi], edx ; APIC ID match? > jz ProgramStack > - add edi, 16 > + add edi, 20 > inc ebx > jmp GetNextProcNumber > > ProgramStack: > mov esp, [edi + 12] > > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index 138b97312b1d..aaabb50c5468 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -182,19 +182,18 @@ GetProcessorNumber: > lea eax, [esi + CpuInfoLocation] > mov edi, [eax] > > GetNextProcNumber: > cmp dword [edi], edx ; APIC ID match? > jz ProgramStack > - add edi, 16 > + add edi, 20 > inc ebx > jmp GetNextProcNumber > > ProgramStack: > - xor rsp, rsp > - mov esp, dword [edi + 12] > + mov rsp, qword [edi + 12] > > CProcedureInvoke: > push rbp ; Push BIST data at top of AP stack > xor rbp, rbp ; Clear ebp for call stack trace > push rbp > mov rbp, rsp > -- > 2.9.2 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-11-17 10:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-17 0:17 [PATCH 0/4] UefiCpuPkg: correct the fixed address AP stack programming on X64 Laszlo Ersek 2016-11-17 0:17 ` [PATCH 1/4] UefiCpuPkg/DxeMpInitLib: remove duplicate HobLib class dependency Laszlo Ersek 2016-11-17 0:46 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 2/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: remove superfluous instruction Laszlo Ersek 2016-11-17 0:46 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 3/4] UefiCpuPkg/MpInitLib/X64/MpFuncs.nasm: fix fatal typo Laszlo Ersek 2016-11-17 0:56 ` Fan, Jeff 2016-11-17 0:17 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses Laszlo Ersek 2016-11-17 1:18 ` Fan, Jeff 2016-11-17 10:01 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox