[edk2-devel] [PATCH] IntelFsp2Pkg: BaseFspSwitchStackLib Support for X64 Build
Chiu, Chasel
chasel.chiu at intel.com
Tue Feb 22 04:16:54 UTC 2022
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 at intel.com>
> Sent: Monday, February 14, 2022 4:38 PM
> To: devel at edk2.groups.io; S, Ashraf Ali <ashraf.ali.s at intel.com>
> Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone at intel.com>; Zeng, Star <star.zeng at intel.com>; Kuo, Ted
> <ted.kuo at intel.com>; Duggapu, Chinni B <chinni.b.duggapu at intel.com>;
> Chaganty, Rangasai V <rangasai.v.chaganty at intel.com>; Solanki, Digant H
> <digant.h.solanki at intel.com>; V, Sangeetha <sangeetha.v at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Ashraf Ali S
> Sent: Monday, February 14, 2022 12:02 AM
> To: devel at edk2.groups.io
> Cc: S, Ashraf Ali <ashraf.ali.s at intel.com>; Chiu, Chasel <chasel.chiu at intel.com>;
> Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Zeng, Star
> <star.zeng at intel.com>; Kuo, Ted <ted.kuo at intel.com>; Duggapu, Chinni B
> <chinni.b.duggapu at intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty at intel.com>; Solanki, Digant H
> <digant.h.solanki at intel.com>; V, Sangeetha <sangeetha.v at 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 at intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Cc: Kuo Ted <ted.kuo at intel.com>
> Cc: Duggapu Chinni B <chinni.b.duggapu at intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty at intel.com>
> Cc: Digant H Solanki <digant.h.solanki at intel.com>
> Cc: Sangeetha V <sangeetha.v at intel.com>
>
> Signed-off-by: Ashraf Ali S <ashraf.ali.s at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list