public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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: [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

* 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

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