* [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build @ 2022-02-13 16:01 Ashraf Ali S 2022-02-14 8:38 ` [edk2-devel] " Ni, Ray 2022-02-22 15:42 ` Ma, Maurice 0 siblings, 2 replies; 4+ messages in thread From: Ashraf Ali S @ 2022-02-13 16:01 UTC (permalink / raw) To: devel Cc: Ashraf Ali S, Chasel Chiu, Nate DeSimone, Star Zeng, Kuo Ted, Duggapu Chinni B, Rangasai V Chaganty, Digant H Solanki, Sangeetha V 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.inf @@ -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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build 2022-02-13 16:01 [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build Ashraf Ali S @ 2022-02-14 8:38 ` Ni, Ray 2022-02-22 4:16 ` Chiu, Chasel 2022-02-22 15:42 ` Ma, Maurice 1 sibling, 1 reply; 4+ messages in thread From: Ni, Ray @ 2022-02-14 8:38 UTC (permalink / raw) To: devel@edk2.groups.io, S, Ashraf Ali Cc: Chiu, Chasel, Desimone, Nathaniel L, Zeng, Star, Kuo, Ted, Duggapu, Chinni B, Chaganty, Rangasai V, Solanki, Digant H, V, Sangeetha The patch itself looks good to me. But when I read the code further, the SwapStack() is implemented as below. I think you need to enhance it to support the case when RSP is above 4GB. 1. Change UINT32 NewStack to UINTN 2. Change CoreStack from UINT32 to UINTN. UINT32 SwapStack ( IN UINT32 NewStack ) { FSP_GLOBAL_DATA *FspData; UINT32 OldStack; FspData = GetFspGlobalDataPointer (); OldStack = FspData->CoreStack; FspData->CoreStack = NewStack; return OldStack; } -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ashraf Ali S Sent: Monday, February 14, 2022 12:02 AM 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.inf @@ -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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build 2022-02-14 8:38 ` [edk2-devel] " Ni, Ray @ 2022-02-22 4:16 ` Chiu, Chasel 0 siblings, 0 replies; 4+ messages in thread From: Chiu, Chasel @ 2022-02-22 4:16 UTC (permalink / raw) To: Ni, Ray, S, Ashraf Ali, Desimone, Nathaniel L, Ma, Maurice Cc: Zeng, Star, Kuo, Ted, Duggapu, Chinni B, Chaganty, Rangasai V, Solanki, Digant H, V, Sangeetha, devel@edk2.groups.io Thanks for good catch, Ray! @Desimone, Nathaniel L and @Ma, Maurice, please also share your comments for this change. I think we could directly change CoreStack size and implementation to support 64bits build because both FspGlobalData and SwapStack() are FSP private/internal usage. @S, Ashraf Ali, I also add some more feedbacks in below patch inline, please help to check and verify. Thanks, Chasel IntelFsp2Pkg\Include\FspGlobalData.h: typedef struct { UINT32 Signature; UINT8 Version; UINT8 Reserved1[3]; UINT32 CoreStack; => change to "UINT64 CoreStack;" can be used by 32bit build too. UINT32 StatusCode; UINT32 Reserved2[8]; IntelFsp2Pkg\Library\BaseFspSwitchStackLib\FspSwitchStackLib.c: UINT32 => UINTN SwapStack ( IN UINT32 NewStack => UINTN ) { FSP_GLOBAL_DATA *FspData; UINT32 OldStack; => UINTN FspData = GetFspGlobalDataPointer (); OldStack = FspData->CoreStack; FspData->CoreStack = NewStack; return OldStack; } > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Monday, February 14, 2022 4:38 PM > To: devel@edk2.groups.io; S, Ashraf Ali <ashraf.ali.s@intel.com> > Cc: 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: RE: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support > for X64 Build > > The patch itself looks good to me. > But when I read the code further, the SwapStack() is implemented as below. > I think you need to enhance it to support the case when RSP is above 4GB. > 1. Change UINT32 NewStack to UINTN > 2. Change CoreStack from UINT32 to UINTN. > > UINT32 > SwapStack ( > IN UINT32 NewStack > ) > { > FSP_GLOBAL_DATA *FspData; > UINT32 OldStack; > > FspData = GetFspGlobalDataPointer (); > OldStack = FspData->CoreStack; > FspData->CoreStack = NewStack; > return OldStack; > } > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ashraf Ali S > Sent: Monday, February 14, 2022 12:02 AM > 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): I think in this function we should change all esp to rsp. > + ; 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 I believe here you should add 8 > + ret > + > -- > 2.30.2.windows.1 > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build 2022-02-13 16:01 [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build Ashraf Ali S 2022-02-14 8:38 ` [edk2-devel] " Ni, Ray @ 2022-02-22 15:42 ` Ma, Maurice 1 sibling, 0 replies; 4+ messages in thread From: Ma, Maurice @ 2022-02-22 15:42 UTC (permalink / raw) To: S, Ashraf Ali Cc: Chiu, Chasel, Desimone, Nathaniel L, Zeng, Star, Kuo, Ted, Duggapu, Chinni B, Chaganty, Rangasai V, Solanki, Digant H, V, Sangeetha, devel@edk2.groups.io 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 > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-22 15:42 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-13 16:01 [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build Ashraf Ali S 2022-02-14 8:38 ` [edk2-devel] " Ni, Ray 2022-02-22 4:16 ` Chiu, Chasel 2022-02-22 15:42 ` Ma, Maurice
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox