* [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. @ 2022-07-14 20:04 Chiu, Chasel 2022-07-18 21:40 ` Nate DeSimone 0 siblings, 1 reply; 6+ messages in thread From: Chiu, Chasel @ 2022-07-14 20:04 UTC (permalink / raw) To: devel; +Cc: Hongbin1 Zhang, Nate DeSimone, Star Zeng, Chasel Chiu From: Hongbin1 Zhang <hongbin1.zhang@intel.com> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 Add FSP-I API entry point for SMM support. Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Star Zeng <star.zeng@intel.com> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> Signed-off-by: Hongbin1 Zhang <hongbin1.zhang@intel.com> --- IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 ++++++++++++++++++++++++++++++++++++++------------------- IntelFsp2Pkg/Include/FspGlobalData.h | 53 ++++++++++++++++++++++++++++------------------------- IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 +++++++++++++++------- IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 ++++++++++++++++---------- IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- 10 files changed, 256 insertions(+), 64 deletions(-) diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c index e22a88cc84..35d223a404 100644 --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c @@ -71,6 +71,19 @@ FspApiCallingCheck ( Status = EFI_INVALID_PARAMETER; } } + } else if (ApiIdx == FspSmmInitApiIndex) { + // + // FspSmmInitApiIndex check + // + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || ((UINTN)FspData == MAX_UINT32)) { + Status = EFI_UNSUPPORTED; + } else { + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { + Status = EFI_UNSUPPORTED; + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, ApiParam))) { + Status = EFI_INVALID_PARAMETER; + } + } } else { Status = EFI_UNSUPPORTED; } diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf new file mode 100644 index 0000000000..d31576c00b --- /dev/null +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf @@ -0,0 +1,54 @@ +## @file +# Sec Core for FSP +# +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = FspSecCoreI + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 + MODULE_TYPE = SEC + VERSION_STRING = 1.0 + +# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = IA32 X64 +# + +[Sources] + SecFspApiChk.c + SecFsp.h + +[Sources.X64] + X64/FspApiEntryI.nasm + X64/FspApiEntryCommon.nasm + X64/FspHelper.nasm + +[Sources.IA32] + Ia32/FspApiEntryI.nasm + Ia32/FspApiEntryCommon.nasm + Ia32/FspHelper.nasm + +[Binaries.Ia32] + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC + +[Packages] + MdePkg/MdePkg.dec + IntelFsp2Pkg/IntelFsp2Pkg.dec + +[LibraryClasses] + BaseMemoryLib + DebugLib + BaseLib + PciCf8Lib + SerialPortLib + FspSwitchStackLib + FspCommonLib + FspSecPlatformLib + + diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm new file mode 100644 index 0000000000..e9365d6832 --- /dev/null +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm @@ -0,0 +1,44 @@ +;; @file +; Provide FSP API entry points. +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; SPDX-License-Identifier: BSD-2-Clause-Patent +;; + + SECTION .text + +; +; Following functions will be provided in C +; +extern ASM_PFX(FspApiCommon) + +;---------------------------------------------------------------------------- +; FspApiCommonContinue API +; +; This is the FSP API common entry point to resume the FSP execution +; +;---------------------------------------------------------------------------- +global ASM_PFX(FspApiCommonContinue) +ASM_PFX(FspApiCommonContinue): + jmp $ + +;---------------------------------------------------------------------------- +; FspSmmInit API +; +; This FSP API will notify the FSP about the different phases in the boot +; process +; +;---------------------------------------------------------------------------- +global ASM_PFX(FspSmmInitApi) +ASM_PFX(FspSmmInitApi): + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex + jmp ASM_PFX(FspApiCommon) + +;---------------------------------------------------------------------------- +; Module Entrypoint API +;---------------------------------------------------------------------------- +global ASM_PFX(_ModuleEntryPoint) +ASM_PFX(_ModuleEntryPoint): + jmp $ + ; Add reference to APIs so that it will not be optimized by compiler + jmp ASM_PFX(FspSmmInitApi) diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm new file mode 100644 index 0000000000..e9365d6832 --- /dev/null +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm @@ -0,0 +1,44 @@ +;; @file +; Provide FSP API entry points. +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; SPDX-License-Identifier: BSD-2-Clause-Patent +;; + + SECTION .text + +; +; Following functions will be provided in C +; +extern ASM_PFX(FspApiCommon) + +;---------------------------------------------------------------------------- +; FspApiCommonContinue API +; +; This is the FSP API common entry point to resume the FSP execution +; +;---------------------------------------------------------------------------- +global ASM_PFX(FspApiCommonContinue) +ASM_PFX(FspApiCommonContinue): + jmp $ + +;---------------------------------------------------------------------------- +; FspSmmInit API +; +; This FSP API will notify the FSP about the different phases in the boot +; process +; +;---------------------------------------------------------------------------- +global ASM_PFX(FspSmmInitApi) +ASM_PFX(FspSmmInitApi): + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex + jmp ASM_PFX(FspApiCommon) + +;---------------------------------------------------------------------------- +; Module Entrypoint API +;---------------------------------------------------------------------------- +global ASM_PFX(_ModuleEntryPoint) +ASM_PFX(_ModuleEntryPoint): + jmp $ + ; Add reference to APIs so that it will not be optimized by compiler + jmp ASM_PFX(FspSmmInitApi) diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h b/IntelFsp2Pkg/Include/FspEas/FspApi.h index b36bc2b9ae..1d6c2fb63d 100644 --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h @@ -135,18 +135,18 @@ typedef struct { /// /// Revision of the structure is 2 for this version of the specification. /// - UINT8 Revision; - UINT8 Reserved[3]; + UINT8 Revision; + UINT8 Reserved[3]; /// /// Length of the structure in bytes. The current value for this field is 32. /// - UINT32 Length; + UINT32 Length; /// /// FspDebugHandler Optional debug handler for the bootloader to receive debug messages /// occurring during FSP execution. /// - EFI_PHYSICAL_ADDRESS FspDebugHandler; - UINT8 Reserved1[16]; + EFI_PHYSICAL_ADDRESS FspDebugHandler; + UINT8 Reserved1[16]; } FSPT_ARCH2_UPD; /// @@ -197,37 +197,37 @@ typedef struct { /// /// Revision of the structure is 3 for this version of the specification. /// - UINT8 Revision; - UINT8 Reserved[3]; + UINT8 Revision; + UINT8 Reserved[3]; /// /// Length of the structure in bytes. The current value for this field is 64. /// - UINT32 Length; + UINT32 Length; /// /// Pointer to the temporary stack base address to be /// consumed inside FspMemoryInit() API. /// - EFI_PHYSICAL_ADDRESS StackBase; + EFI_PHYSICAL_ADDRESS StackBase; /// /// Temporary stack size to be consumed inside /// FspMemoryInit() API. /// - UINT64 StackSize; + UINT64 StackSize; /// /// Size of memory to be reserved by FSP below "top /// of low usable memory" for bootloader usage. /// - UINT32 BootLoaderTolumSize; + UINT32 BootLoaderTolumSize; /// /// Current boot mode. /// - UINT32 BootMode; + UINT32 BootMode; /// /// Optional event handler for the bootloader to be informed of events occurring during FSP execution. /// This value is only valid if Revision is >= 2. /// - EFI_PHYSICAL_ADDRESS FspEventHandler; - UINT8 Reserved1[24]; + EFI_PHYSICAL_ADDRESS FspEventHandler; + UINT8 Reserved1[24]; } FSPM_ARCH2_UPD; /// @@ -266,18 +266,18 @@ typedef struct { /// /// Revision of the structure is 2 for this version of the specification. /// - UINT8 Revision; - UINT8 Reserved[3]; + UINT8 Revision; + UINT8 Reserved[3]; /// /// Length of the structure in bytes. The current value for this field is 32. /// - UINT32 Length; + UINT32 Length; /// /// FspEventHandler Optional event handler for the bootloader to be informed of events /// occurring during FSP execution. /// - EFI_PHYSICAL_ADDRESS FspEventHandler; - UINT8 Reserved1[16]; + EFI_PHYSICAL_ADDRESS FspEventHandler; + UINT8 Reserved1[16]; } FSPS_ARCH2_UPD; /// @@ -609,4 +609,23 @@ EFI_STATUS IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr ); +/** + This FSP API initializes SMM and provide any OS runtime silicon services, + including Reliability, Availability, and Serviceability (RAS) features implemented by the CPU. + + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. + If NULL, FSP will use the default parameters. + + @retval EFI_SUCCESS FSP execution environment was initialized successfully. + @retval EFI_INVALID_PARAMETER Input parameters are invalid. + @retval EFI_UNSUPPORTED The FSP calling conditions were not met. + @retval EFI_DEVICE_ERROR FSP initialization failed. + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status codes will not be returned during S3. +**/ +typedef +EFI_STATUS +(EFIAPI *FSP_SMM_INIT)( + IN VOID *FspiUpdDataPtr + ); + #endif diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h b/IntelFsp2Pkg/Include/FspGlobalData.h index 445540abfa..697b20ed4c 100644 --- a/IntelFsp2Pkg/Include/FspGlobalData.h +++ b/IntelFsp2Pkg/Include/FspGlobalData.h @@ -10,9 +10,9 @@ #include <FspEas.h> -#define FSP_IN_API_MODE 0 -#define FSP_IN_DISPATCH_MODE 1 -#define FSP_GLOBAL_DATA_VERSION 1 +#define FSP_IN_API_MODE 0 +#define FSP_IN_DISPATCH_MODE 1 +#define FSP_GLOBAL_DATA_VERSION 1 #pragma pack(1) @@ -24,16 +24,17 @@ typedef enum { TempRamExitApiIndex, FspSiliconInitApiIndex, FspMultiPhaseSiInitApiIndex, + FspSmmInitApiIndex, FspApiIndexMax } FSP_API_INDEX; typedef struct { - VOID *DataPtr; - UINTN MicrocodeRegionBase; - UINTN MicrocodeRegionSize; - UINTN CodeRegionBase; - UINTN CodeRegionSize; - UINTN Reserved; + VOID *DataPtr; + UINTN MicrocodeRegionBase; + UINTN MicrocodeRegionSize; + UINTN CodeRegionBase; + UINTN CodeRegionSize; + UINTN Reserved; } FSP_PLAT_DATA; #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'D') @@ -41,28 +42,28 @@ typedef struct { #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF typedef struct { - UINT32 Signature; - UINT8 Version; - UINT8 Reserved1[3]; + UINT32 Signature; + UINT8 Version; + UINT8 Reserved1[3]; /// /// Offset 0x08 /// - UINTN CoreStack; - UINTN Reserved2; + UINTN CoreStack; + UINTN Reserved2; /// /// IA32: Offset 0x10; X64: Offset 0x18 /// - UINT32 StatusCode; - UINT8 ApiIdx; + UINT32 StatusCode; + UINT8 ApiIdx; /// /// 0: FSP in API mode; 1: FSP in DISPATCH mode /// - UINT8 FspMode; - UINT8 OnSeparateStack; - UINT8 Reserved3; - UINT32 NumberOfPhases; - UINT32 PhasesExecuted; - UINT32 Reserved4[8]; + UINT8 FspMode; + UINT8 OnSeparateStack; + UINT8 Reserved3; + UINT32 NumberOfPhases; + UINT32 PhasesExecuted; + UINT32 Reserved4[8]; /// /// IA32: Offset 0x40; X64: Offset 0x48 /// Start of UINTN and pointer section @@ -75,21 +76,23 @@ typedef struct { VOID *TempRamInitUpdPtr; VOID *MemoryInitUpdPtr; VOID *SiliconInitUpdPtr; + VOID *SmmInitUpdPtr; /// - /// IA32: Offset 0x64; X64: Offset 0x90 + /// IA32: Offset 0x68; X64: Offset 0x98 /// To store function parameters pointer /// so it can be retrieved after stack switched. /// VOID *FunctionParameterPtr; FSP_INFO_HEADER *FspInfoHeader; VOID *UpdDataPtr; + UINTN Reserved5; /// /// End of UINTN and pointer section /// - UINT8 Reserved5[16]; + UINT8 Reserved6[16]; UINT32 PerfSig; UINT16 PerfLen; - UINT16 Reserved6; + UINT16 Reserved7; UINT32 PerfIdx; UINT64 PerfData[32]; } FSP_GLOBAL_DATA; diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h index c660defac3..c7fb63168f 100644 --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h @@ -26,13 +26,13 @@ #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 -#define FSP_IA32 0 -#define FSP_X64 1 +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 +#define FSP_IA32 0 +#define FSP_X64 1 -#pragma pack(1) + #pragma pack(1) /// /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. @@ -159,6 +159,14 @@ typedef struct { /// Byte 0x4E: Reserved4. /// UINT16 Reserved4; + /// + /// Byte 0x50: Offset for the API for the Multi-Phase memory initialization. + /// + UINT32 FspMultiPhaseMemInitEntryOffset; + /// + /// Byte 0x54: Offset for the API to initialize SMM. + /// + UINT32 FspSmmInitEntryOffset; } FSP_INFO_HEADER; /// @@ -240,7 +248,7 @@ typedef struct { // UINT32 PatchData[]; } FSP_PATCH_TABLE; -#pragma pack() + #pragma pack() extern EFI_GUID gFspHeaderFileGuid; diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc b/IntelFsp2Pkg/IntelFsp2Pkg.dsc index 7cf7e88245..b2d7867880 100644 --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc @@ -68,6 +68,7 @@ IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py index c4fb1f1bb2..128b896592 100644 --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py @@ -953,8 +953,8 @@ EndList return NoFileChange def CreateSplitUpdTxt (self, UpdTxtFile): - GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID'] - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, and _S signature for FSPT, FSPM, FSPS + GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID','FSP_I_UPD_TOOL_GUID'] + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] # _T, _M, _S and _I signature for FSPT, FSPM, FSPS, FSPI for Index in range(len(GuidList)): UpdTxtFile = '' FvDir = self._FvDir @@ -1288,19 +1288,21 @@ EndList Chars.append(chr(Value & 0xFF)) Value = Value >> 8 SignatureStr = ''.join(Chars) - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS accordingly + # Signature will be _T / _M / _S / _I for FSPT / FSPM / FSPS /FSPI accordingly if '_T' in SignatureStr[6:6+2]: TxtBody.append("#define FSPT_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) elif '_M' in SignatureStr[6:6+2]: TxtBody.append("#define FSPM_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) elif '_S' in SignatureStr[6:6+2]: TxtBody.append("#define FSPS_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) + elif '_I' in SignatureStr[6:6+2]: + TxtBody.append("#define FSPI_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) TxtBody.append("\n") for Region in ['UPD']: UpdOffsetTable = [] - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', '_M', '_S'] signature for FSPT, FSPM, FSPS - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] #['_T', '_M', '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', 'FSPI_UPD'] for Item in self._CfgItemList: if Item["cname"] == 'Signature' and Item["value"][0:6] in UpdSignature: Item["offset"] = 0 # re-initialize offset to 0 when new UPD structure starting @@ -1393,11 +1395,12 @@ EndList HeaderTFileName = 'FsptUpd.h' HeaderMFileName = 'FspmUpd.h' HeaderSFileName = 'FspsUpd.h' + HeaderIFileName = 'FspiUpd.h' - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE'] - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD'] + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # FSPX_UPD_REGION + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', 'FSPS_ARCH2_UPD'] IncLines = [] @@ -1420,6 +1423,9 @@ EndList elif UpdRegionCheck[item] == 'FSPS': HeaderFd = open(os.path.join(FvDir, HeaderSFileName), "w") FileBase = os.path.basename(os.path.join(FvDir, HeaderSFileName)) + elif UpdRegionCheck[item] == 'FSPI': + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), "w") + FileBase = os.path.basename(os.path.join(FvDir, HeaderIFileName)) FileName = FileBase.replace(".", "_").upper() HeaderFd.write("%s\n" % (__copyright_h__ % date.today().year)) HeaderFd.write("#ifndef __%s__\n" % FileName) @@ -1696,7 +1702,7 @@ EndList def Usage(): - print ("GenCfgOpt Version 0.57") + print ("GenCfgOpt Version 0.58") print ("Usage:") print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir [-D Macros]") print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir InputHFile [-D Macros]") diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py b/IntelFsp2Pkg/Tools/SplitFspBin.py index f9151b5afd..317d9c1fa0 100644 --- a/IntelFsp2Pkg/Tools/SplitFspBin.py +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py @@ -1,6 +1,6 @@ ## @ SplitFspBin.py # -# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR> +# Copyright (c) 2015 - 2022, Intel Corporation. All rights reserved.<BR> # SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -492,7 +492,7 @@ class FspImage: self.FihOffset = fihoff self.Offset = offset self.FvIdxList = [] - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] self.PatchList = patch self.PatchList.append(fihoff + 0x1C) @@ -869,7 +869,7 @@ def main (): parser_rebase = subparsers.add_parser('rebase', help='rebase a FSP into a new base address') parser_rebase.set_defaults(which='rebase') parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', type=str, help='FSP binary file path', required = True) - parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) + parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) parser_rebase.add_argument('-b', '--newbase', dest='FspBase', nargs='+', type=str, help='Rebased FSP binary file name', default = '', required = True) parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', type=str, help='Output directory path', default = '.') parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', type=str, help='Rebased FSP binary file name', default = '') -- 2.35.0.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. 2022-07-14 20:04 [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support Chiu, Chasel @ 2022-07-18 21:40 ` Nate DeSimone 2022-07-18 21:49 ` [edk2-devel] " Pedro Falcato 2022-07-18 22:02 ` Chiu, Chasel 0 siblings, 2 replies; 6+ messages in thread From: Nate DeSimone @ 2022-07-18 21:40 UTC (permalink / raw) To: Chiu, Chasel, devel@edk2.groups.io; +Cc: Zhang, Hongbin1, Zeng, Star Hi Chasel, Please see comments inline. Here is a summary of my feedback: #1) IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm - line 34 - Bug: eax should be rax #2) IntelFsp2Pkg/Include/FspEas/FspApi.h - Various unnecessary whitespace changes that make the file look worse than before. #3) IntelFsp2Pkg/Include/Guid/FspHeaderFile.h - Why indent the #pragma lines? Thanks, Nate > -----Original Message----- > From: Chiu, Chasel <chasel.chiu@intel.com> > Sent: Thursday, July 14, 2022 1:04 PM > To: devel@edk2.groups.io > Cc: Zhang, Hongbin1 <hongbin1.zhang@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; Chiu, > Chasel <chasel.chiu@intel.com> > Subject: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > From: Hongbin1 Zhang <hongbin1.zhang@intel.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 > Add FSP-I API entry point for SMM support. > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> > Signed-off-by: Hongbin1 Zhang <hongbin1.zhang@intel.com> > --- > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ > IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 ++++++++++++++++++++++++++++++++++++++------------------- > IntelFsp2Pkg/Include/FspGlobalData.h | 53 ++++++++++++++++++++++++++++------------------------- > IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 +++++++++++++++------- > IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + > IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 ++++++++++++++++---------- > IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- > 10 files changed, 256 insertions(+), 64 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > index e22a88cc84..35d223a404 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > @@ -71,6 +71,19 @@ FspApiCallingCheck ( > Status = EFI_INVALID_PARAMETER; > } > } > + } else if (ApiIdx == FspSmmInitApiIndex) { > + // > + // FspSmmInitApiIndex check > + // > + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || ((UINTN)FspData == MAX_UINT32)) { > + Status = EFI_UNSUPPORTED; > + } else { > + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { > + Status = EFI_UNSUPPORTED; > + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, ApiParam))) { > + Status = EFI_INVALID_PARAMETER; > + } > + } > } else { > Status = EFI_UNSUPPORTED; > } > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > new file mode 100644 > index 0000000000..d31576c00b > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > @@ -0,0 +1,54 @@ > +## @file > +# Sec Core for FSP > +# > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = FspSecCoreI > + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 > + MODULE_TYPE = SEC > + VERSION_STRING = 1.0 > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Sources] > + SecFspApiChk.c > + SecFsp.h > + > +[Sources.X64] > + X64/FspApiEntryI.nasm > + X64/FspApiEntryCommon.nasm > + X64/FspHelper.nasm > + > +[Sources.IA32] > + Ia32/FspApiEntryI.nasm > + Ia32/FspApiEntryCommon.nasm > + Ia32/FspHelper.nasm > + > +[Binaries.Ia32] > + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC > + > +[Packages] > + MdePkg/MdePkg.dec > + IntelFsp2Pkg/IntelFsp2Pkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + BaseLib > + PciCf8Lib > + SerialPortLib > + FspSwitchStackLib > + FspCommonLib > + FspSecPlatformLib > + > + > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > new file mode 100644 > index 0000000000..e9365d6832 > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > @@ -0,0 +1,44 @@ > +;; @file > +; Provide FSP API entry points. > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +; SPDX-License-Identifier: BSD-2-Clause-Patent > +;; > + > + SECTION .text > + > +; > +; Following functions will be provided in C > +; > +extern ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; FspApiCommonContinue API > +; > +; This is the FSP API common entry point to resume the FSP execution > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspApiCommonContinue) > +ASM_PFX(FspApiCommonContinue): > + jmp $ > + > +;---------------------------------------------------------------------------- > +; FspSmmInit API > +; > +; This FSP API will notify the FSP about the different phases in the boot > +; process > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspSmmInitApi) > +ASM_PFX(FspSmmInitApi): > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > + jmp ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; Module Entrypoint API > +;---------------------------------------------------------------------------- > +global ASM_PFX(_ModuleEntryPoint) > +ASM_PFX(_ModuleEntryPoint): > + jmp $ > + ; Add reference to APIs so that it will not be optimized by compiler > + jmp ASM_PFX(FspSmmInitApi) > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > new file mode 100644 > index 0000000000..e9365d6832 > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > @@ -0,0 +1,44 @@ > +;; @file > +; Provide FSP API entry points. > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +; SPDX-License-Identifier: BSD-2-Clause-Patent > +;; > + > + SECTION .text > + > +; > +; Following functions will be provided in C > +; > +extern ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; FspApiCommonContinue API > +; > +; This is the FSP API common entry point to resume the FSP execution > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspApiCommonContinue) > +ASM_PFX(FspApiCommonContinue): > + jmp $ > + > +;---------------------------------------------------------------------------- > +; FspSmmInit API > +; > +; This FSP API will notify the FSP about the different phases in the boot > +; process > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspSmmInitApi) > +ASM_PFX(FspSmmInitApi): > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex This is a bug. It should be: mov rax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex Note that IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm compares ApiIndex numbers using rax, so it is important to make sure that the upper 32-bits are zero: https://github.com/tianocore/edk2/blob/c966204049f3d5dae6d5e587ddc298c684142c5c/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm#L65 > + jmp ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; Module Entrypoint API > +;---------------------------------------------------------------------------- > +global ASM_PFX(_ModuleEntryPoint) > +ASM_PFX(_ModuleEntryPoint): > + jmp $ > + ; Add reference to APIs so that it will not be optimized by compiler > + jmp ASM_PFX(FspSmmInitApi) > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index b36bc2b9ae..1d6c2fb63d 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -135,18 +135,18 @@ typedef struct { > /// > /// Revision of the structure is 2 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 32. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// FspDebugHandler Optional debug handler for the bootloader to receive debug messages > /// occurring during FSP execution. > /// > - EFI_PHYSICAL_ADDRESS FspDebugHandler; > - UINT8 Reserved1[16]; > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > + UINT8 Reserved1[16]; Any reason for these whitespace changes? It looks worse than before. > } FSPT_ARCH2_UPD; > > /// > @@ -197,37 +197,37 @@ typedef struct { > /// > /// Revision of the structure is 3 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 64. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// Pointer to the temporary stack base address to be > /// consumed inside FspMemoryInit() API. > /// > - EFI_PHYSICAL_ADDRESS StackBase; > + EFI_PHYSICAL_ADDRESS StackBase; Any reason for this whitespace change? It looks worse than before. > /// > /// Temporary stack size to be consumed inside > /// FspMemoryInit() API. > /// > - UINT64 StackSize; > + UINT64 StackSize; Any reason for this whitespace change? It looks worse than before. > /// > /// Size of memory to be reserved by FSP below "top > /// of low usable memory" for bootloader usage. > /// > - UINT32 BootLoaderTolumSize; > + UINT32 BootLoaderTolumSize; Any reason for this whitespace change? It looks worse than before. > /// > /// Current boot mode. > /// > - UINT32 BootMode; > + UINT32 BootMode; Any reason for this whitespace change? It looks worse than before. > /// > /// Optional event handler for the bootloader to be informed of events occurring during FSP execution. > /// This value is only valid if Revision is >= 2. > /// > - EFI_PHYSICAL_ADDRESS FspEventHandler; > - UINT8 Reserved1[24]; > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + UINT8 Reserved1[24]; Any reason for these whitespace changes? It looks worse than before. > } FSPM_ARCH2_UPD; > > /// > @@ -266,18 +266,18 @@ typedef struct { > /// > /// Revision of the structure is 2 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 32. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// FspEventHandler Optional event handler for the bootloader to be informed of events > /// occurring during FSP execution. > /// > - EFI_PHYSICAL_ADDRESS FspEventHandler; > - UINT8 Reserved1[16]; > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + UINT8 Reserved1[16]; Any reason for these whitespace changes? It looks worse than before. > } FSPS_ARCH2_UPD; > > /// > @@ -609,4 +609,23 @@ EFI_STATUS > IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr > ); > > +/** > + This FSP API initializes SMM and provide any OS runtime silicon services, > + including Reliability, Availability, and Serviceability (RAS) features implemented by the CPU. > + > + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. > + If NULL, FSP will use the default parameters. > + > + @retval EFI_SUCCESS FSP execution environment was initialized successfully. > + @retval EFI_INVALID_PARAMETER Input parameters are invalid. > + @retval EFI_UNSUPPORTED The FSP calling conditions were not met. > + @retval EFI_DEVICE_ERROR FSP initialization failed. > + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status codes will not be returned during S3. > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *FSP_SMM_INIT)( > + IN VOID *FspiUpdDataPtr > + ); > + > #endif > diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h b/IntelFsp2Pkg/Include/FspGlobalData.h > index 445540abfa..697b20ed4c 100644 > --- a/IntelFsp2Pkg/Include/FspGlobalData.h > +++ b/IntelFsp2Pkg/Include/FspGlobalData.h > @@ -10,9 +10,9 @@ > > #include <FspEas.h> > > -#define FSP_IN_API_MODE 0 > -#define FSP_IN_DISPATCH_MODE 1 > -#define FSP_GLOBAL_DATA_VERSION 1 > +#define FSP_IN_API_MODE 0 > +#define FSP_IN_DISPATCH_MODE 1 > +#define FSP_GLOBAL_DATA_VERSION 1 > > #pragma pack(1) > > @@ -24,16 +24,17 @@ typedef enum { > TempRamExitApiIndex, > FspSiliconInitApiIndex, > FspMultiPhaseSiInitApiIndex, > + FspSmmInitApiIndex, > FspApiIndexMax > } FSP_API_INDEX; > > typedef struct { > - VOID *DataPtr; > - UINTN MicrocodeRegionBase; > - UINTN MicrocodeRegionSize; > - UINTN CodeRegionBase; > - UINTN CodeRegionSize; > - UINTN Reserved; > + VOID *DataPtr; > + UINTN MicrocodeRegionBase; > + UINTN MicrocodeRegionSize; > + UINTN CodeRegionBase; > + UINTN CodeRegionSize; > + UINTN Reserved; > } FSP_PLAT_DATA; > > #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'D') > @@ -41,28 +42,28 @@ typedef struct { > #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF > > typedef struct { > - UINT32 Signature; > - UINT8 Version; > - UINT8 Reserved1[3]; > + UINT32 Signature; > + UINT8 Version; > + UINT8 Reserved1[3]; > /// > /// Offset 0x08 > /// > - UINTN CoreStack; > - UINTN Reserved2; > + UINTN CoreStack; > + UINTN Reserved2; > /// > /// IA32: Offset 0x10; X64: Offset 0x18 > /// > - UINT32 StatusCode; > - UINT8 ApiIdx; > + UINT32 StatusCode; > + UINT8 ApiIdx; > /// > /// 0: FSP in API mode; 1: FSP in DISPATCH mode > /// > - UINT8 FspMode; > - UINT8 OnSeparateStack; > - UINT8 Reserved3; > - UINT32 NumberOfPhases; > - UINT32 PhasesExecuted; > - UINT32 Reserved4[8]; > + UINT8 FspMode; > + UINT8 OnSeparateStack; > + UINT8 Reserved3; > + UINT32 NumberOfPhases; > + UINT32 PhasesExecuted; > + UINT32 Reserved4[8]; > /// > /// IA32: Offset 0x40; X64: Offset 0x48 > /// Start of UINTN and pointer section > @@ -75,21 +76,23 @@ typedef struct { > VOID *TempRamInitUpdPtr; > VOID *MemoryInitUpdPtr; > VOID *SiliconInitUpdPtr; > + VOID *SmmInitUpdPtr; > /// > - /// IA32: Offset 0x64; X64: Offset 0x90 > + /// IA32: Offset 0x68; X64: Offset 0x98 > /// To store function parameters pointer > /// so it can be retrieved after stack switched. > /// > VOID *FunctionParameterPtr; > FSP_INFO_HEADER *FspInfoHeader; > VOID *UpdDataPtr; > + UINTN Reserved5; > /// > /// End of UINTN and pointer section > /// > - UINT8 Reserved5[16]; > + UINT8 Reserved6[16]; > UINT32 PerfSig; > UINT16 PerfLen; > - UINT16 Reserved6; > + UINT16 Reserved7; > UINT32 PerfIdx; > UINT64 PerfData[32]; > } FSP_GLOBAL_DATA; > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > index c660defac3..c7fb63168f 100644 > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > @@ -26,13 +26,13 @@ > > #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') > > -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > -#define FSP_IA32 0 > -#define FSP_X64 1 > +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > +#define FSP_IA32 0 > +#define FSP_X64 1 > > -#pragma pack(1) > + #pragma pack(1) Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style guidelines don't require that and it certainly looks worse than before. > > /// > /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. > @@ -159,6 +159,14 @@ typedef struct { > /// Byte 0x4E: Reserved4. > /// > UINT16 Reserved4; > + /// > + /// Byte 0x50: Offset for the API for the Multi-Phase memory initialization. > + /// > + UINT32 FspMultiPhaseMemInitEntryOffset; > + /// > + /// Byte 0x54: Offset for the API to initialize SMM. > + /// > + UINT32 FspSmmInitEntryOffset; > } FSP_INFO_HEADER; > > /// > @@ -240,7 +248,7 @@ typedef struct { > // UINT32 PatchData[]; > } FSP_PATCH_TABLE; > > -#pragma pack() > + #pragma pack() Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style guidelines don't require that and it certainly looks worse than before. > > extern EFI_GUID gFspHeaderFileGuid; > > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > index 7cf7e88245..b2d7867880 100644 > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > @@ -68,6 +68,7 @@ > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf > + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf > IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf > > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py > index c4fb1f1bb2..128b896592 100644 > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > @@ -953,8 +953,8 @@ EndList > return NoFileChange > > def CreateSplitUpdTxt (self, UpdTxtFile): > - GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID'] > - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, and _S signature for FSPT, FSPM, FSPS > + GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID','FSP_I_UPD_TOOL_GUID'] > + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] # _T, _M, _S and _I signature for FSPT, FSPM, FSPS, FSPI > for Index in range(len(GuidList)): > UpdTxtFile = '' > FvDir = self._FvDir > @@ -1288,19 +1288,21 @@ EndList > Chars.append(chr(Value & 0xFF)) > Value = Value >> 8 > SignatureStr = ''.join(Chars) > - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS accordingly > + # Signature will be _T / _M / _S / _I for FSPT / FSPM / FSPS /FSPI accordingly > if '_T' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPT_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > elif '_M' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPM_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > elif '_S' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPS_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > + elif '_I' in SignatureStr[6:6+2]: > + TxtBody.append("#define FSPI_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > TxtBody.append("\n") > > for Region in ['UPD']: > UpdOffsetTable = [] > - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', '_M', '_S'] signature for FSPT, FSPM, FSPS > - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] > + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] #['_T', '_M', '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI > + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', 'FSPI_UPD'] > for Item in self._CfgItemList: > if Item["cname"] == 'Signature' and Item["value"][0:6] in UpdSignature: > Item["offset"] = 0 # re-initialize offset to 0 when new UPD structure starting > @@ -1393,11 +1395,12 @@ EndList > HeaderTFileName = 'FsptUpd.h' > HeaderMFileName = 'FspmUpd.h' > HeaderSFileName = 'FspsUpd.h' > + HeaderIFileName = 'FspiUpd.h' > > - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE'] > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD'] > + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # FSPX_UPD_REGION > + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] > + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] > ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', 'FSPS_ARCH2_UPD'] > > IncLines = [] > @@ -1420,6 +1423,9 @@ EndList > elif UpdRegionCheck[item] == 'FSPS': > HeaderFd = open(os.path.join(FvDir, HeaderSFileName), "w") > FileBase = os.path.basename(os.path.join(FvDir, HeaderSFileName)) > + elif UpdRegionCheck[item] == 'FSPI': > + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), "w") > + FileBase = os.path.basename(os.path.join(FvDir, HeaderIFileName)) > FileName = FileBase.replace(".", "_").upper() > HeaderFd.write("%s\n" % (__copyright_h__ % date.today().year)) > HeaderFd.write("#ifndef __%s__\n" % FileName) > @@ -1696,7 +1702,7 @@ EndList > > > def Usage(): > - print ("GenCfgOpt Version 0.57") > + print ("GenCfgOpt Version 0.58") > print ("Usage:") > print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir [-D Macros]") > print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir InputHFile [-D Macros]") > diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py b/IntelFsp2Pkg/Tools/SplitFspBin.py > index f9151b5afd..317d9c1fa0 100644 > --- a/IntelFsp2Pkg/Tools/SplitFspBin.py > +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py > @@ -1,6 +1,6 @@ > ## @ SplitFspBin.py > # > -# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2015 - 2022, Intel Corporation. All rights reserved.<BR> > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > @@ -492,7 +492,7 @@ class FspImage: > self.FihOffset = fihoff > self.Offset = offset > self.FvIdxList = [] > - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] > + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] > self.PatchList = patch > self.PatchList.append(fihoff + 0x1C) > > @@ -869,7 +869,7 @@ def main (): > parser_rebase = subparsers.add_parser('rebase', help='rebase a FSP into a new base address') > parser_rebase.set_defaults(which='rebase') > parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', type=str, help='FSP binary file path', required = True) > - parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) > + parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) > parser_rebase.add_argument('-b', '--newbase', dest='FspBase', nargs='+', type=str, help='Rebased FSP binary file name', default = '', required = True) > parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', type=str, help='Output directory path', default = '.') > parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', type=str, help='Rebased FSP binary file name', default = '') > -- > 2.35.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. 2022-07-18 21:40 ` Nate DeSimone @ 2022-07-18 21:49 ` Pedro Falcato 2022-07-18 23:40 ` Nate DeSimone 2022-07-18 22:02 ` Chiu, Chasel 1 sibling, 1 reply; 6+ messages in thread From: Pedro Falcato @ 2022-07-18 21:49 UTC (permalink / raw) To: edk2-devel-groups-io, Nate DeSimone Cc: Chiu, Chasel, Zhang, Hongbin1, Zeng, Star [-- Attachment #1: Type: text/plain, Size: 27892 bytes --] On Mon, Jul 18, 2022 at 10:40 PM Nate DeSimone < nathaniel.l.desimone@intel.com> wrote: > Hi Chasel, > > Please see comments inline. Here is a summary of my feedback: > > #1) IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm - line 34 - Bug: eax > should be rax > #2) IntelFsp2Pkg/Include/FspEas/FspApi.h - Various unnecessary whitespace > changes that make the file look worse than before. > #3) IntelFsp2Pkg/Include/Guid/FspHeaderFile.h - Why indent the #pragma > lines? > > Thanks, > Nate > > > -----Original Message----- > > From: Chiu, Chasel <chasel.chiu@intel.com> > > Sent: Thursday, July 14, 2022 1:04 PM > > To: devel@edk2.groups.io > > Cc: Zhang, Hongbin1 <hongbin1.zhang@intel.com>; Desimone, Nathaniel L > > <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; > Chiu, > > Chasel <chasel.chiu@intel.com> > > Subject: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > > > From: Hongbin1 Zhang <hongbin1.zhang@intel.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 > > Add FSP-I API entry point for SMM support. > > > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> > > Signed-off-by: Hongbin1 Zhang <hongbin1.zhang@intel.com> > > --- > > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ > > IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 > ++++++++++++++++++++++++++++++++++++++------------------- > > IntelFsp2Pkg/Include/FspGlobalData.h | 53 > ++++++++++++++++++++++++++++------------------------- > > IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 > +++++++++++++++------- > > IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + > > IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 > ++++++++++++++++---------- > > IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- > > 10 files changed, 256 insertions(+), 64 deletions(-) > > > > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > index e22a88cc84..35d223a404 100644 > > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > @@ -71,6 +71,19 @@ FspApiCallingCheck ( > > Status = EFI_INVALID_PARAMETER; > > } > > } > > + } else if (ApiIdx == FspSmmInitApiIndex) { > > + // > > + // FspSmmInitApiIndex check > > + // > > + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || > ((UINTN)FspData == MAX_UINT32)) { > > + Status = EFI_UNSUPPORTED; > > + } else { > > + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { > > + Status = EFI_UNSUPPORTED; > > + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, > ApiParam))) { > > + Status = EFI_INVALID_PARAMETER; > > + } > > + } > > } else { > > Status = EFI_UNSUPPORTED; > > } > > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > new file mode 100644 > > index 0000000000..d31576c00b > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > @@ -0,0 +1,54 @@ > > +## @file > > +# Sec Core for FSP > > +# > > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > +## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = FspSecCoreI > > + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 > > + MODULE_TYPE = SEC > > + VERSION_STRING = 1.0 > > + > > +# > > +# The following information is for reference only and not required by > the build tools. > > +# > > +# VALID_ARCHITECTURES = IA32 X64 > > +# > > + > > +[Sources] > > + SecFspApiChk.c > > + SecFsp.h > > + > > +[Sources.X64] > > + X64/FspApiEntryI.nasm > > + X64/FspApiEntryCommon.nasm > > + X64/FspHelper.nasm > > + > > +[Sources.IA32] > > + Ia32/FspApiEntryI.nasm > > + Ia32/FspApiEntryCommon.nasm > > + Ia32/FspHelper.nasm > > + > > +[Binaries.Ia32] > > + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + IntelFsp2Pkg/IntelFsp2Pkg.dec > > + > > +[LibraryClasses] > > + BaseMemoryLib > > + DebugLib > > + BaseLib > > + PciCf8Lib > > + SerialPortLib > > + FspSwitchStackLib > > + FspCommonLib > > + FspSecPlatformLib > > + > > + > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +; SPDX-License-Identifier: BSD-2-Clause-Patent > > +;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C > > +; > > +extern ASM_PFX(FspApiCommon) > > + > > > +;---------------------------------------------------------------------------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > > +;---------------------------------------------------------------------------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the > boot > > +; process > > +; > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > + jmp ASM_PFX(FspApiCommon) > > + > > > +;---------------------------------------------------------------------------- > > +; Module Entrypoint API > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +; SPDX-License-Identifier: BSD-2-Clause-Patent > > +;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C > > +; > > +extern ASM_PFX(FspApiCommon) > > + > > > +;---------------------------------------------------------------------------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > > +;---------------------------------------------------------------------------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the > boot > > +; process > > +; > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > This is a bug. It should be: > > mov rax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > Hi Nate, This is not actually a problem, a 32-bit mov to a 32-bit register in long mode will zero the upper part. > > Note that IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm compares > ApiIndex numbers using rax, so it is important to make sure that the upper > 32-bits are zero: > > > https://github.com/tianocore/edk2/blob/c966204049f3d5dae6d5e587ddc298c684142c5c/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm#L65 > > > + jmp ASM_PFX(FspApiCommon) > > + > > > +;---------------------------------------------------------------------------- > > +; Module Entrypoint API > > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > index b36bc2b9ae..1d6c2fb63d 100644 > > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > @@ -135,18 +135,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the > specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this > field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspDebugHandler Optional debug handler for the bootloader to > receive debug messages > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspDebugHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPT_ARCH2_UPD; > > > > /// > > @@ -197,37 +197,37 @@ typedef struct { > > /// > > /// Revision of the structure is 3 for this version of the > specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this > field is 64. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Pointer to the temporary stack base address to be > > /// consumed inside FspMemoryInit() API. > > /// > > - EFI_PHYSICAL_ADDRESS StackBase; > > + EFI_PHYSICAL_ADDRESS StackBase; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Temporary stack size to be consumed inside > > /// FspMemoryInit() API. > > /// > > - UINT64 StackSize; > > + UINT64 StackSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Size of memory to be reserved by FSP below "top > > /// of low usable memory" for bootloader usage. > > /// > > - UINT32 BootLoaderTolumSize; > > + UINT32 BootLoaderTolumSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Current boot mode. > > /// > > - UINT32 BootMode; > > + UINT32 BootMode; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Optional event handler for the bootloader to be informed of > events occurring during FSP execution. > > /// This value is only valid if Revision is >= 2. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[24]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[24]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPM_ARCH2_UPD; > > > > /// > > @@ -266,18 +266,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the > specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this > field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspEventHandler Optional event handler for the bootloader to be > informed of events > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPS_ARCH2_UPD; > > > > /// > > @@ -609,4 +609,23 @@ EFI_STATUS > > IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr > > ); > > > > +/** > > + This FSP API initializes SMM and provide any OS runtime silicon > services, > > + including Reliability, Availability, and Serviceability (RAS) > features implemented by the CPU. > > + > > + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. > > + If NULL, FSP will use the default > parameters. > > + > > + @retval EFI_SUCCESS FSP execution environment was > initialized successfully. > > + @retval EFI_INVALID_PARAMETER Input parameters are invalid. > > + @retval EFI_UNSUPPORTED The FSP calling conditions were > not met. > > + @retval EFI_DEVICE_ERROR FSP initialization failed. > > + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status > codes will not be returned during S3. > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI *FSP_SMM_INIT)( > > + IN VOID *FspiUpdDataPtr > > + ); > > + > > #endif > > diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h > b/IntelFsp2Pkg/Include/FspGlobalData.h > > index 445540abfa..697b20ed4c 100644 > > --- a/IntelFsp2Pkg/Include/FspGlobalData.h > > +++ b/IntelFsp2Pkg/Include/FspGlobalData.h > > @@ -10,9 +10,9 @@ > > > > #include <FspEas.h> > > > > -#define FSP_IN_API_MODE 0 > > -#define FSP_IN_DISPATCH_MODE 1 > > -#define FSP_GLOBAL_DATA_VERSION 1 > > +#define FSP_IN_API_MODE 0 > > +#define FSP_IN_DISPATCH_MODE 1 > > +#define FSP_GLOBAL_DATA_VERSION 1 > > > > #pragma pack(1) > > > > @@ -24,16 +24,17 @@ typedef enum { > > TempRamExitApiIndex, > > FspSiliconInitApiIndex, > > FspMultiPhaseSiInitApiIndex, > > + FspSmmInitApiIndex, > > FspApiIndexMax > > } FSP_API_INDEX; > > > > typedef struct { > > - VOID *DataPtr; > > - UINTN MicrocodeRegionBase; > > - UINTN MicrocodeRegionSize; > > - UINTN CodeRegionBase; > > - UINTN CodeRegionSize; > > - UINTN Reserved; > > + VOID *DataPtr; > > + UINTN MicrocodeRegionBase; > > + UINTN MicrocodeRegionSize; > > + UINTN CodeRegionBase; > > + UINTN CodeRegionSize; > > + UINTN Reserved; > > } FSP_PLAT_DATA; > > > > #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', > 'D') > > @@ -41,28 +42,28 @@ typedef struct { > > #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF > > > > typedef struct { > > - UINT32 Signature; > > - UINT8 Version; > > - UINT8 Reserved1[3]; > > + UINT32 Signature; > > + UINT8 Version; > > + UINT8 Reserved1[3]; > > /// > > /// Offset 0x08 > > /// > > - UINTN CoreStack; > > - UINTN Reserved2; > > + UINTN CoreStack; > > + UINTN Reserved2; > > /// > > /// IA32: Offset 0x10; X64: Offset 0x18 > > /// > > - UINT32 StatusCode; > > - UINT8 ApiIdx; > > + UINT32 StatusCode; > > + UINT8 ApiIdx; > > /// > > /// 0: FSP in API mode; 1: FSP in DISPATCH mode > > /// > > - UINT8 FspMode; > > - UINT8 OnSeparateStack; > > - UINT8 Reserved3; > > - UINT32 NumberOfPhases; > > - UINT32 PhasesExecuted; > > - UINT32 Reserved4[8]; > > + UINT8 FspMode; > > + UINT8 OnSeparateStack; > > + UINT8 Reserved3; > > + UINT32 NumberOfPhases; > > + UINT32 PhasesExecuted; > > + UINT32 Reserved4[8]; > > /// > > /// IA32: Offset 0x40; X64: Offset 0x48 > > /// Start of UINTN and pointer section > > @@ -75,21 +76,23 @@ typedef struct { > > VOID *TempRamInitUpdPtr; > > VOID *MemoryInitUpdPtr; > > VOID *SiliconInitUpdPtr; > > + VOID *SmmInitUpdPtr; > > /// > > - /// IA32: Offset 0x64; X64: Offset 0x90 > > + /// IA32: Offset 0x68; X64: Offset 0x98 > > /// To store function parameters pointer > > /// so it can be retrieved after stack switched. > > /// > > VOID *FunctionParameterPtr; > > FSP_INFO_HEADER *FspInfoHeader; > > VOID *UpdDataPtr; > > + UINTN Reserved5; > > /// > > /// End of UINTN and pointer section > > /// > > - UINT8 Reserved5[16]; > > + UINT8 Reserved6[16]; > > UINT32 PerfSig; > > UINT16 PerfLen; > > - UINT16 Reserved6; > > + UINT16 Reserved7; > > UINT32 PerfIdx; > > UINT64 PerfData[32]; > > } FSP_GLOBAL_DATA; > > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > index c660defac3..c7fb63168f 100644 > > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > @@ -26,13 +26,13 @@ > > > > #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') > > > > -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > -#define FSP_IA32 0 > > -#define FSP_X64 1 > > +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > +#define FSP_IA32 0 > > +#define FSP_X64 1 > > > > -#pragma pack(1) > > + #pragma pack(1) > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding > style guidelines don't require that and it certainly looks worse than > before. > > > > > /// > > /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. > > @@ -159,6 +159,14 @@ typedef struct { > > /// Byte 0x4E: Reserved4. > > /// > > UINT16 Reserved4; > > + /// > > + /// Byte 0x50: Offset for the API for the Multi-Phase memory > initialization. > > + /// > > + UINT32 FspMultiPhaseMemInitEntryOffset; > > + /// > > + /// Byte 0x54: Offset for the API to initialize SMM. > > + /// > > + UINT32 FspSmmInitEntryOffset; > > } FSP_INFO_HEADER; > > > > /// > > @@ -240,7 +248,7 @@ typedef struct { > > // UINT32 PatchData[]; > > } FSP_PATCH_TABLE; > > > > -#pragma pack() > > + #pragma pack() > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding > style guidelines don't require that and it certainly looks worse than > before. > > > > > extern EFI_GUID gFspHeaderFileGuid; > > > > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > index 7cf7e88245..b2d7867880 100644 > > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > @@ -68,6 +68,7 @@ > > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf > > + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf > > IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf > > > > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py > b/IntelFsp2Pkg/Tools/GenCfgOpt.py > > index c4fb1f1bb2..128b896592 100644 > > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > > @@ -953,8 +953,8 @@ EndList > > return NoFileChange > > > > def CreateSplitUpdTxt (self, UpdTxtFile): > > - GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID'] > > - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, > and _S signature for FSPT, FSPM, FSPS > > + GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID','FSP_I_UPD_TOOL_GUID'] > > + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] > # _T, _M, _S and _I signature for FSPT, FSPM, FSPS, FSPI > > for Index in range(len(GuidList)): > > UpdTxtFile = '' > > FvDir = self._FvDir > > @@ -1288,19 +1288,21 @@ EndList > > Chars.append(chr(Value & 0xFF)) > > Value = Value >> 8 > > SignatureStr = ''.join(Chars) > > - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS > accordingly > > + # Signature will be _T / _M / _S / _I for FSPT / FSPM / > FSPS /FSPI accordingly > > if '_T' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPT_UPD_SIGNATURE > %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_M' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPM_UPD_SIGNATURE > %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_S' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPS_UPD_SIGNATURE > %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > > + elif '_I' in SignatureStr[6:6+2]: > > + TxtBody.append("#define FSPI_UPD_SIGNATURE > %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > > TxtBody.append("\n") > > > > for Region in ['UPD']: > > UpdOffsetTable = [] > > - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', > '_M', '_S'] signature for FSPT, FSPM, FSPS > > - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] > > + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] > #['_T', '_M', '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI > > + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', > 'FSPI_UPD'] > > for Item in self._CfgItemList: > > if Item["cname"] == 'Signature' and Item["value"][0:6] > in UpdSignature: > > Item["offset"] = 0 # re-initialize offset to 0 when > new UPD structure starting > > @@ -1393,11 +1395,12 @@ EndList > > HeaderTFileName = 'FsptUpd.h' > > HeaderMFileName = 'FspmUpd.h' > > HeaderSFileName = 'FspsUpd.h' > > + HeaderIFileName = 'FspiUpd.h' > > > > - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > > - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', > 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE'] > > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD'] > > + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # > FSPX_UPD_REGION > > + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # > FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', > 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] > > + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] > > ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', > 'FSPS_ARCH2_UPD'] > > > > IncLines = [] > > @@ -1420,6 +1423,9 @@ EndList > > elif UpdRegionCheck[item] == 'FSPS': > > HeaderFd = open(os.path.join(FvDir, HeaderSFileName), > "w") > > FileBase = os.path.basename(os.path.join(FvDir, > HeaderSFileName)) > > + elif UpdRegionCheck[item] == 'FSPI': > > + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), > "w") > > + FileBase = os.path.basename(os.path.join(FvDir, > HeaderIFileName)) > > FileName = FileBase.replace(".", "_").upper() > > HeaderFd.write("%s\n" % (__copyright_h__ % > date.today().year)) > > HeaderFd.write("#ifndef __%s__\n" % FileName) > > @@ -1696,7 +1702,7 @@ EndList > > > > > > def Usage(): > > - print ("GenCfgOpt Version 0.57") > > + print ("GenCfgOpt Version 0.58") > > print ("Usage:") > > print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir > [-D Macros]") > > print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir > InputHFile [-D Macros]") > > diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py > b/IntelFsp2Pkg/Tools/SplitFspBin.py > > index f9151b5afd..317d9c1fa0 100644 > > --- a/IntelFsp2Pkg/Tools/SplitFspBin.py > > +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py > > @@ -1,6 +1,6 @@ > > ## @ SplitFspBin.py > > # > > -# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR> > > +# Copyright (c) 2015 - 2022, Intel Corporation. All rights reserved.<BR> > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > ## > > @@ -492,7 +492,7 @@ class FspImage: > > self.FihOffset = fihoff > > self.Offset = offset > > self.FvIdxList = [] > > - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> > 12) & 0x0F] > > + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> > 12) & 0x0F] > > self.PatchList = patch > > self.PatchList.append(fihoff + 0x1C) > > > > @@ -869,7 +869,7 @@ def main (): > > parser_rebase = subparsers.add_parser('rebase', help='rebase a > FSP into a new base address') > > parser_rebase.set_defaults(which='rebase') > > parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', > type=str, help='FSP binary file path', required = True) > > - parser_rebase.add_argument('-c', '--fspcomp', > choices=['t','m','s','o'], nargs='+', dest='FspComponent', type=str, > help='FSP component to rebase', default = "['t']", required = True) > > + parser_rebase.add_argument('-c', '--fspcomp', > choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', type=str, > help='FSP component to rebase', default = "['t']", required = True) > > parser_rebase.add_argument('-b', '--newbase', dest='FspBase', > nargs='+', type=str, help='Rebased FSP binary file name', default = '', > required = True) > > parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', > type=str, help='Output directory path', default = '.') > > parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', > type=str, help='Rebased FSP binary file name', default = '') > > -- > > 2.35.0.windows.1 > > > > > > > -- Pedro Falcato [-- Attachment #2: Type: text/html, Size: 36640 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. 2022-07-18 21:49 ` [edk2-devel] " Pedro Falcato @ 2022-07-18 23:40 ` Nate DeSimone 2022-07-19 17:13 ` Chiu, Chasel 0 siblings, 1 reply; 6+ messages in thread From: Nate DeSimone @ 2022-07-18 23:40 UTC (permalink / raw) To: Pedro Falcato, edk2-devel-groups-io Cc: Chiu, Chasel, Zhang, Hongbin1, Zeng, Star Hi Pedro, Wow you are totally right... that is crazy! The exact opposite of the way 32-bit mode works! Good catch thank you! Please consider by code review feedback downgraded from a bug to a recommendation to use rax instead of eax to avoid confusion 😊. Thanks, Nate From: Pedro Falcato <pedro.falcato@gmail.com> Sent: Monday, July 18, 2022 2:50 PM To: edk2-devel-groups-io <devel@edk2.groups.io>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zhang, Hongbin1 <hongbin1.zhang@intel.com>; Zeng, Star <star.zeng@intel.com> Subject: Re: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. On Mon, Jul 18, 2022 at 10:40 PM Nate DeSimone <mailto:nathaniel.l.desimone@intel.com> wrote: Hi Chasel, Please see comments inline. Here is a summary of my feedback: #1) IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm - line 34 - Bug: eax should be rax #2) IntelFsp2Pkg/Include/FspEas/FspApi.h - Various unnecessary whitespace changes that make the file look worse than before. #3) IntelFsp2Pkg/Include/Guid/FspHeaderFile.h - Why indent the #pragma lines? Thanks, Nate > -----Original Message----- > From: Chiu, Chasel <mailto:chasel.chiu@intel.com> > Sent: Thursday, July 14, 2022 1:04 PM > To: mailto:devel@edk2.groups.io > Cc: Zhang, Hongbin1 <mailto:hongbin1.zhang@intel.com>; Desimone, Nathaniel L > <mailto:nathaniel.l.desimone@intel.com>; Zeng, Star <mailto:star.zeng@intel.com>; Chiu, > Chasel <mailto:chasel.chiu@intel.com> > Subject: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > From: Hongbin1 Zhang <mailto:hongbin1.zhang@intel.com> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 > Add FSP-I API entry point for SMM support. > > Cc: Nate DeSimone <mailto:nathaniel.l.desimone@intel.com> > Cc: Star Zeng <mailto:star.zeng@intel.com> > Signed-off-by: Chasel Chiu <mailto:chasel.chiu@intel.com> > Signed-off-by: Hongbin1 Zhang <mailto:hongbin1.zhang@intel.com> > --- > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ > IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 ++++++++++++++++++++++++++++++++++++++++++++ > IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 ++++++++++++++++++++++++++++++++++++++------------------- > IntelFsp2Pkg/Include/FspGlobalData.h | 53 ++++++++++++++++++++++++++++------------------------- > IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 +++++++++++++++------- > IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + > IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 ++++++++++++++++---------- > IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- > 10 files changed, 256 insertions(+), 64 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > index e22a88cc84..35d223a404 100644 > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > @@ -71,6 +71,19 @@ FspApiCallingCheck ( > Status = EFI_INVALID_PARAMETER; > } > } > + } else if (ApiIdx == FspSmmInitApiIndex) { > + // > + // FspSmmInitApiIndex check > + // > + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || ((UINTN)FspData == MAX_UINT32)) { > + Status = EFI_UNSUPPORTED; > + } else { > + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { > + Status = EFI_UNSUPPORTED; > + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, ApiParam))) { > + Status = EFI_INVALID_PARAMETER; > + } > + } > } else { > Status = EFI_UNSUPPORTED; > } > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > new file mode 100644 > index 0000000000..d31576c00b > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > @@ -0,0 +1,54 @@ > +## @file > +# Sec Core for FSP > +# > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = FspSecCoreI > + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 > + MODULE_TYPE = SEC > + VERSION_STRING = 1.0 > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Sources] > + SecFspApiChk.c > + SecFsp.h > + > +[Sources.X64] > + X64/FspApiEntryI.nasm > + X64/FspApiEntryCommon.nasm > + X64/FspHelper.nasm > + > +[Sources.IA32] > + Ia32/FspApiEntryI.nasm > + Ia32/FspApiEntryCommon.nasm > + Ia32/FspHelper.nasm > + > +[Binaries.Ia32] > + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC > + > +[Packages] > + MdePkg/MdePkg.dec > + IntelFsp2Pkg/IntelFsp2Pkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + BaseLib > + PciCf8Lib > + SerialPortLib > + FspSwitchStackLib > + FspCommonLib > + FspSecPlatformLib > + > + > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > new file mode 100644 > index 0000000000..e9365d6832 > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > @@ -0,0 +1,44 @@ > +;; @file > +; Provide FSP API entry points. > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +; SPDX-License-Identifier: BSD-2-Clause-Patent > +;; > + > + SECTION .text > + > +; > +; Following functions will be provided in C > +; > +extern ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; FspApiCommonContinue API > +; > +; This is the FSP API common entry point to resume the FSP execution > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspApiCommonContinue) > +ASM_PFX(FspApiCommonContinue): > + jmp $ > + > +;---------------------------------------------------------------------------- > +; FspSmmInit API > +; > +; This FSP API will notify the FSP about the different phases in the boot > +; process > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspSmmInitApi) > +ASM_PFX(FspSmmInitApi): > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > + jmp ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; Module Entrypoint API > +;---------------------------------------------------------------------------- > +global ASM_PFX(_ModuleEntryPoint) > +ASM_PFX(_ModuleEntryPoint): > + jmp $ > + ; Add reference to APIs so that it will not be optimized by compiler > + jmp ASM_PFX(FspSmmInitApi) > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > new file mode 100644 > index 0000000000..e9365d6832 > --- /dev/null > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > @@ -0,0 +1,44 @@ > +;; @file > +; Provide FSP API entry points. > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > +; SPDX-License-Identifier: BSD-2-Clause-Patent > +;; > + > + SECTION .text > + > +; > +; Following functions will be provided in C > +; > +extern ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; FspApiCommonContinue API > +; > +; This is the FSP API common entry point to resume the FSP execution > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspApiCommonContinue) > +ASM_PFX(FspApiCommonContinue): > + jmp $ > + > +;---------------------------------------------------------------------------- > +; FspSmmInit API > +; > +; This FSP API will notify the FSP about the different phases in the boot > +; process > +; > +;---------------------------------------------------------------------------- > +global ASM_PFX(FspSmmInitApi) > +ASM_PFX(FspSmmInitApi): > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex This is a bug. It should be: mov rax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex Hi Nate, This is not actually a problem, a 32-bit mov to a 32-bit register in long mode will zero the upper part. Note that IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm compares ApiIndex numbers using rax, so it is important to make sure that the upper 32-bits are zero: https://github.com/tianocore/edk2/blob/c966204049f3d5dae6d5e587ddc298c684142c5c/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm#L65 > + jmp ASM_PFX(FspApiCommon) > + > +;---------------------------------------------------------------------------- > +; Module Entrypoint API > +;---------------------------------------------------------------------------- > +global ASM_PFX(_ModuleEntryPoint) > +ASM_PFX(_ModuleEntryPoint): > + jmp $ > + ; Add reference to APIs so that it will not be optimized by compiler > + jmp ASM_PFX(FspSmmInitApi) > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index b36bc2b9ae..1d6c2fb63d 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -135,18 +135,18 @@ typedef struct { > /// > /// Revision of the structure is 2 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 32. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// FspDebugHandler Optional debug handler for the bootloader to receive debug messages > /// occurring during FSP execution. > /// > - EFI_PHYSICAL_ADDRESS FspDebugHandler; > - UINT8 Reserved1[16]; > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > + UINT8 Reserved1[16]; Any reason for these whitespace changes? It looks worse than before. > } FSPT_ARCH2_UPD; > > /// > @@ -197,37 +197,37 @@ typedef struct { > /// > /// Revision of the structure is 3 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 64. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// Pointer to the temporary stack base address to be > /// consumed inside FspMemoryInit() API. > /// > - EFI_PHYSICAL_ADDRESS StackBase; > + EFI_PHYSICAL_ADDRESS StackBase; Any reason for this whitespace change? It looks worse than before. > /// > /// Temporary stack size to be consumed inside > /// FspMemoryInit() API. > /// > - UINT64 StackSize; > + UINT64 StackSize; Any reason for this whitespace change? It looks worse than before. > /// > /// Size of memory to be reserved by FSP below "top > /// of low usable memory" for bootloader usage. > /// > - UINT32 BootLoaderTolumSize; > + UINT32 BootLoaderTolumSize; Any reason for this whitespace change? It looks worse than before. > /// > /// Current boot mode. > /// > - UINT32 BootMode; > + UINT32 BootMode; Any reason for this whitespace change? It looks worse than before. > /// > /// Optional event handler for the bootloader to be informed of events occurring during FSP execution. > /// This value is only valid if Revision is >= 2. > /// > - EFI_PHYSICAL_ADDRESS FspEventHandler; > - UINT8 Reserved1[24]; > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + UINT8 Reserved1[24]; Any reason for these whitespace changes? It looks worse than before. > } FSPM_ARCH2_UPD; > > /// > @@ -266,18 +266,18 @@ typedef struct { > /// > /// Revision of the structure is 2 for this version of the specification. > /// > - UINT8 Revision; > - UINT8 Reserved[3]; > + UINT8 Revision; > + UINT8 Reserved[3]; Any reason for these whitespace changes? It looks worse than before. > /// > /// Length of the structure in bytes. The current value for this field is 32. > /// > - UINT32 Length; > + UINT32 Length; Any reason for this whitespace change? It looks worse than before. > /// > /// FspEventHandler Optional event handler for the bootloader to be informed of events > /// occurring during FSP execution. > /// > - EFI_PHYSICAL_ADDRESS FspEventHandler; > - UINT8 Reserved1[16]; > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + UINT8 Reserved1[16]; Any reason for these whitespace changes? It looks worse than before. > } FSPS_ARCH2_UPD; > > /// > @@ -609,4 +609,23 @@ EFI_STATUS > IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr > ); > > +/** > + This FSP API initializes SMM and provide any OS runtime silicon services, > + including Reliability, Availability, and Serviceability (RAS) features implemented by the CPU. > + > + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. > + If NULL, FSP will use the default parameters. > + > + @retval EFI_SUCCESS FSP execution environment was initialized successfully. > + @retval EFI_INVALID_PARAMETER Input parameters are invalid. > + @retval EFI_UNSUPPORTED The FSP calling conditions were not met. > + @retval EFI_DEVICE_ERROR FSP initialization failed. > + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status codes will not be returned during S3. > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *FSP_SMM_INIT)( > + IN VOID *FspiUpdDataPtr > + ); > + > #endif > diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h b/IntelFsp2Pkg/Include/FspGlobalData.h > index 445540abfa..697b20ed4c 100644 > --- a/IntelFsp2Pkg/Include/FspGlobalData.h > +++ b/IntelFsp2Pkg/Include/FspGlobalData.h > @@ -10,9 +10,9 @@ > > #include <FspEas.h> > > -#define FSP_IN_API_MODE 0 > -#define FSP_IN_DISPATCH_MODE 1 > -#define FSP_GLOBAL_DATA_VERSION 1 > +#define FSP_IN_API_MODE 0 > +#define FSP_IN_DISPATCH_MODE 1 > +#define FSP_GLOBAL_DATA_VERSION 1 > > #pragma pack(1) > > @@ -24,16 +24,17 @@ typedef enum { > TempRamExitApiIndex, > FspSiliconInitApiIndex, > FspMultiPhaseSiInitApiIndex, > + FspSmmInitApiIndex, > FspApiIndexMax > } FSP_API_INDEX; > > typedef struct { > - VOID *DataPtr; > - UINTN MicrocodeRegionBase; > - UINTN MicrocodeRegionSize; > - UINTN CodeRegionBase; > - UINTN CodeRegionSize; > - UINTN Reserved; > + VOID *DataPtr; > + UINTN MicrocodeRegionBase; > + UINTN MicrocodeRegionSize; > + UINTN CodeRegionBase; > + UINTN CodeRegionSize; > + UINTN Reserved; > } FSP_PLAT_DATA; > > #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'D') > @@ -41,28 +42,28 @@ typedef struct { > #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF > > typedef struct { > - UINT32 Signature; > - UINT8 Version; > - UINT8 Reserved1[3]; > + UINT32 Signature; > + UINT8 Version; > + UINT8 Reserved1[3]; > /// > /// Offset 0x08 > /// > - UINTN CoreStack; > - UINTN Reserved2; > + UINTN CoreStack; > + UINTN Reserved2; > /// > /// IA32: Offset 0x10; X64: Offset 0x18 > /// > - UINT32 StatusCode; > - UINT8 ApiIdx; > + UINT32 StatusCode; > + UINT8 ApiIdx; > /// > /// 0: FSP in API mode; 1: FSP in DISPATCH mode > /// > - UINT8 FspMode; > - UINT8 OnSeparateStack; > - UINT8 Reserved3; > - UINT32 NumberOfPhases; > - UINT32 PhasesExecuted; > - UINT32 Reserved4[8]; > + UINT8 FspMode; > + UINT8 OnSeparateStack; > + UINT8 Reserved3; > + UINT32 NumberOfPhases; > + UINT32 PhasesExecuted; > + UINT32 Reserved4[8]; > /// > /// IA32: Offset 0x40; X64: Offset 0x48 > /// Start of UINTN and pointer section > @@ -75,21 +76,23 @@ typedef struct { > VOID *TempRamInitUpdPtr; > VOID *MemoryInitUpdPtr; > VOID *SiliconInitUpdPtr; > + VOID *SmmInitUpdPtr; > /// > - /// IA32: Offset 0x64; X64: Offset 0x90 > + /// IA32: Offset 0x68; X64: Offset 0x98 > /// To store function parameters pointer > /// so it can be retrieved after stack switched. > /// > VOID *FunctionParameterPtr; > FSP_INFO_HEADER *FspInfoHeader; > VOID *UpdDataPtr; > + UINTN Reserved5; > /// > /// End of UINTN and pointer section > /// > - UINT8 Reserved5[16]; > + UINT8 Reserved6[16]; > UINT32 PerfSig; > UINT16 PerfLen; > - UINT16 Reserved6; > + UINT16 Reserved7; > UINT32 PerfIdx; > UINT64 PerfData[32]; > } FSP_GLOBAL_DATA; > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > index c660defac3..c7fb63168f 100644 > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > @@ -26,13 +26,13 @@ > > #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') > > -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > -#define FSP_IA32 0 > -#define FSP_X64 1 > +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > +#define FSP_IA32 0 > +#define FSP_X64 1 > > -#pragma pack(1) > + #pragma pack(1) Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style guidelines don't require that and it certainly looks worse than before. > > /// > /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. > @@ -159,6 +159,14 @@ typedef struct { > /// Byte 0x4E: Reserved4. > /// > UINT16 Reserved4; > + /// > + /// Byte 0x50: Offset for the API for the Multi-Phase memory initialization. > + /// > + UINT32 FspMultiPhaseMemInitEntryOffset; > + /// > + /// Byte 0x54: Offset for the API to initialize SMM. > + /// > + UINT32 FspSmmInitEntryOffset; > } FSP_INFO_HEADER; > > /// > @@ -240,7 +248,7 @@ typedef struct { > // UINT32 PatchData[]; > } FSP_PATCH_TABLE; > > -#pragma pack() > + #pragma pack() Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style guidelines don't require that and it certainly looks worse than before. > > extern EFI_GUID gFspHeaderFileGuid; > > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > index 7cf7e88245..b2d7867880 100644 > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > @@ -68,6 +68,7 @@ > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf > + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf > IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf > > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py b/IntelFsp2Pkg/Tools/GenCfgOpt.py > index c4fb1f1bb2..128b896592 100644 > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > @@ -953,8 +953,8 @@ EndList > return NoFileChange > > def CreateSplitUpdTxt (self, UpdTxtFile): > - GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID'] > - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, and _S signature for FSPT, FSPM, FSPS > + GuidList = ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID','FSP_I_UPD_TOOL_GUID'] > + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] # _T, _M, _S and _I signature for FSPT, FSPM, FSPS, FSPI > for Index in range(len(GuidList)): > UpdTxtFile = '' > FvDir = self._FvDir > @@ -1288,19 +1288,21 @@ EndList > Chars.append(chr(Value & 0xFF)) > Value = Value >> 8 > SignatureStr = ''.join(Chars) > - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS accordingly > + # Signature will be _T / _M / _S / _I for FSPT / FSPM / FSPS /FSPI accordingly > if '_T' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPT_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > elif '_M' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPM_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > elif '_S' in SignatureStr[6:6+2]: > TxtBody.append("#define FSPS_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > + elif '_I' in SignatureStr[6:6+2]: > + TxtBody.append("#define FSPI_UPD_SIGNATURE %s /* '%s' */\n\n" % (Item['value'], SignatureStr)) > TxtBody.append("\n") > > for Region in ['UPD']: > UpdOffsetTable = [] > - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', '_M', '_S'] signature for FSPT, FSPM, FSPS > - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] > + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] #['_T', '_M', '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI > + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', 'FSPI_UPD'] > for Item in self._CfgItemList: > if Item["cname"] == 'Signature' and Item["value"][0:6] in UpdSignature: > Item["offset"] = 0 # re-initialize offset to 0 when new UPD structure starting > @@ -1393,11 +1395,12 @@ EndList > HeaderTFileName = 'FsptUpd.h' > HeaderMFileName = 'FspmUpd.h' > HeaderSFileName = 'FspsUpd.h' > + HeaderIFileName = 'FspiUpd.h' > > - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE'] > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD'] > + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # FSPX_UPD_REGION > + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # FSP_X_CONFIG, FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] > + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] > ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', 'FSPS_ARCH2_UPD'] > > IncLines = [] > @@ -1420,6 +1423,9 @@ EndList > elif UpdRegionCheck[item] == 'FSPS': > HeaderFd = open(os.path.join(FvDir, HeaderSFileName), "w") > FileBase = os.path.basename(os.path.join(FvDir, HeaderSFileName)) > + elif UpdRegionCheck[item] == 'FSPI': > + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), "w") > + FileBase = os.path.basename(os.path.join(FvDir, HeaderIFileName)) > FileName = FileBase.replace(".", "_").upper() > HeaderFd.write("%s\n" % (__copyright_h__ % date.today().year)) > HeaderFd.write("#ifndef __%s__\n" % FileName) > @@ -1696,7 +1702,7 @@ EndList > > > def Usage(): > - print ("GenCfgOpt Version 0.57") > + print ("GenCfgOpt Version 0.58") > print ("Usage:") > print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir [-D Macros]") > print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir InputHFile [-D Macros]") > diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py b/IntelFsp2Pkg/Tools/SplitFspBin.py > index f9151b5afd..317d9c1fa0 100644 > --- a/IntelFsp2Pkg/Tools/SplitFspBin.py > +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py > @@ -1,6 +1,6 @@ > ## @ SplitFspBin.py > # > -# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2015 - 2022, Intel Corporation. All rights reserved.<BR> > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > @@ -492,7 +492,7 @@ class FspImage: > self.FihOffset = fihoff > self.Offset = offset > self.FvIdxList = [] > - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] > + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & 0x0F] > self.PatchList = patch > self.PatchList.append(fihoff + 0x1C) > > @@ -869,7 +869,7 @@ def main (): > parser_rebase = subparsers.add_parser('rebase', help='rebase a FSP into a new base address') > parser_rebase.set_defaults(which='rebase') > parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', type=str, help='FSP binary file path', required = True) > - parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) > + parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', default = "['t']", required = True) > parser_rebase.add_argument('-b', '--newbase', dest='FspBase', nargs='+', type=str, help='Rebased FSP binary file name', default = '', required = True) > parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', type=str, help='Output directory path', default = '.') > parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', type=str, help='Rebased FSP binary file name', default = '') > -- > 2.35.0.windows.1 -- Pedro Falcato ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. 2022-07-18 23:40 ` Nate DeSimone @ 2022-07-19 17:13 ` Chiu, Chasel 0 siblings, 0 replies; 6+ messages in thread From: Chiu, Chasel @ 2022-07-19 17:13 UTC (permalink / raw) To: Desimone, Nathaniel L, Pedro Falcato, edk2-devel-groups-io Cc: Zhang, Hongbin1, Zeng, Star Hi, I have sent V2 to assign ApiIdx to RAX in X64 case to avoid confusion. Please help to review again, Thanks, Chasel > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Monday, July 18, 2022 4:41 PM > To: Pedro Falcato <pedro.falcato@gmail.com>; edk2-devel-groups-io > <devel@edk2.groups.io> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zhang, Hongbin1 > <hongbin1.zhang@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: RE: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for > SMM support. > > Hi Pedro, > > Wow you are totally right... that is crazy! The exact opposite of the way 32-bit > mode works! > > Good catch thank you! > > Please consider by code review feedback downgraded from a bug to a > recommendation to use rax instead of eax to avoid confusion 😊. > > Thanks, > Nate > > From: Pedro Falcato <pedro.falcato@gmail.com> > Sent: Monday, July 18, 2022 2:50 PM > To: edk2-devel-groups-io <devel@edk2.groups.io>; Desimone, Nathaniel L > <nathaniel.l.desimone@intel.com> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Zhang, Hongbin1 > <hongbin1.zhang@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2-devel] [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for > SMM support. > > > > On Mon, Jul 18, 2022 at 10:40 PM Nate DeSimone > <mailto:nathaniel.l.desimone@intel.com> wrote: > Hi Chasel, > > Please see comments inline. Here is a summary of my feedback: > > #1) IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm - line 34 - Bug: eax should > be rax > #2) IntelFsp2Pkg/Include/FspEas/FspApi.h - Various unnecessary whitespace > changes that make the file look worse than before. > #3) IntelFsp2Pkg/Include/Guid/FspHeaderFile.h - Why indent the #pragma lines? > > Thanks, > Nate > > > -----Original Message----- > > From: Chiu, Chasel <mailto:chasel.chiu@intel.com> > > Sent: Thursday, July 14, 2022 1:04 PM > > To: mailto:devel@edk2.groups.io > > Cc: Zhang, Hongbin1 <mailto:hongbin1.zhang@intel.com>; Desimone, > Nathaniel L > > <mailto:nathaniel.l.desimone@intel.com>; Zeng, Star > <mailto:star.zeng@intel.com>; Chiu, > > Chasel <mailto:chasel.chiu@intel.com> > > Subject: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > > > From: Hongbin1 Zhang <mailto:hongbin1.zhang@intel.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 > > Add FSP-I API entry point for SMM support. > > > > Cc: Nate DeSimone <mailto:nathaniel.l.desimone@intel.com> > > Cc: Star Zeng <mailto:star.zeng@intel.com> > > Signed-off-by: Chasel Chiu <mailto:chasel.chiu@intel.com> > > Signed-off-by: Hongbin1 Zhang <mailto:hongbin1.zhang@intel.com> > > --- > > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ > > IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 > ++++++++++++++++++++++++++++++++++++++------------------- > > IntelFsp2Pkg/Include/FspGlobalData.h | 53 > ++++++++++++++++++++++++++++------------------------- > > IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 +++++++++++++++------- > > IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + > > IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 ++++++++++++++++---------- > > IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- > > 10 files changed, 256 insertions(+), 64 deletions(-) > > > > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > index e22a88cc84..35d223a404 100644 > > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > @@ -71,6 +71,19 @@ FspApiCallingCheck ( > > Status = EFI_INVALID_PARAMETER; > > } > > } > > + } else if (ApiIdx == FspSmmInitApiIndex) { > > + // > > + // FspSmmInitApiIndex check > > + // > > + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || > ((UINTN)FspData == MAX_UINT32)) { > > + Status = EFI_UNSUPPORTED; > > + } else { > > + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { > > + Status = EFI_UNSUPPORTED; > > + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, > ApiParam))) { > > + Status = EFI_INVALID_PARAMETER; > > + } > > + } > > } else { > > Status = EFI_UNSUPPORTED; > > } > > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > new file mode 100644 > > index 0000000000..d31576c00b > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > @@ -0,0 +1,54 @@ > > +## @file > > +# Sec Core for FSP > > +# > > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > +## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = FspSecCoreI > > + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 > > + MODULE_TYPE = SEC > > + VERSION_STRING = 1.0 > > + > > +# > > +# The following information is for reference only and not required by the > build tools. > > +# > > +# VALID_ARCHITECTURES = IA32 X64 > > +# > > + > > +[Sources] > > + SecFspApiChk.c > > + SecFsp.h > > + > > +[Sources.X64] > > + X64/FspApiEntryI.nasm > > + X64/FspApiEntryCommon.nasm > > + X64/FspHelper.nasm > > + > > +[Sources.IA32] > > + Ia32/FspApiEntryI.nasm > > + Ia32/FspApiEntryCommon.nasm > > + Ia32/FspHelper.nasm > > + > > +[Binaries.Ia32] > > + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + IntelFsp2Pkg/IntelFsp2Pkg.dec > > + > > +[LibraryClasses] > > + BaseMemoryLib > > + DebugLib > > + BaseLib > > + PciCf8Lib > > + SerialPortLib > > + FspSwitchStackLib > > + FspCommonLib > > + FspSecPlatformLib > > + > > + > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +; SPDX-License-Identifier: BSD-2-Clause-Patent > > +;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C > > +; > > +extern ASM_PFX(FspApiCommon) > > + > > +;---------------------------------------------------------------------------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > +;---------------------------------------------------------------------------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the boot > > +; process > > +; > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > + jmp ASM_PFX(FspApiCommon) > > + > > +;---------------------------------------------------------------------------- > > +; Module Entrypoint API > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > +; SPDX-License-Identifier: BSD-2-Clause-Patent > > +;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C > > +; > > +extern ASM_PFX(FspApiCommon) > > + > > +;---------------------------------------------------------------------------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > +;---------------------------------------------------------------------------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the boot > > +; process > > +; > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > This is a bug. It should be: > > mov rax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > Hi Nate, > > This is not actually a problem, a 32-bit mov to a 32-bit register in long mode will > zero the upper part. > > > Note that IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm compares > ApiIndex numbers using rax, so it is important to make sure that the upper 32- > bits are zero: > > https://github.com/tianocore/edk2/blob/c966204049f3d5dae6d5e587ddc298c6 > 84142c5c/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm#L65 > > > + jmp ASM_PFX(FspApiCommon) > > + > > +;---------------------------------------------------------------------------- > > +; Module Entrypoint API > > +;---------------------------------------------------------------------------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > index b36bc2b9ae..1d6c2fb63d 100644 > > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > @@ -135,18 +135,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspDebugHandler Optional debug handler for the bootloader to receive > debug messages > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspDebugHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPT_ARCH2_UPD; > > > > /// > > @@ -197,37 +197,37 @@ typedef struct { > > /// > > /// Revision of the structure is 3 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 64. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Pointer to the temporary stack base address to be > > /// consumed inside FspMemoryInit() API. > > /// > > - EFI_PHYSICAL_ADDRESS StackBase; > > + EFI_PHYSICAL_ADDRESS StackBase; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Temporary stack size to be consumed inside > > /// FspMemoryInit() API. > > /// > > - UINT64 StackSize; > > + UINT64 StackSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Size of memory to be reserved by FSP below "top > > /// of low usable memory" for bootloader usage. > > /// > > - UINT32 BootLoaderTolumSize; > > + UINT32 BootLoaderTolumSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Current boot mode. > > /// > > - UINT32 BootMode; > > + UINT32 BootMode; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Optional event handler for the bootloader to be informed of events > occurring during FSP execution. > > /// This value is only valid if Revision is >= 2. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[24]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[24]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPM_ARCH2_UPD; > > > > /// > > @@ -266,18 +266,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspEventHandler Optional event handler for the bootloader to be > informed of events > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPS_ARCH2_UPD; > > > > /// > > @@ -609,4 +609,23 @@ EFI_STATUS > > IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr > > ); > > > > +/** > > + This FSP API initializes SMM and provide any OS runtime silicon services, > > + including Reliability, Availability, and Serviceability (RAS) features > implemented by the CPU. > > + > > + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. > > + If NULL, FSP will use the default parameters. > > + > > + @retval EFI_SUCCESS FSP execution environment was initialized > successfully. > > + @retval EFI_INVALID_PARAMETER Input parameters are invalid. > > + @retval EFI_UNSUPPORTED The FSP calling conditions were not met. > > + @retval EFI_DEVICE_ERROR FSP initialization failed. > > + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status > codes will not be returned during S3. > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI *FSP_SMM_INIT)( > > + IN VOID *FspiUpdDataPtr > > + ); > > + > > #endif > > diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h > b/IntelFsp2Pkg/Include/FspGlobalData.h > > index 445540abfa..697b20ed4c 100644 > > --- a/IntelFsp2Pkg/Include/FspGlobalData.h > > +++ b/IntelFsp2Pkg/Include/FspGlobalData.h > > @@ -10,9 +10,9 @@ > > > > #include <FspEas.h> > > > > -#define FSP_IN_API_MODE 0 > > -#define FSP_IN_DISPATCH_MODE 1 > > -#define FSP_GLOBAL_DATA_VERSION 1 > > +#define FSP_IN_API_MODE 0 > > +#define FSP_IN_DISPATCH_MODE 1 > > +#define FSP_GLOBAL_DATA_VERSION 1 > > > > #pragma pack(1) > > > > @@ -24,16 +24,17 @@ typedef enum { > > TempRamExitApiIndex, > > FspSiliconInitApiIndex, > > FspMultiPhaseSiInitApiIndex, > > + FspSmmInitApiIndex, > > FspApiIndexMax > > } FSP_API_INDEX; > > > > typedef struct { > > - VOID *DataPtr; > > - UINTN MicrocodeRegionBase; > > - UINTN MicrocodeRegionSize; > > - UINTN CodeRegionBase; > > - UINTN CodeRegionSize; > > - UINTN Reserved; > > + VOID *DataPtr; > > + UINTN MicrocodeRegionBase; > > + UINTN MicrocodeRegionSize; > > + UINTN CodeRegionBase; > > + UINTN CodeRegionSize; > > + UINTN Reserved; > > } FSP_PLAT_DATA; > > > > #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'D') > > @@ -41,28 +42,28 @@ typedef struct { > > #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF > > > > typedef struct { > > - UINT32 Signature; > > - UINT8 Version; > > - UINT8 Reserved1[3]; > > + UINT32 Signature; > > + UINT8 Version; > > + UINT8 Reserved1[3]; > > /// > > /// Offset 0x08 > > /// > > - UINTN CoreStack; > > - UINTN Reserved2; > > + UINTN CoreStack; > > + UINTN Reserved2; > > /// > > /// IA32: Offset 0x10; X64: Offset 0x18 > > /// > > - UINT32 StatusCode; > > - UINT8 ApiIdx; > > + UINT32 StatusCode; > > + UINT8 ApiIdx; > > /// > > /// 0: FSP in API mode; 1: FSP in DISPATCH mode > > /// > > - UINT8 FspMode; > > - UINT8 OnSeparateStack; > > - UINT8 Reserved3; > > - UINT32 NumberOfPhases; > > - UINT32 PhasesExecuted; > > - UINT32 Reserved4[8]; > > + UINT8 FspMode; > > + UINT8 OnSeparateStack; > > + UINT8 Reserved3; > > + UINT32 NumberOfPhases; > > + UINT32 PhasesExecuted; > > + UINT32 Reserved4[8]; > > /// > > /// IA32: Offset 0x40; X64: Offset 0x48 > > /// Start of UINTN and pointer section > > @@ -75,21 +76,23 @@ typedef struct { > > VOID *TempRamInitUpdPtr; > > VOID *MemoryInitUpdPtr; > > VOID *SiliconInitUpdPtr; > > + VOID *SmmInitUpdPtr; > > /// > > - /// IA32: Offset 0x64; X64: Offset 0x90 > > + /// IA32: Offset 0x68; X64: Offset 0x98 > > /// To store function parameters pointer > > /// so it can be retrieved after stack switched. > > /// > > VOID *FunctionParameterPtr; > > FSP_INFO_HEADER *FspInfoHeader; > > VOID *UpdDataPtr; > > + UINTN Reserved5; > > /// > > /// End of UINTN and pointer section > > /// > > - UINT8 Reserved5[16]; > > + UINT8 Reserved6[16]; > > UINT32 PerfSig; > > UINT16 PerfLen; > > - UINT16 Reserved6; > > + UINT16 Reserved7; > > UINT32 PerfIdx; > > UINT64 PerfData[32]; > > } FSP_GLOBAL_DATA; > > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > index c660defac3..c7fb63168f 100644 > > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > @@ -26,13 +26,13 @@ > > > > #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') > > > > -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > -#define FSP_IA32 0 > > -#define FSP_X64 1 > > +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > +#define FSP_IA32 0 > > +#define FSP_X64 1 > > > > -#pragma pack(1) > > + #pragma pack(1) > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style > guidelines don't require that and it certainly looks worse than before. > > > > > /// > > /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. > > @@ -159,6 +159,14 @@ typedef struct { > > /// Byte 0x4E: Reserved4. > > /// > > UINT16 Reserved4; > > + /// > > + /// Byte 0x50: Offset for the API for the Multi-Phase memory initialization. > > + /// > > + UINT32 FspMultiPhaseMemInitEntryOffset; > > + /// > > + /// Byte 0x54: Offset for the API to initialize SMM. > > + /// > > + UINT32 FspSmmInitEntryOffset; > > } FSP_INFO_HEADER; > > > > /// > > @@ -240,7 +248,7 @@ typedef struct { > > // UINT32 PatchData[]; > > } FSP_PATCH_TABLE; > > > > -#pragma pack() > > + #pragma pack() > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style > guidelines don't require that and it certainly looks worse than before. > > > > > extern EFI_GUID gFspHeaderFileGuid; > > > > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > index 7cf7e88245..b2d7867880 100644 > > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > @@ -68,6 +68,7 @@ > > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf > > + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf > > IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf > > > > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py > b/IntelFsp2Pkg/Tools/GenCfgOpt.py > > index c4fb1f1bb2..128b896592 100644 > > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > > @@ -953,8 +953,8 @@ EndList > > return NoFileChange > > > > def CreateSplitUpdTxt (self, UpdTxtFile): > > - GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID > '] > > - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, and _S > signature for FSPT, FSPM, FSPS > > + GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID > ','FSP_I_UPD_TOOL_GUID'] > > + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] # _T, _M, _S > and _I signature for FSPT, FSPM, FSPS, FSPI > > for Index in range(len(GuidList)): > > UpdTxtFile = '' > > FvDir = self._FvDir > > @@ -1288,19 +1288,21 @@ EndList > > Chars.append(chr(Value & 0xFF)) > > Value = Value >> 8 > > SignatureStr = ''.join(Chars) > > - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS accordingly > > + # Signature will be _T / _M / _S / _I for FSPT / FSPM / FSPS /FSPI > accordingly > > if '_T' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPT_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_M' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPM_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_S' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPS_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > + elif '_I' in SignatureStr[6:6+2]: > > + TxtBody.append("#define FSPI_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > TxtBody.append("\n") > > > > for Region in ['UPD']: > > UpdOffsetTable = [] > > - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', '_M', '_S'] > signature for FSPT, FSPM, FSPS > > - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] > > + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] #['_T', '_M', > '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI > > + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', 'FSPI_UPD'] > > for Item in self._CfgItemList: > > if Item["cname"] == 'Signature' and Item["value"][0:6] in > UpdSignature: > > Item["offset"] = 0 # re-initialize offset to 0 when new UPD > structure starting > > @@ -1393,11 +1395,12 @@ EndList > > HeaderTFileName = 'FsptUpd.h' > > HeaderMFileName = 'FspmUpd.h' > > HeaderSFileName = 'FspsUpd.h' > > + HeaderIFileName = 'FspiUpd.h' > > > > - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > > - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', > 'FSPS_UPD_SIGNATURE'] > > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD'] > > + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # FSPX_UPD_REGION > > + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', > 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] > > + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] > > ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', > 'FSPS_ARCH2_UPD'] > > > > IncLines = [] > > @@ -1420,6 +1423,9 @@ EndList > > elif UpdRegionCheck[item] == 'FSPS': > > HeaderFd = open(os.path.join(FvDir, HeaderSFileName), "w") > > FileBase = os.path.basename(os.path.join(FvDir, HeaderSFileName)) > > + elif UpdRegionCheck[item] == 'FSPI': > > + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), "w") > > + FileBase = os.path.basename(os.path.join(FvDir, HeaderIFileName)) > > FileName = FileBase.replace(".", "_").upper() > > HeaderFd.write("%s\n" % (__copyright_h__ % date.today().year)) > > HeaderFd.write("#ifndef __%s__\n" % FileName) > > @@ -1696,7 +1702,7 @@ EndList > > > > > > def Usage(): > > - print ("GenCfgOpt Version 0.57") > > + print ("GenCfgOpt Version 0.58") > > print ("Usage:") > > print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir [-D > Macros]") > > print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir InputHFile [-D > Macros]") > > diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py > b/IntelFsp2Pkg/Tools/SplitFspBin.py > > index f9151b5afd..317d9c1fa0 100644 > > --- a/IntelFsp2Pkg/Tools/SplitFspBin.py > > +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py > > @@ -1,6 +1,6 @@ > > ## @ SplitFspBin.py > > # > > -# Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR> > > +# Copyright (c) 2015 - 2022, Intel Corporation. All rights reserved.<BR> > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > ## > > @@ -492,7 +492,7 @@ class FspImage: > > self.FihOffset = fihoff > > self.Offset = offset > > self.FvIdxList = [] > > - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & > 0x0F] > > + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & > 0x0F] > > self.PatchList = patch > > self.PatchList.append(fihoff + 0x1C) > > > > @@ -869,7 +869,7 @@ def main (): > > parser_rebase = subparsers.add_parser('rebase', help='rebase a FSP into a > new base address') > > parser_rebase.set_defaults(which='rebase') > > parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', type=str, > help='FSP binary file path', required = True) > > - parser_rebase.add_argument('-c', '--fspcomp', > choices=['t','m','s','o'], nargs='+', dest='FspComponent', type=str, help='FSP > component to rebase', default = "['t']", required = True) > > + parser_rebase.add_argument('-c', '--fspcomp', > choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', type=str, help='FSP > component to rebase', default = "['t']", required = True) > > parser_rebase.add_argument('-b', '--newbase', dest='FspBase', nargs='+', > type=str, help='Rebased FSP binary file name', default = '', required = True) > > parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', type=str, > help='Output directory path', default = '.') > > parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', type=str, > help='Rebased FSP binary file name', default = '') > > -- > > 2.35.0.windows.1 > > > > > > > > > -- > Pedro Falcato ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. 2022-07-18 21:40 ` Nate DeSimone 2022-07-18 21:49 ` [edk2-devel] " Pedro Falcato @ 2022-07-18 22:02 ` Chiu, Chasel 1 sibling, 0 replies; 6+ messages in thread From: Chiu, Chasel @ 2022-07-18 22:02 UTC (permalink / raw) To: Desimone, Nathaniel L, devel@edk2.groups.io; +Cc: Zhang, Hongbin1, Zeng, Star Hi Nate, Those whitespace and indents are for passing Uncrustify check. I notice Uncrustify was enabled in Edk2 merging check and one of my previous patch was failed due to spaces/indents were not agreed by Uncrustify, that's why this time I run the Uncrustify offline before submitting the patch for reviewing. Wiki link: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#recommended-usage-visual-studio-vs-code-plugin I will fix ApiIdx issue and resend patch again. Thanks, Chasel > -----Original Message----- > From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com> > Sent: Monday, July 18, 2022 2:41 PM > To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io > Cc: Zhang, Hongbin1 <hongbin1.zhang@intel.com>; Zeng, Star > <star.zeng@intel.com> > Subject: RE: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > Hi Chasel, > > Please see comments inline. Here is a summary of my feedback: > > #1) IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm - line 34 - Bug: eax should > be rax > #2) IntelFsp2Pkg/Include/FspEas/FspApi.h - Various unnecessary whitespace > changes that make the file look worse than before. > #3) IntelFsp2Pkg/Include/Guid/FspHeaderFile.h - Why indent the #pragma lines? > > Thanks, > Nate > > > -----Original Message----- > > From: Chiu, Chasel <chasel.chiu@intel.com> > > Sent: Thursday, July 14, 2022 1:04 PM > > To: devel@edk2.groups.io > > Cc: Zhang, Hongbin1 <hongbin1.zhang@intel.com>; Desimone, Nathaniel L > > <nathaniel.l.desimone@intel.com>; Zeng, Star <star.zeng@intel.com>; > > Chiu, Chasel <chasel.chiu@intel.com> > > Subject: [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support. > > > > From: Hongbin1 Zhang <hongbin1.zhang@intel.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3993 > > Add FSP-I API entry point for SMM support. > > > > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Signed-off-by: Chasel Chiu <chasel.chiu@intel.com> > > Signed-off-by: Hongbin1 Zhang <hongbin1.zhang@intel.com> > > --- > > IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 13 +++++++++++++ > > IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf | 54 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm | 44 > > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm | 44 > ++++++++++++++++++++++++++++++++++++++++++++ > > IntelFsp2Pkg/Include/FspEas/FspApi.h | 57 > ++++++++++++++++++++++++++++++++++++++------------------- > > IntelFsp2Pkg/Include/FspGlobalData.h | 53 > ++++++++++++++++++++++++++++------------------------- > > IntelFsp2Pkg/Include/Guid/FspHeaderFile.h | 22 +++++++++++++++------- > > IntelFsp2Pkg/IntelFsp2Pkg.dsc | 1 + > > IntelFsp2Pkg/Tools/GenCfgOpt.py | 26 ++++++++++++++++---------- > > IntelFsp2Pkg/Tools/SplitFspBin.py | 6 +++--- > > 10 files changed, 256 insertions(+), 64 deletions(-) > > > > diff --git a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > index e22a88cc84..35d223a404 100644 > > --- a/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > +++ b/IntelFsp2Pkg/FspSecCore/SecFspApiChk.c > > @@ -71,6 +71,19 @@ FspApiCallingCheck ( > > Status = EFI_INVALID_PARAMETER; > > } > > } > > + } else if (ApiIdx == FspSmmInitApiIndex) { > > + // > > + // FspSmmInitApiIndex check > > + // > > + if ((FspData == NULL) || ((UINTN)FspData == MAX_ADDRESS) || > ((UINTN)FspData == MAX_UINT32)) { > > + Status = EFI_UNSUPPORTED; > > + } else { > > + if (FspData->Signature != FSP_GLOBAL_DATA_SIGNATURE) { > > + Status = EFI_UNSUPPORTED; > > + } else if (EFI_ERROR (FspUpdSignatureCheck (FspSmmInitApiIndex, > ApiParam))) { > > + Status = EFI_INVALID_PARAMETER; > > + } > > + } > > } else { > > Status = EFI_UNSUPPORTED; > > } > > diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > new file mode 100644 > > index 0000000000..d31576c00b > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > @@ -0,0 +1,54 @@ > > +## @file > > +# Sec Core for FSP > > +# > > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> # > > +# SPDX-License-Identifier: BSD-2-Clause-Patent # ## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > + BASE_NAME = FspSecCoreI > > + FILE_GUID = 558782b5-782d-415e-ab9e-0ceb79dc3425 > > + MODULE_TYPE = SEC > > + VERSION_STRING = 1.0 > > + > > +# > > +# The following information is for reference only and not required by the > build tools. > > +# > > +# VALID_ARCHITECTURES = IA32 X64 > > +# > > + > > +[Sources] > > + SecFspApiChk.c > > + SecFsp.h > > + > > +[Sources.X64] > > + X64/FspApiEntryI.nasm > > + X64/FspApiEntryCommon.nasm > > + X64/FspHelper.nasm > > + > > +[Sources.IA32] > > + Ia32/FspApiEntryI.nasm > > + Ia32/FspApiEntryCommon.nasm > > + Ia32/FspHelper.nasm > > + > > +[Binaries.Ia32] > > + RAW|Vtf0/Bin/ResetVec.ia32.raw |GCC > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + IntelFsp2Pkg/IntelFsp2Pkg.dec > > + > > +[LibraryClasses] > > + BaseMemoryLib > > + DebugLib > > + BaseLib > > + PciCf8Lib > > + SerialPortLib > > + FspSwitchStackLib > > + FspCommonLib > > + FspSecPlatformLib > > + > > + > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > > +SPDX-License-Identifier: BSD-2-Clause-Patent ;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C ; extern > > +ASM_PFX(FspApiCommon) > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the > > +boot ; process ; > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > + jmp ASM_PFX(FspApiCommon) > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; Module Entrypoint API > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by > > +compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > new file mode 100644 > > index 0000000000..e9365d6832 > > --- /dev/null > > +++ b/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryI.nasm > > @@ -0,0 +1,44 @@ > > +;; @file > > +; Provide FSP API entry points. > > +; > > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > > +SPDX-License-Identifier: BSD-2-Clause-Patent ;; > > + > > + SECTION .text > > + > > +; > > +; Following functions will be provided in C ; extern > > +ASM_PFX(FspApiCommon) > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; FspApiCommonContinue API > > +; > > +; This is the FSP API common entry point to resume the FSP execution > > +; > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(FspApiCommonContinue) > > +ASM_PFX(FspApiCommonContinue): > > + jmp $ > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; FspSmmInit API > > +; > > +; This FSP API will notify the FSP about the different phases in the > > +boot ; process ; > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(FspSmmInitApi) > > +ASM_PFX(FspSmmInitApi): > > + mov eax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > This is a bug. It should be: > > mov rax, 7 ; FSP_API_INDEX.FspSmmInitApiIndex > > Note that IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm compares > ApiIndex numbers using rax, so it is important to make sure that the upper 32- > bits are zero: > > https://github.com/tianocore/edk2/blob/c966204049f3d5dae6d5e587ddc298c6 > 84142c5c/IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm#L65 > > > + jmp ASM_PFX(FspApiCommon) > > + > > +;-------------------------------------------------------------------- > > +-------- > > +; Module Entrypoint API > > +;-------------------------------------------------------------------- > > +-------- > > +global ASM_PFX(_ModuleEntryPoint) > > +ASM_PFX(_ModuleEntryPoint): > > + jmp $ > > + ; Add reference to APIs so that it will not be optimized by > > +compiler > > + jmp ASM_PFX(FspSmmInitApi) > > diff --git a/IntelFsp2Pkg/Include/FspEas/FspApi.h > > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > index b36bc2b9ae..1d6c2fb63d 100644 > > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > > @@ -135,18 +135,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspDebugHandler Optional debug handler for the bootloader to receive > debug messages > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspDebugHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPT_ARCH2_UPD; > > > > /// > > @@ -197,37 +197,37 @@ typedef struct { > > /// > > /// Revision of the structure is 3 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 64. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Pointer to the temporary stack base address to be > > /// consumed inside FspMemoryInit() API. > > /// > > - EFI_PHYSICAL_ADDRESS StackBase; > > + EFI_PHYSICAL_ADDRESS StackBase; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Temporary stack size to be consumed inside > > /// FspMemoryInit() API. > > /// > > - UINT64 StackSize; > > + UINT64 StackSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Size of memory to be reserved by FSP below "top > > /// of low usable memory" for bootloader usage. > > /// > > - UINT32 BootLoaderTolumSize; > > + UINT32 BootLoaderTolumSize; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Current boot mode. > > /// > > - UINT32 BootMode; > > + UINT32 BootMode; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// Optional event handler for the bootloader to be informed of events > occurring during FSP execution. > > /// This value is only valid if Revision is >= 2. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[24]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[24]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPM_ARCH2_UPD; > > > > /// > > @@ -266,18 +266,18 @@ typedef struct { > > /// > > /// Revision of the structure is 2 for this version of the specification. > > /// > > - UINT8 Revision; > > - UINT8 Reserved[3]; > > + UINT8 Revision; > > + UINT8 Reserved[3]; > > Any reason for these whitespace changes? It looks worse than before. > > > /// > > /// Length of the structure in bytes. The current value for this field is 32. > > /// > > - UINT32 Length; > > + UINT32 Length; > > Any reason for this whitespace change? It looks worse than before. > > > /// > > /// FspEventHandler Optional event handler for the bootloader to be > informed of events > > /// occurring during FSP execution. > > /// > > - EFI_PHYSICAL_ADDRESS FspEventHandler; > > - UINT8 Reserved1[16]; > > + EFI_PHYSICAL_ADDRESS FspEventHandler; > > + UINT8 Reserved1[16]; > > Any reason for these whitespace changes? It looks worse than before. > > > } FSPS_ARCH2_UPD; > > > > /// > > @@ -609,4 +609,23 @@ EFI_STATUS > > IN FSP_MULTI_PHASE_PARAMS *MultiPhaseSiInitParamPtr > > ); > > > > +/** > > + This FSP API initializes SMM and provide any OS runtime silicon > > +services, > > + including Reliability, Availability, and Serviceability (RAS) features > implemented by the CPU. > > + > > + @param[in] FspiUpdDataPtr Pointer to the FSPI_UPD data structure. > > + If NULL, FSP will use the default parameters. > > + > > + @retval EFI_SUCCESS FSP execution environment was initialized > successfully. > > + @retval EFI_INVALID_PARAMETER Input parameters are invalid. > > + @retval EFI_UNSUPPORTED The FSP calling conditions were not met. > > + @retval EFI_DEVICE_ERROR FSP initialization failed. > > + @retval FSP_STATUS_RESET_REQUIREDx A reset is required. These status > codes will not be returned during S3. > > +**/ > > +typedef > > +EFI_STATUS > > +(EFIAPI *FSP_SMM_INIT)( > > + IN VOID *FspiUpdDataPtr > > + ); > > + > > #endif > > diff --git a/IntelFsp2Pkg/Include/FspGlobalData.h > > b/IntelFsp2Pkg/Include/FspGlobalData.h > > index 445540abfa..697b20ed4c 100644 > > --- a/IntelFsp2Pkg/Include/FspGlobalData.h > > +++ b/IntelFsp2Pkg/Include/FspGlobalData.h > > @@ -10,9 +10,9 @@ > > > > #include <FspEas.h> > > > > -#define FSP_IN_API_MODE 0 > > -#define FSP_IN_DISPATCH_MODE 1 > > -#define FSP_GLOBAL_DATA_VERSION 1 > > +#define FSP_IN_API_MODE 0 > > +#define FSP_IN_DISPATCH_MODE 1 > > +#define FSP_GLOBAL_DATA_VERSION 1 > > > > #pragma pack(1) > > > > @@ -24,16 +24,17 @@ typedef enum { > > TempRamExitApiIndex, > > FspSiliconInitApiIndex, > > FspMultiPhaseSiInitApiIndex, > > + FspSmmInitApiIndex, > > FspApiIndexMax > > } FSP_API_INDEX; > > > > typedef struct { > > - VOID *DataPtr; > > - UINTN MicrocodeRegionBase; > > - UINTN MicrocodeRegionSize; > > - UINTN CodeRegionBase; > > - UINTN CodeRegionSize; > > - UINTN Reserved; > > + VOID *DataPtr; > > + UINTN MicrocodeRegionBase; > > + UINTN MicrocodeRegionSize; > > + UINTN CodeRegionBase; > > + UINTN CodeRegionSize; > > + UINTN Reserved; > > } FSP_PLAT_DATA; > > > > #define FSP_GLOBAL_DATA_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'D') > > @@ -41,28 +42,28 @@ typedef struct { > > #define FSP_PERFORMANCE_DATA_TIMER_MASK 0xFFFFFFFFFFFFFF > > > > typedef struct { > > - UINT32 Signature; > > - UINT8 Version; > > - UINT8 Reserved1[3]; > > + UINT32 Signature; > > + UINT8 Version; > > + UINT8 Reserved1[3]; > > /// > > /// Offset 0x08 > > /// > > - UINTN CoreStack; > > - UINTN Reserved2; > > + UINTN CoreStack; > > + UINTN Reserved2; > > /// > > /// IA32: Offset 0x10; X64: Offset 0x18 > > /// > > - UINT32 StatusCode; > > - UINT8 ApiIdx; > > + UINT32 StatusCode; > > + UINT8 ApiIdx; > > /// > > /// 0: FSP in API mode; 1: FSP in DISPATCH mode > > /// > > - UINT8 FspMode; > > - UINT8 OnSeparateStack; > > - UINT8 Reserved3; > > - UINT32 NumberOfPhases; > > - UINT32 PhasesExecuted; > > - UINT32 Reserved4[8]; > > + UINT8 FspMode; > > + UINT8 OnSeparateStack; > > + UINT8 Reserved3; > > + UINT32 NumberOfPhases; > > + UINT32 PhasesExecuted; > > + UINT32 Reserved4[8]; > > /// > > /// IA32: Offset 0x40; X64: Offset 0x48 > > /// Start of UINTN and pointer section @@ -75,21 +76,23 @@ typedef > > struct { > > VOID *TempRamInitUpdPtr; > > VOID *MemoryInitUpdPtr; > > VOID *SiliconInitUpdPtr; > > + VOID *SmmInitUpdPtr; > > /// > > - /// IA32: Offset 0x64; X64: Offset 0x90 > > + /// IA32: Offset 0x68; X64: Offset 0x98 > > /// To store function parameters pointer > > /// so it can be retrieved after stack switched. > > /// > > VOID *FunctionParameterPtr; > > FSP_INFO_HEADER *FspInfoHeader; > > VOID *UpdDataPtr; > > + UINTN Reserved5; > > /// > > /// End of UINTN and pointer section > > /// > > - UINT8 Reserved5[16]; > > + UINT8 Reserved6[16]; > > UINT32 PerfSig; > > UINT16 PerfLen; > > - UINT16 Reserved6; > > + UINT16 Reserved7; > > UINT32 PerfIdx; > > UINT64 PerfData[32]; > > } FSP_GLOBAL_DATA; > > diff --git a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > index c660defac3..c7fb63168f 100644 > > --- a/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > +++ b/IntelFsp2Pkg/Include/Guid/FspHeaderFile.h > > @@ -26,13 +26,13 @@ > > > > #define FSP_INFO_HEADER_SIGNATURE SIGNATURE_32 ('F', 'S', 'P', 'H') > > > > -#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > -#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > -#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > -#define FSP_IA32 0 > > -#define FSP_X64 1 > > +#define IMAGE_ATTRIBUTE_GRAPHICS_SUPPORT BIT0 > > +#define IMAGE_ATTRIBUTE_DISPATCH_MODE_SUPPORT BIT1 > > +#define IMAGE_ATTRIBUTE_64BIT_MODE_SUPPORT BIT2 > > +#define FSP_IA32 0 > > +#define FSP_X64 1 > > > > -#pragma pack(1) > > + #pragma pack(1) > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style > guidelines don't require that and it certainly looks worse than before. > > > > > /// > > /// FSP Information Header as described in FSP v2.0 Spec section 5.1.1. > > @@ -159,6 +159,14 @@ typedef struct { > > /// Byte 0x4E: Reserved4. > > /// > > UINT16 Reserved4; > > + /// > > + /// Byte 0x50: Offset for the API for the Multi-Phase memory initialization. > > + /// > > + UINT32 FspMultiPhaseMemInitEntryOffset; > > + /// > > + /// Byte 0x54: Offset for the API to initialize SMM. > > + /// > > + UINT32 FspSmmInitEntryOffset; > > } FSP_INFO_HEADER; > > > > /// > > @@ -240,7 +248,7 @@ typedef struct { > > // UINT32 PatchData[]; > > } FSP_PATCH_TABLE; > > > > -#pragma pack() > > + #pragma pack() > > Why add an indent to the #pragma line? I'm pretty sure that edk2 coding style > guidelines don't require that and it certainly looks worse than before. > > > > > extern EFI_GUID gFspHeaderFileGuid; > > > > diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > b/IntelFsp2Pkg/IntelFsp2Pkg.dsc index 7cf7e88245..b2d7867880 100644 > > --- a/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dsc > > @@ -68,6 +68,7 @@ > > IntelFsp2Pkg/FspSecCore/FspSecCoreT.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf > > IntelFsp2Pkg/FspSecCore/FspSecCoreS.inf > > + IntelFsp2Pkg/FspSecCore/FspSecCoreI.inf > > IntelFsp2Pkg/FspSecCore/Fsp22SecCoreS.inf > > IntelFsp2Pkg/FspNotifyPhase/FspNotifyPhasePeim.inf > > > > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py > > b/IntelFsp2Pkg/Tools/GenCfgOpt.py index c4fb1f1bb2..128b896592 100644 > > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > > @@ -953,8 +953,8 @@ EndList > > return NoFileChange > > > > def CreateSplitUpdTxt (self, UpdTxtFile): > > - GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID > '] > > - SignatureList = ['0x545F', '0x4D5F','0x535F'] # _T, _M, and _S > signature for FSPT, FSPM, FSPS > > + GuidList = > ['FSP_T_UPD_TOOL_GUID','FSP_M_UPD_TOOL_GUID','FSP_S_UPD_TOOL_GUID > ','FSP_I_UPD_TOOL_GUID'] > > + SignatureList = ['0x545F', '0x4D5F','0x535F','0x495F'] # _T, _M, _S > and _I signature for FSPT, FSPM, FSPS, FSPI > > for Index in range(len(GuidList)): > > UpdTxtFile = '' > > FvDir = self._FvDir > > @@ -1288,19 +1288,21 @@ EndList > > Chars.append(chr(Value & 0xFF)) > > Value = Value >> 8 > > SignatureStr = ''.join(Chars) > > - # Signature will be _T / _M / _S for FSPT / FSPM / FSPS accordingly > > + # Signature will be _T / _M / _S / _I for FSPT / FSPM > > + / FSPS /FSPI accordingly > > if '_T' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPT_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_M' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPM_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > elif '_S' in SignatureStr[6:6+2]: > > TxtBody.append("#define FSPS_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > + elif '_I' in SignatureStr[6:6+2]: > > + TxtBody.append("#define FSPI_UPD_SIGNATURE %s /* > '%s' */\n\n" % (Item['value'], SignatureStr)) > > TxtBody.append("\n") > > > > for Region in ['UPD']: > > UpdOffsetTable = [] > > - UpdSignature = ['0x545F', '0x4D5F', '0x535F'] #['_T', '_M', '_S'] > signature for FSPT, FSPM, FSPS > > - UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD'] > > + UpdSignature = ['0x545F', '0x4D5F', '0x535F', '0x495F'] #['_T', '_M', > '_S', '_I'] signature for FSPT, FSPM, FSPS, FSPI > > + UpdStructure = ['FSPT_UPD', 'FSPM_UPD', 'FSPS_UPD', > > + 'FSPI_UPD'] > > for Item in self._CfgItemList: > > if Item["cname"] == 'Signature' and Item["value"][0:6] in > UpdSignature: > > Item["offset"] = 0 # re-initialize offset to 0 > > when new UPD structure starting @@ -1393,11 +1395,12 @@ EndList > > HeaderTFileName = 'FsptUpd.h' > > HeaderMFileName = 'FspmUpd.h' > > HeaderSFileName = 'FspsUpd.h' > > + HeaderIFileName = 'FspiUpd.h' > > > > - UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > > - UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > - UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', 'FSPM_UPD_SIGNATURE', > 'FSPS_UPD_SIGNATURE'] > > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD'] > > + UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS', 'FSPI'] # FSPX_UPD_REGION > > + UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S', 'FSP_I'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > > + UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', > 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE', 'FSPI_UPD_SIGNATURE'] > > + ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > > + 'FSPS_ARCH_UPD', 'FSPI_ARCH_UPD'] > > ExcludedSpecificUpd1 = ['FSPT_ARCH2_UPD', 'FSPM_ARCH2_UPD', > > 'FSPS_ARCH2_UPD'] > > > > IncLines = [] > > @@ -1420,6 +1423,9 @@ EndList > > elif UpdRegionCheck[item] == 'FSPS': > > HeaderFd = open(os.path.join(FvDir, HeaderSFileName), "w") > > FileBase = os.path.basename(os.path.join(FvDir, > > HeaderSFileName)) > > + elif UpdRegionCheck[item] == 'FSPI': > > + HeaderFd = open(os.path.join(FvDir, HeaderIFileName), "w") > > + FileBase = os.path.basename(os.path.join(FvDir, > > + HeaderIFileName)) > > FileName = FileBase.replace(".", "_").upper() > > HeaderFd.write("%s\n" % (__copyright_h__ % date.today().year)) > > HeaderFd.write("#ifndef __%s__\n" % FileName) > > @@ -1696,7 +1702,7 @@ EndList > > > > > > def Usage(): > > - print ("GenCfgOpt Version 0.57") > > + print ("GenCfgOpt Version 0.58") > > print ("Usage:") > > print (" GenCfgOpt UPDTXT PlatformDscFile BuildFvDir [-D > Macros]") > > print (" GenCfgOpt HEADER PlatformDscFile BuildFvDir InputHFile [-D > Macros]") > > diff --git a/IntelFsp2Pkg/Tools/SplitFspBin.py > > b/IntelFsp2Pkg/Tools/SplitFspBin.py > > index f9151b5afd..317d9c1fa0 100644 > > --- a/IntelFsp2Pkg/Tools/SplitFspBin.py > > +++ b/IntelFsp2Pkg/Tools/SplitFspBin.py > > @@ -1,6 +1,6 @@ > > ## @ SplitFspBin.py > > # > > -# Copyright (c) 2015 - 2021, Intel Corporation. All rights > > reserved.<BR> > > +# Copyright (c) 2015 - 2022, Intel Corporation. All rights > > +reserved.<BR> > > # SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -492,7 > > +492,7 @@ class FspImage: > > self.FihOffset = fihoff > > self.Offset = offset > > self.FvIdxList = [] > > - self.Type = "XTMSXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & > 0x0F] > > + self.Type = "XTMSIXXXXOXXXXXXX"[(fih.ComponentAttribute >> 12) & > 0x0F] > > self.PatchList = patch > > self.PatchList.append(fihoff + 0x1C) > > > > @@ -869,7 +869,7 @@ def main (): > > parser_rebase = subparsers.add_parser('rebase', help='rebase a FSP into a > new base address') > > parser_rebase.set_defaults(which='rebase') > > parser_rebase.add_argument('-f', '--fspbin' , dest='FspBinary', type=str, > help='FSP binary file path', required = True) > > - parser_rebase.add_argument('-c', '--fspcomp', choices=['t','m','s','o'], > nargs='+', dest='FspComponent', type=str, help='FSP component to rebase', > default = "['t']", required = True) > > + parser_rebase.add_argument('-c', '--fspcomp', > > + choices=['t','m','s','o','i'], nargs='+', dest='FspComponent', > > + type=str, help='FSP component to rebase', default = "['t']", > > + required = True) > > parser_rebase.add_argument('-b', '--newbase', dest='FspBase', nargs='+', > type=str, help='Rebased FSP binary file name', default = '', required = True) > > parser_rebase.add_argument('-o', '--outdir' , dest='OutputDir', type=str, > help='Output directory path', default = '.') > > parser_rebase.add_argument('-n', '--outfile', dest='OutputFile', > > type=str, help='Rebased FSP binary file name', default = '') > > -- > > 2.35.0.windows.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-19 17:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-14 20:04 [PATCH] IntelFsp2Pkg/FspSecCore: Add FSP-I API for SMM support Chiu, Chasel 2022-07-18 21:40 ` Nate DeSimone 2022-07-18 21:49 ` [edk2-devel] " Pedro Falcato 2022-07-18 23:40 ` Nate DeSimone 2022-07-19 17:13 ` Chiu, Chasel 2022-07-18 22:02 ` Chiu, Chasel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox