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
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86633): https://edk2.groups.io/g/devel/message/86633
Mute This Topic: https://groups.io/mt/89115596/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#86641): https://edk2.groups.io/g/devel/message/86641
Mute This Topic: https://groups.io/mt/89115596/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.