diff mbox

[edk2,4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses

Message ID 20161117001754.4383-5-lersek@redhat.com
State New
Headers show

Commit Message

Laszlo Ersek Nov. 17, 2016, 12:17 a.m. UTC 
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(-)

-- 
2.9.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Nov. 17, 2016, 10:01 a.m. UTC | #1 
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

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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