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...@intel.com> > Sent: Monday, February 14, 2022 4:38 PM > To: devel@edk2.groups.io; S, Ashraf Ali <ashraf.al...@intel.com> > Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Zeng, Star <star.z...@intel.com>; Kuo, Ted > <ted....@intel.com>; Duggapu, Chinni B <chinni.b.dugg...@intel.com>; > Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Solanki, Digant H > <digant.h.sola...@intel.com>; V, Sangeetha <sangeeth...@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.al...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; > Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Zeng, Star > <star.z...@intel.com>; Kuo, Ted <ted....@intel.com>; Duggapu, Chinni B > <chinni.b.dugg...@intel.com>; Chaganty, Rangasai V > <rangasai.v.chaga...@intel.com>; Solanki, Digant H > <digant.h.sola...@intel.com>; V, Sangeetha <sangeeth...@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.c...@intel.com> > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Cc: Kuo Ted <ted....@intel.com> > Cc: Duggapu Chinni B <chinni.b.dugg...@intel.com> > Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com> > Cc: Digant H Solanki <digant.h.sola...@intel.com> > Cc: Sangeetha V <sangeeth...@intel.com> > > Signed-off-by: Ashraf Ali S <ashraf.al...@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 > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#86845): https://edk2.groups.io/g/devel/message/86845 Mute This Topic: https://groups.io/mt/89115596/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-