Re: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build

["Ma, Maurice" <maurice.ma@intel.com>]

Several comments as below:

1.  Need to ensure sufficient space to save IDT
     The following will only reserve 8 bytes on stack to save IDT. It is fine for x86, but for x64 mode, the SIDT needs at least 80 bits.  So we might want to reserve 16 byte for the changes listed below to void buffer overflow.
     Also it needs to use rsp instead.	
      +    sub     esp, 8
      +    sidt    [esp]
      .....
      +    lidt    [esp]
      +    add     esp, 8

2.  Ensure rsp is always aligned at 8.
     Change:
      +    add     esp, 4
     To:
      +    add     rsp, 8

3.  Function prototype needs to be 64bit.  The following comments will need to be updated from "UINT32" to "UINTN" to match both x86 and x64 mode.

     +;------------------------------------------------------------------------------
     +; UINT32
     +; EFIAPI
     +; Pei2LoaderSwitchStack (
     +;   VOID
     +;   )
     +;-------

     +
     +;------------------------------------------------------------------------------
     +; UINT32
     +; EFIAPI
     +; Loader2PeiSwitchStack (
     +;   VOID
     +;   )
     +;------------------------------------------------------------------------------

     +;------------------------------------------------------------------------------
     +; UINT32
     +; EFIAPI
     +; FspSwitchStack (
     +;   VOID
     +;   )
     +;---------

4.  Need to ensure UEFI x64 calling convention is followed.  UEFI x64 calling convention requires "A caller must always call with the stack 16 bytes aligned."
     So before calling SwapStack C function, the rsp needs to be aligned at 16 bytes.  So please verify if the changes satisfy this requirement.


Regards
Maurice

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ashraf
> Ali S
> Sent: Sunday, February 13, 2022 8:02
> To: devel@edk2.groups.io
> Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; Kuo,
> Ted <ted.kuo@intel.com>; Duggapu, Chinni B
> <chinni.b.duggapu@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Solanki, Digant H
> <digant.h.solanki@intel.com>; V, Sangeetha <sangeetha.v@intel.com>
> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support
> for X64 Build
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3832
> 
> BaseFspSwitchStackLib Currently Support for IA32 build only, adding support
> for X64 build, fix typecasting issues for X64 build.
> 0xFFFF_FFFF will be replaced by MAX_ADDRESS which is set based on the
> type of Library which is it building.
> if it's a IA32 MAX_ADDRESS = 0xFFFF_FFFF
> for X64        MAX_ADDRESS = 0xFFFF_FFFF_FFFF_FFFFULL
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kuo Ted <ted.kuo@intel.com>
> Cc: Duggapu Chinni B <chinni.b.duggapu@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Digant H Solanki <digant.h.solanki@intel.com>
> Cc: Sangeetha V <sangeetha.v@intel.com>
> 
> Signed-off-by: Ashraf Ali S <ashraf.ali.s@intel.com>
> ---
>  IntelFsp2Pkg/FspSecCore/SecFsp.h              |   3 +-
>  IntelFsp2Pkg/FspSecCore/SecFspApiChk.c        |  10 +-
>  .../BaseFspSwitchStackLib.inf                 |   7 +-
>  .../BaseFspSwitchStackLib/X64/Stack.nasm      | 124 ++++++++++++++++++
>  4 files changed, 136 insertions(+), 8 deletions(-)  create mode 100644
> IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
> 
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFsp.h
> b/IntelFsp2Pkg/FspSecCore/SecFsp.h
> index aacd32f7f7..9a6fc14d23 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFsp.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.h
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2022, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -10,6 +10,7 @@
> 
>  #include <PiPei.h>
>  #include <FspEas.h>
> +#include <Base.h>
>  #include <Library/PcdLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> index 7d6ef11fe7..b70d3ffcf1 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2022, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -31,7 +31,7 @@ FspApiCallingCheck (
>      //
>      // NotifyPhase check
>      //
> -    if ((FspData == NULL) || ((UINT32)FspData == 0xFFFFFFFF)) {
> +    if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS)) {
>        Status = EFI_UNSUPPORTED;
>      } else {
>        if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { @@ -42,7
> +42,7 @@ FspApiCallingCheck (
>      //
>      // FspMemoryInit check
>      //
> -    if ((UINT32)FspData != 0xFFFFFFFF) {
> +    if ((UINTN)FspData != MAX_ADDRESS) {
>        Status = EFI_UNSUPPORTED;
>      } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) {
>        Status = EFI_INVALID_PARAMETER;
> @@ -51,7 +51,7 @@ FspApiCallingCheck (
>      //
>      // TempRamExit check
>      //
> -    if ((FspData == NULL) || ((UINT32)FspData == 0xFFFFFFFF)) {
> +    if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS)) {
>        Status = EFI_UNSUPPORTED;
>      } else {
>        if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { @@ -62,7
> +62,7 @@ FspApiCallingCheck (
>      //
>      // FspSiliconInit check
>      //
> -    if ((FspData == NULL) || ((UINT32)FspData == 0xFFFFFFFF)) {
> +    if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS)) {
>        Status = EFI_UNSUPPORTED;
>      } else {
>        if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { diff --git
> a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/BaseFspSwitchStackLib.inf
> b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/BaseFspSwitchStackLib.inf
> index 3dcf3b9598..cd7d89e43a 100644
> --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/BaseFspSwitchStackLib.inf
> +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/BaseFspSwitchStackLib.i
> +++ nf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Instance of BaseFspSwitchStackLib
>  #
> -#  Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2014 - 2022, Intel Corporation. All rights
> +reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -15,12 +15,15 @@
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = FspSwitchStackLib
> 
> -[Sources.IA32]
> +[Sources]
>    FspSwitchStackLib.c
> 
>  [Sources.IA32]
>    Ia32/Stack.nasm
> 
> +[Sources.X64]
> +  X64/Stack.nasm
> +
>  [Packages]
>    MdePkg/MdePkg.dec
>    IntelFsp2Pkg/IntelFsp2Pkg.dec
> diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
> b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
> new file mode 100644
> index 0000000000..f94f39fc13
> --- /dev/null
> +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm
> @@ -0,0 +1,124 @@
> +;----------------------------------------------------------------------
> +--------
> +;
> +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ;
> +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract:
> +;
> +;   Switch the stack from temporary memory to permanent memory.
> +;
> +;----------------------------------------------------------------------
> +--------
> +
> +    SECTION .text
> +
> +extern ASM_PFX(SwapStack)
> +
> +;-----------------------------------------------------------------------------
> +;  Macro:        PUSHA_64
> +;
> +;  Description:  Saves all registers on stack ;
> +;  Input:        None
> +;
> +;  Output:       None
> +;----------------------------------------------------------------------
> +-------
> +%macro PUSHA_64 0
> +    push    rsp
> +    push    rbp
> +    push    rax
> +    push    rbx
> +    push    rcx
> +    push    rdx
> +    push    rsi
> +    push    rdi
> +    push    r8
> +    push    r9
> +    push    r10
> +    push    r11
> +    push    r12
> +    push    r13
> +    push    r14
> +    push    r15
> +%endmacro
> +
> +;-----------------------------------------------------------------------------
> +;  Macro:        POPA_64
> +;
> +;  Description:  Restores all registers from stack ;
> +;  Input:        None
> +;
> +;  Output:       None
> +;----------------------------------------------------------------------
> +-------
> +%macro POPA_64 0
> +    pop    r15
> +    pop    r14
> +    pop    r13
> +    pop    r12
> +    pop    r11
> +    pop    r10
> +    pop    r9
> +    pop    r8
> +    pop    rdi
> +    pop    rsi
> +    pop    rdx
> +    pop    rcx
> +    pop    rbx
> +    pop    rax
> +    pop    rbp
> +    pop    rsp
> +%endmacro
> +
> +;----------------------------------------------------------------------
> +--------
> +; UINT32
> +; EFIAPI
> +; Pei2LoaderSwitchStack (
> +;   VOID
> +;   )
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(Pei2LoaderSwitchStack)
> +ASM_PFX(Pei2LoaderSwitchStack):
> +    xor     rax, rax
> +    jmp     ASM_PFX(FspSwitchStack)
> +
> +;----------------------------------------------------------------------
> +--------
> +; UINT32
> +; EFIAPI
> +; Loader2PeiSwitchStack (
> +;   VOID
> +;   )
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(Loader2PeiSwitchStack)
> +ASM_PFX(Loader2PeiSwitchStack):
> +    jmp     ASM_PFX(FspSwitchStack)
> +
> +;----------------------------------------------------------------------
> +--------
> +; UINT32
> +; EFIAPI
> +; FspSwitchStack (
> +;   VOID
> +;   )
> +;----------------------------------------------------------------------
> +--------
> +global ASM_PFX(FspSwitchStack)
> +ASM_PFX(FspSwitchStack):
> +    ; Save current contexts
> +    push    rax
> +    pushfq
> +    cli
> +    PUSHA_64
> +    sub     esp, 8
> +    sidt    [esp]
> +
> +    ; Load new stack
> +    mov     rcx, rsp
> +    call    ASM_PFX(SwapStack)
> +    mov     rsp, rax
> +
> +    ; Restore previous contexts
> +    lidt    [esp]
> +    add     esp, 8
> +    POPA_64
> +    popfq
> +    add     esp, 4
> +    ret
> +
> --
> 2.30.2.windows.1
> 
> 
> 
> 
>