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.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):
> + ; 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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86859): https://edk2.groups.io/g/devel/message/86859
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]
-=-=-=-=-=-=-=-=-=-=-=-
