Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
Patch merged:
https://github.com/tianocore/edk2/commit/b84f32ae5b475ce657ea1c9db29d4e4ec7711948
Thanks,
Chasel
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Chiu,
> Chasel
> Sent: Tuesday, November 1, 2022 10:46 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Desimone, Nathaniel L
> ; Zeng, Star
> Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input
> UPD as NULL.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
>
> FSP specification supports input UPD as NULL cases which FSP will use built-
> in UPD region instead.
> FSP should not return INVALID_PARAMETER in such cases.
>
> In FSP-T entry point case, the valid FSP-T UPD region pointer will be passed
> to platform FSP code to consume.
> In FSP-M and FSP-S cases, valid UPD pointer will be decided when updating
> corresponding pointer field in FspGlobalData.
>
> Cc: Nate DeSimone
> Cc: Star Zeng
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
> IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
> +++--
>
> IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
> ++--
> 3 files changed, 91 insertions(+), 34 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> index a44fbf2a50..5f59938518 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> @@ -44,6 +44,8 @@ FspApiCallingCheck (
> // if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData !=
> MAX_UINT32)) { Status = EFI_UNSUPPORTED;+} else if (ApiParam ==
> NULL) {+ Status = EFI_SUCCESS; } else if (EFI_ERROR
> (FspUpdSignatureCheck (ApiIdx, ApiParam))) { Status =
> EFI_INVALID_PARAMETER; }@@ -67,9 +69,13 @@ FspApiCallingCheck (
> } else { if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE)
> { Status = EFI_UNSUPPORTED;- } else if (EFI_ERROR
> (FspUpdSignatureCheck (FspSiliconInitApiIndex, ApiParam))) {-Status =
> EFI_INVALID_PARAMETER; } else if (ApiIdx == FspSiliconInitApiIndex) {+
> if (ApiParam == NULL) {+ Status = EFI_SUCCESS;+} else if
> (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex, ApiParam))) {+
> Status = EFI_INVALID_PARAMETER;+}+ // // Reset
> MultiPhase
> NumberOfPhases to zero //@@ -89,6 +95,8 @@ FspApiCallingCheck (
> } else { if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE)
> { Status = EFI_UNSUPPORTED;+ } else if (ApiParam == NULL) {+
> Status = EFI_SUCCESS; } else if (EFI_ERROR (FspUpdSignatureCheck
> (FspSmmInitApiIndex, ApiParam))) { Status =
> EFI_INVALID_PARAMETER; }diff --git
> a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> index 61030a843b..73821ad22a 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> @@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32
> (PcdFspReservedBufferSize))
> ; Following functions will be provided in PlatformSecLib ; extern
> ASM_PFX(AsmGetFspBaseAddress)-extern
> ASM_PFX(AsmGetFspInfoHeader)+extern
> ASM_PFX(AsmGetFspInfoHeaderNoStack) ;extern
> ASM_PFX(LoadMicrocode); @todo: needs a weak implementation extern
> ASM_PFX(SecPlatformInit) ; @todo: needs a weak implementation extern
> ASM_PFX(SecCarInit)@@ -160,6 +160,47 @@ endstruc
> RET_ESI_EXT mm7 %endmacro +%macro CALL_EDI 1++ mov edi,
> %%ReturnAddress+ jmp %1+%%ReturnAddress:++%endmacro++%macro
> CALL_EBP 1+ mov ebp, %%ReturnAddress+ jmp
> %1+%%ReturnAddress:+%endmacro++%macro RET_EBP 0+ jmp
> ebp ; restore EIP from EBP+%endmacro++;+; Load UPD
> region pointer in ECX+;+global
> ASM_PFX(LoadUpdPointerToECX)+ASM_PFX(LoadUpdPointerToECX):+ ;+ ;
> esp + 4 is input UPD parameter+ ; If esp + 4 is NULL the default UPD should
> be used+ ; ecx will be the UPD region that should be used+ ;+ mov
> ecx,
> dword [esp + 4]+ cmp ecx, 0+ jnz ParamValid++ ;+ ; Fall back
> to
> default UPD region+ ;+ CALL_EDI
> ASM_PFX(AsmGetFspInfoHeaderNoStack)+ mov ecx, DWORD [eax +
> 01Ch] ; Read FsptImageBaseAddress+ add ecx, DWORD [eax +
> 024h] ; Get Cfg Region base address = FsptImageBaseAddress +
> CfgRegionOffset+ParamValid:+ RET_EBP+ ; ; @todo: The strong/weak
> implementation does not work. ;This needs to be reviewed later.@@ -
> 187,10 +228,9 @@ endstruc
> global ASM_PF
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
Reviewed-by: Ted Kuo
-Original Message-
From: Chiu, Chasel
Sent: Friday, November 4, 2022 1:02 AM
To: Kuo, Ted ; devel@edk2.groups.io
Cc: Desimone, Nathaniel L ; Zeng, Star
Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD
as NULL.
Hi Ted,
Please see my reply below inline.
Thanks,
Chasel
> -Original Message-
> From: Kuo, Ted
> Sent: Wednesday, November 2, 2022 7:32 PM
> To: devel@edk2.groups.io; Chiu, Chasel
> Cc: Desimone, Nathaniel L ; Zeng, Star
>
> Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support
> input UPD as NULL.
>
> Hi Chasel,
>
> I have few comments. Please find [Ted] inline.
>
> Thanks,
> Ted
>
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Chiu,
> Chasel
> Sent: Wednesday, November 2, 2022 1:46 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Desimone, Nathaniel L
> ; Zeng, Star
> Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support
> input UPD as NULL.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
>
> FSP specification supports input UPD as NULL cases which FSP will use
> built- in UPD region instead.
> FSP should not return INVALID_PARAMETER in such cases.
>
> In FSP-T entry point case, the valid FSP-T UPD region pointer will be
> passed to platform FSP code to consume.
> In FSP-M and FSP-S cases, valid UPD pointer will be decided when
> updating corresponding pointer field in FspGlobalData.
>
> Cc: Nate DeSimone
> Cc: Star Zeng
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
> IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
> +++--
>
> IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
> ++--
> 3 files changed, 91 insertions(+), 34 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> index a44fbf2a50..5f59938518 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> @@ -44,6 +44,8 @@ FspApiCallingCheck (
> //
> if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData !=
> MAX_UINT32)) {
>Status = EFI_UNSUPPORTED;
> +} else if (ApiParam == NULL) {
> + Status = EFI_SUCCESS;
> } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) {
>Status = EFI_INVALID_PARAMETER;
> }
> @@ -67,9 +69,13 @@ FspApiCallingCheck (
> } else {
>if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
> Status = EFI_UNSUPPORTED;
> - } else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
> ApiParam))) {
> -Status = EFI_INVALID_PARAMETER;
>} else if (ApiIdx == FspSiliconInitApiIndex) {
> +if (ApiParam == NULL) {
> + Status = EFI_SUCCESS;
> +} else if (EFI_ERROR (FspUpdSignatureCheck
> + (FspSiliconInitApiIndex,
> ApiParam))) {
> + Status = EFI_INVALID_PARAMETER;
> +}
> +
> //
> // Reset MultiPhase NumberOfPhases to zero
> //
> @@ -89,6 +95,8 @@ FspApiCallingCheck (
> } else {
>if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
> Status = EFI_UNSUPPORTED;
> + } else if (ApiParam == NULL) {
> +Status = EFI_SUCCESS;
>} else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex,
> ApiParam))) {
> Status = EFI_INVALID_PARAMETER;
>}
> diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> index 61030a843b..73821ad22a 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> @@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32
> (PcdFspReservedBufferSize))
> ; Following functions will be provided in PlatformSecLib ; extern
> ASM_PFX(AsmGetFspBaseAddress) -extern
> ASM_PFX(AsmGetFspInfoHeader)
> +extern ASM_PFX(AsmGetFspInfoHeaderNoStack)
> ;extern ASM_PFX(LoadMicrocode); @todo: needs a weak
> implementation
> extern ASM_PFX(SecPlatformInit) ; @todo: needs a weak implementation
> extern ASM_PFX(SecCarInit)
> @@ -160,6 +160,47 @@ endstruc
> RET_ESI_EXT mm7
> %endmacro
>
> +%macro CALL_EDI 1
> +
> + mov edi, %%ReturnAddress
> + jmp %1
> +%%ReturnAddress:
> +
> +%endmacro
> +
> +%macro CALL_EBP 1
> + mov ebp, %%ReturnAddress
> + jmp %1
> +%%ReturnAddress:
> +%endmacro
> +
> +%macro RET_EBP 0
> + jmp ebp
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
Hi Ted,
Please see my reply below inline.
Thanks,
Chasel
> -Original Message-
> From: Kuo, Ted
> Sent: Wednesday, November 2, 2022 7:32 PM
> To: devel@edk2.groups.io; Chiu, Chasel
> Cc: Desimone, Nathaniel L ; Zeng, Star
>
> Subject: RE: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support
> input UPD as NULL.
>
> Hi Chasel,
>
> I have few comments. Please find [Ted] inline.
>
> Thanks,
> Ted
>
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Chiu,
> Chasel
> Sent: Wednesday, November 2, 2022 1:46 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel ; Desimone, Nathaniel L
> ; Zeng, Star
> Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input
> UPD as NULL.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
>
> FSP specification supports input UPD as NULL cases which FSP will use built-
> in UPD region instead.
> FSP should not return INVALID_PARAMETER in such cases.
>
> In FSP-T entry point case, the valid FSP-T UPD region pointer will be passed
> to platform FSP code to consume.
> In FSP-M and FSP-S cases, valid UPD pointer will be decided when updating
> corresponding pointer field in FspGlobalData.
>
> Cc: Nate DeSimone
> Cc: Star Zeng
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
> IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
> +++--
>
> IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
> ++--
> 3 files changed, 91 insertions(+), 34 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> index a44fbf2a50..5f59938518 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
> @@ -44,6 +44,8 @@ FspApiCallingCheck (
> //
> if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData !=
> MAX_UINT32)) {
>Status = EFI_UNSUPPORTED;
> +} else if (ApiParam == NULL) {
> + Status = EFI_SUCCESS;
> } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) {
>Status = EFI_INVALID_PARAMETER;
> }
> @@ -67,9 +69,13 @@ FspApiCallingCheck (
> } else {
>if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
> Status = EFI_UNSUPPORTED;
> - } else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
> ApiParam))) {
> -Status = EFI_INVALID_PARAMETER;
>} else if (ApiIdx == FspSiliconInitApiIndex) {
> +if (ApiParam == NULL) {
> + Status = EFI_SUCCESS;
> +} else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
> ApiParam))) {
> + Status = EFI_INVALID_PARAMETER;
> +}
> +
> //
> // Reset MultiPhase NumberOfPhases to zero
> //
> @@ -89,6 +95,8 @@ FspApiCallingCheck (
> } else {
>if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
> Status = EFI_UNSUPPORTED;
> + } else if (ApiParam == NULL) {
> +Status = EFI_SUCCESS;
>} else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex,
> ApiParam))) {
> Status = EFI_INVALID_PARAMETER;
>}
> diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> index 61030a843b..73821ad22a 100644
> --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
> @@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32
> (PcdFspReservedBufferSize))
> ; Following functions will be provided in PlatformSecLib ; extern
> ASM_PFX(AsmGetFspBaseAddress) -extern
> ASM_PFX(AsmGetFspInfoHeader)
> +extern ASM_PFX(AsmGetFspInfoHeaderNoStack)
> ;extern ASM_PFX(LoadMicrocode); @todo: needs a weak
> implementation
> extern ASM_PFX(SecPlatformInit) ; @todo: needs a weak implementation
> extern ASM_PFX(SecCarInit)
> @@ -160,6 +160,47 @@ endstruc
> RET_ESI_EXT mm7
> %endmacro
>
> +%macro CALL_EDI 1
> +
> + mov edi, %%ReturnAddress
> + jmp %1
> +%%ReturnAddress:
> +
> +%endmacro
> +
> +%macro CALL_EBP 1
> + mov ebp, %%ReturnAddress
> + jmp %1
> +%%ReturnAddress:
> +%endmacro
> +
> +%macro RET_EBP 0
> + jmp ebp ; restore EIP from EBP
> +%endmacro
> +
> +;
> +; Load UPD region pointer in ECX
> +;
> +global ASM_PFX(LoadUpdPointerToECX)
> +ASM_PFX(LoadUpdPointerToECX):
> + ;
> + ; esp + 4 is input UPD parameter
> + ; If esp + 4 is NULL the
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
Hi Chasel,
I have few comments. Please find [Ted] inline.
Thanks,
Ted
-Original Message-
From: devel@edk2.groups.io On Behalf Of Chiu, Chasel
Sent: Wednesday, November 2, 2022 1:46 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel ; Desimone, Nathaniel L
; Zeng, Star
Subject: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as
NULL.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
FSP specification supports input UPD as NULL cases which FSP will
use built-in UPD region instead.
FSP should not return INVALID_PARAMETER in such cases.
In FSP-T entry point case, the valid FSP-T UPD region pointer will be
passed to platform FSP code to consume.
In FSP-M and FSP-S cases, valid UPD pointer will be decided when
updating corresponding pointer field in FspGlobalData.
Cc: Nate DeSimone
Cc: Star Zeng
Signed-off-by: Chasel Chiu
---
IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
+++--
IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
++--
3 files changed, 91 insertions(+), 34 deletions(-)
diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
index a44fbf2a50..5f59938518 100644
--- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
+++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
@@ -44,6 +44,8 @@ FspApiCallingCheck (
//
if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) {
Status = EFI_UNSUPPORTED;
+} else if (ApiParam == NULL) {
+ Status = EFI_SUCCESS;
} else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) {
Status = EFI_INVALID_PARAMETER;
}
@@ -67,9 +69,13 @@ FspApiCallingCheck (
} else {
if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;
- } else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
ApiParam))) {
-Status = EFI_INVALID_PARAMETER;
} else if (ApiIdx == FspSiliconInitApiIndex) {
+if (ApiParam == NULL) {
+ Status = EFI_SUCCESS;
+} else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
ApiParam))) {
+ Status = EFI_INVALID_PARAMETER;
+}
+
//
// Reset MultiPhase NumberOfPhases to zero
//
@@ -89,6 +95,8 @@ FspApiCallingCheck (
} else {
if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;
+ } else if (ApiParam == NULL) {
+Status = EFI_SUCCESS;
} else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex,
ApiParam))) {
Status = EFI_INVALID_PARAMETER;
}
diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
index 61030a843b..73821ad22a 100644
--- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
@@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))
; Following functions will be provided in PlatformSecLib
;
extern ASM_PFX(AsmGetFspBaseAddress)
-extern ASM_PFX(AsmGetFspInfoHeader)
+extern ASM_PFX(AsmGetFspInfoHeaderNoStack)
;extern ASM_PFX(LoadMicrocode); @todo: needs a weak implementation
extern ASM_PFX(SecPlatformInit) ; @todo: needs a weak implementation
extern ASM_PFX(SecCarInit)
@@ -160,6 +160,47 @@ endstruc
RET_ESI_EXT mm7
%endmacro
+%macro CALL_EDI 1
+
+ mov edi, %%ReturnAddress
+ jmp %1
+%%ReturnAddress:
+
+%endmacro
+
+%macro CALL_EBP 1
+ mov ebp, %%ReturnAddress
+ jmp %1
+%%ReturnAddress:
+%endmacro
+
+%macro RET_EBP 0
+ jmp ebp ; restore EIP from EBP
+%endmacro
+
+;
+; Load UPD region pointer in ECX
+;
+global ASM_PFX(LoadUpdPointerToECX)
+ASM_PFX(LoadUpdPointerToECX):
+ ;
+ ; esp + 4 is input UPD parameter
+ ; If esp + 4 is NULL the default UPD should be used
+ ; ecx will be the UPD region that should be used
+ ;
+ mov ecx, dword [esp + 4]
+ cmp ecx, 0
+ jnz ParamValid
+
+ ;
+ ; Fall back to default UPD region
+ ;
+ CALL_EDI ASM_PFX(AsmGetFspInfoHeaderNoStack)
+ mov ecx, DWORD [eax + 01Ch] ; Read FsptImageBaseAddress
+ add ecx, DWORD [eax + 024h] ; Get Cfg Region base address =
FsptImageBaseAddress + CfgRegionOffset
+ParamValid:
+ RET_EBP
+
;
; @todo: The strong/weak implementation does not work.
;This needs to be reviewed later.
@@ -187,10 +228,9 @@ endstruc
global ASM_PFX(LoadMicrocodeDefault)
ASM_PFX(LoadMicrocodeDefault):
; Inputs:
- ; esp -> LoadMicrocodeParams pointer
+ ; ecx -> UPD region contains LoadMicrocodeParams pointer
; Register Usage:
- ; esp Preserved
- ; All others destroyed
+ ; All are destroyed
; Assumptions:
; No memory available, stack is hard-coded and used for return address
; Execu
Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
Reviewed-by: Nate DeSimone
-Original Message-
From: Chiu, Chasel
Sent: Tuesday, November 1, 2022 10:46 PM
To: devel@edk2.groups.io
Cc: Chiu, Chasel ; Desimone, Nathaniel L
; Zeng, Star
Subject: [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
FSP specification supports input UPD as NULL cases which FSP will use built-in
UPD region instead.
FSP should not return INVALID_PARAMETER in such cases.
In FSP-T entry point case, the valid FSP-T UPD region pointer will be passed to
platform FSP code to consume.
In FSP-M and FSP-S cases, valid UPD pointer will be decided when updating
corresponding pointer field in FspGlobalData.
Cc: Nate DeSimone
Cc: Star Zeng
Signed-off-by: Chasel Chiu
---
IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
+++--
IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
++--
3 files changed, 91 insertions(+), 34 deletions(-)
diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
index a44fbf2a50..5f59938518 100644
--- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
+++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
@@ -44,6 +44,8 @@ FspApiCallingCheck (
// if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData !=
MAX_UINT32)) { Status = EFI_UNSUPPORTED;+} else if (ApiParam == NULL)
{+ Status = EFI_SUCCESS; } else if (EFI_ERROR (FspUpdSignatureCheck
(ApiIdx, ApiParam))) { Status = EFI_INVALID_PARAMETER; }@@ -67,9
+69,13 @@ FspApiCallingCheck (
} else { if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;- } else if (EFI_ERROR (FspUpdSignatureCheck
(FspSiliconInitApiIndex, ApiParam))) {-Status = EFI_INVALID_PARAMETER;
} else if (ApiIdx == FspSiliconInitApiIndex) {+if (ApiParam ==
NULL) {+ Status = EFI_SUCCESS;+} else if (EFI_ERROR
(FspUpdSignatureCheck (FspSiliconInitApiIndex, ApiParam))) {+ Status =
EFI_INVALID_PARAMETER;+}+ // // Reset MultiPhase
NumberOfPhases to zero //@@ -89,6 +95,8 @@ FspApiCallingCheck (
} else { if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;+ } else if (ApiParam == NULL) {+
Status = EFI_SUCCESS; } else if (EFI_ERROR (FspUpdSignatureCheck
(FspSmmInitApiIndex, ApiParam))) { Status = EFI_INVALID_PARAMETER;
}diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
index 61030a843b..73821ad22a 100644
--- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
@@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))
; Following functions will be provided in PlatformSecLib ; extern
ASM_PFX(AsmGetFspBaseAddress)-extern ASM_PFX(AsmGetFspInfoHeader)+extern
ASM_PFX(AsmGetFspInfoHeaderNoStack) ;extern ASM_PFX(LoadMicrocode); @todo:
needs a weak implementation extern ASM_PFX(SecPlatformInit) ; @todo: needs a
weak implementation extern ASM_PFX(SecCarInit)@@ -160,6 +160,47 @@ endstruc
RET_ESI_EXT mm7 %endmacro +%macro CALL_EDI 1++ mov edi,
%%ReturnAddress+ jmp %1+%%ReturnAddress:++%endmacro++%macro CALL_EBP 1+
mov ebp, %%ReturnAddress+ jmp %1+%%ReturnAddress:+%endmacro++%macro
RET_EBP 0+ jmp ebp ; restore EIP from
EBP+%endmacro++;+; Load UPD region pointer in ECX+;+global
ASM_PFX(LoadUpdPointerToECX)+ASM_PFX(LoadUpdPointerToECX):+ ;+ ; esp + 4 is
input UPD parameter+ ; If esp + 4 is NULL the default UPD should be used+ ;
ecx will be the UPD region that should be used+ ;+ mov ecx, dword [esp
+ 4]+ cmp ecx, 0+ jnz ParamValid++ ;+ ; Fall back to default
UPD region+ ;+ CALL_EDI ASM_PFX(AsmGetFspInfoHeaderNoStack)+ mov ecx,
DWORD [eax + 01Ch] ; Read FsptImageBaseAddress+ add ecx, DWORD [eax
+ 024h] ; Get Cfg Region base address = FsptImageBaseAddress +
CfgRegionOffset+ParamValid:+ RET_EBP+ ; ; @todo: The strong/weak
implementation does not work. ;This needs to be reviewed later.@@
-187,10 +228,9 @@ endstruc
global ASM_PFX(LoadMicrocodeDefault) ASM_PFX(LoadMicrocodeDefault):;
Inputs:- ; esp -> LoadMicrocodeParams pointer+ ; ecx -> UPD region
contains LoadMicrocodeParams pointer; Register Usage:- ; esp
Preserved- ; All others destroyed+ ; All are destroyed;
Assumptions:; No memory available, stack is hard-coded and used for
return address; Executed by SBSP and NBSP@@ -201,12 +241,9 @@
ASM_PFX(LoadMicrocodeDefault):
;movd ebp, mm7 + movesp, ecx ; ECX has been assigned to UPD
regioncmpesp, 0
[edk2-devel] [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4114
FSP specification supports input UPD as NULL cases which FSP will
use built-in UPD region instead.
FSP should not return INVALID_PARAMETER in such cases.
In FSP-T entry point case, the valid FSP-T UPD region pointer will be
passed to platform FSP code to consume.
In FSP-M and FSP-S cases, valid UPD pointer will be decided when
updating corresponding pointer field in FspGlobalData.
Cc: Nate DeSimone
Cc: Star Zeng
Signed-off-by: Chasel Chiu
---
IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 12 ++--
IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 73
+++--
IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 40
++--
3 files changed, 91 insertions(+), 34 deletions(-)
diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
index a44fbf2a50..5f59938518 100644
--- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
+++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c
@@ -44,6 +44,8 @@ FspApiCallingCheck (
//
if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) {
Status = EFI_UNSUPPORTED;
+} else if (ApiParam == NULL) {
+ Status = EFI_SUCCESS;
} else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) {
Status = EFI_INVALID_PARAMETER;
}
@@ -67,9 +69,13 @@ FspApiCallingCheck (
} else {
if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;
- } else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
ApiParam))) {
-Status = EFI_INVALID_PARAMETER;
} else if (ApiIdx == FspSiliconInitApiIndex) {
+if (ApiParam == NULL) {
+ Status = EFI_SUCCESS;
+} else if (EFI_ERROR (FspUpdSignatureCheck (FspSiliconInitApiIndex,
ApiParam))) {
+ Status = EFI_INVALID_PARAMETER;
+}
+
//
// Reset MultiPhase NumberOfPhases to zero
//
@@ -89,6 +95,8 @@ FspApiCallingCheck (
} else {
if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) {
Status = EFI_UNSUPPORTED;
+ } else if (ApiParam == NULL) {
+Status = EFI_SUCCESS;
} else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex,
ApiParam))) {
Status = EFI_INVALID_PARAMETER;
}
diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
index 61030a843b..73821ad22a 100644
--- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
@@ -21,7 +21,7 @@ extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))
; Following functions will be provided in PlatformSecLib
;
extern ASM_PFX(AsmGetFspBaseAddress)
-extern ASM_PFX(AsmGetFspInfoHeader)
+extern ASM_PFX(AsmGetFspInfoHeaderNoStack)
;extern ASM_PFX(LoadMicrocode); @todo: needs a weak implementation
extern ASM_PFX(SecPlatformInit) ; @todo: needs a weak implementation
extern ASM_PFX(SecCarInit)
@@ -160,6 +160,47 @@ endstruc
RET_ESI_EXT mm7
%endmacro
+%macro CALL_EDI 1
+
+ mov edi, %%ReturnAddress
+ jmp %1
+%%ReturnAddress:
+
+%endmacro
+
+%macro CALL_EBP 1
+ mov ebp, %%ReturnAddress
+ jmp %1
+%%ReturnAddress:
+%endmacro
+
+%macro RET_EBP 0
+ jmp ebp ; restore EIP from EBP
+%endmacro
+
+;
+; Load UPD region pointer in ECX
+;
+global ASM_PFX(LoadUpdPointerToECX)
+ASM_PFX(LoadUpdPointerToECX):
+ ;
+ ; esp + 4 is input UPD parameter
+ ; If esp + 4 is NULL the default UPD should be used
+ ; ecx will be the UPD region that should be used
+ ;
+ mov ecx, dword [esp + 4]
+ cmp ecx, 0
+ jnz ParamValid
+
+ ;
+ ; Fall back to default UPD region
+ ;
+ CALL_EDI ASM_PFX(AsmGetFspInfoHeaderNoStack)
+ mov ecx, DWORD [eax + 01Ch] ; Read FsptImageBaseAddress
+ add ecx, DWORD [eax + 024h] ; Get Cfg Region base address =
FsptImageBaseAddress + CfgRegionOffset
+ParamValid:
+ RET_EBP
+
;
; @todo: The strong/weak implementation does not work.
;This needs to be reviewed later.
@@ -187,10 +228,9 @@ endstruc
global ASM_PFX(LoadMicrocodeDefault)
ASM_PFX(LoadMicrocodeDefault):
; Inputs:
- ; esp -> LoadMicrocodeParams pointer
+ ; ecx -> UPD region contains LoadMicrocodeParams pointer
; Register Usage:
- ; esp Preserved
- ; All others destroyed
+ ; All are destroyed
; Assumptions:
; No memory available, stack is hard-coded and used for return address
; Executed by SBSP and NBSP
@@ -201,12 +241,9 @@ ASM_PFX(LoadMicrocodeDefault):
;
movd ebp, mm7
+ movesp, ecx ; ECX has been assigned to UPD region
cmpesp, 0
jz ParamError
- moveax, dword [esp + 4]; Parameter pointer
- cmpeax, 0
- jz ParamError
- movesp, eax
; skip loading Microcode if the MicrocodeCodeSize is zero
;
