* [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes @ 2024-02-16 5:07 cbduggap 2024-02-23 22:51 ` Nate DeSimone 0 siblings, 1 reply; 4+ messages in thread From: cbduggap @ 2024-02-16 5:07 UTC (permalink / raw) To: devel; +Cc: Sai Chaganty, Nate DeSimone, Chiu Chasel, Duggapu Chinni B Changes to support spec changes 1. Remove usage of Pcd. 2. Change code to validate the Temporary Ram size input. 3. Consume the input saved in YMM Register Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Chiu Chasel <chasel.chiu@intel.com> Cc: Duggapu Chinni B <chinni.b.duggapu@intel.com> Signed-off-by: Duggapu Chinni B <chinni.b.duggapu@intel.com> --- IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 2 +- IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 2 +- IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf | 1 + .../FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 1 - .../FspSecCore/Ia32/FspApiEntryM.nasm | 1 - .../FspSecCore/Ia32/FspApiEntryT.nasm | 110 ++++++++++++------ .../FspSecCore/Ia32/SaveRestoreSseNasm.inc | 11 ++ IntelFsp2Pkg/FspSecCore/SecFsp.c | 23 ++-- IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 4 +- IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 79 +++++++++++-- IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm | 6 +- IntelFsp2Pkg/Include/FspEas/FspApi.h | 5 +- .../Include/SaveRestoreSseAvxNasm.inc | 21 ++++ IntelFsp2Pkg/IntelFsp2Pkg.dec | 4 + .../SecRamInitData.c | 3 +- 15 files changed, 206 insertions(+), 67 deletions(-) diff --git a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf index cb011f99f9..cf8cb2eda9 100644 --- a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf +++ b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf @@ -63,11 +63,11 @@ [Pcd] gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES [Ppis] gEfiTemporaryRamSupportPpiGuid ## PRODUCES diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf index 8029832235..717941c33f 100644 --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf @@ -62,11 +62,11 @@ [Pcd] gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES [Ppis] gEfiTemporaryRamSupportPpiGuid ## PRODUCES diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf index e5a6eaa164..05c0d5f48b 100644 --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf @@ -51,6 +51,7 @@ gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedBufferSize ## CONSUMES + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES [Ppis] gEfiTemporaryRamSupportPpiGuid ## PRODUCES diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm index 15f8ecea83..5fa5c03569 100644 --- a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm +++ b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm @@ -11,7 +11,6 @@ ; Following are fixed PCDs ; extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm index 61ab4612a3..861cce4d01 100644 --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm @@ -11,7 +11,6 @@ ; Following are fixed PCDs ; extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm index 900126b93b..2f8465df3d 100644 --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm @@ -109,7 +109,8 @@ struc LoadMicrocodeParamsFsp24 .FsptArchReserved: resb 3 .FsptArchLength: resd 1 .FspDebugHandler resq 1 - .FsptArchUpd: resd 4 + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 + .FsptArchUpd: resd 3 ; } ; FSPT_CORE_UPD { .MicrocodeCodeAddr: resq 1 @@ -178,29 +179,6 @@ endstruc 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. @@ -267,7 +245,7 @@ ASM_PFX(LoadMicrocodeDefault): cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 jb Fsp20UpdHeader cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 - je Fsp24UpdHeader + jae Fsp24UpdHeader jmp Fsp22UpdHeader Fsp20UpdHeader: @@ -405,7 +383,7 @@ CheckAddress: cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 jb Fsp20UpdHeader1 cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 - je Fsp24UpdHeader1; + jae Fsp24UpdHeader1; jmp Fsp22UpdHeader1 Fsp20UpdHeader1: @@ -497,7 +475,8 @@ ASM_PFX(EstablishStackFsp): ; Enable FSP STACK ; mov esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] - add esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] + LOAD_TEMPORARY_RAM_SIZE eax + add esp, eax push DATA_LEN_OF_MCUD ; Size of the data region push 4455434Dh ; Signature of the data region 'MCUD' @@ -506,7 +485,7 @@ ASM_PFX(EstablishStackFsp): cmp byte [edx + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 jb Fsp20UpdHeader2 cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 - je Fsp24UpdHeader2 + jae Fsp24UpdHeader2 jmp Fsp22UpdHeader2 Fsp20UpdHeader2: @@ -554,12 +533,13 @@ ContinueAfterUpdPush: ; ; Set ECX/EDX to the BootLoader temporary memory range ; - mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] - mov edx, ecx - add edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] + mov edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] + LOAD_TEMPORARY_RAM_SIZE ecx + add edx, ecx sub edx, [ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))] + mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] - cmp ecx, edx ;If PcdFspReservedBufferSize >= PcdTemporaryRamSize, then error. + cmp ecx, edx ;If PcdFspReservedBufferSize >= TemporaryRamSize, then error. jb EstablishStackFspSuccess mov eax, 80000003h ;EFI_UNSUPPORTED jmp EstablishStackFspExit @@ -589,6 +569,30 @@ ASM_PFX(TempRamInitApi): ; SAVE_REGS + ; + ; + ; Read Fsp Info header and save in esi + ; + CALL_EDI ASM_PFX(AsmGetFspInfoHeaderNoStack) + xor esi, esi + mov esi, eax + ; + ; 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 + ; + xor ecx, ecx + mov ecx, DWORD [esi + 01Ch] ; Read FsptImageBaseAddress + add ecx, DWORD [esi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset +ParamValid: + SAVE_ECX ; ; Save timestamp into XMM6 ; @@ -596,9 +600,47 @@ ASM_PFX(TempRamInitApi): SAVE_EAX SAVE_EDX - CALL_EBP ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param - SAVE_ECX ; save UPD param to slot 3 in xmm6 + mov edx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) + mov edx, DWORD [edx] + ; + ; Read ARCH2 UPD input value. + ; + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] + ; + ; As per spec, if Bootloader pass zero, use Fsp defined Size + ; Irrespective of whether this UPD is supported or not, Fallback + ; to Fsp defined size if input is zero. + ; + cmp ebx, 0 + jz UseTemporaryRamSizePcd + xor eax, eax + mov ax, WORD [esi + 020h] ; Read ImageAttribute + test ax, 16 ; check if Bit4 is set + jnz ConsumeInputConfiguration + ; + ; Sometimes user may change input value even if it is not supported + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. + ; + cmp ebx, edx + je UseTemporaryRamSizePcd + mov eax, 080000002h ; RETURN_INVALID_PARAMETER + jmp TempRamInitExit +ConsumeInputConfiguration: + ; + ; Read Fsp Arch2 revision + ; + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 + jb UseTemporaryRamSizePcd + ; + ; Read ARCH2 UPD value and Save. + ; + SAVE_TEMPORARY_RAM_SIZE ebx + jmp GotTemporaryRamSize +UseTemporaryRamSizePcd: + SAVE_TEMPORARY_RAM_SIZE edx +GotTemporaryRamSize: + LOAD_ECX ; ; Sec Platform Init ; diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc index 016f943b43..4d6ec1e984 100644 --- a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc +++ b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc @@ -128,6 +128,17 @@ SXMMN xmm5, 1, eax %endmacro +; +; XMM5 slot 2 for TemporaryRamSize +; +%macro LOAD_TEMPORARY_RAM_SIZE 1 + LXMMN xmm5, %1, 2 + %endmacro + +%macro SAVE_TEMPORARY_RAM_SIZE 1 + SXMMN xmm5, 2, %1 + %endmacro + %macro ENABLE_SSE 0 ; ; Initialize floating point units diff --git a/IntelFsp2Pkg/FspSecCore/SecFsp.c b/IntelFsp2Pkg/FspSecCore/SecFsp.c index 11be1f97ca..0c74e533a4 100644 --- a/IntelFsp2Pkg/FspSecCore/SecFsp.c +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.c @@ -47,11 +47,11 @@ FspGetExceptionHandler ( VOID EFIAPI SecGetPlatformData ( - IN OUT FSP_GLOBAL_DATA *FspData + IN OUT FSP_GLOBAL_DATA *FspData, + IN UINTN TopOfCar ) { FSP_PLAT_DATA *FspPlatformData; - UINT32 TopOfCar; UINT32 *StackPtr; UINT32 DwordSize; @@ -68,11 +68,11 @@ SecGetPlatformData ( FspPlatformData->CodeRegionBase = 0; FspPlatformData->CodeRegionSize = 0; - // - // Pointer to the size field - // - TopOfCar = PcdGet32 (PcdTemporaryRamBase) + PcdGet32 (PcdTemporaryRamSize); - StackPtr = (UINT32 *)(TopOfCar - sizeof (UINT32)); + if (TopOfCar == 0) { + return; + } + + StackPtr = (UINT32 *)((UINT32)TopOfCar - sizeof (UINT32)); if (*(StackPtr - 1) == FSP_MCUD_SIGNATURE) { while (*StackPtr != 0) { @@ -123,7 +123,14 @@ FspGlobalDataInit ( VOID *FspmUpdDataPtr; CHAR8 ImageId[9]; UINTN Idx; + UINTN *TopOfCar; + // + // If TempRam is initilized using FspTempRamInitApi (), GlobalDataPointer + // will point to the Top of the Car and any Data that FspTempRamInitApi + // wants to Handoff to later stages will be pushed on to the Top of the car. + // + TopOfCar = *(VOID **)(UINTN)PcdGet32 (PcdGlobalDataPointerAddress); // // Set FSP Global Data pointer // @@ -147,7 +154,7 @@ FspGlobalDataInit ( // It may have multiple FVs, so look into the last one for FSP header // PeiFspData->FspInfoHeader = (FSP_INFO_HEADER *)(UINTN)AsmGetFspInfoHeader (); - SecGetPlatformData (PeiFspData); + SecGetPlatformData (PeiFspData, (UINTN)TopOfCar); // // Set API calling mode diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c index 5f59938518..33aaac66c1 100644 --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c @@ -42,9 +42,7 @@ FspApiCallingCheck ( // // FspMemoryInit check // - if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) { - Status = EFI_UNSUPPORTED; - } else if (ApiParam == NULL) { + if (ApiParam == NULL) { Status = EFI_SUCCESS; } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) { Status = EFI_INVALID_PARAMETER; diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm index 698bb063a7..c2b7b25b36 100644 --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm @@ -16,6 +16,7 @@ extern ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) extern ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) +extern ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) ; ; Following functions will be provided in PlatformSecLib @@ -76,7 +77,8 @@ struc LoadMicrocodeParamsFsp24 .FsptArchReserved: resb 3 .FsptArchLength: resd 1 .FspDebugHandler resq 1 - .FsptArchUpd: resd 4 + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 + .FsptArchUpd: resd 3 ; } ; FSPT_CORE_UPD { .MicrocodeCodeAddr: resq 1 @@ -163,7 +165,7 @@ ASM_PFX(LoadMicrocodeDefault): cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 jb ParamError cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 - jne ParamError + jb ParamError ; UPD structure is compliant with FSP spec 2.4 mov rax, qword [rsp + LoadMicrocodeParamsFsp24.MicrocodeCodeSize] @@ -273,7 +275,7 @@ CheckAddress: cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 jb ParamError cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 - jne ParamError + jb ParamError ; UPD structure is compliant with FSP spec 2.4 ; Is automatic size detection ? @@ -337,9 +339,16 @@ ASM_PFX(EstablishStackFsp): ; mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) mov esp, DWORD[rax] - mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) - add esp, DWORD[rax] - + LOAD_TEMPORARY_RAM_SIZE rax + add esp, eax + ; + ; Save top of the TemporaryRam in PcdGlobalDataPointerAddress + ; where the FspTempRamInitAPI information is pushed as part of below code + ; which will be used in other Fsp APIs to retrive the same. + ; + mov rax, ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) + mov eax, DWORD[rax] + mov DWORD[eax], esp sub esp, 4 mov dword[esp], DATA_LEN_OF_MCUD ; Size of the data region sub esp, 4 @@ -349,7 +358,7 @@ ASM_PFX(EstablishStackFsp): cmp byte [rdx + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 jb ParamError1 cmp byte [rdx + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 - je Fsp24UpdHeader + jnb Fsp24UpdHeader ParamError1: mov rax, 08000000000000002h @@ -397,8 +406,8 @@ ContinueAfterUpdPush: ; mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) mov edx, [ecx] - mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) - add edx, [ecx] + LOAD_TEMPORARY_RAM_SIZE rcx + add edx, ecx mov rcx, ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) sub edx, [ecx] mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) @@ -439,6 +448,12 @@ ASM_PFX(TempRamInitApi): ; SAVE_BFV rbp + ; + ; Read Fsp Info header and save in rsi + ; + CALL_RDI ASM_PFX(AsmGetFspInfoHeaderNoStack) + xor rsi, rsi + mov rsi, rax ; ; Save Input Parameter in YMM10 ; @@ -448,10 +463,9 @@ ASM_PFX(TempRamInitApi): ; ; 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 + mov ecx, DWORD [rsi + 01Ch] ; Read FsptImageBaseAddress + add ecx, DWORD [rsi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset ParamValid: SAVE_RCX @@ -463,6 +477,47 @@ ParamValid: or rax, rdx SAVE_TS rax + mov rdx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) + mov edx, DWORD [rdx] + ; + ; Read ARCH2 UPD input value. + ; + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] + ; + ; As per spec, if Bootloader pass zero, use Fsp defined Size + ; Irrespective of whether this UPD is supported or not, Fallback + ; to Fsp defined size if input is zero. + ; + cmp ebx, 0 + jz UseTemporaryRamSizePcd + + xor rax, rax + mov ax, WORD [rsi + 020h] ; Read ImageAttribute + test ax, 16 ; check if Bit4 is set + jnz ConsumeInputConfiguration + ; + ; Sometimes user may change input value even if it is not supported + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. + ; + cmp ebx, edx + je UseTemporaryRamSizePcd + mov rax, 08000000000000002h ; RETURN_INVALID_PARAMETER + jmp TempRamInitExit +ConsumeInputConfiguration: + ; + ; Read Fsp Arch2 revision + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 + jb UseTemporaryRamSizePcd + ; + ; Read ARCH2 UPD value and Save. + ; Only low-32 bits of rbx/rdx holds the temporary ram size. + ; + SAVE_TEMPORARY_RAM_SIZE rbx + jmp GotTemporaryRamSize +UseTemporaryRamSizePcd: + SAVE_TEMPORARY_RAM_SIZE rdx + +GotTemporaryRamSize: ; ; Sec Platform Init ; diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm index 32a60270b8..61ebaa8ccf 100644 --- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm +++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm @@ -28,7 +28,7 @@ ASM_PFX(FspInfoHeaderRelativeOff): global ASM_PFX(AsmGetFspInfoHeaderNoStack) ASM_PFX(AsmGetFspInfoHeaderNoStack): lea rax, [ASM_PFX(AsmGetFspInfoHeader)] - lea rcx, [ASM_PFX(FspInfoHeaderRelativeOff)] - mov ecx, [rcx] - sub rax, rcx + lea rsi, [ASM_PFX(FspInfoHeaderRelativeOff)] + mov esi, [rsi] + sub rax, rsi jmp rdi diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h b/IntelFsp2Pkg/Include/FspEas/FspApi.h index 40e063e944..27d5ec3a3c 100644 --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h @@ -139,7 +139,7 @@ typedef struct { /// typedef struct { /// - /// Revision of the structure is 2 for this version of the specification. + /// Revision of the structure is 3 for this version of the specification. /// UINT8 Revision; UINT8 Reserved[3]; @@ -152,7 +152,8 @@ typedef struct { /// occurring during FSP execution. /// EFI_PHYSICAL_ADDRESS FspDebugHandler; - UINT8 Reserved1[16]; + UINT32 FspTemporaryRamSize; + UINT8 Reserved1[12]; } FSPT_ARCH2_UPD; /// diff --git a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc index 002a5a1412..2168564e6d 100644 --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc @@ -201,6 +201,27 @@ movq rcx, xmm5 %endmacro +; +; Save TemporaryRamSize to YMM10[192:255] +; arg 1:general purpose register which holds TemporaryRamSize +; Modified: XMM5 and YMM10[192:255] +; +%macro SAVE_TEMPORARY_RAM_SIZE 1 + LYMMN ymm10, xmm5, 1 + SXMMN xmm5, 1, %1 + SYMMN ymm10, 1, xmm5 + %endmacro + +; +; Restore TemporaryRamSize from YMM10[192:255] +; arg 1:general purpose register where to save TemporaryRamSize +; Modified: XMM5 and %1 +; +%macro LOAD_TEMPORARY_RAM_SIZE 1 + LYMMN ymm10, xmm5, 1 + LXMMN xmm5, %1, 1 + %endmacro + ; ; YMM7[128:191] for calling stack ; arg 1:Entry diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec b/IntelFsp2Pkg/IntelFsp2Pkg.dec index d1c3d3ee7b..47ced1759e 100644 --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec @@ -85,6 +85,10 @@ gFspEventEndOfFirmwareGuid = { 0xbd44f629, 0xeae7, 0x4198, { 0x87, 0xf1, 0x39, 0xfa, 0xb0, 0xfd, 0x71, 0x7e } } [PcdsFixedAtBuild] + # + # As part of FSP-T execution, this will be initialized to TopOfCar address where Fsp T data will be saved. + # Once Fsp Global Data is initialed later in FSP-M, this will hold the Global data buffer address. + # gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress |0xFED00108|UINT32|0x00000001 gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase |0xFEF00000|UINT32|0x10001001 gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize | 0x2000|UINT32|0x10001002 diff --git a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c index fb0d9a8683..316c2fa86a 100644 --- a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c +++ b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c @@ -49,8 +49,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST FSPT_UPD_CORE_DATA FsptUpdDataPtr = { }, 0x00000020, 0x00000000, + 0x00000000, { - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 } }, -- 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115536): https://edk2.groups.io/g/devel/message/115536 Mute This Topic: https://groups.io/mt/104388042/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes 2024-02-16 5:07 [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes cbduggap @ 2024-02-23 22:51 ` Nate DeSimone 2024-02-26 5:14 ` cbduggap [not found] ` <17B751C23B60AA20.13095@groups.io> 0 siblings, 2 replies; 4+ messages in thread From: Nate DeSimone @ 2024-02-23 22:51 UTC (permalink / raw) To: Duggapu, Chinni B, devel@edk2.groups.io Cc: Chaganty, Rangasai V, Chiu, Chasel Hi Chinni, Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. Moreover, you have added the assumption that FspGlobalDataPointer is initialized to top of CAR to SecFsp.c but you don't actually write top of CAR to that location in the IA32 version of TempRamInit(), only the X64 version has that addition. Since SecFsp.c is used on both, you have effectively broken all 32-bit FSP builds. For obvious reasons, your patch will not be merged without additional work. Additional feedback below inline. Thanks, Nate > -----Original Message----- > From: Duggapu, Chinni B <chinni.b.duggapu@intel.com> > Sent: Thursday, February 15, 2024 9:07 PM > To: devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, > Nathaniel L <nathaniel.l.desimone@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Duggapu, Chinni B <chinni.b.duggapu@intel.com> > Subject: [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes > > Changes to support spec changes > > 1. Remove usage of Pcd. > 2. Change code to validate the Temporary Ram size input. > 3. Consume the input saved in YMM Register > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Chiu Chasel <chasel.chiu@intel.com> > Cc: Duggapu Chinni B <chinni.b.duggapu@intel.com> > > > Signed-off-by: Duggapu Chinni B <chinni.b.duggapu@intel.com> > --- > IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf | 1 + > .../FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryT.nasm | 110 ++++++++++++------ > .../FspSecCore/Ia32/SaveRestoreSseNasm.inc | 11 ++ > IntelFsp2Pkg/FspSecCore/SecFsp.c | 23 ++-- > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 4 +- > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 79 +++++++++++-- > IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm | 6 +- > IntelFsp2Pkg/Include/FspEas/FspApi.h | 5 +- > .../Include/SaveRestoreSseAvxNasm.inc | 21 ++++ > IntelFsp2Pkg/IntelFsp2Pkg.dec | 4 + > .../SecRamInitData.c | 3 +- > 15 files changed, 206 insertions(+), 67 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > index cb011f99f9..cf8cb2eda9 100644 > --- a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > @@ -63,11 +63,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > index 8029832235..717941c33f 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > @@ -62,11 +62,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > index e5a6eaa164..05c0d5f48b 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > @@ -51,6 +51,7 @@ > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedBufferSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > index 15f8ecea83..5fa5c03569 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > index 61ab4612a3..861cce4d01 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > index 900126b93b..2f8465df3d 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > @@ -109,7 +109,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -178,29 +179,6 @@ endstruc > 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. > @@ -267,7 +245,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jae Fsp24UpdHeader > jmp Fsp22UpdHeader > > Fsp20UpdHeader: > @@ -405,7 +383,7 @@ CheckAddress: > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader1 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader1; > + jae Fsp24UpdHeader1; > jmp Fsp22UpdHeader1 > > Fsp20UpdHeader1: > @@ -497,7 +475,8 @@ ASM_PFX(EstablishStackFsp): > ; Enable FSP STACK > ; > mov esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - add esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + LOAD_TEMPORARY_RAM_SIZE eax > + add esp, eax > > push DATA_LEN_OF_MCUD ; Size of the data region > push 4455434Dh ; Signature of the data region 'MCUD' > @@ -506,7 +485,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [edx + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader2 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader2 > + jae Fsp24UpdHeader2 > jmp Fsp22UpdHeader2 > > Fsp20UpdHeader2: > @@ -554,12 +533,13 @@ ContinueAfterUpdPush: > ; > ; Set ECX/EDX to the BootLoader temporary memory range > ; > - mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - mov edx, ecx > - add edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + mov edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > + LOAD_TEMPORARY_RAM_SIZE ecx > + add edx, ecx > sub edx, [ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))] > + mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > > - cmp ecx, edx ;If PcdFspReservedBufferSize >= PcdTemporaryRamSize, then error. > + cmp ecx, edx ;If PcdFspReservedBufferSize >= TemporaryRamSize, then error. > jb EstablishStackFspSuccess > mov eax, 80000003h ;EFI_UNSUPPORTED > jmp EstablishStackFspExit > @@ -589,6 +569,30 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_REGS > > + ; > + ; > + ; Read Fsp Info header and save in esi > + ; > + CALL_EDI ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor esi, esi > + mov esi, eax > + ; > + ; 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 > + ; > + xor ecx, ecx > + mov ecx, DWORD [esi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [esi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > +ParamValid: > + SAVE_ECX What is the reason for removing the LoadUpdPointerToECX function and putting the equivalent logic inline? > ; > ; Save timestamp into XMM6 > ; > @@ -596,9 +600,47 @@ ASM_PFX(TempRamInitApi): > SAVE_EAX > SAVE_EDX > > - CALL_EBP ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param > - SAVE_ECX ; save UPD param to slot 3 in xmm6 > + mov edx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [edx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size > + ; Irrespective of whether this UPD is supported or not, Fallback > + ; to Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > > + xor eax, eax > + mov ax, WORD [esi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov eax, 080000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + ; > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; > + SAVE_TEMPORARY_RAM_SIZE ebx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE edx > +GotTemporaryRamSize: > + LOAD_ECX > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > index 016f943b43..4d6ec1e984 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > @@ -128,6 +128,17 @@ > SXMMN xmm5, 1, eax > %endmacro > > +; > +; XMM5 slot 2 for TemporaryRamSize > +; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LXMMN xmm5, %1, 2 > + %endmacro > + > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + SXMMN xmm5, 2, %1 > + %endmacro > + > %macro ENABLE_SSE 0 > ; > ; Initialize floating point units > diff --git a/IntelFsp2Pkg/FspSecCore/SecFsp.c b/IntelFsp2Pkg/FspSecCore/SecFsp.c > index 11be1f97ca..0c74e533a4 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFsp.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.c > @@ -47,11 +47,11 @@ FspGetExceptionHandler ( > VOID > EFIAPI > SecGetPlatformData ( > - IN OUT FSP_GLOBAL_DATA *FspData > + IN OUT FSP_GLOBAL_DATA *FspData, > + IN UINTN TopOfCar > ) > { > FSP_PLAT_DATA *FspPlatformData; > - UINT32 TopOfCar; > UINT32 *StackPtr; > UINT32 DwordSize; > > @@ -68,11 +68,11 @@ SecGetPlatformData ( > FspPlatformData->CodeRegionBase = 0; > FspPlatformData->CodeRegionSize = 0; > > - // > - // Pointer to the size field > - // > - TopOfCar = PcdGet32 (PcdTemporaryRamBase) + PcdGet32 (PcdTemporaryRamSize); > - StackPtr = (UINT32 *)(TopOfCar - sizeof (UINT32)); > + if (TopOfCar == 0) { > + return; > + } > + > + StackPtr = (UINT32 *)((UINT32)TopOfCar - sizeof (UINT32)); > > if (*(StackPtr - 1) == FSP_MCUD_SIGNATURE) { > while (*StackPtr != 0) { > @@ -123,7 +123,14 @@ FspGlobalDataInit ( > VOID *FspmUpdDataPtr; > CHAR8 ImageId[9]; > UINTN Idx; > + UINTN *TopOfCar; > > + // > + // If TempRam is initilized using FspTempRamInitApi (), GlobalDataPointer > + // will point to the Top of the Car and any Data that FspTempRamInitApi > + // wants to Handoff to later stages will be pushed on to the Top of the car. > + // > + TopOfCar = *(VOID **)(UINTN)PcdGet32 (PcdGlobalDataPointerAddress); There are some customers that do not run FSP-T. The assumption that this will be initialized to the TopOfCar during FSP-T would not hold on those platforms. > // > // Set FSP Global Data pointer > // > @@ -147,7 +154,7 @@ FspGlobalDataInit ( > // It may have multiple FVs, so look into the last one for FSP header > // > PeiFspData->FspInfoHeader = (FSP_INFO_HEADER *)(UINTN)AsmGetFspInfoHeader (); > - SecGetPlatformData (PeiFspData); > + SecGetPlatformData (PeiFspData, (UINTN)TopOfCar); > > // > // Set API calling mode > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > index 5f59938518..33aaac66c1 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > @@ -42,9 +42,7 @@ FspApiCallingCheck ( > // > // FspMemoryInit check > // > - if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) { > - Status = EFI_UNSUPPORTED; > - } else if (ApiParam == NULL) { One cannot assume that FspGlobalDataPointer gets initialized by FSP-T as mentioned above. > + if (ApiParam == NULL) { > Status = EFI_SUCCESS; > } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) { > Status = EFI_INVALID_PARAMETER; > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > index 698bb063a7..c2b7b25b36 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > @@ -16,6 +16,7 @@ > extern ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > extern ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > +extern ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > > ; > ; Following functions will be provided in PlatformSecLib > @@ -76,7 +77,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -163,7 +165,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > mov rax, qword [rsp + LoadMicrocodeParamsFsp24.MicrocodeCodeSize] > @@ -273,7 +275,7 @@ CheckAddress: > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > ; Is automatic size detection ? > @@ -337,9 +339,16 @@ ASM_PFX(EstablishStackFsp): > ; > mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov esp, DWORD[rax] > - mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add esp, DWORD[rax] > - > + LOAD_TEMPORARY_RAM_SIZE rax > + add esp, eax > + ; > + ; Save top of the TemporaryRam in PcdGlobalDataPointerAddress > + ; where the FspTempRamInitAPI information is pushed as part of below code > + ; which will be used in other Fsp APIs to retrive the same. > + ; > + mov rax, ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > + mov eax, DWORD[rax] > + mov DWORD[eax], esp Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. > sub esp, 4 > mov dword[esp], DATA_LEN_OF_MCUD ; Size of the data region > sub esp, 4 > @@ -349,7 +358,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [rdx + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError1 > cmp byte [rdx + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jnb Fsp24UpdHeader > > ParamError1: > mov rax, 08000000000000002h > @@ -397,8 +406,8 @@ ContinueAfterUpdPush: > ; > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov edx, [ecx] > - mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add edx, [ecx] > + LOAD_TEMPORARY_RAM_SIZE rcx > + add edx, ecx > mov rcx, ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > sub edx, [ecx] > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > @@ -439,6 +448,12 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_BFV rbp > > + ; > + ; Read Fsp Info header and save in rsi > + ; > + CALL_RDI ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor rsi, rsi > + mov rsi, rax > ; > ; Save Input Parameter in YMM10 > ; > @@ -448,10 +463,9 @@ ASM_PFX(TempRamInitApi): > ; > ; 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 > + mov ecx, DWORD [rsi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [rsi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > ParamValid: > SAVE_RCX > > @@ -463,6 +477,47 @@ ParamValid: > or rax, rdx > SAVE_TS rax > > + mov rdx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [rdx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size > + ; Irrespective of whether this UPD is supported or not, Fallback > + ; to Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > + > + xor rax, rax > + mov ax, WORD [rsi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov rax, 08000000000000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; Only low-32 bits of rbx/rdx holds the temporary ram size. > + ; > + SAVE_TEMPORARY_RAM_SIZE rbx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE rdx > + > +GotTemporaryRamSize: > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > index 32a60270b8..61ebaa8ccf 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > @@ -28,7 +28,7 @@ ASM_PFX(FspInfoHeaderRelativeOff): > global ASM_PFX(AsmGetFspInfoHeaderNoStack) > ASM_PFX(AsmGetFspInfoHeaderNoStack): > lea rax, [ASM_PFX(AsmGetFspInfoHeader)] > - lea rcx, [ASM_PFX(FspInfoHeaderRelativeOff)] > - mov ecx, [rcx] > - sub rax, rcx > + lea rsi, [ASM_PFX(FspInfoHeaderRelativeOff)] > + mov esi, [rsi] > + sub rax, rsi > jmp rdi > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index 40e063e944..27d5ec3a3c 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -139,7 +139,7 @@ typedef struct { > /// > typedef struct { > /// > - /// Revision of the structure is 2 for this version of the specification. > + /// Revision of the structure is 3 for this version of the specification. > /// > UINT8 Revision; > UINT8 Reserved[3]; > @@ -152,7 +152,8 @@ typedef struct { > /// occurring during FSP execution. > /// > EFI_PHYSICAL_ADDRESS FspDebugHandler; > - UINT8 Reserved1[16]; > + UINT32 FspTemporaryRamSize; > + UINT8 Reserved1[12]; > } FSPT_ARCH2_UPD; > > /// > diff --git a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > index 002a5a1412..2168564e6d 100644 > --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > @@ -201,6 +201,27 @@ > movq rcx, xmm5 > %endmacro > > +; > +; Save TemporaryRamSize to YMM10[192:255] > +; arg 1:general purpose register which holds TemporaryRamSize > +; Modified: XMM5 and YMM10[192:255] > +; > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + SXMMN xmm5, 1, %1 > + SYMMN ymm10, 1, xmm5 > + %endmacro > + > +; > +; Restore TemporaryRamSize from YMM10[192:255] > +; arg 1:general purpose register where to save TemporaryRamSize > +; Modified: XMM5 and %1 > +; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + LXMMN xmm5, %1, 1 > + %endmacro > + > ; > ; YMM7[128:191] for calling stack > ; arg 1:Entry > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec b/IntelFsp2Pkg/IntelFsp2Pkg.dec > index d1c3d3ee7b..47ced1759e 100644 > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec > @@ -85,6 +85,10 @@ > gFspEventEndOfFirmwareGuid = { 0xbd44f629, 0xeae7, 0x4198, { 0x87, 0xf1, 0x39, 0xfa, 0xb0, 0xfd, 0x71, 0x7e } } > > [PcdsFixedAtBuild] > + # > + # As part of FSP-T execution, this will be initialized to TopOfCar address where Fsp T data will be saved. > + # Once Fsp Global Data is initialed later in FSP-M, this will hold the Global data buffer address. > + # > gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress |0xFED00108|UINT32|0x00000001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase |0xFEF00000|UINT32|0x10001001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize | 0x2000|UINT32|0x10001002 > diff --git a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c > index fb0d9a8683..316c2fa86a 100644 > --- a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c > +++ b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRamInitData.c > @@ -49,8 +49,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST FSPT_UPD_CORE_DATA FsptUpdDataPtr = { > }, > 0x00000020, > 0x00000000, > + 0x00000000, > { > - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > } > }, > -- > 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115897): https://edk2.groups.io/g/devel/message/115897 Mute This Topic: https://groups.io/mt/104388042/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes 2024-02-23 22:51 ` Nate DeSimone @ 2024-02-26 5:14 ` cbduggap [not found] ` <17B751C23B60AA20.13095@groups.io> 1 sibling, 0 replies; 4+ messages in thread From: cbduggap @ 2024-02-26 5:14 UTC (permalink / raw) To: Desimone, Nathaniel L, devel@edk2.groups.io Cc: Chaganty, Rangasai V, Chiu, Chasel Thanks, Nate, for taking some time to review the patch. I agree that GlobalDatapointer usage to pass Top of CAR from FSP-T to FSP-M is an ugly hack where I only convinced 90% (with my approach 😊) but ended up in adding that logic as we covered all corner cases. Here is the background why: 1. FSP-M Sec Core function SecGetPlatformData is using Top of CAR to retrieve "FSP T Core UPD information" and "Time Stamp for FSP T entry and exit". This code is FSP API Mode only code. 2. Both are invalid if some Boot loader not calling FSP-T. 3. Further in the changes, we are safely returning from FSP-M SecGetPlatformData Function when the value of the Register is all F's. Few Options that are explored before adding this logic: 1. Initially was thinking to stop using SecGetPlatformData function in FSP-M Sec because we don’t have any use case where we need to save the details of FSP T Core UPD information for later stages. But we would need Top of CAR to capture FSP-T entry & exit time (which still can be retrieved in Bootloader code as we pass Start & end range of TempRam in ECX and EDX as a output). 2. Further thought of using some YMM register to save the value (inside FSP-T ) for later consumption but there is no guarantee that Bootloaders will preserve that value until they call FSP-M. I agree that we need to add the same logic inside IA32 implementation also which was missed (to find during Unit testing ) as we haven’t seen any Boot failures with these changes on another intel program where we are still using FSP 32 BIT (This is because we don’t use FSP T Core UPDs in FSP-M But might not be able to capture the FSP-T entry & exit time in FPDT for API Mode). I will take care of other feedback and re-send Patch V3 But need some inputs on "How we can avoid the GlobalDatapointer hack". Other feedback : 1. What is the reason for removing the LoadUpdPointerToECX function and putting the equivalent logic inline? Initially I thought that we need changes to this logic and cannot keep this as a separate function, but I agree we can reuse the same function now. Thanks, Chinni. -----Original Message----- From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> Sent: Saturday, February 24, 2024 4:22 AM To: Duggapu, Chinni B <chinni.b.duggapu@intel.com>; devel@edk2.groups.io Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: RE: [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes Hi Chinni, Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. Moreover, you have added the assumption that FspGlobalDataPointer is initialized to top of CAR to SecFsp.c but you don't actually write top of CAR to that location in the IA32 version of TempRamInit(), only the X64 version has that addition. Since SecFsp.c is used on both, you have effectively broken all 32-bit FSP builds. For obvious reasons, your patch will not be merged without additional work. Additional feedback below inline. Thanks, Nate > -----Original Message----- > From: Duggapu, Chinni B <chinni.b.duggapu@intel.com> > Sent: Thursday, February 15, 2024 9:07 PM > To: devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, > Nathaniel L <nathaniel.l.desimone@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Duggapu, Chinni B > <chinni.b.duggapu@intel.com> > Subject: [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes > > Changes to support spec changes > > 1. Remove usage of Pcd. > 2. Change code to validate the Temporary Ram size input. > 3. Consume the input saved in YMM Register > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Chiu Chasel <chasel.chiu@intel.com> > Cc: Duggapu Chinni B <chinni.b.duggapu@intel.com> > > > Signed-off-by: Duggapu Chinni B <chinni.b.duggapu@intel.com> > --- > IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf | 1 + > .../FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryT.nasm | 110 ++++++++++++------ > .../FspSecCore/Ia32/SaveRestoreSseNasm.inc | 11 ++ > IntelFsp2Pkg/FspSecCore/SecFsp.c | 23 ++-- > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 4 +- > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 79 +++++++++++-- > IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm | 6 +- > IntelFsp2Pkg/Include/FspEas/FspApi.h | 5 +- > .../Include/SaveRestoreSseAvxNasm.inc | 21 ++++ > IntelFsp2Pkg/IntelFsp2Pkg.dec | 4 + > .../SecRamInitData.c | 3 +- > 15 files changed, 206 insertions(+), 67 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > index cb011f99f9..cf8cb2eda9 100644 > --- a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > @@ -63,11 +63,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > index 8029832235..717941c33f 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > @@ -62,11 +62,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > index e5a6eaa164..05c0d5f48b 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > @@ -51,6 +51,7 @@ > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedBufferSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > index 15f8ecea83..5fa5c03569 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > index 61ab4612a3..861cce4d01 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > index 900126b93b..2f8465df3d 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > @@ -109,7 +109,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -178,29 +179,6 @@ endstruc > 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. > @@ -267,7 +245,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jae Fsp24UpdHeader > jmp Fsp22UpdHeader > > Fsp20UpdHeader: > @@ -405,7 +383,7 @@ CheckAddress: > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader1 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader1; > + jae Fsp24UpdHeader1; > jmp Fsp22UpdHeader1 > > Fsp20UpdHeader1: > @@ -497,7 +475,8 @@ ASM_PFX(EstablishStackFsp): > ; Enable FSP STACK > ; > mov esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - add esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + LOAD_TEMPORARY_RAM_SIZE eax > + add esp, eax > > push DATA_LEN_OF_MCUD ; Size of the data region > push 4455434Dh ; Signature of the data region 'MCUD' > @@ -506,7 +485,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [edx + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader2 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader2 > + jae Fsp24UpdHeader2 > jmp Fsp22UpdHeader2 > > Fsp20UpdHeader2: > @@ -554,12 +533,13 @@ ContinueAfterUpdPush: > ; > ; Set ECX/EDX to the BootLoader temporary memory range > ; > - mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - mov edx, ecx > - add edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + mov edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > + LOAD_TEMPORARY_RAM_SIZE ecx > + add edx, ecx > sub edx, [ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))] > + mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > > - cmp ecx, edx ;If PcdFspReservedBufferSize >= PcdTemporaryRamSize, then error. > + cmp ecx, edx ;If PcdFspReservedBufferSize >= TemporaryRamSize, then error. > jb EstablishStackFspSuccess > mov eax, 80000003h ;EFI_UNSUPPORTED > jmp EstablishStackFspExit > @@ -589,6 +569,30 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_REGS > > + ; > + ; > + ; Read Fsp Info header and save in esi ; CALL_EDI > + ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor esi, esi > + mov esi, eax > + ; > + ; 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 > + ; > + xor ecx, ecx > + mov ecx, DWORD [esi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [esi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > +ParamValid: > + SAVE_ECX What is the reason for removing the LoadUpdPointerToECX function and putting the equivalent logic inline? > ; > ; Save timestamp into XMM6 > ; > @@ -596,9 +600,47 @@ ASM_PFX(TempRamInitApi): > SAVE_EAX > SAVE_EDX > > - CALL_EBP ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param > - SAVE_ECX ; save UPD param to slot 3 in xmm6 > + mov edx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [edx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size ; > + Irrespective of whether this UPD is supported or not, Fallback ; to > + Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > > + xor eax, eax > + mov ax, WORD [esi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov eax, 080000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + ; > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; > + SAVE_TEMPORARY_RAM_SIZE ebx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE edx > +GotTemporaryRamSize: > + LOAD_ECX > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > index 016f943b43..4d6ec1e984 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > @@ -128,6 +128,17 @@ > SXMMN xmm5, 1, eax > %endmacro > > +; > +; XMM5 slot 2 for TemporaryRamSize > +; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LXMMN xmm5, %1, 2 > + %endmacro > + > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + SXMMN xmm5, 2, %1 > + %endmacro > + > %macro ENABLE_SSE 0 > ; > ; Initialize floating point units diff --git > a/IntelFsp2Pkg/FspSecCore/SecFsp.c b/IntelFsp2Pkg/FspSecCore/SecFsp.c > index 11be1f97ca..0c74e533a4 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFsp.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.c > @@ -47,11 +47,11 @@ FspGetExceptionHandler ( VOID EFIAPI > SecGetPlatformData ( > - IN OUT FSP_GLOBAL_DATA *FspData > + IN OUT FSP_GLOBAL_DATA *FspData, > + IN UINTN TopOfCar > ) > { > FSP_PLAT_DATA *FspPlatformData; > - UINT32 TopOfCar; > UINT32 *StackPtr; > UINT32 DwordSize; > > @@ -68,11 +68,11 @@ SecGetPlatformData ( > FspPlatformData->CodeRegionBase = 0; > FspPlatformData->CodeRegionSize = 0; > > - // > - // Pointer to the size field > - // > - TopOfCar = PcdGet32 (PcdTemporaryRamBase) + PcdGet32 > (PcdTemporaryRamSize); > - StackPtr = (UINT32 *)(TopOfCar - sizeof (UINT32)); > + if (TopOfCar == 0) { > + return; > + } > + > + StackPtr = (UINT32 *)((UINT32)TopOfCar - sizeof (UINT32)); > > if (*(StackPtr - 1) == FSP_MCUD_SIGNATURE) { > while (*StackPtr != 0) { > @@ -123,7 +123,14 @@ FspGlobalDataInit ( > VOID *FspmUpdDataPtr; > CHAR8 ImageId[9]; > UINTN Idx; > + UINTN *TopOfCar; > > + // > + // If TempRam is initilized using FspTempRamInitApi (), > + GlobalDataPointer // will point to the Top of the Car and any Data > + that FspTempRamInitApi // wants to Handoff to later stages will be pushed on to the Top of the car. > + // > + TopOfCar = *(VOID **)(UINTN)PcdGet32 > + (PcdGlobalDataPointerAddress); There are some customers that do not run FSP-T. The assumption that this will be initialized to the TopOfCar during FSP-T would not hold on those platforms. > // > // Set FSP Global Data pointer > // > @@ -147,7 +154,7 @@ FspGlobalDataInit ( > // It may have multiple FVs, so look into the last one for FSP header > // > PeiFspData->FspInfoHeader = (FSP_INFO_HEADER > *)(UINTN)AsmGetFspInfoHeader (); > - SecGetPlatformData (PeiFspData); > + SecGetPlatformData (PeiFspData, (UINTN)TopOfCar); > > // > // Set API calling mode > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > index 5f59938518..33aaac66c1 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > @@ -42,9 +42,7 @@ FspApiCallingCheck ( > // > // FspMemoryInit check > // > - if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) { > - Status = EFI_UNSUPPORTED; > - } else if (ApiParam == NULL) { One cannot assume that FspGlobalDataPointer gets initialized by FSP-T as mentioned above. > + if (ApiParam == NULL) { > Status = EFI_SUCCESS; > } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) { > Status = EFI_INVALID_PARAMETER; diff --git > a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > index 698bb063a7..c2b7b25b36 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > @@ -16,6 +16,7 @@ > extern ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > extern ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > +extern ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > > ; > ; Following functions will be provided in PlatformSecLib @@ -76,7 > +77,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -163,7 +165,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > mov rax, qword [rsp + LoadMicrocodeParamsFsp24.MicrocodeCodeSize] > @@ -273,7 +275,7 @@ CheckAddress: > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > ; Is automatic size detection ? > @@ -337,9 +339,16 @@ ASM_PFX(EstablishStackFsp): > ; > mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov esp, DWORD[rax] > - mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add esp, DWORD[rax] > - > + LOAD_TEMPORARY_RAM_SIZE rax > + add esp, eax > + ; > + ; Save top of the TemporaryRam in PcdGlobalDataPointerAddress ; > + where the FspTempRamInitAPI information is pushed as part of below > + code ; which will be used in other Fsp APIs to retrive the same. > + ; > + mov rax, ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > + mov eax, DWORD[rax] > + mov DWORD[eax], esp Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. > sub esp, 4 > mov dword[esp], DATA_LEN_OF_MCUD ; Size of the data region > sub esp, 4 > @@ -349,7 +358,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [rdx + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError1 > cmp byte [rdx + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jnb Fsp24UpdHeader > > ParamError1: > mov rax, 08000000000000002h > @@ -397,8 +406,8 @@ ContinueAfterUpdPush: > ; > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov edx, [ecx] > - mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add edx, [ecx] > + LOAD_TEMPORARY_RAM_SIZE rcx > + add edx, ecx > mov rcx, ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > sub edx, [ecx] > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > @@ -439,6 +448,12 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_BFV rbp > > + ; > + ; Read Fsp Info header and save in rsi ; CALL_RDI > + ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor rsi, rsi > + mov rsi, rax > ; > ; Save Input Parameter in YMM10 > ; > @@ -448,10 +463,9 @@ ASM_PFX(TempRamInitApi): > ; > ; 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 > + mov ecx, DWORD [rsi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [rsi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > ParamValid: > SAVE_RCX > > @@ -463,6 +477,47 @@ ParamValid: > or rax, rdx > SAVE_TS rax > > + mov rdx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [rdx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size ; > + Irrespective of whether this UPD is supported or not, Fallback ; to > + Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > + > + xor rax, rax > + mov ax, WORD [rsi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov rax, 08000000000000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; Only low-32 bits of rbx/rdx holds the temporary ram size. > + ; > + SAVE_TEMPORARY_RAM_SIZE rbx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE rdx > + > +GotTemporaryRamSize: > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > index 32a60270b8..61ebaa8ccf 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > @@ -28,7 +28,7 @@ ASM_PFX(FspInfoHeaderRelativeOff): > global ASM_PFX(AsmGetFspInfoHeaderNoStack) > ASM_PFX(AsmGetFspInfoHeaderNoStack): > lea rax, [ASM_PFX(AsmGetFspInfoHeader)] > - lea rcx, [ASM_PFX(FspInfoHeaderRelativeOff)] > - mov ecx, [rcx] > - sub rax, rcx > + lea rsi, [ASM_PFX(FspInfoHeaderRelativeOff)] > + mov esi, [rsi] > + sub rax, rsi > jmp rdi > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index 40e063e944..27d5ec3a3c 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -139,7 +139,7 @@ typedef struct { > /// > typedef struct { > /// > - /// Revision of the structure is 2 for this version of the specification. > + /// Revision of the structure is 3 for this version of the specification. > /// > UINT8 Revision; > UINT8 Reserved[3]; > @@ -152,7 +152,8 @@ typedef struct { > /// occurring during FSP execution. > /// > EFI_PHYSICAL_ADDRESS FspDebugHandler; > - UINT8 Reserved1[16]; > + UINT32 FspTemporaryRamSize; > + UINT8 Reserved1[12]; > } FSPT_ARCH2_UPD; > > /// > diff --git a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > index 002a5a1412..2168564e6d 100644 > --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > @@ -201,6 +201,27 @@ > movq rcx, xmm5 > %endmacro > > +; > +; Save TemporaryRamSize to YMM10[192:255] ; arg 1:general purpose > +register which holds TemporaryRamSize ; Modified: XMM5 and > +YMM10[192:255] ; > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + SXMMN xmm5, 1, %1 > + SYMMN ymm10, 1, xmm5 > + %endmacro > + > +; > +; Restore TemporaryRamSize from YMM10[192:255] ; arg 1:general > +purpose register where to save TemporaryRamSize ; Modified: XMM5 and > +%1 ; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + LXMMN xmm5, %1, 1 > + %endmacro > + > ; > ; YMM7[128:191] for calling stack > ; arg 1:Entry > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec > b/IntelFsp2Pkg/IntelFsp2Pkg.dec index d1c3d3ee7b..47ced1759e 100644 > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec > @@ -85,6 +85,10 @@ > gFspEventEndOfFirmwareGuid = { 0xbd44f629, 0xeae7, 0x4198, { 0x87, 0xf1, 0x39, 0xfa, 0xb0, 0xfd, 0x71, 0x7e } } > > [PcdsFixedAtBuild] > + # > + # As part of FSP-T execution, this will be initialized to TopOfCar address where Fsp T data will be saved. > + # Once Fsp Global Data is initialed later in FSP-M, this will hold the Global data buffer address. > + # > gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress |0xFED00108|UINT32|0x00000001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase |0xFEF00000|UINT32|0x10001001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize | 0x2000|UINT32|0x10001002 > diff --git > a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > index fb0d9a8683..316c2fa86a 100644 > --- > a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > +++ b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/Se > +++ cRamInitData.c > @@ -49,8 +49,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST FSPT_UPD_CORE_DATA FsptUpdDataPtr = { > }, > 0x00000020, > 0x00000000, > + 0x00000000, > { > - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > } > }, > -- > 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115940): https://edk2.groups.io/g/devel/message/115940 Mute This Topic: https://groups.io/mt/104388042/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <17B751C23B60AA20.13095@groups.io>]
* Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes [not found] ` <17B751C23B60AA20.13095@groups.io> @ 2024-02-26 7:26 ` cbduggap 0 siblings, 0 replies; 4+ messages in thread From: cbduggap @ 2024-02-26 7:26 UTC (permalink / raw) To: devel@edk2.groups.io, Duggapu, Chinni B, Desimone, Nathaniel L Cc: Chaganty, Rangasai V, Chiu, Chasel Corrected #3 in background. Thanks, Chinni. -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of cbduggap Sent: Monday, February 26, 2024 10:45 AM To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: Re: [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes Thanks, Nate, for taking some time to review the patch. I agree that GlobalDatapointer usage to pass Top of CAR from FSP-T to FSP-M is an ugly hack where I only convinced 90% (with my approach 😊) but ended up in adding that logic as we covered all corner cases. Here is the background why: 1. FSP-M Sec Core function SecGetPlatformData is using Top of CAR to retrieve "FSP T Core UPD information" and "Time Stamp for FSP T entry and exit". This code is FSP API Mode only code. 2. Both are invalid if some Boot loader not calling FSP-T. 3. Further in the changes, we are safely returning from FSP-M SecGetPlatformData Function when the value of the Register is 0 or if signature is not found. Few Options that are explored before adding this logic: 1. Initially was thinking to stop using SecGetPlatformData function in FSP-M Sec because we don’t have any use case where we need to save the details of FSP T Core UPD information for later stages. But we would need Top of CAR to capture FSP-T entry & exit time (which still can be retrieved in Bootloader code as we pass Start & end range of TempRam in ECX and EDX as a output). 2. Further thought of using some YMM register to save the value (inside FSP-T ) for later consumption but there is no guarantee that Bootloaders will preserve that value until they call FSP-M. I agree that we need to add the same logic inside IA32 implementation also which was missed (to find during Unit testing ) as we haven’t seen any Boot failures with these changes on another intel program where we are still using FSP 32 BIT (This is because we don’t use FSP T Core UPDs in FSP-M But might not be able to capture the FSP-T entry & exit time in FPDT for API Mode). I will take care of other feedback and re-send Patch V3 But need some inputs on "How we can avoid the GlobalDatapointer hack". Other feedback : 1. What is the reason for removing the LoadUpdPointerToECX function and putting the equivalent logic inline? Initially I thought that we need changes to this logic and cannot keep this as a separate function, but I agree we can reuse the same function now. Thanks, Chinni. -----Original Message----- From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> Sent: Saturday, February 24, 2024 4:22 AM To: Duggapu, Chinni B <chinni.b.duggapu@intel.com>; devel@edk2.groups.io Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@intel.com> Subject: RE: [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes Hi Chinni, Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. Moreover, you have added the assumption that FspGlobalDataPointer is initialized to top of CAR to SecFsp.c but you don't actually write top of CAR to that location in the IA32 version of TempRamInit(), only the X64 version has that addition. Since SecFsp.c is used on both, you have effectively broken all 32-bit FSP builds. For obvious reasons, your patch will not be merged without additional work. Additional feedback below inline. Thanks, Nate > -----Original Message----- > From: Duggapu, Chinni B <chinni.b.duggapu@intel.com> > Sent: Thursday, February 15, 2024 9:07 PM > To: devel@edk2.groups.io > Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Desimone, > Nathaniel L <nathaniel.l.desimone@intel.com>; Chiu, Chasel > <chasel.chiu@intel.com>; Duggapu, Chinni B > <chinni.b.duggapu@intel.com> > Subject: [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes > > Changes to support spec changes > > 1. Remove usage of Pcd. > 2. Change code to validate the Temporary Ram size input. > 3. Consume the input saved in YMM Register > > Cc: Sai Chaganty <rangasai.v.chaganty@intel.com> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Chiu Chasel <chasel.chiu@intel.com> > Cc: Duggapu Chinni B <chinni.b.duggapu@intel.com> > > > Signed-off-by: Duggapu Chinni B <chinni.b.duggapu@intel.com> > --- > IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 2 +- > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf | 1 + > .../FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryM.nasm | 1 - > .../FspSecCore/Ia32/FspApiEntryT.nasm | 110 ++++++++++++------ > .../FspSecCore/Ia32/SaveRestoreSseNasm.inc | 11 ++ > IntelFsp2Pkg/FspSecCore/SecFsp.c | 23 ++-- > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 4 +- > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm | 79 +++++++++++-- > IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm | 6 +- > IntelFsp2Pkg/Include/FspEas/FspApi.h | 5 +- > .../Include/SaveRestoreSseAvxNasm.inc | 21 ++++ > IntelFsp2Pkg/IntelFsp2Pkg.dec | 4 + > .../SecRamInitData.c | 3 +- > 15 files changed, 206 insertions(+), 67 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > index cb011f99f9..cf8cb2eda9 100644 > --- a/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf > @@ -63,11 +63,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > index 8029832235..717941c33f 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > @@ -62,11 +62,11 @@ > > [Pcd] > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > - gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspPrivateTemporaryRamSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > index e5a6eaa164..05c0d5f48b 100644 > --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > @@ -51,6 +51,7 @@ > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES > gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedBufferSize ## CONSUMES > + gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress ## CONSUMES > > [Ppis] > gEfiTemporaryRamSupportPpiGuid ## PRODUCES > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > index 15f8ecea83..5fa5c03569 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > index 61ab4612a3..861cce4d01 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > @@ -11,7 +11,6 @@ > ; Following are fixed PCDs > ; > extern ASM_PFX(PcdGet32(PcdTemporaryRamBase)) > -extern ASM_PFX(PcdGet32(PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32(PcdFspTemporaryRamSize)) > extern ASM_PFX(PcdGet8 (PcdFspHeapSizePercentage)) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > index 900126b93b..2f8465df3d 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > @@ -109,7 +109,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -178,29 +179,6 @@ endstruc > 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. > @@ -267,7 +245,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jae Fsp24UpdHeader > jmp Fsp22UpdHeader > > Fsp20UpdHeader: > @@ -405,7 +383,7 @@ CheckAddress: > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader1 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader1; > + jae Fsp24UpdHeader1; > jmp Fsp22UpdHeader1 > > Fsp20UpdHeader1: > @@ -497,7 +475,8 @@ ASM_PFX(EstablishStackFsp): > ; Enable FSP STACK > ; > mov esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - add esp, DWORD [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + LOAD_TEMPORARY_RAM_SIZE eax > + add esp, eax > > push DATA_LEN_OF_MCUD ; Size of the data region > push 4455434Dh ; Signature of the data region 'MCUD' > @@ -506,7 +485,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [edx + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], 2 > jb Fsp20UpdHeader2 > cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > - je Fsp24UpdHeader2 > + jae Fsp24UpdHeader2 > jmp Fsp22UpdHeader2 > > Fsp20UpdHeader2: > @@ -554,12 +533,13 @@ ContinueAfterUpdPush: > ; > ; Set ECX/EDX to the BootLoader temporary memory range > ; > - mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > - mov edx, ecx > - add edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamSize))] > + mov edx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > + LOAD_TEMPORARY_RAM_SIZE ecx > + add edx, ecx > sub edx, [ASM_PFX(PcdGet32 (PcdFspReservedBufferSize))] > + mov ecx, [ASM_PFX(PcdGet32 (PcdTemporaryRamBase))] > > - cmp ecx, edx ;If PcdFspReservedBufferSize >= PcdTemporaryRamSize, then error. > + cmp ecx, edx ;If PcdFspReservedBufferSize >= TemporaryRamSize, then error. > jb EstablishStackFspSuccess > mov eax, 80000003h ;EFI_UNSUPPORTED > jmp EstablishStackFspExit > @@ -589,6 +569,30 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_REGS > > + ; > + ; > + ; Read Fsp Info header and save in esi ; CALL_EDI > + ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor esi, esi > + mov esi, eax > + ; > + ; 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 > + ; > + xor ecx, ecx > + mov ecx, DWORD [esi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [esi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > +ParamValid: > + SAVE_ECX What is the reason for removing the LoadUpdPointerToECX function and putting the equivalent logic inline? > ; > ; Save timestamp into XMM6 > ; > @@ -596,9 +600,47 @@ ASM_PFX(TempRamInitApi): > SAVE_EAX > SAVE_EDX > > - CALL_EBP ASM_PFX(LoadUpdPointerToECX) ; ECX for UPD param > - SAVE_ECX ; save UPD param to slot 3 in xmm6 > + mov edx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [edx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size ; > + Irrespective of whether this UPD is supported or not, Fallback ; to > + Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > > + xor eax, eax > + mov ax, WORD [esi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov eax, 080000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + ; > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; > + SAVE_TEMPORARY_RAM_SIZE ebx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE edx > +GotTemporaryRamSize: > + LOAD_ECX > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > index 016f943b43..4d6ec1e984 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/SaveRestoreSseNasm.inc > @@ -128,6 +128,17 @@ > SXMMN xmm5, 1, eax > %endmacro > > +; > +; XMM5 slot 2 for TemporaryRamSize > +; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LXMMN xmm5, %1, 2 > + %endmacro > + > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + SXMMN xmm5, 2, %1 > + %endmacro > + > %macro ENABLE_SSE 0 > ; > ; Initialize floating point units diff --git > a/IntelFsp2Pkg/FspSecCore/SecFsp.c b/IntelFsp2Pkg/FspSecCore/SecFsp.c > index 11be1f97ca..0c74e533a4 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFsp.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFsp.c > @@ -47,11 +47,11 @@ FspGetExceptionHandler ( VOID EFIAPI > SecGetPlatformData ( > - IN OUT FSP_GLOBAL_DATA *FspData > + IN OUT FSP_GLOBAL_DATA *FspData, > + IN UINTN TopOfCar > ) > { > FSP_PLAT_DATA *FspPlatformData; > - UINT32 TopOfCar; > UINT32 *StackPtr; > UINT32 DwordSize; > > @@ -68,11 +68,11 @@ SecGetPlatformData ( > FspPlatformData->CodeRegionBase = 0; > FspPlatformData->CodeRegionSize = 0; > > - // > - // Pointer to the size field > - // > - TopOfCar = PcdGet32 (PcdTemporaryRamBase) + PcdGet32 > (PcdTemporaryRamSize); > - StackPtr = (UINT32 *)(TopOfCar - sizeof (UINT32)); > + if (TopOfCar == 0) { > + return; > + } > + > + StackPtr = (UINT32 *)((UINT32)TopOfCar - sizeof (UINT32)); > > if (*(StackPtr - 1) == FSP_MCUD_SIGNATURE) { > while (*StackPtr != 0) { > @@ -123,7 +123,14 @@ FspGlobalDataInit ( > VOID *FspmUpdDataPtr; > CHAR8 ImageId[9]; > UINTN Idx; > + UINTN *TopOfCar; > > + // > + // If TempRam is initilized using FspTempRamInitApi (), > + GlobalDataPointer // will point to the Top of the Car and any Data > + that FspTempRamInitApi // wants to Handoff to later stages will be pushed on to the Top of the car. > + // > + TopOfCar = *(VOID **)(UINTN)PcdGet32 > + (PcdGlobalDataPointerAddress); There are some customers that do not run FSP-T. The assumption that this will be initialized to the TopOfCar during FSP-T would not hold on those platforms. > // > // Set FSP Global Data pointer > // > @@ -147,7 +154,7 @@ FspGlobalDataInit ( > // It may have multiple FVs, so look into the last one for FSP header > // > PeiFspData->FspInfoHeader = (FSP_INFO_HEADER > *)(UINTN)AsmGetFspInfoHeader (); > - SecGetPlatformData (PeiFspData); > + SecGetPlatformData (PeiFspData, (UINTN)TopOfCar); > > // > // Set API calling mode > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > index 5f59938518..33aaac66c1 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > @@ -42,9 +42,7 @@ FspApiCallingCheck ( > // > // FspMemoryInit check > // > - if (((UINTN)FspData != MAX_ADDRESS) && ((UINTN)FspData != MAX_UINT32)) { > - Status = EFI_UNSUPPORTED; > - } else if (ApiParam == NULL) { One cannot assume that FspGlobalDataPointer gets initialized by FSP-T as mentioned above. > + if (ApiParam == NULL) { > Status = EFI_SUCCESS; > } else if (EFI_ERROR (FspUpdSignatureCheck (ApiIdx, ApiParam))) { > Status = EFI_INVALID_PARAMETER; diff --git > a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > index 698bb063a7..c2b7b25b36 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryT.nasm > @@ -16,6 +16,7 @@ > extern ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > extern ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > extern ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > +extern ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > > ; > ; Following functions will be provided in PlatformSecLib @@ -76,7 > +77,8 @@ struc LoadMicrocodeParamsFsp24 > .FsptArchReserved: resb 3 > .FsptArchLength: resd 1 > .FspDebugHandler resq 1 > - .FsptArchUpd: resd 4 > + .FspTemporaryRamSize: resd 1 ; Supported only if ArchRevison is >= 3 > + .FsptArchUpd: resd 3 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resq 1 > @@ -163,7 +165,7 @@ ASM_PFX(LoadMicrocodeDefault): > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > mov rax, qword [rsp + LoadMicrocodeParamsFsp24.MicrocodeCodeSize] > @@ -273,7 +275,7 @@ CheckAddress: > cmp byte [rsp + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError > cmp byte [rsp + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - jne ParamError > + jb ParamError > > ; UPD structure is compliant with FSP spec 2.4 > ; Is automatic size detection ? > @@ -337,9 +339,16 @@ ASM_PFX(EstablishStackFsp): > ; > mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov esp, DWORD[rax] > - mov rax, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add esp, DWORD[rax] > - > + LOAD_TEMPORARY_RAM_SIZE rax > + add esp, eax > + ; > + ; Save top of the TemporaryRam in PcdGlobalDataPointerAddress ; > + where the FspTempRamInitAPI information is pushed as part of below > + code ; which will be used in other Fsp APIs to retrive the same. > + ; > + mov rax, ASM_PFX(PcdGet32 (PcdGlobalDataPointerAddress)) > + mov eax, DWORD[rax] > + mov DWORD[eax], esp Using the FspGlobalDataPointer as a scratchpad for your top of CAR early and then putting the real global data into it later is an ugly hack. If you are going to initialize the FspGlobalDataPointer in FSP-T, then FSP-T needs to actually initialize that FSP global data structure fully. > sub esp, 4 > mov dword[esp], DATA_LEN_OF_MCUD ; Size of the data region > sub esp, 4 > @@ -349,7 +358,7 @@ ASM_PFX(EstablishStackFsp): > cmp byte [rdx + LoadMicrocodeParamsFsp24.FspUpdHeaderRevision], 2 > jb ParamError1 > cmp byte [rdx + LoadMicrocodeParamsFsp24.FsptArchRevision], 2 > - je Fsp24UpdHeader > + jnb Fsp24UpdHeader > > ParamError1: > mov rax, 08000000000000002h > @@ -397,8 +406,8 @@ ContinueAfterUpdPush: > ; > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > mov edx, [ecx] > - mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > - add edx, [ecx] > + LOAD_TEMPORARY_RAM_SIZE rcx > + add edx, ecx > mov rcx, ASM_PFX(PcdGet32 (PcdFspReservedBufferSize)) > sub edx, [ecx] > mov rcx, ASM_PFX(PcdGet32 (PcdTemporaryRamBase)) > @@ -439,6 +448,12 @@ ASM_PFX(TempRamInitApi): > ; > SAVE_BFV rbp > > + ; > + ; Read Fsp Info header and save in rsi ; CALL_RDI > + ASM_PFX(AsmGetFspInfoHeaderNoStack) > + xor rsi, rsi > + mov rsi, rax > ; > ; Save Input Parameter in YMM10 > ; > @@ -448,10 +463,9 @@ ASM_PFX(TempRamInitApi): > ; > ; 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 > + mov ecx, DWORD [rsi + 01Ch] ; Read FsptImageBaseAddress > + add ecx, DWORD [rsi + 024h] ; Get Cfg Region base address = FsptImageBaseAddress + CfgRegionOffset > ParamValid: > SAVE_RCX > > @@ -463,6 +477,47 @@ ParamValid: > or rax, rdx > SAVE_TS rax > > + mov rdx, ASM_PFX(PcdGet32 (PcdTemporaryRamSize)) > + mov edx, DWORD [rdx] > + ; > + ; Read ARCH2 UPD input value. > + ; > + mov ebx, DWORD [ecx + LoadMicrocodeParamsFsp24.FspTemporaryRamSize] > + ; > + ; As per spec, if Bootloader pass zero, use Fsp defined Size ; > + Irrespective of whether this UPD is supported or not, Fallback ; to > + Fsp defined size if input is zero. > + ; > + cmp ebx, 0 > + jz UseTemporaryRamSizePcd > + > + xor rax, rax > + mov ax, WORD [rsi + 020h] ; Read ImageAttribute > + test ax, 16 ; check if Bit4 is set > + jnz ConsumeInputConfiguration > + ; > + ; Sometimes user may change input value even if it is not supported > + ; return error if input is Non-Zero and not same as PcdTemporaryRamSize. > + ; > + cmp ebx, edx > + je UseTemporaryRamSizePcd > + mov rax, 08000000000000002h ; RETURN_INVALID_PARAMETER > + jmp TempRamInitExit > +ConsumeInputConfiguration: > + ; > + ; Read Fsp Arch2 revision > + cmp byte [ecx + LoadMicrocodeParamsFsp24.FsptArchRevision], 3 > + jb UseTemporaryRamSizePcd > + ; > + ; Read ARCH2 UPD value and Save. > + ; Only low-32 bits of rbx/rdx holds the temporary ram size. > + ; > + SAVE_TEMPORARY_RAM_SIZE rbx > + jmp GotTemporaryRamSize > +UseTemporaryRamSizePcd: > + SAVE_TEMPORARY_RAM_SIZE rdx > + > +GotTemporaryRamSize: > ; > ; Sec Platform Init > ; > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > index 32a60270b8..61ebaa8ccf 100644 > --- a/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspHelper.nasm > @@ -28,7 +28,7 @@ ASM_PFX(FspInfoHeaderRelativeOff): > global ASM_PFX(AsmGetFspInfoHeaderNoStack) > ASM_PFX(AsmGetFspInfoHeaderNoStack): > lea rax, [ASM_PFX(AsmGetFspInfoHeader)] > - lea rcx, [ASM_PFX(FspInfoHeaderRelativeOff)] > - mov ecx, [rcx] > - sub rax, rcx > + lea rsi, [ASM_PFX(FspInfoHeaderRelativeOff)] > + mov esi, [rsi] > + sub rax, rsi > jmp rdi > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index 40e063e944..27d5ec3a3c 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -139,7 +139,7 @@ typedef struct { > /// > typedef struct { > /// > - /// Revision of the structure is 2 for this version of the specification. > + /// Revision of the structure is 3 for this version of the specification. > /// > UINT8 Revision; > UINT8 Reserved[3]; > @@ -152,7 +152,8 @@ typedef struct { > /// occurring during FSP execution. > /// > EFI_PHYSICAL_ADDRESS FspDebugHandler; > - UINT8 Reserved1[16]; > + UINT32 FspTemporaryRamSize; > + UINT8 Reserved1[12]; > } FSPT_ARCH2_UPD; > > /// > diff --git a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > index 002a5a1412..2168564e6d 100644 > --- a/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > +++ b/IntelFsp2Pkg/Include/SaveRestoreSseAvxNasm.inc > @@ -201,6 +201,27 @@ > movq rcx, xmm5 > %endmacro > > +; > +; Save TemporaryRamSize to YMM10[192:255] ; arg 1:general purpose > +register which holds TemporaryRamSize ; Modified: XMM5 and > +YMM10[192:255] ; > +%macro SAVE_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + SXMMN xmm5, 1, %1 > + SYMMN ymm10, 1, xmm5 > + %endmacro > + > +; > +; Restore TemporaryRamSize from YMM10[192:255] ; arg 1:general > +purpose register where to save TemporaryRamSize ; Modified: XMM5 and > +%1 ; > +%macro LOAD_TEMPORARY_RAM_SIZE 1 > + LYMMN ymm10, xmm5, 1 > + LXMMN xmm5, %1, 1 > + %endmacro > + > ; > ; YMM7[128:191] for calling stack > ; arg 1:Entry > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec > b/IntelFsp2Pkg/IntelFsp2Pkg.dec index d1c3d3ee7b..47ced1759e 100644 > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec > @@ -85,6 +85,10 @@ > gFspEventEndOfFirmwareGuid = { 0xbd44f629, 0xeae7, 0x4198, { 0x87, 0xf1, 0x39, 0xfa, 0xb0, 0xfd, 0x71, 0x7e } } > > [PcdsFixedAtBuild] > + # > + # As part of FSP-T execution, this will be initialized to TopOfCar address where Fsp T data will be saved. > + # Once Fsp Global Data is initialed later in FSP-M, this will hold the Global data buffer address. > + # > gIntelFsp2PkgTokenSpaceGuid.PcdGlobalDataPointerAddress |0xFED00108|UINT32|0x00000001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamBase |0xFEF00000|UINT32|0x10001001 > gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize | 0x2000|UINT32|0x10001002 > diff --git > a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > index fb0d9a8683..316c2fa86a 100644 > --- > a/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/SecRam > InitData.c > +++ b/IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/Se > +++ cRamInitData.c > @@ -49,8 +49,9 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST FSPT_UPD_CORE_DATA FsptUpdDataPtr = { > }, > 0x00000020, > 0x00000000, > + 0x00000000, > { > - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 > } > }, > -- > 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115945): https://edk2.groups.io/g/devel/message/115945 Mute This Topic: https://groups.io/mt/104388042/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-26 7:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-16 5:07 [edk2-devel] [PATCH v2] IntelFsp2Pkg: Fsp 2.x Changes cbduggap
2024-02-23 22:51 ` Nate DeSimone
2024-02-26 5:14 ` cbduggap
[not found] ` <17B751C23B60AA20.13095@groups.io>
2024-02-26 7:26 ` cbduggap
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox