Re: [PATCH v2] IntelFsp2Pkg: FSP should support input UPD as NULL.

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>

-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com> 
Sent: Tuesday, November 1, 2022 10:46 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>
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 <nathaniel.l.desimone@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
---
 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 +   mov    esp, ecx  ; ECX has been assigned to UPD region    cmp    esp, 0    jz     ParamError-   mov    eax, dword [esp + 4]    ; Parameter pointer-   cmp    eax, 0-   jz     ParamError-   mov    esp, eax     ; skip loading Microcode if the MicrocodeCodeSize is zero    ; and report error if size is less than 2k@@ -444,13 +481,15 @@ Done:
 Exit2:    jmp   ebp -+;+; EstablishStackFsp: EDI should be preserved cross this function+; global ASM_PFX(EstablishStackFsp) ASM_PFX(EstablishStackFsp):   ;   ; Save parameter pointer in edx   ;-  mov       edx, dword [esp + 4]+  mov       edx, ecx   ; ECX has been assigned to UPD region    ;   ; Enable FSP STACK@@ -555,39 +594,37 @@ ASM_PFX(TempRamInitApi):
   SAVE_EAX   SAVE_EDX -  ;-  ; Check Parameter-  ;-  mov       eax, dword [esp + 4]-  cmp       eax, 0-  mov       eax, 80000002h-  jz        TempRamInitExit-   ;   ; Sec Platform Init   ;+  CALL_EBP  ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param   CALL_MMX  ASM_PFX(SecPlatformInit)   cmp       eax, 0   jnz       TempRamInitExit    ; Load microcode   LOAD_ESP+  CALL_EBP  ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param   CALL_MMX  ASM_PFX(LoadMicrocodeDefault)   SXMMN     xmm6, 3, eax            ;Save microcode return status in ECX-SLOT 3 in xmm6.   ;@note If return value eax is not 0, microcode did not load, but continue and attempt to boot.    ; Call Sec CAR Init   LOAD_ESP+  CALL_EBP  ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param   CALL_MMX  ASM_PFX(SecCarInit)   cmp       eax, 0   jnz       TempRamInitExit    LOAD_ESP+  CALL_EBP  ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param+  mov       edi, ecx                     ; Save UPD param to EDI for later code use   CALL_MMX  ASM_PFX(EstablishStackFsp)   cmp       eax, 0   jnz       TempRamInitExit    LXMMN     xmm6, eax, 3  ;Restore microcode status if no CAR init error from ECX-SLOT 3 in xmm6.+  SXMMN     xmm6, 3, edi  ;Save FSP-T UPD parameter pointer in ECX-SLOT 3 in xmm6.  TempRamInitExit:   mov       bl, al                  ; save al data in bldiff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
index 7dd89c531a..cdebe90fab 100644
--- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm
+++ b/IntelFsp2Pkg/FspSecCore/X64/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)@@ -87,6 +87,14 @@ struc LoadMicrocodeParamsFsp24
     .size: endstruc +%macro CALL_RDI  1++  mov     rdi,  %%ReturnAddress+  jmp     %1+%%ReturnAddress:++%endmacro+ ; ; @todo: The strong/weak implementation does not work. ;        This needs to be reviewed later.@@ -116,8 +124,7 @@ ASM_PFX(LoadMicrocodeDefault):
    ; Inputs:    ;   rcx -> LoadMicrocodeParams pointer    ; Register Usage:-   ;   rsp  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@@ -420,10 +427,6 @@ ASM_PFX(TempRamInitApi):
   ENABLE_SSE   ENABLE_AVX   ;-  ; Save Input Parameter in YMM10-  ;-  SAVE_RCX-  ;   ; Save RBP, RBX, RSI, RDI and RSP in YMM7, YMM8 and YMM6   ;   SAVE_REGS@@ -433,6 +436,22 @@ ASM_PFX(TempRamInitApi):
   ;   SAVE_BFV  rbp +  ;+  ; Save Input Parameter in YMM10+  ;+  cmp       rcx, 0+  jnz       ParamValid++  ;+  ; Fall back to default UPD+  ;+  CALL_RDI  ASM_PFX(AsmGetFspInfoHeaderNoStack)+  xor       rcx, rcx+  mov       ecx,  DWORD [rax + 01Ch]      ; Read FsptImageBaseAddress+  add       ecx,  DWORD [rax + 024h]      ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset+ParamValid:+  SAVE_RCX+   ;   ; Save timestamp into YMM6   ;@@ -441,13 +460,6 @@ ASM_PFX(TempRamInitApi):
   or        rax, rdx   SAVE_TS   rax -  ;-  ; Check Parameter-  ;-  cmp       rcx, 0-  mov       rcx, 08000000000000002h-  jz        TempRamInitExit-   ;   ; Sec Platform Init   ;-- 
2.35.0.windows.1