public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Shadow microcode patch according to FIT microcode table.
@ 2020-01-09  2:14 Siyuan, Fu
  2020-01-09  2:14 ` [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
  2020-01-09  2:14 ` [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
  0 siblings, 2 replies; 8+ messages in thread
From: Siyuan, Fu @ 2020-01-09  2:14 UTC (permalink / raw)
  To: devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, lersek,
	Tom Lendacky

The existing MpInitLib will shadow the microcode update patches from
flash to memory and this is done by searching microcode region specified
by PCD PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
This brings a limition to platform FW that all the microcode patches must
be placed in one continuous flash space.

This patch set shadows microcode update according to FIT microcode entries
if it's present, otherwise it will fallback to original logic (by PCD).

V2:
Add Feature PCD for Patch 2/2 for enabling/disabling this support.

Patch 1/2: Add FIT header file to MdePkg.
Patch 2/2: Update microcode loader to shadow microcode according to FIT.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Siyuan Fu (2):
  MdePkg: Add header file for Firmware Interface Table specification.
  UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.

 .../IndustryStandard/FirmwareInterfaceTable.h |  76 +++++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 262 +++++++++++++-----
 UefiCpuPkg/Library/MpInitLib/MpLib.c          |   4 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   7 +-
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   4 +-
 UefiCpuPkg/UefiCpuPkg.dec                     |   6 +
 7 files changed, 292 insertions(+), 68 deletions(-)
 create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h

-- 
2.19.1.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.
  2020-01-09  2:14 [PATCH v2 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
@ 2020-01-09  2:14 ` Siyuan, Fu
  2020-01-10  3:09   ` Liming Gao
  2020-01-09  2:14 ` [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
  1 sibling, 1 reply; 8+ messages in thread
From: Siyuan, Fu @ 2020-01-09  2:14 UTC (permalink / raw)
  To: devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, lersek

This patch add FirmwareInterfaceTable.h for the Firmware Interface Table
BIOS specification.

This is to remove future edk2 dependency on edk2-platforms repo. The file
content comes from
 edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\IndustryStandard

BZ link: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
---
 .../IndustryStandard/FirmwareInterfaceTable.h | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h

diff --git a/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
new file mode 100644
index 0000000000..be3e34ae1b
--- /dev/null
+++ b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
@@ -0,0 +1,76 @@
+/** @file
+  Industry Standard Definitions of Firmware Interface Table BIOS Specification 1.0.
+
+  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __FIRMWARE_INTERFACE_TABLE_H__
+#define __FIRMWARE_INTERFACE_TABLE_H__
+
+//
+// FIT Entry type definitions
+//
+#define FIT_TYPE_00_HEADER                  0x00
+#define FIT_TYPE_01_MICROCODE               0x01
+#define FIT_TYPE_02_STARTUP_ACM             0x02
+#define FIT_TYPE_07_BIOS_STARTUP_MODULE     0x07
+#define FIT_TYPE_08_TPM_POLICY              0x08
+#define FIT_TYPE_09_BIOS_POLICY             0x09
+#define FIT_TYPE_0A_TXT_POLICY              0x0A
+#define FIT_TYPE_0B_KEY_MANIFEST            0x0B
+#define FIT_TYPE_0C_BOOT_POLICY_MANIFEST    0x0C
+#define FIT_TYPE_10_CSE_SECURE_BOOT         0x10
+#define FIT_TYPE_2D_TXTSX_POLICY            0x2D
+#define FIT_TYPE_2F_JMP_DEBUG_POLICY        0x2F
+#define FIT_TYPE_7F_SKIP                    0x7F
+
+#define FIT_POINTER_ADDRESS                 0xFFFFFFC0 ///< Fixed address at 4G - 40h
+
+#define FIT_TYPE_VERSION                    0x0100
+
+#define FIT_TYPE_00_SIGNATURE  SIGNATURE_64 ('_', 'F', 'I', 'T', '_', ' ', ' ', ' ')
+
+#pragma pack(1)
+
+typedef struct {
+  //
+  // Address is the base address of the firmware component
+  // must be aligned on 16 byte boundary
+  //
+  UINT64 Address;
+  //
+  // Size is the span of the component in multiple of 16 bytes
+  //
+  UINT8  Size[3];
+  //
+  // Reserved must be set to 0
+  //
+  UINT8  Reserved;
+  //
+  // Component's version number in binary coded decimal (BCD) format.
+  // For the FIT header entry, the value in this field will indicate the revision
+  // number of the FIT data structure. The upper byte of the revision field
+  // indicates the major revision and the lower byte indicates the minor revision.
+  //
+  UINT16 Version;
+  //
+  // FIT types 0x00 to 0x7F
+  //
+  UINT8  Type : 7;
+  //
+  // Checksum Valid indicates whether component has valid checksum.
+  //
+  UINT8  C_V  : 1;
+  //
+  // Component's checksum. The modulo sum of all the bytes in the component and
+  // the value in this field (Chksum) must add up to zero. This field is only
+  // valid if the C_V flag is non-zero.
+  //
+  UINT8  Chksum;
+} FIRMWARE_INTERFACE_TABLE_ENTRY;
+
+#pragma pack()
+
+#endif
-- 
2.19.1.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.
  2020-01-09  2:14 [PATCH v2 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
  2020-01-09  2:14 ` [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
@ 2020-01-09  2:14 ` Siyuan, Fu
  2020-01-09 13:32   ` Laszlo Ersek
  2020-01-10  3:38   ` [edk2-devel] " Dong, Eric
  1 sibling, 2 replies; 8+ messages in thread
From: Siyuan, Fu @ 2020-01-09  2:14 UTC (permalink / raw)
  To: devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni, lersek

The existing MpInitLib will shadow the microcode update patches from
flash to memory and this is done by searching microcode region specified
by PCD PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
This brings a limition to platform FW that all the microcode patches must
be placed in one continuous flash space.

This patch shadows microcode update according to FIT microcode entries if
it's present, otherwise it will fallback to original logic (by PCD).

A new featured PCD gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit
is added for enabling/disabling this support.

TEST: Tested on FIT enabled platform.
BZ: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 262 +++++++++++++-----
 UefiCpuPkg/Library/MpInitLib/MpLib.c          |   4 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   7 +-
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   4 +-
 UefiCpuPkg/UefiCpuPkg.dec                     |   6 +
 6 files changed, 216 insertions(+), 68 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index cd912ab0c5..0fd420ac93 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -69,4 +69,5 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ## CONSUMES
 
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 8f4d4da40c..9389e52ae5 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -318,7 +318,7 @@ Done:
 }
 
 /**
-  Determine if a microcode patch will be loaded into memory.
+  Determine if a microcode patch matchs the specific processor signature and flag.
 
   @param[in]  CpuMpData             The pointer to CPU MP Data structure.
   @param[in]  ProcessorSignature    The processor signature field value
@@ -330,7 +330,7 @@ Done:
   @retval FALSE    The specified microcode patch will not be loaded.
 **/
 BOOLEAN
-IsMicrocodePatchNeedLoad (
+IsProcessorMatchedMicrocodePatch (
   IN CPU_MP_DATA                 *CpuMpData,
   IN UINT32                      ProcessorSignature,
   IN UINT32                      ProcessorFlags
@@ -351,7 +351,77 @@ IsMicrocodePatchNeedLoad (
 }
 
 /**
-  Actual worker function that loads the required microcode patches into memory.
+  Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
+  patch header with the CPUID and PlatformID of the processors within
+  system to decide if it will be copied into memory.
+
+  @param[in]  CpuMpData             The pointer to CPU MP Data structure.
+  @param[in]  MicrocodeEntryPoint   The pointer to the microcode patch header.
+
+  @retval TRUE     The specified microcode patch need to be loaded.
+  @retval FALSE    The specified microcode patch dosen't need to be loaded.
+**/
+BOOLEAN
+IsMicrocodePatchNeedLoad (
+  IN CPU_MP_DATA                 *CpuMpData,
+  CPU_MICROCODE_HEADER           *MicrocodeEntryPoint
+  )
+{
+  BOOLEAN                                NeedLoad;
+  UINTN                                  DataSize;
+  UINTN                                  TotalSize;
+  CPU_MICROCODE_EXTENDED_TABLE_HEADER    *ExtendedTableHeader;
+  UINT32                                 ExtendedTableCount;
+  CPU_MICROCODE_EXTENDED_TABLE           *ExtendedTable;
+  UINTN                                  Index;
+
+  //
+  // Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch header.
+  //
+  NeedLoad = IsProcessorMatchedMicrocodePatch (
+               CpuMpData,
+               MicrocodeEntryPoint->ProcessorSignature.Uint32,
+               MicrocodeEntryPoint->ProcessorFlags
+               );
+
+  //
+  // If the Extended Signature Table exists, check if the processor is in the
+  // support list
+  //
+  DataSize  = MicrocodeEntryPoint->DataSize;
+  TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+  if ((!NeedLoad) && (DataSize != 0) &&
+      (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
+                              sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
+    ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
+                            + DataSize + sizeof (CPU_MICROCODE_HEADER));
+    ExtendedTableCount  = ExtendedTableHeader->ExtendedSignatureCount;
+    ExtendedTable       = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
+
+    for (Index = 0; Index < ExtendedTableCount; Index ++) {
+      //
+      // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
+      // Signature Table entry with the CPUID and PlatformID of the processors
+      // within system to decide if it will be copied into memory
+      //
+      NeedLoad = IsProcessorMatchedMicrocodePatch (
+                   CpuMpData,
+                   ExtendedTable->ProcessorSignature.Uint32,
+                   ExtendedTable->ProcessorFlag
+                   );
+      if (NeedLoad) {
+        break;
+      }
+      ExtendedTable ++;
+    }
+  }
+
+  return NeedLoad;
+}
+
+
+/**
+  Actual worker function that shadows the required microcode patches into memory.
 
   @param[in, out]  CpuMpData        The pointer to CPU MP Data structure.
   @param[in]       Patches          The pointer to an array of information on
@@ -363,7 +433,7 @@ IsMicrocodePatchNeedLoad (
                                     to be loaded.
 **/
 VOID
-LoadMicrocodePatchWorker (
+ShadowMicrocodePatchWorker (
   IN OUT CPU_MP_DATA             *CpuMpData,
   IN     MICROCODE_PATCH_INFO    *Patches,
   IN     UINTN                   PatchCount,
@@ -390,7 +460,6 @@ LoadMicrocodePatchWorker (
       (VOID *) Patches[Index].Address,
       Patches[Index].Size
       );
-
     Walker += Patches[Index].Size;
   }
 
@@ -410,12 +479,13 @@ LoadMicrocodePatchWorker (
 }
 
 /**
-  Load the required microcode patches data into memory.
+  Shadow the required microcode patches data into memory according to PCD
+  PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
 
   @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
 **/
 VOID
-LoadMicrocodePatch (
+ShadowMicrocodePatchByPcd (
   IN OUT CPU_MP_DATA             *CpuMpData
   )
 {
@@ -423,15 +493,10 @@ LoadMicrocodePatch (
   UINTN                                  MicrocodeEnd;
   UINTN                                  DataSize;
   UINTN                                  TotalSize;
-  CPU_MICROCODE_EXTENDED_TABLE_HEADER    *ExtendedTableHeader;
-  UINT32                                 ExtendedTableCount;
-  CPU_MICROCODE_EXTENDED_TABLE           *ExtendedTable;
   MICROCODE_PATCH_INFO                   *PatchInfoBuffer;
   UINTN                                  MaxPatchNumber;
   UINTN                                  PatchCount;
   UINTN                                  TotalLoadSize;
-  UINTN                                  Index;
-  BOOLEAN                                NeedLoad;
 
   //
   // Initialize the microcode patch related fields in CpuMpData as the values
@@ -487,55 +552,7 @@ LoadMicrocodePatch (
       continue;
     }
 
-    //
-    // Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
-    // patch header with the CPUID and PlatformID of the processors within
-    // system to decide if it will be copied into memory
-    //
-    NeedLoad = IsMicrocodePatchNeedLoad (
-                 CpuMpData,
-                 MicrocodeEntryPoint->ProcessorSignature.Uint32,
-                 MicrocodeEntryPoint->ProcessorFlags
-                 );
-
-    //
-    // If the Extended Signature Table exists, check if the processor is in the
-    // support list
-    //
-    if ((!NeedLoad) && (DataSize != 0) &&
-        (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
-                                sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
-      ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
-                              + DataSize + sizeof (CPU_MICROCODE_HEADER));
-      ExtendedTableCount  = ExtendedTableHeader->ExtendedSignatureCount;
-      ExtendedTable       = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
-
-      for (Index = 0; Index < ExtendedTableCount; Index ++) {
-        //
-        // Avoid access content beyond MicrocodeEnd
-        //
-        if ((UINTN) ExtendedTable > MicrocodeEnd - sizeof (CPU_MICROCODE_EXTENDED_TABLE)) {
-          break;
-        }
-
-        //
-        // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
-        // Signature Table entry with the CPUID and PlatformID of the processors
-        // within system to decide if it will be copied into memory
-        //
-        NeedLoad = IsMicrocodePatchNeedLoad (
-                     CpuMpData,
-                     ExtendedTable->ProcessorSignature.Uint32,
-                     ExtendedTable->ProcessorFlag
-                     );
-        if (NeedLoad) {
-          break;
-        }
-        ExtendedTable ++;
-      }
-    }
-
-    if (NeedLoad) {
+    if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
       PatchCount++;
       if (PatchCount > MaxPatchNumber) {
         //
@@ -581,7 +598,7 @@ LoadMicrocodePatch (
       __FUNCTION__, PatchCount, TotalLoadSize
       ));
 
-    LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
+    ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
   }
 
 OnExit:
@@ -590,3 +607,124 @@ OnExit:
   }
   return;
 }
+
+/**
+  Shadow the required microcode patches data into memory according to FIT microcode entry.
+
+  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
+
+  @return EFI_SUCCESS           Microcode patch is shadowed into memory.
+  @return EFI_UNSUPPORTED       FIT based microcode shadowing is not supported.
+  @return EFI_OUT_OF_RESOURCES  No enough memory resource.
+  @return EFI_NOT_FOUND         There is something wrong in FIT microcode entry.
+
+**/
+EFI_STATUS
+ShadowMicrocodePatchByFit (
+  IN OUT CPU_MP_DATA             *CpuMpData
+  )
+{
+  UINT64                            FitPointer;
+  FIRMWARE_INTERFACE_TABLE_ENTRY    *FitEntry;
+  UINT32                            EntryNum;
+  UINT32                            Index;
+  MICROCODE_PATCH_INFO              *PatchInfoBuffer;
+  UINTN                             MaxPatchNumber;
+  CPU_MICROCODE_HEADER              *MicrocodeEntryPoint;
+  UINTN                             PatchCount;
+  UINTN                             TotalSize;
+  UINTN                             TotalLoadSize;
+
+  if (!FeaturePcdGet (PcdCpuShadowMicrocodeByFit)) {
+    return EFI_UNSUPPORTED;
+  }
+
+  FitPointer = *(UINT64 *) (UINTN) FIT_POINTER_ADDRESS;
+  if ((FitPointer == 0) ||
+      (FitPointer == 0xFFFFFFFFFFFFFFFF) ||
+      (FitPointer == 0xEEEEEEEEEEEEEEEE)) {
+    //
+    // No FIT table.
+    //
+    ASSERT (FALSE);
+    return EFI_NOT_FOUND;
+  }
+  FitEntry = (FIRMWARE_INTERFACE_TABLE_ENTRY *) (UINTN) FitPointer;
+  if ((FitEntry[0].Type != FIT_TYPE_00_HEADER) ||
+      (FitEntry[0].Address != FIT_TYPE_00_SIGNATURE)) {
+    //
+    // Invalid FIT table, treat it as no FIT table.
+    //
+    ASSERT (FALSE);
+    return EFI_NOT_FOUND;
+  }
+
+  EntryNum = *(UINT32 *)(&FitEntry[0].Size[0]) & 0xFFFFFF;
+
+  //
+  // Calculate microcode entry number
+  //
+  MaxPatchNumber = 0;
+  for (Index = 0; Index < EntryNum; Index++) {
+    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
+      MaxPatchNumber++;
+    }
+  }
+  if (MaxPatchNumber == 0) {
+    return EFI_NOT_FOUND;
+  }
+
+  PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
+  if (PatchInfoBuffer == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  //
+  // Fill up microcode patch info buffer according to FIT table.
+  //
+  PatchCount = 0;
+  TotalLoadSize = 0;
+  for (Index = 0; Index < EntryNum; Index++) {
+    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
+      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) FitEntry[Index].Address;
+      TotalSize = (MicrocodeEntryPoint->DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+      if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
+        PatchInfoBuffer[PatchCount].Address     = (UINTN) MicrocodeEntryPoint;
+        PatchInfoBuffer[PatchCount].Size        = TotalSize;
+        TotalLoadSize += TotalSize;
+        PatchCount++;
+      }
+    }
+  }
+
+  if (PatchCount != 0) {
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: 0x%x microcode patches will be loaded into memory, with size 0x%x.\n",
+      __FUNCTION__, PatchCount, TotalLoadSize
+      ));
+
+    ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
+  }
+
+  FreePool (PatchInfoBuffer);
+  return EFI_SUCCESS;
+}
+
+/**
+  Shadow the required microcode patches data into memory.
+
+  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
+**/
+VOID
+ShadowMicrocodeUpdatePatch (
+  IN OUT CPU_MP_DATA             *CpuMpData
+  )
+{
+  EFI_STATUS     Status;
+
+  Status = ShadowMicrocodePatchByFit (CpuMpData);
+  if (EFI_ERROR (Status)) {
+    ShadowMicrocodePatchByPcd (CpuMpData);
+  }
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index e611a8ca40..6ec9b172b8 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1,7 +1,7 @@
 /** @file
   CPU MP Initialize Library common functions.
 
-  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -1744,7 +1744,7 @@ MpInitLibInitialize (
     //
     // Load required microcode patches data into memory
     //
-    LoadMicrocodePatch (CpuMpData);
+    ShadowMicrocodeUpdatePatch (CpuMpData);
   } else {
     //
     // APs have been wakeup before, just get the CPU Information
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b6e5a1afab..7c62d75acc 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -29,6 +29,9 @@
 #include <Library/MtrrLib.h>
 #include <Library/HobLib.h>
 
+#include <IndustryStandard/FirmwareInterfaceTable.h>
+
+
 #define WAKEUP_AP_SIGNAL SIGNATURE_32 ('S', 'T', 'A', 'P')
 
 #define CPU_INIT_MP_LIB_HOB_GUID \
@@ -587,12 +590,12 @@ MicrocodeDetect (
   );
 
 /**
-  Load the required microcode patches data into memory.
+  Shadow the required microcode patches data into memory.
 
   @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
 **/
 VOID
-LoadMicrocodePatch (
+ShadowMicrocodeUpdatePatch (
   IN OUT CPU_MP_DATA             *CpuMpData
   );
 
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 326703cc9a..5b4a1f31c8 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  MP Initialize Library instance for PEI driver.
 #
-#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -60,7 +60,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
-
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ## CONSUMES
 [Guids]
   gEdkiiS3SmmInitDoneGuid
   gEdkiiMicrocodePatchHobGuid
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 45b267ac61..a6ebdde1cf 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -139,6 +139,12 @@
   # @Prompt Lock SMM Feature Control MSR.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B
 
+  ## Indicates if FIT based microcode shadowing will be enabled.<BR><BR>
+  #   TRUE  - FIT base microcode shadowing will be enabled.<BR>
+  #   FALSE - FIT base microcode shadowing will be disabled.<BR>
+  # @Prompt FIT based microcode shadowing.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit|FALSE|BOOLEAN|0x3213210D
+
 [PcdsFixedAtBuild]
   ## List of exception vectors which need switching stack.
   #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
-- 
2.19.1.windows.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.
  2020-01-09  2:14 ` [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
@ 2020-01-09 13:32   ` Laszlo Ersek
  2020-01-10  3:38   ` [edk2-devel] " Dong, Eric
  1 sibling, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2020-01-09 13:32 UTC (permalink / raw)
  To: Siyuan Fu, devel; +Cc: michael.d.kinney, liming.gao, eric.dong, ray.ni

Hi,

On 01/09/20 03:14, Siyuan Fu wrote:
> The existing MpInitLib will shadow the microcode update patches from
> flash to memory and this is done by searching microcode region specified
> by PCD PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
> This brings a limition to platform FW that all the microcode patches must
> be placed in one continuous flash space.
>
> This patch shadows microcode update according to FIT microcode entries if
> it's present, otherwise it will fallback to original logic (by PCD).
>
> A new featured PCD gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit
> is added for enabling/disabling this support.
>
> TEST: Tested on FIT enabled platform.
> BZ: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 262 +++++++++++++-----
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          |   4 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   7 +-
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   4 +-
>  UefiCpuPkg/UefiCpuPkg.dec                     |   6 +
>  6 files changed, 216 insertions(+), 68 deletions(-)

> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 45b267ac61..a6ebdde1cf 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -139,6 +139,12 @@
>    # @Prompt Lock SMM Feature Control MSR.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B
>
> +  ## Indicates if FIT based microcode shadowing will be enabled.<BR><BR>
> +  #   TRUE  - FIT base microcode shadowing will be enabled.<BR>
> +  #   FALSE - FIT base microcode shadowing will be disabled.<BR>
> +  # @Prompt FIT based microcode shadowing.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit|FALSE|BOOLEAN|0x3213210D
> +
>  [PcdsFixedAtBuild]
>    ## List of exception vectors which need switching stack.
>    #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
>

(1) This looks good, but I think you should extend the "UefiCpuPkg.uni"
file accordingly.


> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 326703cc9a..5b4a1f31c8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  MP Initialize Library instance for PEI driver.
>  #
> -#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
>  ##
> @@ -60,7 +60,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ## CONSUMES
>  [Guids]
>    gEdkiiS3SmmInitDoneGuid
>    gEdkiiMicrocodePatchHobGuid


(2) The whitespace update is not ideal here -- I don't think we should
remove the empty line, just before [Guids]. Instead, we should add the
new entry above the empty line (and keep the empty line).


> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index cd912ab0c5..0fd420ac93 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -69,4 +69,5 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ## CONSUMES

(3) I think it would look nicer if we kept the new entry right next to
the other gUefiCpuPkgTokenSpaceGuid.Xxx PCDs. Because as written,
"gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard" is embedded between
multiple gUefiCpuPkgTokenSpaceGuid.Xxx PCDs.


> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index b6e5a1afab..7c62d75acc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -29,6 +29,9 @@
>  #include <Library/MtrrLib.h>
>  #include <Library/HobLib.h>
>
> +#include <IndustryStandard/FirmwareInterfaceTable.h>
> +
> +
>  #define WAKEUP_AP_SIGNAL SIGNATURE_32 ('S', 'T', 'A', 'P')
>
>  #define CPU_INIT_MP_LIB_HOB_GUID \
> @@ -587,12 +590,12 @@ MicrocodeDetect (
>    );
>
>  /**
> -  Load the required microcode patches data into memory.
> +  Shadow the required microcode patches data into memory.
>
>    @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
>  **/
>  VOID
> -LoadMicrocodePatch (
> +ShadowMicrocodeUpdatePatch (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    );
>

OK.


> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e611a8ca40..6ec9b172b8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    CPU MP Initialize Library common functions.
>
> -  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -1744,7 +1744,7 @@ MpInitLibInitialize (
>      //
>      // Load required microcode patches data into memory
>      //
> -    LoadMicrocodePatch (CpuMpData);
> +    ShadowMicrocodeUpdatePatch (CpuMpData);
>    } else {
>      //
>      // APs have been wakeup before, just get the CPU Information

OK.


> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 8f4d4da40c..9389e52ae5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c

(snipping some stuff)

>  /**
> -  Load the required microcode patches data into memory.
> +  Shadow the required microcode patches data into memory according to PCD
> +  PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
>
>    @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
>  **/
>  VOID
> -LoadMicrocodePatch (
> +ShadowMicrocodePatchByPcd (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    )
>  {
> @@ -423,15 +493,10 @@ LoadMicrocodePatch (
>    UINTN                                  MicrocodeEnd;
>    UINTN                                  DataSize;
>    UINTN                                  TotalSize;
> -  CPU_MICROCODE_EXTENDED_TABLE_HEADER    *ExtendedTableHeader;
> -  UINT32                                 ExtendedTableCount;
> -  CPU_MICROCODE_EXTENDED_TABLE           *ExtendedTable;
>    MICROCODE_PATCH_INFO                   *PatchInfoBuffer;
>    UINTN                                  MaxPatchNumber;
>    UINTN                                  PatchCount;
>    UINTN                                  TotalLoadSize;
> -  UINTN                                  Index;
> -  BOOLEAN                                NeedLoad;
>
>    //
>    // Initialize the microcode patch related fields in CpuMpData as the values
> @@ -487,55 +552,7 @@ LoadMicrocodePatch (
>        continue;
>      }
>

OK.

> +/**
> +  Shadow the required microcode patches data into memory according to FIT microcode entry.
> +
> +  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
> +
> +  @return EFI_SUCCESS           Microcode patch is shadowed into memory.
> +  @return EFI_UNSUPPORTED       FIT based microcode shadowing is not supported.
> +  @return EFI_OUT_OF_RESOURCES  No enough memory resource.
> +  @return EFI_NOT_FOUND         There is something wrong in FIT microcode entry.
> +
> +**/
> +EFI_STATUS
> +ShadowMicrocodePatchByFit (
> +  IN OUT CPU_MP_DATA             *CpuMpData
> +  )
> +{
> +  UINT64                            FitPointer;
> +  FIRMWARE_INTERFACE_TABLE_ENTRY    *FitEntry;
> +  UINT32                            EntryNum;
> +  UINT32                            Index;
> +  MICROCODE_PATCH_INFO              *PatchInfoBuffer;
> +  UINTN                             MaxPatchNumber;
> +  CPU_MICROCODE_HEADER              *MicrocodeEntryPoint;
> +  UINTN                             PatchCount;
> +  UINTN                             TotalSize;
> +  UINTN                             TotalLoadSize;
> +
> +  if (!FeaturePcdGet (PcdCpuShadowMicrocodeByFit)) {
> +    return EFI_UNSUPPORTED;
> +  }

OK.

> +
> +  FitPointer = *(UINT64 *) (UINTN) FIT_POINTER_ADDRESS;
> +  if ((FitPointer == 0) ||
> +      (FitPointer == 0xFFFFFFFFFFFFFFFF) ||
> +      (FitPointer == 0xEEEEEEEEEEEEEEEE)) {
> +    //
> +    // No FIT table.
> +    //
> +    ASSERT (FALSE);
> +    return EFI_NOT_FOUND;
> +  }
> +  FitEntry = (FIRMWARE_INTERFACE_TABLE_ENTRY *) (UINTN) FitPointer;
> +  if ((FitEntry[0].Type != FIT_TYPE_00_HEADER) ||
> +      (FitEntry[0].Address != FIT_TYPE_00_SIGNATURE)) {
> +    //
> +    // Invalid FIT table, treat it as no FIT table.
> +    //
> +    ASSERT (FALSE);
> +    return EFI_NOT_FOUND;
> +  }

OK.

(Snipping the rest of ShadowMicrocodePatchByFit().)


> +/**
> +  Shadow the required microcode patches data into memory.
> +
> +  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
> +**/
> +VOID
> +ShadowMicrocodeUpdatePatch (
> +  IN OUT CPU_MP_DATA             *CpuMpData
> +  )
> +{
> +  EFI_STATUS     Status;
> +
> +  Status = ShadowMicrocodePatchByFit (CpuMpData);
> +  if (EFI_ERROR (Status)) {
> +    ShadowMicrocodePatchByPcd (CpuMpData);
> +  }
> +}

OK.

With (1) through (3) fixed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.
  2020-01-09  2:14 ` [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
@ 2020-01-10  3:09   ` Liming Gao
  2020-01-10 10:41     ` Ni, Ray
  0 siblings, 1 reply; 8+ messages in thread
From: Liming Gao @ 2020-01-10  3:09 UTC (permalink / raw)
  To: Fu, Siyuan, devel@edk2.groups.io
  Cc: Kinney, Michael D, Dong, Eric, Ni, Ray, lersek@redhat.com

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Fu, Siyuan <siyuan.fu@intel.com>
> Sent: Thursday, January 9, 2020 10:14 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; lersek@redhat.com
> Subject: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.
> 
> This patch add FirmwareInterfaceTable.h for the Firmware Interface Table
> BIOS specification.
> 
> This is to remove future edk2 dependency on edk2-platforms repo. The file
> content comes from
>  edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\IndustryStandard
> 
> BZ link: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> ---
>  .../IndustryStandard/FirmwareInterfaceTable.h | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> 
> diff --git a/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> new file mode 100644
> index 0000000000..be3e34ae1b
> --- /dev/null
> +++ b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> @@ -0,0 +1,76 @@
> +/** @file
> +  Industry Standard Definitions of Firmware Interface Table BIOS Specification 1.0.
> +
> +  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __FIRMWARE_INTERFACE_TABLE_H__
> +#define __FIRMWARE_INTERFACE_TABLE_H__
> +
> +//
> +// FIT Entry type definitions
> +//
> +#define FIT_TYPE_00_HEADER                  0x00
> +#define FIT_TYPE_01_MICROCODE               0x01
> +#define FIT_TYPE_02_STARTUP_ACM             0x02
> +#define FIT_TYPE_07_BIOS_STARTUP_MODULE     0x07
> +#define FIT_TYPE_08_TPM_POLICY              0x08
> +#define FIT_TYPE_09_BIOS_POLICY             0x09
> +#define FIT_TYPE_0A_TXT_POLICY              0x0A
> +#define FIT_TYPE_0B_KEY_MANIFEST            0x0B
> +#define FIT_TYPE_0C_BOOT_POLICY_MANIFEST    0x0C
> +#define FIT_TYPE_10_CSE_SECURE_BOOT         0x10
> +#define FIT_TYPE_2D_TXTSX_POLICY            0x2D
> +#define FIT_TYPE_2F_JMP_DEBUG_POLICY        0x2F
> +#define FIT_TYPE_7F_SKIP                    0x7F
> +
> +#define FIT_POINTER_ADDRESS                 0xFFFFFFC0 ///< Fixed address at 4G - 40h
> +
> +#define FIT_TYPE_VERSION                    0x0100
> +
> +#define FIT_TYPE_00_SIGNATURE  SIGNATURE_64 ('_', 'F', 'I', 'T', '_', ' ', ' ', ' ')
> +
> +#pragma pack(1)
> +
> +typedef struct {
> +  //
> +  // Address is the base address of the firmware component
> +  // must be aligned on 16 byte boundary
> +  //
> +  UINT64 Address;
> +  //
> +  // Size is the span of the component in multiple of 16 bytes
> +  //
> +  UINT8  Size[3];
> +  //
> +  // Reserved must be set to 0
> +  //
> +  UINT8  Reserved;
> +  //
> +  // Component's version number in binary coded decimal (BCD) format.
> +  // For the FIT header entry, the value in this field will indicate the revision
> +  // number of the FIT data structure. The upper byte of the revision field
> +  // indicates the major revision and the lower byte indicates the minor revision.
> +  //
> +  UINT16 Version;
> +  //
> +  // FIT types 0x00 to 0x7F
> +  //
> +  UINT8  Type : 7;
> +  //
> +  // Checksum Valid indicates whether component has valid checksum.
> +  //
> +  UINT8  C_V  : 1;
> +  //
> +  // Component's checksum. The modulo sum of all the bytes in the component and
> +  // the value in this field (Chksum) must add up to zero. This field is only
> +  // valid if the C_V flag is non-zero.
> +  //
> +  UINT8  Chksum;
> +} FIRMWARE_INTERFACE_TABLE_ENTRY;
> +
> +#pragma pack()
> +
> +#endif
> --
> 2.19.1.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.
  2020-01-09  2:14 ` [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
  2020-01-09 13:32   ` Laszlo Ersek
@ 2020-01-10  3:38   ` Dong, Eric
  1 sibling, 0 replies; 8+ messages in thread
From: Dong, Eric @ 2020-01-10  3:38 UTC (permalink / raw)
  To: devel@edk2.groups.io, Fu, Siyuan
  Cc: Kinney, Michael D, Gao, Liming, Ni, Ray, lersek@redhat.com

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Siyuan, Fu
> Sent: Thursday, January 9, 2020 10:14 AM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> <ray.ni@intel.com>; lersek@redhat.com
> Subject: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch
> according to FIT microcode entry.
> 
> The existing MpInitLib will shadow the microcode update patches from flash
> to memory and this is done by searching microcode region specified by PCD
> PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize.
> This brings a limition to platform FW that all the microcode patches must be
> placed in one continuous flash space.
> 
> This patch shadows microcode update according to FIT microcode entries if
> it's present, otherwise it will fallback to original logic (by PCD).
> 
> A new featured PCD
> gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit
> is added for enabling/disabling this support.
> 
> TEST: Tested on FIT enabled platform.
> BZ: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 262 +++++++++++++-----
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          |   4 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   7 +-
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   4 +-
>  UefiCpuPkg/UefiCpuPkg.dec                     |   6 +
>  6 files changed, 216 insertions(+), 68 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index cd912ab0c5..0fd420ac93 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -69,4 +69,5 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ##
> CONSUMES
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 8f4d4da40c..9389e52ae5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -318,7 +318,7 @@ Done:
>  }
> 
>  /**
> -  Determine if a microcode patch will be loaded into memory.
> +  Determine if a microcode patch matchs the specific processor signature
> and flag.
> 
>    @param[in]  CpuMpData             The pointer to CPU MP Data structure.
>    @param[in]  ProcessorSignature    The processor signature field value
> @@ -330,7 +330,7 @@ Done:
>    @retval FALSE    The specified microcode patch will not be loaded.
>  **/
>  BOOLEAN
> -IsMicrocodePatchNeedLoad (
> +IsProcessorMatchedMicrocodePatch (
>    IN CPU_MP_DATA                 *CpuMpData,
>    IN UINT32                      ProcessorSignature,
>    IN UINT32                      ProcessorFlags
> @@ -351,7 +351,77 @@ IsMicrocodePatchNeedLoad (  }
> 
>  /**
> -  Actual worker function that loads the required microcode patches into
> memory.
> +  Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
> + patch header with the CPUID and PlatformID of the processors within
> + system to decide if it will be copied into memory.
> +
> +  @param[in]  CpuMpData             The pointer to CPU MP Data structure.
> +  @param[in]  MicrocodeEntryPoint   The pointer to the microcode patch
> header.
> +
> +  @retval TRUE     The specified microcode patch need to be loaded.
> +  @retval FALSE    The specified microcode patch dosen't need to be loaded.
> +**/
> +BOOLEAN
> +IsMicrocodePatchNeedLoad (
> +  IN CPU_MP_DATA                 *CpuMpData,
> +  CPU_MICROCODE_HEADER           *MicrocodeEntryPoint
> +  )
> +{
> +  BOOLEAN                                NeedLoad;
> +  UINTN                                  DataSize;
> +  UINTN                                  TotalSize;
> +  CPU_MICROCODE_EXTENDED_TABLE_HEADER    *ExtendedTableHeader;
> +  UINT32                                 ExtendedTableCount;
> +  CPU_MICROCODE_EXTENDED_TABLE           *ExtendedTable;
> +  UINTN                                  Index;
> +
> +  //
> +  // Check the 'ProcessorSignature' and 'ProcessorFlags' in microcode patch
> header.
> +  //
> +  NeedLoad = IsProcessorMatchedMicrocodePatch (
> +               CpuMpData,
> +               MicrocodeEntryPoint->ProcessorSignature.Uint32,
> +               MicrocodeEntryPoint->ProcessorFlags
> +               );
> +
> +  //
> +  // If the Extended Signature Table exists, check if the processor is
> + in the  // support list  //  DataSize  =
> + MicrocodeEntryPoint->DataSize;  TotalSize = (DataSize == 0) ? 2048 :
> + MicrocodeEntryPoint->TotalSize;  if ((!NeedLoad) && (DataSize != 0) &&
> +      (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
> +                              sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
> +    ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER
> *) ((UINT8 *) (MicrocodeEntryPoint)
> +                            + DataSize + sizeof (CPU_MICROCODE_HEADER));
> +    ExtendedTableCount  = ExtendedTableHeader->ExtendedSignatureCount;
> +    ExtendedTable       = (CPU_MICROCODE_EXTENDED_TABLE *)
> (ExtendedTableHeader + 1);
> +
> +    for (Index = 0; Index < ExtendedTableCount; Index ++) {
> +      //
> +      // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
> +      // Signature Table entry with the CPUID and PlatformID of the
> processors
> +      // within system to decide if it will be copied into memory
> +      //
> +      NeedLoad = IsProcessorMatchedMicrocodePatch (
> +                   CpuMpData,
> +                   ExtendedTable->ProcessorSignature.Uint32,
> +                   ExtendedTable->ProcessorFlag
> +                   );
> +      if (NeedLoad) {
> +        break;
> +      }
> +      ExtendedTable ++;
> +    }
> +  }
> +
> +  return NeedLoad;
> +}
> +
> +
> +/**
> +  Actual worker function that shadows the required microcode patches into
> memory.
> 
>    @param[in, out]  CpuMpData        The pointer to CPU MP Data structure.
>    @param[in]       Patches          The pointer to an array of information on
> @@ -363,7 +433,7 @@ IsMicrocodePatchNeedLoad (
>                                      to be loaded.
>  **/
>  VOID
> -LoadMicrocodePatchWorker (
> +ShadowMicrocodePatchWorker (
>    IN OUT CPU_MP_DATA             *CpuMpData,
>    IN     MICROCODE_PATCH_INFO    *Patches,
>    IN     UINTN                   PatchCount,
> @@ -390,7 +460,6 @@ LoadMicrocodePatchWorker (
>        (VOID *) Patches[Index].Address,
>        Patches[Index].Size
>        );
> -
>      Walker += Patches[Index].Size;
>    }
> 
> @@ -410,12 +479,13 @@ LoadMicrocodePatchWorker (  }
> 
>  /**
> -  Load the required microcode patches data into memory.
> +  Shadow the required microcode patches data into memory according to
> + PCD  PcdCpuMicrocodePatchAddress and
> PcdCpuMicrocodePatchRegionSize.
> 
>    @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
>  **/
>  VOID
> -LoadMicrocodePatch (
> +ShadowMicrocodePatchByPcd (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    )
>  {
> @@ -423,15 +493,10 @@ LoadMicrocodePatch (
>    UINTN                                  MicrocodeEnd;
>    UINTN                                  DataSize;
>    UINTN                                  TotalSize;
> -  CPU_MICROCODE_EXTENDED_TABLE_HEADER    *ExtendedTableHeader;
> -  UINT32                                 ExtendedTableCount;
> -  CPU_MICROCODE_EXTENDED_TABLE           *ExtendedTable;
>    MICROCODE_PATCH_INFO                   *PatchInfoBuffer;
>    UINTN                                  MaxPatchNumber;
>    UINTN                                  PatchCount;
>    UINTN                                  TotalLoadSize;
> -  UINTN                                  Index;
> -  BOOLEAN                                NeedLoad;
> 
>    //
>    // Initialize the microcode patch related fields in CpuMpData as the values
> @@ -487,55 +552,7 @@ LoadMicrocodePatch (
>        continue;
>      }
> 
> -    //
> -    // Check the 'ProcessorSignature' and 'ProcessorFlags' of the microcode
> -    // patch header with the CPUID and PlatformID of the processors within
> -    // system to decide if it will be copied into memory
> -    //
> -    NeedLoad = IsMicrocodePatchNeedLoad (
> -                 CpuMpData,
> -                 MicrocodeEntryPoint->ProcessorSignature.Uint32,
> -                 MicrocodeEntryPoint->ProcessorFlags
> -                 );
> -
> -    //
> -    // If the Extended Signature Table exists, check if the processor is in the
> -    // support list
> -    //
> -    if ((!NeedLoad) && (DataSize != 0) &&
> -        (TotalSize - DataSize > sizeof (CPU_MICROCODE_HEADER) +
> -                                sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
> -      ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER
> *) ((UINT8 *) (MicrocodeEntryPoint)
> -                              + DataSize + sizeof (CPU_MICROCODE_HEADER));
> -      ExtendedTableCount  = ExtendedTableHeader-
> >ExtendedSignatureCount;
> -      ExtendedTable       = (CPU_MICROCODE_EXTENDED_TABLE *)
> (ExtendedTableHeader + 1);
> -
> -      for (Index = 0; Index < ExtendedTableCount; Index ++) {
> -        //
> -        // Avoid access content beyond MicrocodeEnd
> -        //
> -        if ((UINTN) ExtendedTable > MicrocodeEnd - sizeof
> (CPU_MICROCODE_EXTENDED_TABLE)) {
> -          break;
> -        }
> -
> -        //
> -        // Check the 'ProcessorSignature' and 'ProcessorFlag' of the Extended
> -        // Signature Table entry with the CPUID and PlatformID of the
> processors
> -        // within system to decide if it will be copied into memory
> -        //
> -        NeedLoad = IsMicrocodePatchNeedLoad (
> -                     CpuMpData,
> -                     ExtendedTable->ProcessorSignature.Uint32,
> -                     ExtendedTable->ProcessorFlag
> -                     );
> -        if (NeedLoad) {
> -          break;
> -        }
> -        ExtendedTable ++;
> -      }
> -    }
> -
> -    if (NeedLoad) {
> +    if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
>        PatchCount++;
>        if (PatchCount > MaxPatchNumber) {
>          //
> @@ -581,7 +598,7 @@ LoadMicrocodePatch (
>        __FUNCTION__, PatchCount, TotalLoadSize
>        ));
> 
> -    LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount,
> TotalLoadSize);
> +    ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer,
> PatchCount,
> + TotalLoadSize);
>    }
> 
>  OnExit:
> @@ -590,3 +607,124 @@ OnExit:
>    }
>    return;
>  }
> +
> +/**
> +  Shadow the required microcode patches data into memory according to
> FIT microcode entry.
> +
> +  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
> +
> +  @return EFI_SUCCESS           Microcode patch is shadowed into memory.
> +  @return EFI_UNSUPPORTED       FIT based microcode shadowing is not
> supported.
> +  @return EFI_OUT_OF_RESOURCES  No enough memory resource.
> +  @return EFI_NOT_FOUND         There is something wrong in FIT microcode
> entry.
> +
> +**/
> +EFI_STATUS
> +ShadowMicrocodePatchByFit (
> +  IN OUT CPU_MP_DATA             *CpuMpData
> +  )
> +{
> +  UINT64                            FitPointer;
> +  FIRMWARE_INTERFACE_TABLE_ENTRY    *FitEntry;
> +  UINT32                            EntryNum;
> +  UINT32                            Index;
> +  MICROCODE_PATCH_INFO              *PatchInfoBuffer;
> +  UINTN                             MaxPatchNumber;
> +  CPU_MICROCODE_HEADER              *MicrocodeEntryPoint;
> +  UINTN                             PatchCount;
> +  UINTN                             TotalSize;
> +  UINTN                             TotalLoadSize;
> +
> +  if (!FeaturePcdGet (PcdCpuShadowMicrocodeByFit)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  FitPointer = *(UINT64 *) (UINTN) FIT_POINTER_ADDRESS;  if
> + ((FitPointer == 0) ||
> +      (FitPointer == 0xFFFFFFFFFFFFFFFF) ||
> +      (FitPointer == 0xEEEEEEEEEEEEEEEE)) {
> +    //
> +    // No FIT table.
> +    //
> +    ASSERT (FALSE);
> +    return EFI_NOT_FOUND;
> +  }
> +  FitEntry = (FIRMWARE_INTERFACE_TABLE_ENTRY *) (UINTN) FitPointer;  if
> + ((FitEntry[0].Type != FIT_TYPE_00_HEADER) ||
> +      (FitEntry[0].Address != FIT_TYPE_00_SIGNATURE)) {
> +    //
> +    // Invalid FIT table, treat it as no FIT table.
> +    //
> +    ASSERT (FALSE);
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  EntryNum = *(UINT32 *)(&FitEntry[0].Size[0]) & 0xFFFFFF;
> +
> +  //
> +  // Calculate microcode entry number
> +  //
> +  MaxPatchNumber = 0;
> +  for (Index = 0; Index < EntryNum; Index++) {
> +    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
> +      MaxPatchNumber++;
> +    }
> +  }
> +  if (MaxPatchNumber == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof
> + (MICROCODE_PATCH_INFO));  if (PatchInfoBuffer == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Fill up microcode patch info buffer according to FIT table.
> +  //
> +  PatchCount = 0;
> +  TotalLoadSize = 0;
> +  for (Index = 0; Index < EntryNum; Index++) {
> +    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE) {
> +      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN)
> FitEntry[Index].Address;
> +      TotalSize = (MicrocodeEntryPoint->DataSize == 0) ? 2048 :
> MicrocodeEntryPoint->TotalSize;
> +      if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
> +        PatchInfoBuffer[PatchCount].Address     = (UINTN)
> MicrocodeEntryPoint;
> +        PatchInfoBuffer[PatchCount].Size        = TotalSize;
> +        TotalLoadSize += TotalSize;
> +        PatchCount++;
> +      }
> +    }
> +  }
> +
> +  if (PatchCount != 0) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "%a: 0x%x microcode patches will be loaded into memory, with size
> 0x%x.\n",
> +      __FUNCTION__, PatchCount, TotalLoadSize
> +      ));
> +
> +    ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer,
> PatchCount,
> + TotalLoadSize);  }
> +
> +  FreePool (PatchInfoBuffer);
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Shadow the required microcode patches data into memory.
> +
> +  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
> +**/
> +VOID
> +ShadowMicrocodeUpdatePatch (
> +  IN OUT CPU_MP_DATA             *CpuMpData
> +  )
> +{
> +  EFI_STATUS     Status;
> +
> +  Status = ShadowMicrocodePatchByFit (CpuMpData);
> +  if (EFI_ERROR (Status)) {
> +    ShadowMicrocodePatchByPcd (CpuMpData);
> +  }
> +}
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e611a8ca40..6ec9b172b8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    CPU MP Initialize Library common functions.
> 
> -  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2020, Intel Corporation. All rights
> + reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> @@ -1744,7 +1744,7 @@ MpInitLibInitialize (
>      //
>      // Load required microcode patches data into memory
>      //
> -    LoadMicrocodePatch (CpuMpData);
> +    ShadowMicrocodeUpdatePatch (CpuMpData);
>    } else {
>      //
>      // APs have been wakeup before, just get the CPU Information diff --git
> a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index b6e5a1afab..7c62d75acc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -29,6 +29,9 @@
>  #include <Library/MtrrLib.h>
>  #include <Library/HobLib.h>
> 
> +#include <IndustryStandard/FirmwareInterfaceTable.h>
> +
> +
>  #define WAKEUP_AP_SIGNAL SIGNATURE_32 ('S', 'T', 'A', 'P')
> 
>  #define CPU_INIT_MP_LIB_HOB_GUID \
> @@ -587,12 +590,12 @@ MicrocodeDetect (
>    );
> 
>  /**
> -  Load the required microcode patches data into memory.
> +  Shadow the required microcode patches data into memory.
> 
>    @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
>  **/
>  VOID
> -LoadMicrocodePatch (
> +ShadowMicrocodeUpdatePatch (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    );
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 326703cc9a..5b4a1f31c8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  MP Initialize Library instance for PEI driver.
>  #
> -#  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2016 - 2020, Intel Corporation. All rights
> +reserved.<BR>
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent  #  ## @@ -60,7 +60,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit             ##
> CONSUMES
>  [Guids]
>    gEdkiiS3SmmInitDoneGuid
>    gEdkiiMicrocodePatchHobGuid
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 45b267ac61..a6ebdde1cf 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -139,6 +139,12 @@
>    # @Prompt Lock SMM Feature Control MSR.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|B
> OOLEAN|0x3213210B
> 
> +  ## Indicates if FIT based microcode shadowing will be enabled.<BR><BR>
> +  #   TRUE  - FIT base microcode shadowing will be enabled.<BR>
> +  #   FALSE - FIT base microcode shadowing will be disabled.<BR>
> +  # @Prompt FIT based microcode shadowing.
> +
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit|FALSE|BOOLE
> AN|0x3
> + 213210D
> +
>  [PcdsFixedAtBuild]
>    ## List of exception vectors which need switching stack.
>    #  This PCD will only take into effect if PcdCpuStackGuard is enabled.
> --
> 2.19.1.windows.1
> 
> 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.
  2020-01-10  3:09   ` Liming Gao
@ 2020-01-10 10:41     ` Ni, Ray
  2020-01-13  0:56       ` Liming Gao
  0 siblings, 1 reply; 8+ messages in thread
From: Ni, Ray @ 2020-01-10 10:41 UTC (permalink / raw)
  To: Kinney, Michael D
  Cc: Dong, Eric, lersek@redhat.com, Gao, Liming, Fu, Siyuan,
	devel@edk2.groups.io

Siyuan,
FIT is Intel-only interface that's not consumed by other vendors. We may need to keep it in IntelSiliconPkg.
I proposed this movement earlier in offline discussion with you but based on new discussion with Mike, I withdrew my proposal. Sorry about that!

+ Mike for additional comments.

Thanks,
Ray

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Friday, January 10, 2020 11:09 AM
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Subject: RE: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface
> Table specification.
> 
> Reviewed-by: Liming Gao <liming.gao@intel.com>
> 
> > -----Original Message-----
> > From: Fu, Siyuan <siyuan.fu@intel.com>
> > Sent: Thursday, January 9, 2020 10:14 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; lersek@redhat.com
> > Subject: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table
> specification.
> >
> > This patch add FirmwareInterfaceTable.h for the Firmware Interface Table
> > BIOS specification.
> >
> > This is to remove future edk2 dependency on edk2-platforms repo. The file
> > content comes from
> >  edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\IndustryStandard
> >
> > BZ link: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> > ---
> >  .../IndustryStandard/FirmwareInterfaceTable.h | 76 +++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644
> MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> >
> > diff --git a/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> > new file mode 100644
> > index 0000000000..be3e34ae1b
> > --- /dev/null
> > +++ b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> > @@ -0,0 +1,76 @@
> > +/** @file
> > +  Industry Standard Definitions of Firmware Interface Table BIOS Specification
> 1.0.
> > +
> > +  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef __FIRMWARE_INTERFACE_TABLE_H__
> > +#define __FIRMWARE_INTERFACE_TABLE_H__
> > +
> > +//
> > +// FIT Entry type definitions
> > +//
> > +#define FIT_TYPE_00_HEADER                  0x00
> > +#define FIT_TYPE_01_MICROCODE               0x01
> > +#define FIT_TYPE_02_STARTUP_ACM             0x02
> > +#define FIT_TYPE_07_BIOS_STARTUP_MODULE     0x07
> > +#define FIT_TYPE_08_TPM_POLICY              0x08
> > +#define FIT_TYPE_09_BIOS_POLICY             0x09
> > +#define FIT_TYPE_0A_TXT_POLICY              0x0A
> > +#define FIT_TYPE_0B_KEY_MANIFEST            0x0B
> > +#define FIT_TYPE_0C_BOOT_POLICY_MANIFEST    0x0C
> > +#define FIT_TYPE_10_CSE_SECURE_BOOT         0x10
> > +#define FIT_TYPE_2D_TXTSX_POLICY            0x2D
> > +#define FIT_TYPE_2F_JMP_DEBUG_POLICY        0x2F
> > +#define FIT_TYPE_7F_SKIP                    0x7F
> > +
> > +#define FIT_POINTER_ADDRESS                 0xFFFFFFC0 ///< Fixed address at 4G
> - 40h
> > +
> > +#define FIT_TYPE_VERSION                    0x0100
> > +
> > +#define FIT_TYPE_00_SIGNATURE  SIGNATURE_64 ('_', 'F', 'I', 'T', '_', ' ', ' ', ' ')
> > +
> > +#pragma pack(1)
> > +
> > +typedef struct {
> > +  //
> > +  // Address is the base address of the firmware component
> > +  // must be aligned on 16 byte boundary
> > +  //
> > +  UINT64 Address;
> > +  //
> > +  // Size is the span of the component in multiple of 16 bytes
> > +  //
> > +  UINT8  Size[3];
> > +  //
> > +  // Reserved must be set to 0
> > +  //
> > +  UINT8  Reserved;
> > +  //
> > +  // Component's version number in binary coded decimal (BCD) format.
> > +  // For the FIT header entry, the value in this field will indicate the revision
> > +  // number of the FIT data structure. The upper byte of the revision field
> > +  // indicates the major revision and the lower byte indicates the minor
> revision.
> > +  //
> > +  UINT16 Version;
> > +  //
> > +  // FIT types 0x00 to 0x7F
> > +  //
> > +  UINT8  Type : 7;
> > +  //
> > +  // Checksum Valid indicates whether component has valid checksum.
> > +  //
> > +  UINT8  C_V  : 1;
> > +  //
> > +  // Component's checksum. The modulo sum of all the bytes in the
> component and
> > +  // the value in this field (Chksum) must add up to zero. This field is only
> > +  // valid if the C_V flag is non-zero.
> > +  //
> > +  UINT8  Chksum;
> > +} FIRMWARE_INTERFACE_TABLE_ENTRY;
> > +
> > +#pragma pack()
> > +
> > +#endif
> > --
> > 2.19.1.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.
  2020-01-10 10:41     ` Ni, Ray
@ 2020-01-13  0:56       ` Liming Gao
  0 siblings, 0 replies; 8+ messages in thread
From: Liming Gao @ 2020-01-13  0:56 UTC (permalink / raw)
  To: Ni, Ray, Kinney, Michael D
  Cc: Dong, Eric, lersek@redhat.com, Fu, Siyuan, devel@edk2.groups.io

Ray:
  MdePkg has included Intel specific register. You can see MdePkg\Include\Register\Intel and MdePkg\Include\Register\AMD. 
  And, MdePkg also includes the vendor specific definition in MdePkg\Include\IndustryStandard directory, such as Intel ServiceProcessorManagementInterfaceTable.h and Microsoft SerialPortConsoleRedirectionTable.h

  So, I think MdePkg allows to add vendor specific definition. Then, I give my Review-By. 

Thanks
Liming
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: 2020年1月10日 18:41
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; lersek@redhat.com; Gao, Liming <liming.gao@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
Subject: RE: [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification.

Siyuan,
FIT is Intel-only interface that's not consumed by other vendors. We may need to keep it in IntelSiliconPkg.
I proposed this movement earlier in offline discussion with you but based on new discussion with Mike, I withdrew my proposal. Sorry about that!

+ Mike for additional comments.

Thanks,
Ray

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Friday, January 10, 2020 11:09 AM
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric 
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com
> Subject: RE: [PATCH v2 1/2] MdePkg: Add header file for Firmware 
> Interface Table specification.
> 
> Reviewed-by: Liming Gao <liming.gao@intel.com>
> 
> > -----Original Message-----
> > From: Fu, Siyuan <siyuan.fu@intel.com>
> > Sent: Thursday, January 9, 2020 10:14 AM
> > To: devel@edk2.groups.io
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; lersek@redhat.com
> > Subject: [PATCH v2 1/2] MdePkg: Add header file for Firmware 
> > Interface Table
> specification.
> >
> > This patch add FirmwareInterfaceTable.h for the Firmware Interface 
> > Table BIOS specification.
> >
> > This is to remove future edk2 dependency on edk2-platforms repo. The 
> > file content comes from  
> > edk2-platforms\Silicon\Intel\IntelSiliconPkg\Include\IndustryStandar
> > d
> >
> > BZ link: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> > ---
> >  .../IndustryStandard/FirmwareInterfaceTable.h | 76 
> > +++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644
> MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> >
> > diff --git 
> > a/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> > new file mode 100644
> > index 0000000000..be3e34ae1b
> > --- /dev/null
> > +++ b/MdePkg/Include/IndustryStandard/FirmwareInterfaceTable.h
> > @@ -0,0 +1,76 @@
> > +/** @file
> > +  Industry Standard Definitions of Firmware Interface Table BIOS 
> > +Specification
> 1.0.
> > +
> > +  Copyright (c) 2016 - 2020, Intel Corporation. All rights 
> > + reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#ifndef __FIRMWARE_INTERFACE_TABLE_H__ #define 
> > +__FIRMWARE_INTERFACE_TABLE_H__
> > +
> > +//
> > +// FIT Entry type definitions
> > +//
> > +#define FIT_TYPE_00_HEADER                  0x00
> > +#define FIT_TYPE_01_MICROCODE               0x01
> > +#define FIT_TYPE_02_STARTUP_ACM             0x02
> > +#define FIT_TYPE_07_BIOS_STARTUP_MODULE     0x07
> > +#define FIT_TYPE_08_TPM_POLICY              0x08
> > +#define FIT_TYPE_09_BIOS_POLICY             0x09
> > +#define FIT_TYPE_0A_TXT_POLICY              0x0A
> > +#define FIT_TYPE_0B_KEY_MANIFEST            0x0B
> > +#define FIT_TYPE_0C_BOOT_POLICY_MANIFEST    0x0C
> > +#define FIT_TYPE_10_CSE_SECURE_BOOT         0x10
> > +#define FIT_TYPE_2D_TXTSX_POLICY            0x2D
> > +#define FIT_TYPE_2F_JMP_DEBUG_POLICY        0x2F
> > +#define FIT_TYPE_7F_SKIP                    0x7F
> > +
> > +#define FIT_POINTER_ADDRESS                 0xFFFFFFC0 ///< Fixed address at 4G
> - 40h
> > +
> > +#define FIT_TYPE_VERSION                    0x0100
> > +
> > +#define FIT_TYPE_00_SIGNATURE  SIGNATURE_64 ('_', 'F', 'I', 'T', 
> > +'_', ' ', ' ', ' ')
> > +
> > +#pragma pack(1)
> > +
> > +typedef struct {
> > +  //
> > +  // Address is the base address of the firmware component
> > +  // must be aligned on 16 byte boundary
> > +  //
> > +  UINT64 Address;
> > +  //
> > +  // Size is the span of the component in multiple of 16 bytes
> > +  //
> > +  UINT8  Size[3];
> > +  //
> > +  // Reserved must be set to 0
> > +  //
> > +  UINT8  Reserved;
> > +  //
> > +  // Component's version number in binary coded decimal (BCD) format.
> > +  // For the FIT header entry, the value in this field will 
> > +indicate the revision
> > +  // number of the FIT data structure. The upper byte of the 
> > +revision field
> > +  // indicates the major revision and the lower byte indicates the 
> > +minor
> revision.
> > +  //
> > +  UINT16 Version;
> > +  //
> > +  // FIT types 0x00 to 0x7F
> > +  //
> > +  UINT8  Type : 7;
> > +  //
> > +  // Checksum Valid indicates whether component has valid checksum.
> > +  //
> > +  UINT8  C_V  : 1;
> > +  //
> > +  // Component's checksum. The modulo sum of all the bytes in the
> component and
> > +  // the value in this field (Chksum) must add up to zero. This 
> > +field is only
> > +  // valid if the C_V flag is non-zero.
> > +  //
> > +  UINT8  Chksum;
> > +} FIRMWARE_INTERFACE_TABLE_ENTRY;
> > +
> > +#pragma pack()
> > +
> > +#endif
> > --
> > 2.19.1.windows.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-01-13  0:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-09  2:14 [PATCH v2 0/2] Shadow microcode patch according to FIT microcode table Siyuan, Fu
2020-01-09  2:14 ` [PATCH v2 1/2] MdePkg: Add header file for Firmware Interface Table specification Siyuan, Fu
2020-01-10  3:09   ` Liming Gao
2020-01-10 10:41     ` Ni, Ray
2020-01-13  0:56       ` Liming Gao
2020-01-09  2:14 ` [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry Siyuan, Fu
2020-01-09 13:32   ` Laszlo Ersek
2020-01-10  3:38   ` [edk2-devel] " Dong, Eric

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox