* [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
* [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
* [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
* [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 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
* 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
* 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
* 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