Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: support 64-bit AP stack addresses

[Laszlo Ersek <lersek@redhat.com>]

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
>