From: "Chiu, Chasel" <chasel.chiu@intel.com>
To: "Kuo, Ted" <ted.kuo@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com>,
"Zeng, Star" <star.zeng@intel.com>,
"S, Ashraf Ali" <ashraf.ali.s@intel.com>
Subject: Re: [edk2-devel][PATCH v2 1/8] IntelFsp2Pkg: X64 compatible changes to support PEI in 64bit
Date: Wed, 6 Apr 2022 14:54:02 +0000 [thread overview]
Message-ID: <BN9PR11MB5483AE48214B86AC7F0EBD26E6E79@BN9PR11MB5483.namprd11.prod.outlook.com> (raw)
In-Reply-To: <0d8259c44249ad6f4279965cfdfa90108c6ce7c8.1649053236.git.ted.kuo@intel.com>
Hi Ted,
Please see my comments inline below.
Thanks,
Chasel
> -----Original Message-----
> From: Kuo, Ted <ted.kuo@intel.com>
> Sent: Monday, April 4, 2022 2:23 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; S,
> Ashraf Ali <ashraf.ali.s@intel.com>
> Subject: [edk2-devel][PATCH v2 1/8] IntelFsp2Pkg: X64 compatible changes
> to support PEI in 64bit
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3893
> 1.Added EFIAPI to FspNotifyPhasePeimEntryPoint.
> 2.Changed AsmReadEsp to AsmReadStackPointer.
> 3.Changed the type of the return value of AsmReadStackPointer
> from UINT32 to UINTN.
> 4.Changed the type of TemporaryMemoryBase, PermenentMemoryBase
> and BootLoaderStack from UINT32 to UINTN.
> 5.Some type casting to pointers are UINT32. Changed them to
> UINTN to accommodate both IA32 and X64.
> 6.Corrected some typos.
>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> Signed-off-by: Ted Kuo <ted.kuo@intel.com>
> ---
> IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.c | 1 +
> IntelFsp2Pkg/FspSecCore/Ia32/ReadEsp.nasm | 8 ++++----
> IntelFsp2Pkg/FspSecCore/Ia32/Stack.nasm | 10 +++++-----
> IntelFsp2Pkg/FspSecCore/SecFsp.c | 8 ++++----
> IntelFsp2Pkg/FspSecCore/SecFsp.h | 2 +-
> IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 8 ++++----
> IntelFsp2Pkg/FspSecCore/SecMain.c | 8 ++++----
> IntelFsp2Pkg/FspSecCore/SecMain.h | 10 +++++-----
> IntelFsp2Pkg/Library/SecFspSecPlatformLibNull/Ia32/Flat32.nasm | 2 +-
> 9 files changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.c
> b/IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.c
> index 88f5540fef..66d39cc70c 100644
> --- a/IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.c
> +++ b/IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.c
> @@ -112,6 +112,7 @@ WaitForNotify (
> @retval EFI_OUT_OF_RESOURCES Insufficient resources to create
> database
> **/
> EFI_STATUS
> +EFIAPI
> FspNotifyPhasePeimEntryPoint (
> IN EFI_PEI_FILE_HANDLE FileHandle,
> IN CONST EFI_PEI_SERVICES **PeiServices
> diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/ReadEsp.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/ReadEsp.nasm
> index 8046b43745..d40dad5a52 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/ReadEsp.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/ReadEsp.nasm
> @@ -9,14 +9,14 @@
> SECTION .text
>
> ;------------------------------------------------------------------------------
> -; UINT32
> +; UINTN
> ; EFIAPI
> -; AsmReadEsp (
> +; AsmReadStackPointer (
> ; VOID
> ; );
> ;------------------------------------------------------------------------------
> -global ASM_PFX(AsmReadEsp)
> -ASM_PFX(AsmReadEsp):
> +global ASM_PFX(AsmReadStackPointer)
> +ASM_PFX(AsmReadStackPointer):
> mov eax, esp
> ret
>
> diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/Stack.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/Stack.nasm
> index 5a7e27c240..ce20639890 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/Stack.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/Stack.nasm
> @@ -9,20 +9,20 @@
> ;
> ;------------------------------------------------------------------------------
>
> -SECTION .text
> + SECTION .text
>
> ;------------------------------------------------------------------------------
> ; VOID
> ; EFIAPI
> ; SecSwitchStack (
> ; UINT32 TemporaryMemoryBase,
> -; UINT32 PermenentMemoryBase
> +; UINT32 PermanentMemoryBase
> ; );
> ;------------------------------------------------------------------------------
> global ASM_PFX(SecSwitchStack)
> ASM_PFX(SecSwitchStack):
> ;
> - ; Save three register: eax, ebx, ecx
> + ; Save four register: eax, ebx, ecx, edx
> ;
> push eax
> push ebx
> @@ -55,7 +55,7 @@ ASM_PFX(SecSwitchStack):
> mov dword [eax + 12], edx
> mov edx, dword [esp + 16] ; Update this function's return address
> into permanent memory
> mov dword [eax + 16], edx
> - mov esp, eax ; From now, esp is pointed to permanent
> memory
> + mov esp, eax ; From now, esp is pointed to permanent
> memory
>
> ;
> ; Fixup the ebp point to permanent memory @@ -63,7 +63,7 @@
> ASM_PFX(SecSwitchStack):
> mov eax, ebp
> sub eax, ebx
> add eax, ecx
> - mov ebp, eax ; From now, ebp is pointed to permanent
> memory
> + mov ebp, eax ; From now, ebp is pointed to permanent
> memory
>
> pop edx
> pop ecx
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFsp.c
> b/IntelFsp2Pkg/FspSecCore/SecFsp.c
> index 68e588dd41..85fbc7664c 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFsp.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.c
> @@ -26,7 +26,7 @@ FspGetExceptionHandler (
> IA32_IDT_GATE_DESCRIPTOR *IdtGateDescriptor;
> FSP_INFO_HEADER *FspInfoHeader;
>
> - FspInfoHeader = (FSP_INFO_HEADER *)AsmGetFspInfoHeader
> ();
> + FspInfoHeader = (FSP_INFO_HEADER
> *)(UINTN)AsmGetFspInfoHeader ();
> ExceptionHandler = IdtEntryTemplate;
> IdtGateDescriptor = (IA32_IDT_GATE_DESCRIPTOR
> *)&ExceptionHandler;
> Entry = (IdtGateDescriptor->Bits.OffsetHigh << 16) |
> IdtGateDescriptor->Bits.OffsetLow;
> @@ -115,7 +115,7 @@ SecGetPlatformData ( VOID FspGlobalDataInit (
> IN OUT FSP_GLOBAL_DATA *PeiFspData,
> - IN UINT32 BootLoaderStack,
> + IN UINTN BootLoaderStack,
> IN UINT8 ApiIdx
> )
> {
> @@ -141,7 +141,7 @@ FspGlobalDataInit (
> // Get FSP Header offset
> // It may have multiple FVs, so look into the last one for FSP header
> //
> - PeiFspData->FspInfoHeader = (FSP_INFO_HEADER
> *)AsmGetFspInfoHeader ();
> + PeiFspData->FspInfoHeader = (FSP_INFO_HEADER
> + *)(UINTN)AsmGetFspInfoHeader ();
> SecGetPlatformData (PeiFspData);
>
> //
> @@ -154,7 +154,7 @@ FspGlobalDataInit (
> //
> FspmUpdDataPtr = (VOID *)GetFspApiParameter ();
> if (FspmUpdDataPtr == NULL) {
> - FspmUpdDataPtr = (VOID *)(PeiFspData->FspInfoHeader->ImageBase +
> PeiFspData->FspInfoHeader->CfgRegionOffset);
> + FspmUpdDataPtr = (VOID
> + *)(UINTN)(PeiFspData->FspInfoHeader->ImageBase +
> + PeiFspData->FspInfoHeader->CfgRegionOffset);
> }
>
> SetFspUpdDataPointer (FspmUpdDataPtr); diff --git
> a/IntelFsp2Pkg/FspSecCore/SecFsp.h b/IntelFsp2Pkg/FspSecCore/SecFsp.h
> index 7c9be85fe0..7fb31c3f87 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFsp.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.h
> @@ -48,7 +48,7 @@ FspGetExceptionHandler ( VOID FspGlobalDataInit (
> IN OUT FSP_GLOBAL_DATA *PeiFspData,
> - IN UINT32 BootLoaderStack,
> + IN UINTN BootLoaderStack,
> IN UINT8 ApiIdx
> );
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> index 7d6ef11fe7..c57247bfaf 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> @@ -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 != 0xFFFFFFFF) {
Compare with MAX_ADDRESS instead of 0xFFFFFFFF
> 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/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index d376fb8361..9e9332ffcd 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -54,7 +54,7 @@ SecStartup (
> IN UINT32 TempRamBase,
> IN VOID *BootFirmwareVolume,
> IN PEI_CORE_ENTRY PeiCore,
> - IN UINT32 BootLoaderStack,
> + IN UINTN BootLoaderStack,
> IN UINT32 ApiIdx
> )
> {
> @@ -233,7 +233,7 @@ SecTemporaryRamSupport (
> GetFspGlobalDataPointer ()->OnSeparateStack = 1;
>
> if (PcdGet8 (PcdFspHeapSizePercentage) == 0) {
> - CurrentStack = AsmReadEsp ();
> + CurrentStack = AsmReadStackPointer ();
> FspStackBase = (UINTN)GetFspEntryStack ();
>
> StackSize = FspStackBase - CurrentStack; @@ -292,8 +292,8 @@
> SecTemporaryRamSupport (
> // permanent memory.
> //
> SecSwitchStack (
> - (UINT32)(UINTN)OldStack,
> - (UINT32)(UINTN)NewStack
> + (UINTN)OldStack,
> + (UINTN)NewStack
> );
>
> return EFI_SUCCESS;
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 7794255af1..3c60b15f01 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -51,8 +51,8 @@ typedef struct _SEC_IDT_TABLE { VOID EFIAPI
> SecSwitchStack (
> - IN UINT32 TemporaryMemoryBase,
> - IN UINT32 PermenentMemoryBase
> + IN UINTN TemporaryMemoryBase,
> + IN UINTN PermenentMemoryBase
> );
>
> /**
> @@ -104,7 +104,7 @@ SecStartup (
> IN UINT32 TempRamBase,
> IN VOID *BootFirmwareVolume,
> IN PEI_CORE_ENTRY PeiCore,
> - IN UINT32 BootLoaderStack,
> + IN UINTN BootLoaderStack,
> IN UINT32 ApiIdx
> );
>
> @@ -127,9 +127,9 @@ ProcessLibraryConstructorList (
> @return value of esp.
>
> **/
> -UINT32
> +UINTN
> EFIAPI
> -AsmReadEsp (
> +AsmReadStackPointer (
> VOID
> );
>
> diff --git
> a/IntelFsp2Pkg/Library/SecFspSecPlatformLibNull/Ia32/Flat32.nasm
> b/IntelFsp2Pkg/Library/SecFspSecPlatformLibNull/Ia32/Flat32.nasm
> index aef7f96d1d..7be570c4e5 100644
> --- a/IntelFsp2Pkg/Library/SecFspSecPlatformLibNull/Ia32/Flat32.nasm
> +++ b/IntelFsp2Pkg/Library/SecFspSecPlatformLibNull/Ia32/Flat32.nasm
> @@ -16,7 +16,7 @@ SECTION .text
>
> %macro RET_ESI 0
>
> - movd esi, mm7 ; restore ESP from MM7
> + movd esi, mm7 ; restore EIP from MM7
> jmp esi
>
> %endmacro
> --
> 2.16.2.windows.1
next prev parent reply other threads:[~2022-04-06 14:54 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-04 6:23 [edk2-devel][PATCH v2 0/8] Support PEI 64bit in IntelFsp2Pkg and IntelFsp2WrapperPkg Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 1/8] IntelFsp2Pkg: X64 compatible changes to support PEI in 64bit Kuo, Ted
2022-04-06 14:54 ` Chiu, Chasel [this message]
2022-04-04 6:23 ` [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add FSPx_ARCH2_UPD support for X64 Kuo, Ted
2022-04-06 14:39 ` Chiu, Chasel
2022-04-06 15:09 ` Chiu, Chasel
2022-04-04 6:23 ` [edk2-devel][PATCH v2 3/8] IntelFsp2Pkg: Update FSP_GLOBAL_DATA and FSP_PLAT_DATA " Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 4/8] IntelFsp2Pkg: FspSecCore support " Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 5/8] IntelFsp2Pkg: SecFspSecPlatformLibNull " Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 6/8] IntelFsp2WrapperPkg: Adopt FSPM_UPD_COMMON_FSP24 " Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 7/8] IntelFsp2WrapperPkg: BaseFspWrapperApiLib support " Kuo, Ted
2022-04-04 6:23 ` [edk2-devel][PATCH v2 8/8] IntelFsp2WrapperPkg: SecFspWrapperPlatformSecLibSample " Kuo, Ted
2022-04-06 15:15 ` Chiu, Chasel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN9PR11MB5483AE48214B86AC7F0EBD26E6E79@BN9PR11MB5483.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox