public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
@ 2021-04-02  5:58 Ni, Ray
  2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ni, Ray @ 2021-04-02  5:58 UTC (permalink / raw)
  To: devel

The patch set creates a new MicrocodeLib for loading microcode.
Then updates all platforms to include this lib in DSC.
Then updates the MpInitLib to consume this lib.

Edk2-platforms change will be sent out in a separate patch set.


Ray Ni (4):
  UefiCpuPkg: Add MicrocodeLib for loading microcode
  OvmfPkg: Add MicrocodeLib in DSC files.
  UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
  UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code

 OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
 OvmfPkg/Bhyve/BhyveX64.dsc                    |   1 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
 OvmfPkg/OvmfPkgX64.dsc                        |   1 +
 OvmfPkg/OvmfXen.dsc                           |   1 +
 UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++
 .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++
 .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
 UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
 UefiPayloadPkg/UefiPayloadPkg.dsc             |   1 +
 16 files changed, 582 insertions(+), 392 deletions(-)
 create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

-- 
2.27.0.windows.1


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

* [PATCH 1/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
@ 2021-04-02  5:58 ` Ni, Ray
  2021-04-07 13:05   ` [edk2-devel] " Laszlo Ersek
  2021-04-08  2:19   ` Dong, Eric
  2021-04-02  5:58 ` [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files Ni, Ray
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 14+ messages in thread
From: Ni, Ray @ 2021-04-02  5:58 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Laszlo Ersek, Rahul Kumar

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++++
 .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++++++++
 .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
 UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
 UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
 5 files changed, 479 insertions(+), 1 deletion(-)
 create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

diff --git a/UefiCpuPkg/Include/Library/MicrocodeLib.h b/UefiCpuPkg/Include/Library/MicrocodeLib.h
new file mode 100644
index 0000000000..2570c43cce
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/MicrocodeLib.h
@@ -0,0 +1,120 @@
+/** @file
+  Public include file for Microcode library.
+
+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __MICROCODE_LIB_H__
+#define __MICROCODE_LIB_H__
+
+#include <Register/Intel/Microcode.h>
+#include <Ppi/ShadowMicrocode.h>
+
+/**
+  Get microcode update signature of currently loaded microcode update.
+
+  @return  Microcode signature.
+**/
+UINT32
+EFIAPI
+GetProcessorMicrocodeSignature (
+  VOID
+  );
+
+/**
+  Get the processor signature and platform ID for current processor.
+
+  @param MicrocodeCpuId  Return the processor signature and platform ID.
+**/
+VOID
+EFIAPI
+GetProcessorMicrocodeCpuId (
+  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId
+  );
+
+/**
+  Return the total size of the microcode entry.
+
+  Logic follows pseudo code in SDM as below:
+
+     N = 512
+     If (Update.DataSize != 00000000H)
+       N = Update.TotalSize / 4
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode  Pointer to the microcode entry.
+
+  @return The microcode total size.
+**/
+UINT32
+EFIAPI
+GetMicrocodeLength (
+  IN CPU_MICROCODE_HEADER *Microcode
+  );
+
+/**
+  Load the microcode to the processor.
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode  Pointer to the microcode entry.
+**/
+VOID
+EFIAPI
+LoadMicrocode (
+  IN CPU_MICROCODE_HEADER *Microcode
+  );
+
+/**
+  Detect whether specified processor can find matching microcode patch and load it.
+
+  Microcode format is as below:
+  +----------------------------------------+-------------------------------------------------+
+  |          CPU_MICROCODE_HEADER          |                                                 |
+  +----------------------------------------+                                                 V
+  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum
+  +----------------------------------------+-------+                                         ^
+  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |
+  +----------------------------------------+       V                                         |
+  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |
+  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |
+  |                   ...                  |       |                                         |
+  +----------------------------------------+-------+-----------------------------------------+
+
+  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
+  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
+  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode            Pointer to a microcode entry.
+  @param MicrocodeLength      The total length of the microcode entry.
+  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.
+                              Caller can supply value get from GetProcessorMicrocodeSignature() to check
+                              whether the microcode is newer than loaded one.
+                              Caller can supply 0 to treat any revision (except 0) microcode as valid.
+  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents
+                              a set of processors.
+                              Caller can supply zero-element array to skip the processor signature and
+                              platform ID check.
+  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.
+  @param VerifyChecksum       FALSE to skip all the checksum verifications.
+
+  @retval TRUE  The microcode is valid.
+  @retval FALSE The microcode is invalid.
+**/
+BOOLEAN
+EFIAPI
+IsValidMicrocode (
+  IN CPU_MICROCODE_HEADER       *Microcode,
+  IN UINTN                      MicrocodeLength,
+  IN UINT32                     MinimumRevision,
+  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,
+  IN UINTN                      MicrocodeCpuIdCount,
+  IN BOOLEAN                    VerifyChecksum
+  );
+
+#endif
\ No newline at end of file
diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
new file mode 100644
index 0000000000..03a43fdae7
--- /dev/null
+++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
@@ -0,0 +1,322 @@
+/** @file
+  Implementation of MicrocodeLib.
+
+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi/UefiBaseType.h>
+#include <Register/Intel/Cpuid.h>
+#include <Register/Intel/ArchitecturalMsr.h>
+#include <Register/Intel/Microcode.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Ppi/ShadowMicrocode.h>
+
+/**
+  Get microcode update signature of currently loaded microcode update.
+
+  @return  Microcode signature.
+**/
+UINT32
+EFIAPI
+GetProcessorMicrocodeSignature (
+  VOID
+  )
+{
+  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;
+
+  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);
+  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);
+  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);
+  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;
+}
+
+/**
+  Get the processor signature and platform ID for current processor.
+
+  @param MicrocodeCpuId  Return the processor signature and platform ID.
+**/
+VOID
+EFIAPI
+GetProcessorMicrocodeCpuId (
+  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId
+  )
+{
+  MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
+
+  ASSERT (MicrocodeCpuId != NULL);
+
+  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
+  MicrocodeCpuId->PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+  AsmCpuid (CPUID_VERSION_INFO, &MicrocodeCpuId->ProcessorSignature, NULL, NULL, NULL);
+}
+
+/**
+  Return the total size of the microcode entry.
+
+  Logic follows pseudo code in SDM as below:
+
+     N = 512
+     If (Update.DataSize != 00000000H)
+       N = Update.TotalSize / 4
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode  Pointer to the microcode entry.
+
+  @return The microcode total size.
+**/
+UINT32
+EFIAPI
+GetMicrocodeLength (
+  IN CPU_MICROCODE_HEADER *Microcode
+  )
+{
+  UINT32   TotalSize;
+
+  ASSERT (Microcode != NULL);
+
+  TotalSize = 2048;
+  if (Microcode->DataSize != 0) {
+    TotalSize = Microcode->TotalSize;
+  }
+  return TotalSize;
+}
+
+/**
+  Load the microcode to the processor.
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode  Pointer to the microcode entry.
+**/
+VOID
+EFIAPI
+LoadMicrocode (
+  IN CPU_MICROCODE_HEADER *Microcode
+  )
+{
+  ASSERT (Microcode != NULL);
+
+  AsmWriteMsr64 (MSR_IA32_BIOS_UPDT_TRIG, (UINT64) (UINTN) (Microcode + 1));
+}
+
+/**
+  Determine if a microcode patch matchs the specific processor signature and flag.
+
+  @param[in]  ProcessorSignature    The processor signature field value in a
+                                    microcode patch.
+  @param[in]  ProcessorFlags        The processor flags field value in a
+                                    microcode patch.
+  @param[in]  MicrocodeCpuId        A pointer to an array of EDKII_PEI_MICROCODE_CPU_ID
+                                    structures.
+  @param[in]  MicrocodeCpuIdCount   Number of elements in MicrocodeCpuId array.
+
+  @retval TRUE     The specified microcode patch matches to one of the MicrocodeCpuId.
+  @retval FALSE    The specified microcode patch doesn't match to any of the MicrocodeCpuId.
+**/
+BOOLEAN
+IsProcessorMatchedMicrocode (
+  IN UINT32                           ProcessorSignature,
+  IN UINT32                           ProcessorFlags,
+  IN EDKII_PEI_MICROCODE_CPU_ID       *MicrocodeCpuId,
+  IN UINTN                            MicrocodeCpuIdCount
+  )
+{
+  UINTN          Index;
+
+  if (MicrocodeCpuIdCount == 0) {
+    return TRUE;
+  }
+
+  for (Index = 0; Index < MicrocodeCpuIdCount; Index++) {
+    if ((ProcessorSignature == MicrocodeCpuId[Index].ProcessorSignature) &&
+        (ProcessorFlags & (1 << MicrocodeCpuId[Index].PlatformId)) != 0) {
+      return TRUE;
+    }
+  }
+
+  return FALSE;
+}
+
+/**
+  Detect whether specified processor can find matching microcode patch and load it.
+
+  Microcode format is as below:
+  +----------------------------------------+-------------------------------------------------+
+  |          CPU_MICROCODE_HEADER          |                                                 |
+  +----------------------------------------+                                                 V
+  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum
+  +----------------------------------------+-------+                                         ^
+  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |
+  +----------------------------------------+       V                                         |
+  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |
+  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |
+  |                   ...                  |       |                                         |
+  +----------------------------------------+-------+-----------------------------------------+
+
+  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
+  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
+  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
+
+  If Microcode is NULL, then ASSERT.
+
+  @param Microcode            Pointer to a microcode entry.
+  @param MicrocodeLength      The total length of the microcode entry.
+  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.
+                              Caller can supply value get from GetProcessorMicrocodeSignature() to check
+                              whether the microcode is newer than loaded one.
+                              Caller can supply 0 to treat any revision (except 0) microcode as valid.
+  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents
+                              a set of processors.
+                              Caller can supply zero-element array to skip the processor signature and
+                              platform ID check.
+  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.
+  @param VerifyChecksum       FALSE to skip all the checksum verifications.
+
+  @retval TRUE  The microcode is valid.
+  @retval FALSE The microcode is invalid.
+**/
+BOOLEAN
+EFIAPI
+IsValidMicrocode (
+  IN CPU_MICROCODE_HEADER       *Microcode,
+  IN UINTN                      MicrocodeLength,
+  IN UINT32                     MinimumRevision,
+  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,
+  IN UINTN                      MicrocodeCpuIdCount,
+  IN BOOLEAN                    VerifyChecksum
+  )
+{
+  UINTN                                   Index;
+  UINT32                                  DataSize;
+  UINT32                                  TotalSize;
+  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;
+  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;
+  UINT32                                  ExtendedTableLength;
+  UINT32                                  Sum32;
+  BOOLEAN                                 Match;
+
+  ASSERT (Microcode != NULL);
+
+  //
+  // It's invalid when:
+  //   the input microcode buffer is so small that even cannot contain the header.
+  //   the input microcode buffer is so large that exceeds MAX_ADDRESS.
+  //
+  if ((MicrocodeLength < sizeof (CPU_MICROCODE_HEADER)) || (MicrocodeLength > (MAX_ADDRESS - (UINTN) Microcode))) {
+    return FALSE;
+  }
+
+  //
+  // Per SDM, HeaderVersion and LoaderRevision should both be 1.
+  //
+  if ((Microcode->HeaderVersion != 1) || (Microcode->LoaderRevision != 1)) {
+    return FALSE;
+  }
+
+  //
+  // The microcode revision should be larger than the minimum revision.
+  //
+  if (Microcode->UpdateRevision <= MinimumRevision) {
+    return FALSE;
+  }
+
+  DataSize = Microcode->DataSize;
+  if (DataSize == 0) {
+    DataSize = 2000;
+  }
+
+  //
+  // Per SDM, DataSize should be multiple of DWORDs.
+  //
+  if ((DataSize % 4) != 0) {
+    return FALSE;
+  }
+
+  TotalSize = GetMicrocodeLength (Microcode);
+
+  //
+  // Check whether the whole microcode is within the buffer.
+  // TotalSize should be multiple of 1024.
+  //
+  if (((TotalSize % SIZE_1KB) != 0) || (TotalSize > MicrocodeLength)) {
+    return FALSE;
+  }
+
+  //
+  // The summation of all DWORDs in microcode should be zero.
+  //
+  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) Microcode, TotalSize) != 0)) {
+    return FALSE;
+  }
+
+  Sum32 = Microcode->ProcessorSignature.Uint32 + Microcode->ProcessorFlags + Microcode->Checksum;
+
+  //
+  // Check the processor signature and platform ID in the primary header.
+  //
+  Match = IsProcessorMatchedMicrocode (
+            Microcode->ProcessorSignature.Uint32,
+            Microcode->ProcessorFlags,
+            MicrocodeCpuIds,
+            MicrocodeCpuIdCount
+            );
+  if (Match) {
+    return TRUE;
+  }
+
+  ExtendedTableLength = TotalSize - (DataSize + sizeof (CPU_MICROCODE_HEADER));
+  if ((ExtendedTableLength < sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) || ((ExtendedTableLength % 4) != 0)) {
+    return FALSE;
+  }
+  //
+  // Extended Table exist, check if the CPU in support list
+  //
+  ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINTN) (Microcode + 1) + DataSize);
+  if (ExtendedTableHeader->ExtendedSignatureCount > MAX_UINT32 / sizeof (CPU_MICROCODE_EXTENDED_TABLE)) {
+    return FALSE;
+  }
+  if (ExtendedTableHeader->ExtendedSignatureCount * sizeof (CPU_MICROCODE_EXTENDED_TABLE)
+      > ExtendedTableLength - sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) {
+    return FALSE;
+  }
+  //
+  // Check the extended table checksum
+  //
+  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength) != 0)) {
+    return FALSE;
+  }
+
+  ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
+  for (Index = 0; Index < ExtendedTableHeader->ExtendedSignatureCount; Index ++) {
+    if (VerifyChecksum &&
+        (ExtendedTable[Index].ProcessorSignature.Uint32 + ExtendedTable[Index].ProcessorFlag
+        + ExtendedTable[Index].Checksum != Sum32)) {
+      //
+      // The extended table entry is valid when the summation of Processor Signature, Processor Flags
+      // and Checksum equal to the coresponding summation from primary header. Because:
+      //    CalculateSum32 (Header + Update Binary) == 0
+      //    CalculateSum32 (Header + Update Binary)
+      //        - (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)
+      //        + (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum) == 0
+      // So,
+      //    (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)
+      //     == (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum)
+      //
+      continue;
+    }
+    Match = IsProcessorMatchedMicrocode (
+              ExtendedTable[Index].ProcessorSignature.Uint32,
+              ExtendedTable[Index].ProcessorFlag,
+              MicrocodeCpuIds,
+              MicrocodeCpuIdCount
+              );
+    if (Match) {
+      return TRUE;
+    }
+  }
+  return FALSE;
+}
diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
new file mode 100644
index 0000000000..c6f8f52e95
--- /dev/null
+++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
@@ -0,0 +1,32 @@
+##  @file
+#   Library for microcode verification and load.
+#
+#  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010006
+  BASE_NAME                      = MicrocodeLib
+  FILE_GUID                      = EB8C72BC-8A48-4F80-996B-E52F68416D57
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = MicrocodeLib
+
+#
+#  VALID_ARCHITECTURES           = IA32 X64 EBC
+#
+
+[Sources.common]
+  MicrocodeLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index a639ce5412..62acb291f3 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -1,7 +1,7 @@
 ## @file  UefiCpuPkg.dec
 # This Package provides UEFI compatible CPU modules and libraries.
 #
-# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved.<BR>
 #
 # SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -59,6 +59,9 @@ [LibraryClasses.IA32, LibraryClasses.X64]
   ##  @libraryclass  Provides function to get CPU cache information.
   CpuCacheInfoLib|Include/Library/CpuCacheInfoLib.h
 
+  ##  @libraryclass  Provides function for loading microcode.
+  MicrocodeLib|Include/Library/MicrocodeLib.h
+
 [Guids]
   gUefiCpuPkgTokenSpaceGuid      = { 0xac05bf33, 0x995a, 0x4ed4, { 0xaa, 0xb8, 0xef, 0x7a, 0xe8, 0xf, 0x5c, 0xb0 }}
   gMsegSmramGuid                 = { 0x5802bce4, 0xeeee, 0x4e33, { 0xa1, 0x30, 0xeb, 0xad, 0x27, 0xf0, 0xe4, 0x39 }}
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 98c4c53465..b932cf63ec 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -60,6 +60,7 @@ [LibraryClasses]
   PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
   VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
 
 [LibraryClasses.common.SEC]
   PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
-- 
2.27.0.windows.1


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

* [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files.
  2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
  2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
@ 2021-04-02  5:58 ` Ni, Ray
  2021-04-07 13:05   ` [edk2-devel] " Laszlo Ersek
  2021-04-02  5:58 ` [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib Ni, Ray
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2021-04-02  5:58 UTC (permalink / raw)
  To: devel; +Cc: Laszlo Ersek, Ard Biesheuvel, Jordan Justen

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
 OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
 OvmfPkg/OvmfPkgIa32.dsc      | 1 +
 OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
 OvmfPkg/OvmfPkgX64.dsc       | 1 +
 OvmfPkg/OvmfXen.dsc          | 1 +
 6 files changed, 6 insertions(+)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index de21312e6f..cdb29d5314 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -153,6 +153,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 52cbed9c2e..7d9e880400 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -152,6 +152,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 396159ebe2..1730b6558b 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -158,6 +158,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 21d58dca98..78a559da0d 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -162,6 +162,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2d61926dab..a7d747f6b4 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -162,6 +162,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 0336a74e70..86abe277c3 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -151,6 +151,7 @@ [LibraryClasses]
   OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
   UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
   UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
-- 
2.27.0.windows.1


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

* [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
  2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
  2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
  2021-04-02  5:58 ` [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files Ni, Ray
@ 2021-04-02  5:58 ` Ni, Ray
  2021-04-08  1:56   ` Ma, Maurice
  2021-04-02  5:58 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code Ni, Ray
  2021-04-06 12:03 ` [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Laszlo Ersek
  4 siblings, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2021-04-02  5:58 UTC (permalink / raw)
  To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index e3b017858e..37ad5a0ae7 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -180,6 +180,7 @@ [LibraryClasses]
   #
   MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
   LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
 
   #
   # Platform
-- 
2.27.0.windows.1


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

* [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code
  2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
                   ` (2 preceding siblings ...)
  2021-04-02  5:58 ` [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib Ni, Ray
@ 2021-04-02  5:58 ` Ni, Ray
  2021-04-07 13:08   ` [edk2-devel] " Laszlo Ersek
  2021-04-08 14:24   ` Dong, Eric
  2021-04-06 12:03 ` [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Laszlo Ersek
  4 siblings, 2 replies; 14+ messages in thread
From: Ni, Ray @ 2021-04-02  5:58 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Laszlo Ersek, Rahul Kumar

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
 4 files changed, 96 insertions(+), 391 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 860a9750e2..d34419c2a5 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
   SynchronizationLib
   PcdLib
   VmgExitLib
+  MicrocodeLib
 
 [Protocols]
   gEfiTimerArchProtocolGuid                     ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 297c2abcd1..105a9f84bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -1,70 +1,16 @@
 /** @file
   Implementation of loading microcode on processors.
 
-  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
 #include "MpLib.h"
 
-/**
-  Get microcode update signature of currently loaded microcode update.
-
-  @return  Microcode signature.
-**/
-UINT32
-GetCurrentMicrocodeSignature (
-  VOID
-  )
-{
-  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;
-
-  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);
-  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);
-  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);
-  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;
-}
-
 /**
   Detect whether specified processor can find matching microcode patch and load it.
 
-  Microcode Payload as the following format:
-  +----------------------------------------+------------------+
-  |          CPU_MICROCODE_HEADER          |                  |
-  +----------------------------------------+  CheckSum Part1  |
-  |            Microcode Binary            |                  |
-  +----------------------------------------+------------------+
-  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |                  |
-  +----------------------------------------+  CheckSum Part2  |
-  |      CPU_MICROCODE_EXTENDED_TABLE      |                  |
-  |                   ...                  |                  |
-  +----------------------------------------+------------------+
-
-  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
-  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
-  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
-
-  When we are trying to verify the CheckSum32 with extended table.
-  We should use the fields of exnteded table to replace the corresponding
-  fields in CPU_MICROCODE_HEADER structure, and recalculate the
-  CheckSum32 with CPU_MICROCODE_HEADER + Microcode Binary. We named
-  it as CheckSum Part3.
-
-  The CheckSum Part2 is used to verify the CPU_MICROCODE_EXTENDED_TABLE_HEADER
-  and CPU_MICROCODE_EXTENDED_TABLE parts. We should make sure CheckSum Part2
-  is correct before we are going to verify each CPU_MICROCODE_EXTENDED_TABLE.
-
-  Only ProcessorSignature, ProcessorFlag and CheckSum are different between
-  CheckSum Part1 and CheckSum Part3. To avoid multiple computing CheckSum Part3.
-  Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
-  When we are going to calculate CheckSum32, just should use the corresponding part
-  of the ProcessorSignature, ProcessorFlag and CheckSum with in-complete CheckSum32.
-
-  Notes: CheckSum32 is not a strong verification.
-         It does not guarantee that the data has not been modified.
-         CPU has its own mechanism to verify Microcode Binary part.
-
   @param[in]  CpuMpData        The pointer to CPU MP Data structure.
   @param[in]  ProcessorNumber  The handle number of the processor. The range is
                                from 0 to the total number of logical processors
@@ -76,26 +22,13 @@ MicrocodeDetect (
   IN UINTN                   ProcessorNumber
   )
 {
-  UINT32                                  ExtendedTableLength;
-  UINT32                                  ExtendedTableCount;
-  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;
-  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;
-  CPU_MICROCODE_HEADER                    *MicrocodeEntryPoint;
+  CPU_MICROCODE_HEADER                    *Microcode;
   UINTN                                   MicrocodeEnd;
-  UINTN                                   Index;
-  UINT8                                   PlatformId;
-  CPUID_VERSION_INFO_EAX                  Eax;
-  CPU_AP_DATA                             *CpuData;
-  UINT32                                  CurrentRevision;
+  CPU_AP_DATA                             *BspData;
   UINT32                                  LatestRevision;
-  UINTN                                   TotalSize;
-  UINT32                                  CheckSum32;
-  UINT32                                  InCompleteCheckSum32;
-  BOOLEAN                                 CorrectMicrocode;
-  VOID                                    *MicrocodeData;
-  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
+  CPU_MICROCODE_HEADER                    *LatestMicrocode;
   UINT32                                  ThreadId;
-  BOOLEAN                                 IsBspCallIn;
+  EDKII_PEI_MICROCODE_CPU_ID              MicrocodeCpuId;
 
   if (CpuMpData->MicrocodePatchRegionSize == 0) {
     //
@@ -104,9 +37,6 @@ MicrocodeDetect (
     return;
   }
 
-  CurrentRevision = GetCurrentMicrocodeSignature ();
-  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
-
   GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);
   if (ThreadId != 0) {
     //
@@ -115,156 +45,35 @@ MicrocodeDetect (
     return;
   }
 
-  ExtendedTableLength = 0;
-  //
-  // Here data of CPUID leafs have not been collected into context buffer, so
-  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
-  //
-  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+  GetProcessorMicrocodeCpuId (&MicrocodeCpuId);
 
-  //
-  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID
-  //
-  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
-  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
-
-
-  //
-  // Check whether AP has same processor with BSP.
-  // If yes, direct use microcode info saved by BSP.
-  //
-  if (!IsBspCallIn) {
+  if (ProcessorNumber != (UINTN) CpuMpData->BspNumber) {
     //
-    // Get the CPU data for BSP
+    // Direct use microcode of BSP if AP is the same as BSP.
+    // Assume BSP calls this routine() before AP.
     //
-    CpuData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);
-    if ((CpuData->ProcessorSignature == Eax.Uint32) &&
-        (CpuData->PlatformId == PlatformId) &&
-        (CpuData->MicrocodeEntryAddr != 0)) {
-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *)(UINTN) CpuData->MicrocodeEntryAddr;
-      MicrocodeData       = (VOID *) (MicrocodeEntryPoint + 1);
-      LatestRevision      = MicrocodeEntryPoint->UpdateRevision;
-      goto Done;
+    BspData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);
+    if ((BspData->ProcessorSignature == MicrocodeCpuId.ProcessorSignature) &&
+        (BspData->PlatformId == MicrocodeCpuId.PlatformId) &&
+        (BspData->MicrocodeEntryAddr != 0)) {
+      LatestMicrocode = (CPU_MICROCODE_HEADER *)(UINTN) BspData->MicrocodeEntryAddr;
+      LatestRevision  = LatestMicrocode->UpdateRevision;
+      goto LoadMicrocode;
     }
   }
 
-  LatestRevision = 0;
-  MicrocodeData  = NULL;
-  MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress + CpuMpData->MicrocodePatchRegionSize);
-  MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
+  //
+  // BSP or AP which is different from BSP runs here
+  // Use 0 as the starting revision to search for microcode because MicrocodePatchInfo HOB needs
+  // the latest microcode location even it's loaded to the processor.
+  //
+  LatestRevision  = 0;
+  LatestMicrocode = NULL;
+  Microcode       = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
+  MicrocodeEnd    = (UINTN) Microcode + (UINTN) CpuMpData->MicrocodePatchRegionSize;
 
   do {
-    //
-    // Check if the microcode is for the Cpu and the version is newer
-    // and the update can be processed on the platform
-    //
-    CorrectMicrocode = FALSE;
-
-    if (MicrocodeEntryPoint->DataSize == 0) {
-      TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
-    } else {
-      TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;
-    }
-
-    ///
-    /// 0x0       MicrocodeBegin  MicrocodeEntry  MicrocodeEnd      0xffffffff
-    /// |--------------|---------------|---------------|---------------|
-    ///                                 valid TotalSize
-    /// TotalSize is only valid between 0 and (MicrocodeEnd - MicrocodeEntry).
-    /// And it should be aligned with 4 bytes.
-    /// If the TotalSize is invalid, skip 1KB to check next entry.
-    ///
-    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
-         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
-         (TotalSize & 0x3) != 0
-       ) {
-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
-      continue;
-    }
-
-    //
-    // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
-    //
-    InCompleteCheckSum32 = CalculateSum32 (
-                             (UINT32 *) MicrocodeEntryPoint,
-                             TotalSize
-                             );
-    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;
-    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
-    InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
-
-    if (MicrocodeEntryPoint->HeaderVersion == 0x1) {
-      //
-      // It is the microcode header. It is not the padding data between microcode patches
-      // because the padding data should not include 0x00000001 and it should be the repeated
-      // byte format (like 0xXYXYXYXY....).
-      //
-      if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&
-          MicrocodeEntryPoint->UpdateRevision > LatestRevision &&
-          (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))
-          ) {
-        //
-        // Calculate CheckSum Part1.
-        //
-        CheckSum32 = InCompleteCheckSum32;
-        CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;
-        CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;
-        CheckSum32 += MicrocodeEntryPoint->Checksum;
-        if (CheckSum32 == 0) {
-          CorrectMicrocode = TRUE;
-        }
-      } else if ((MicrocodeEntryPoint->DataSize != 0) &&
-                 (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {
-        ExtendedTableLength = MicrocodeEntryPoint->TotalSize - (MicrocodeEntryPoint->DataSize +
-                                sizeof (CPU_MICROCODE_HEADER));
-        if (ExtendedTableLength != 0) {
-          //
-          // Extended Table exist, check if the CPU in support list
-          //
-          ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
-                                  + MicrocodeEntryPoint->DataSize + sizeof (CPU_MICROCODE_HEADER));
-          //
-          // Calculate Extended Checksum
-          //
-          if ((ExtendedTableLength % 4) == 0) {
-            //
-            // Calculate CheckSum Part2.
-            //
-            CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength);
-            if (CheckSum32 == 0) {
-              //
-              // Checksum correct
-              //
-              ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;
-              ExtendedTable      = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
-              for (Index = 0; Index < ExtendedTableCount; Index ++) {
-                //
-                // Calculate CheckSum Part3.
-                //
-                CheckSum32 = InCompleteCheckSum32;
-                CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;
-                CheckSum32 += ExtendedTable->ProcessorFlag;
-                CheckSum32 += ExtendedTable->Checksum;
-                if (CheckSum32 == 0) {
-                  //
-                  // Verify Header
-                  //
-                  if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&
-                      (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {
-                    //
-                    // Find one
-                    //
-                    CorrectMicrocode = TRUE;
-                    break;
-                  }
-                }
-                ExtendedTable ++;
-              }
-            }
-          }
-        }
-      }
-    } else {
+    if (!IsValidMicrocode (Microcode, MicrocodeEnd - (UINTN) Microcode, LatestRevision, &MicrocodeCpuId, 1, TRUE)) {
       //
       // It is the padding data between the microcode patches for microcode patches alignment.
       // Because the microcode patch is the multiple of 1-KByte, the padding data should not
@@ -272,156 +81,40 @@ MicrocodeDetect (
       // alignment value should be larger than 1-KByte. We could skip SIZE_1KB padding data to
       // find the next possible microcode patch header.
       //
-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
+      Microcode = (CPU_MICROCODE_HEADER *) ((UINTN) Microcode + SIZE_1KB);
       continue;
     }
-    //
-    // Get the next patch.
-    //
-    if (MicrocodeEntryPoint->DataSize == 0) {
-      TotalSize = 2048;
-    } else {
-      TotalSize = MicrocodeEntryPoint->TotalSize;
-    }
+    LatestMicrocode = Microcode;
+    LatestRevision  = LatestMicrocode->UpdateRevision;
 
-    if (CorrectMicrocode) {
-      LatestRevision = MicrocodeEntryPoint->UpdateRevision;
-      MicrocodeData = (VOID *) ((UINTN) MicrocodeEntryPoint + sizeof (CPU_MICROCODE_HEADER));
-    }
-
-    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);
-  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
+    Microcode = (CPU_MICROCODE_HEADER *) (((UINTN) Microcode) + GetMicrocodeLength (Microcode));
+  } while ((UINTN) Microcode < MicrocodeEnd);
 
-Done:
+LoadMicrocode:
   if (LatestRevision != 0) {
     //
-    // Save the detected microcode patch entry address (including the
-    // microcode patch header) for each processor.
+    // Save the detected microcode patch entry address (including the microcode
+    // patch header) for each processor even it's the same as the loaded one.
     // It will be used when building the microcode patch cache HOB.
     //
-    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr =
-      (UINTN) MicrocodeData -  sizeof (CPU_MICROCODE_HEADER);
+    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr = (UINTN) LatestMicrocode;
   }
 
-  if (LatestRevision > CurrentRevision) {
+  if (LatestRevision > GetProcessorMicrocodeSignature ()) {
     //
     // BIOS only authenticate updates that contain a numerically larger revision
     // than the currently loaded revision, where Current Signature < New Update
     // Revision. A processor with no loaded update is considered to have a
     // revision equal to zero.
     //
-    ASSERT (MicrocodeData != NULL);
-    AsmWriteMsr64 (
-        MSR_IA32_BIOS_UPDT_TRIG,
-        (UINT64) (UINTN) MicrocodeData
-        );
-  }
-  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();
-}
-
-/**
-  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
-                                    supported by a microcode patch.
-  @param[in]  ProcessorFlags        The prcessor flags field value supported by
-                                    a microcode patch.
-
-  @retval TRUE     The specified microcode patch will be loaded.
-  @retval FALSE    The specified microcode patch will not be loaded.
-**/
-BOOLEAN
-IsProcessorMatchedMicrocodePatch (
-  IN CPU_MP_DATA                 *CpuMpData,
-  IN UINT32                      ProcessorSignature,
-  IN UINT32                      ProcessorFlags
-  )
-{
-  UINTN          Index;
-  CPU_AP_DATA    *CpuData;
-
-  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
-    CpuData = &CpuMpData->CpuData[Index];
-    if ((ProcessorSignature == CpuData->ProcessorSignature) &&
-        (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {
-      return TRUE;
-    }
+    LoadMicrocode (LatestMicrocode);
   }
-
-  return FALSE;
-}
-
-/**
-  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.
+  // It's possible that the microcode fails to load. Just capture the CPU microcode revision after loading.
   //
-  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;
+  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetProcessorMicrocodeSignature ();
 }
 
-
 /**
   Actual worker function that shadows the required microcode patches into memory.
 
@@ -491,14 +184,16 @@ ShadowMicrocodePatchByPcd (
   IN OUT CPU_MP_DATA             *CpuMpData
   )
 {
+  UINTN                                  Index;
   CPU_MICROCODE_HEADER                   *MicrocodeEntryPoint;
   UINTN                                  MicrocodeEnd;
-  UINTN                                  DataSize;
   UINTN                                  TotalSize;
   MICROCODE_PATCH_INFO                   *PatchInfoBuffer;
   UINTN                                  MaxPatchNumber;
   UINTN                                  PatchCount;
   UINTN                                  TotalLoadSize;
+  EDKII_PEI_MICROCODE_CPU_ID             *MicrocodeCpuIds;
+  BOOLEAN                                Valid;
 
   //
   // Initialize the microcode patch related fields in CpuMpData as the values
@@ -526,12 +221,34 @@ ShadowMicrocodePatchByPcd (
     return;
   }
 
+  MicrocodeCpuIds = AllocatePages (
+                      EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID))
+                      );
+  if (MicrocodeCpuIds == NULL) {
+    FreePool (PatchInfoBuffer);
+    return;
+  }
+
+  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+    MicrocodeCpuIds[Index].PlatformId         = CpuMpData->CpuData[Index].PlatformId;
+    MicrocodeCpuIds[Index].ProcessorSignature = CpuMpData->CpuData[Index].ProcessorSignature;
+  }
+
   //
   // Process the header of each microcode patch within the region.
   // The purpose is to decide which microcode patch(es) will be loaded into memory.
+  // Microcode checksum is not verified because it's slow when performing on flash.
   //
   do {
-    if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
+    Valid = IsValidMicrocode (
+              MicrocodeEntryPoint,
+              MicrocodeEnd - (UINTN) MicrocodeEntryPoint,
+              0,
+              MicrocodeCpuIds,
+              CpuMpData->CpuCount,
+              FALSE
+              );
+    if (!Valid) {
       //
       // Padding data between the microcode patches, skip 1KB to check next entry.
       //
@@ -539,59 +256,44 @@ ShadowMicrocodePatchByPcd (
       continue;
     }
 
-    DataSize  = MicrocodeEntryPoint->DataSize;
-    TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
-    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
-         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
-         (DataSize & 0x3) != 0 ||
-         (TotalSize & (SIZE_1KB - 1)) != 0 ||
-         TotalSize < DataSize
-       ) {
+    PatchCount++;
+    if (PatchCount > MaxPatchNumber) {
       //
-      // Not a valid microcode header, skip 1KB to check next entry.
+      // Current 'PatchInfoBuffer' cannot hold the information, double the size
+      // and allocate a new buffer.
       //
-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
-      continue;
-    }
-
-    if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
-      PatchCount++;
-      if (PatchCount > MaxPatchNumber) {
+      if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {
         //
-        // Current 'PatchInfoBuffer' cannot hold the information, double the size
-        // and allocate a new buffer.
+        // Overflow check for MaxPatchNumber
         //
-        if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {
-          //
-          // Overflow check for MaxPatchNumber
-          //
-          goto OnExit;
-        }
-
-        PatchInfoBuffer = ReallocatePool (
-                            MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
-                            2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
-                            PatchInfoBuffer
-                            );
-        if (PatchInfoBuffer == NULL) {
-          goto OnExit;
-        }
-        MaxPatchNumber = MaxPatchNumber * 2;
+        goto OnExit;
       }
 
-      //
-      // Store the information of this microcode patch
-      //
-      PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;
-      PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;
-      TotalLoadSize += TotalSize;
+      PatchInfoBuffer = ReallocatePool (
+                          MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
+                          2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
+                          PatchInfoBuffer
+                          );
+      if (PatchInfoBuffer == NULL) {
+        goto OnExit;
+      }
+      MaxPatchNumber = MaxPatchNumber * 2;
     }
 
+    TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);
+
+    //
+    // Store the information of this microcode patch
+    //
+    PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;
+    PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;
+    TotalLoadSize += TotalSize;
+
     //
     // Process the next microcode patch
     //
-    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);
-  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
+    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) ((UINTN) MicrocodeEntryPoint + TotalSize);
+  } while ((UINTN) MicrocodeEntryPoint < MicrocodeEnd);
 
   if (PatchCount != 0) {
     DEBUG ((
@@ -607,7 +309,7 @@ OnExit:
   if (PatchInfoBuffer != NULL) {
     FreePool (PatchInfoBuffer);
   }
-  return;
+  FreePages (MicrocodeCpuIds, EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID)));
 }
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 66f9eb2304..e88a5355c9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -32,6 +32,7 @@
 #include <Library/MtrrLib.h>
 #include <Library/HobLib.h>
 #include <Library/PcdLib.h>
+#include <Library/MicrocodeLib.h>
 
 #include <Guid/MicrocodePatchHob.h>
 
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 49b0ffe8be..36fcb96b58 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -51,6 +51,7 @@ [LibraryClasses]
   PeiServicesLib
   PcdLib
   VmgExitLib
+  MicrocodeLib
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
-- 
2.27.0.windows.1


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

* Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
                   ` (3 preceding siblings ...)
  2021-04-02  5:58 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code Ni, Ray
@ 2021-04-06 12:03 ` Laszlo Ersek
  2021-04-07  2:43   ` Ni, Ray
  4 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2021-04-06 12:03 UTC (permalink / raw)
  To: devel, ray.ni

Hi Ray,

On 04/02/21 07:58, Ni, Ray wrote:
> The patch set creates a new MicrocodeLib for loading microcode.
> Then updates all platforms to include this lib in DSC.
> Then updates the MpInitLib to consume this lib.
> 
> Edk2-platforms change will be sent out in a separate patch set.
> 
> 
> Ray Ni (4):
>   UefiCpuPkg: Add MicrocodeLib for loading microcode
>   OvmfPkg: Add MicrocodeLib in DSC files.
>   UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
>   UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code
> 
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |   1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc                    |   1 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>  OvmfPkg/OvmfXen.dsc                           |   1 +
>  UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++
>  .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++
>  .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
>  UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
>  UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
>  UefiPayloadPkg/UefiPayloadPkg.dsc             |   1 +
>  16 files changed, 582 insertions(+), 392 deletions(-)
>  create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> 

(1) I think we should use a new TianoCore feature request BZ for this
feature, and the commit messages should link it. (I understand the
library only factors out existent logic, but still.)

(2) As I understand it, a platform can provide microcode in three ways:
- via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
- via the "shadow microcode" PPI (PEI phase only),
- via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).

If a platform uses none of these methods (for example, OVMF does not),
then I think it would benefit from a Null instance of the new
MicrocodeLib class.

Would you consider introducing a Null instance too, and using that one
in the OVMF DSC files?

Thanks,
Laszlo


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

* Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-06 12:03 ` [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Laszlo Ersek
@ 2021-04-07  2:43   ` Ni, Ray
  2021-04-07 13:04     ` Laszlo Ersek
  0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2021-04-07  2:43 UTC (permalink / raw)
  To: Laszlo Ersek, devel

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Tue, Apr  6, 2021 at 08:03 PM, Laszlo Ersek wrote:

>
> (1) I think we should use a new TianoCore feature request BZ for this
> feature, and the commit messages should link it. (I understand the
> library only factors out existent logic, but still.)

sure. https://bugzilla.tianocore.org/show_bug.cgi?id=3303

> 
> (2) As I understand it, a platform can provide microcode in three ways:
> - via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
> - via the "shadow microcode" PPI (PEI phase only),
> - via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).
> 
> If a platform uses none of these methods (for example, OVMF does not),
> then I think it would benefit from a Null instance of the new
> MicrocodeLib class.
> 
> Would you consider introducing a Null instance too, and using that one
> in the OVMF DSC files?
>

No. I don't think it's necessary for a NULL instance.
Because:
1. MicrocodePatch GUIDed HOB is only produced when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
    I will further simplify today's MpInitLib to skip loading microcode when this HOB exists (because DXE re-load is unnecessary).
    This is captured by https://bugzilla.tianocore.org/show_bug.cgi?id=3155.

2. Today's logic only calls the MicrocodeLib API when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
    Even NULL instance is provided, it's not called.

3. MicrocodeLib calls ASSERT() when the supplied microcode binary is NULL.
    If the logic in MpInitLib is changed by accident to allow the call to MicrocodeLib even in OVMF, the assertion can catch this.

[-- Attachment #2: Type: text/html, Size: 1956 bytes --]

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

* Re: [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-07  2:43   ` Ni, Ray
@ 2021-04-07 13:04     ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2021-04-07 13:04 UTC (permalink / raw)
  To: devel, ray.ni

On 04/07/21 04:43, Ni, Ray wrote:
> On Tue, Apr  6, 2021 at 08:03 PM, Laszlo Ersek wrote:
> 
>>
>> (1) I think we should use a new TianoCore feature request BZ for this
>> feature, and the commit messages should link it. (I understand the
>> library only factors out existent logic, but still.)
> 
> sure. https://bugzilla.tianocore.org/show_bug.cgi?id=3303
> 
>>
>> (2) As I understand it, a platform can provide microcode in three ways:
>> - via the "microcode patch" GUIDed HOB (PEI and DXE phases both),
>> - via the "shadow microcode" PPI (PEI phase only),
>> - via the PcdCpuMicrocodePatch* PCDs (PEI and DXE phases both).
>>
>> If a platform uses none of these methods (for example, OVMF does not),
>> then I think it would benefit from a Null instance of the new
>> MicrocodeLib class.
>>
>> Would you consider introducing a Null instance too, and using that one
>> in the OVMF DSC files?
>>
> 
> No. I don't think it's necessary for a NULL instance.
> Because:
> 1. MicrocodePatch GUIDed HOB is only produced when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
>     I will further simplify today's MpInitLib to skip loading microcode when this HOB exists (because DXE re-load is unnecessary).
>     This is captured by https://bugzilla.tianocore.org/show_bug.cgi?id=3155.
> 
> 2. Today's logic only calls the MicrocodeLib API when "shadow microcode" PPI or "PcdCpuMicrocodePatch *" exists.
>     Even NULL instance is provided, it's not called.
> 
> 3. MicrocodeLib calls ASSERT() when the supplied microcode binary is NULL.
>     If the logic in MpInitLib is changed by accident to allow the call to MicrocodeLib even in OVMF, the assertion can catch this.

I was thinking that a Null instance would be useful for eliminating dead
code from the binary (because: the PPI check is dynamic, unlike a
compile-time PCD check, so it can not be optimized out). But it's not
critical.

Thanks
Laszlo


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

* Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
@ 2021-04-07 13:05   ` Laszlo Ersek
  2021-04-08  2:19   ` Dong, Eric
  1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2021-04-07 13:05 UTC (permalink / raw)
  To: devel, ray.ni; +Cc: Eric Dong, Rahul Kumar

On 04/02/21 07:58, Ni, Ray wrote:
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++++
>  .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++++++++
>  .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
>  UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
>  UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
>  5 files changed, 479 insertions(+), 1 deletion(-)
>  create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
>  create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

With the BZ ref added:

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

Thanks
Laszlo


> diff --git a/UefiCpuPkg/Include/Library/MicrocodeLib.h b/UefiCpuPkg/Include/Library/MicrocodeLib.h
> new file mode 100644
> index 0000000000..2570c43cce
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Library/MicrocodeLib.h
> @@ -0,0 +1,120 @@
> +/** @file
> +  Public include file for Microcode library.
> +
> +  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __MICROCODE_LIB_H__
> +#define __MICROCODE_LIB_H__
> +
> +#include <Register/Intel/Microcode.h>
> +#include <Ppi/ShadowMicrocode.h>
> +
> +/**
> +  Get microcode update signature of currently loaded microcode update.
> +
> +  @return  Microcode signature.
> +**/
> +UINT32
> +EFIAPI
> +GetProcessorMicrocodeSignature (
> +  VOID
> +  );
> +
> +/**
> +  Get the processor signature and platform ID for current processor.
> +
> +  @param MicrocodeCpuId  Return the processor signature and platform ID.
> +**/
> +VOID
> +EFIAPI
> +GetProcessorMicrocodeCpuId (
> +  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId
> +  );
> +
> +/**
> +  Return the total size of the microcode entry.
> +
> +  Logic follows pseudo code in SDM as below:
> +
> +     N = 512
> +     If (Update.DataSize != 00000000H)
> +       N = Update.TotalSize / 4
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode  Pointer to the microcode entry.
> +
> +  @return The microcode total size.
> +**/
> +UINT32
> +EFIAPI
> +GetMicrocodeLength (
> +  IN CPU_MICROCODE_HEADER *Microcode
> +  );
> +
> +/**
> +  Load the microcode to the processor.
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode  Pointer to the microcode entry.
> +**/
> +VOID
> +EFIAPI
> +LoadMicrocode (
> +  IN CPU_MICROCODE_HEADER *Microcode
> +  );
> +
> +/**
> +  Detect whether specified processor can find matching microcode patch and load it.
> +
> +  Microcode format is as below:
> +  +----------------------------------------+-------------------------------------------------+
> +  |          CPU_MICROCODE_HEADER          |                                                 |
> +  +----------------------------------------+                                                 V
> +  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum
> +  +----------------------------------------+-------+                                         ^
> +  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |
> +  +----------------------------------------+       V                                         |
> +  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |
> +  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |
> +  |                   ...                  |       |                                         |
> +  +----------------------------------------+-------+-----------------------------------------+
> +
> +  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
> +  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
> +  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode            Pointer to a microcode entry.
> +  @param MicrocodeLength      The total length of the microcode entry.
> +  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.
> +                              Caller can supply value get from GetProcessorMicrocodeSignature() to check
> +                              whether the microcode is newer than loaded one.
> +                              Caller can supply 0 to treat any revision (except 0) microcode as valid.
> +  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents
> +                              a set of processors.
> +                              Caller can supply zero-element array to skip the processor signature and
> +                              platform ID check.
> +  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.
> +  @param VerifyChecksum       FALSE to skip all the checksum verifications.
> +
> +  @retval TRUE  The microcode is valid.
> +  @retval FALSE The microcode is invalid.
> +**/
> +BOOLEAN
> +EFIAPI
> +IsValidMicrocode (
> +  IN CPU_MICROCODE_HEADER       *Microcode,
> +  IN UINTN                      MicrocodeLength,
> +  IN UINT32                     MinimumRevision,
> +  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,
> +  IN UINTN                      MicrocodeCpuIdCount,
> +  IN BOOLEAN                    VerifyChecksum
> +  );
> +
> +#endif
> \ No newline at end of file
> diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
> new file mode 100644
> index 0000000000..03a43fdae7
> --- /dev/null
> +++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
> @@ -0,0 +1,322 @@
> +/** @file
> +  Implementation of MicrocodeLib.
> +
> +  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Register/Intel/Cpuid.h>
> +#include <Register/Intel/ArchitecturalMsr.h>
> +#include <Register/Intel/Microcode.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Ppi/ShadowMicrocode.h>
> +
> +/**
> +  Get microcode update signature of currently loaded microcode update.
> +
> +  @return  Microcode signature.
> +**/
> +UINT32
> +EFIAPI
> +GetProcessorMicrocodeSignature (
> +  VOID
> +  )
> +{
> +  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;
> +
> +  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);
> +  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);
> +  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);
> +  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;
> +}
> +
> +/**
> +  Get the processor signature and platform ID for current processor.
> +
> +  @param MicrocodeCpuId  Return the processor signature and platform ID.
> +**/
> +VOID
> +EFIAPI
> +GetProcessorMicrocodeCpuId (
> +  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId
> +  )
> +{
> +  MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
> +
> +  ASSERT (MicrocodeCpuId != NULL);
> +
> +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> +  MicrocodeCpuId->PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +  AsmCpuid (CPUID_VERSION_INFO, &MicrocodeCpuId->ProcessorSignature, NULL, NULL, NULL);
> +}
> +
> +/**
> +  Return the total size of the microcode entry.
> +
> +  Logic follows pseudo code in SDM as below:
> +
> +     N = 512
> +     If (Update.DataSize != 00000000H)
> +       N = Update.TotalSize / 4
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode  Pointer to the microcode entry.
> +
> +  @return The microcode total size.
> +**/
> +UINT32
> +EFIAPI
> +GetMicrocodeLength (
> +  IN CPU_MICROCODE_HEADER *Microcode
> +  )
> +{
> +  UINT32   TotalSize;
> +
> +  ASSERT (Microcode != NULL);
> +
> +  TotalSize = 2048;
> +  if (Microcode->DataSize != 0) {
> +    TotalSize = Microcode->TotalSize;
> +  }
> +  return TotalSize;
> +}
> +
> +/**
> +  Load the microcode to the processor.
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode  Pointer to the microcode entry.
> +**/
> +VOID
> +EFIAPI
> +LoadMicrocode (
> +  IN CPU_MICROCODE_HEADER *Microcode
> +  )
> +{
> +  ASSERT (Microcode != NULL);
> +
> +  AsmWriteMsr64 (MSR_IA32_BIOS_UPDT_TRIG, (UINT64) (UINTN) (Microcode + 1));
> +}
> +
> +/**
> +  Determine if a microcode patch matchs the specific processor signature and flag.
> +
> +  @param[in]  ProcessorSignature    The processor signature field value in a
> +                                    microcode patch.
> +  @param[in]  ProcessorFlags        The processor flags field value in a
> +                                    microcode patch.
> +  @param[in]  MicrocodeCpuId        A pointer to an array of EDKII_PEI_MICROCODE_CPU_ID
> +                                    structures.
> +  @param[in]  MicrocodeCpuIdCount   Number of elements in MicrocodeCpuId array.
> +
> +  @retval TRUE     The specified microcode patch matches to one of the MicrocodeCpuId.
> +  @retval FALSE    The specified microcode patch doesn't match to any of the MicrocodeCpuId.
> +**/
> +BOOLEAN
> +IsProcessorMatchedMicrocode (
> +  IN UINT32                           ProcessorSignature,
> +  IN UINT32                           ProcessorFlags,
> +  IN EDKII_PEI_MICROCODE_CPU_ID       *MicrocodeCpuId,
> +  IN UINTN                            MicrocodeCpuIdCount
> +  )
> +{
> +  UINTN          Index;
> +
> +  if (MicrocodeCpuIdCount == 0) {
> +    return TRUE;
> +  }
> +
> +  for (Index = 0; Index < MicrocodeCpuIdCount; Index++) {
> +    if ((ProcessorSignature == MicrocodeCpuId[Index].ProcessorSignature) &&
> +        (ProcessorFlags & (1 << MicrocodeCpuId[Index].PlatformId)) != 0) {
> +      return TRUE;
> +    }
> +  }
> +
> +  return FALSE;
> +}
> +
> +/**
> +  Detect whether specified processor can find matching microcode patch and load it.
> +
> +  Microcode format is as below:
> +  +----------------------------------------+-------------------------------------------------+
> +  |          CPU_MICROCODE_HEADER          |                                                 |
> +  +----------------------------------------+                                                 V
> +  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum
> +  +----------------------------------------+-------+                                         ^
> +  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |
> +  +----------------------------------------+       V                                         |
> +  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |
> +  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |
> +  |                   ...                  |       |                                         |
> +  +----------------------------------------+-------+-----------------------------------------+
> +
> +  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
> +  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
> +  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
> +
> +  If Microcode is NULL, then ASSERT.
> +
> +  @param Microcode            Pointer to a microcode entry.
> +  @param MicrocodeLength      The total length of the microcode entry.
> +  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.
> +                              Caller can supply value get from GetProcessorMicrocodeSignature() to check
> +                              whether the microcode is newer than loaded one.
> +                              Caller can supply 0 to treat any revision (except 0) microcode as valid.
> +  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents
> +                              a set of processors.
> +                              Caller can supply zero-element array to skip the processor signature and
> +                              platform ID check.
> +  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.
> +  @param VerifyChecksum       FALSE to skip all the checksum verifications.
> +
> +  @retval TRUE  The microcode is valid.
> +  @retval FALSE The microcode is invalid.
> +**/
> +BOOLEAN
> +EFIAPI
> +IsValidMicrocode (
> +  IN CPU_MICROCODE_HEADER       *Microcode,
> +  IN UINTN                      MicrocodeLength,
> +  IN UINT32                     MinimumRevision,
> +  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,
> +  IN UINTN                      MicrocodeCpuIdCount,
> +  IN BOOLEAN                    VerifyChecksum
> +  )
> +{
> +  UINTN                                   Index;
> +  UINT32                                  DataSize;
> +  UINT32                                  TotalSize;
> +  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;
> +  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;
> +  UINT32                                  ExtendedTableLength;
> +  UINT32                                  Sum32;
> +  BOOLEAN                                 Match;
> +
> +  ASSERT (Microcode != NULL);
> +
> +  //
> +  // It's invalid when:
> +  //   the input microcode buffer is so small that even cannot contain the header.
> +  //   the input microcode buffer is so large that exceeds MAX_ADDRESS.
> +  //
> +  if ((MicrocodeLength < sizeof (CPU_MICROCODE_HEADER)) || (MicrocodeLength > (MAX_ADDRESS - (UINTN) Microcode))) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Per SDM, HeaderVersion and LoaderRevision should both be 1.
> +  //
> +  if ((Microcode->HeaderVersion != 1) || (Microcode->LoaderRevision != 1)) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // The microcode revision should be larger than the minimum revision.
> +  //
> +  if (Microcode->UpdateRevision <= MinimumRevision) {
> +    return FALSE;
> +  }
> +
> +  DataSize = Microcode->DataSize;
> +  if (DataSize == 0) {
> +    DataSize = 2000;
> +  }
> +
> +  //
> +  // Per SDM, DataSize should be multiple of DWORDs.
> +  //
> +  if ((DataSize % 4) != 0) {
> +    return FALSE;
> +  }
> +
> +  TotalSize = GetMicrocodeLength (Microcode);
> +
> +  //
> +  // Check whether the whole microcode is within the buffer.
> +  // TotalSize should be multiple of 1024.
> +  //
> +  if (((TotalSize % SIZE_1KB) != 0) || (TotalSize > MicrocodeLength)) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // The summation of all DWORDs in microcode should be zero.
> +  //
> +  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) Microcode, TotalSize) != 0)) {
> +    return FALSE;
> +  }
> +
> +  Sum32 = Microcode->ProcessorSignature.Uint32 + Microcode->ProcessorFlags + Microcode->Checksum;
> +
> +  //
> +  // Check the processor signature and platform ID in the primary header.
> +  //
> +  Match = IsProcessorMatchedMicrocode (
> +            Microcode->ProcessorSignature.Uint32,
> +            Microcode->ProcessorFlags,
> +            MicrocodeCpuIds,
> +            MicrocodeCpuIdCount
> +            );
> +  if (Match) {
> +    return TRUE;
> +  }
> +
> +  ExtendedTableLength = TotalSize - (DataSize + sizeof (CPU_MICROCODE_HEADER));
> +  if ((ExtendedTableLength < sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) || ((ExtendedTableLength % 4) != 0)) {
> +    return FALSE;
> +  }
> +  //
> +  // Extended Table exist, check if the CPU in support list
> +  //
> +  ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINTN) (Microcode + 1) + DataSize);
> +  if (ExtendedTableHeader->ExtendedSignatureCount > MAX_UINT32 / sizeof (CPU_MICROCODE_EXTENDED_TABLE)) {
> +    return FALSE;
> +  }
> +  if (ExtendedTableHeader->ExtendedSignatureCount * sizeof (CPU_MICROCODE_EXTENDED_TABLE)
> +      > ExtendedTableLength - sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) {
> +    return FALSE;
> +  }
> +  //
> +  // Check the extended table checksum
> +  //
> +  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength) != 0)) {
> +    return FALSE;
> +  }
> +
> +  ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
> +  for (Index = 0; Index < ExtendedTableHeader->ExtendedSignatureCount; Index ++) {
> +    if (VerifyChecksum &&
> +        (ExtendedTable[Index].ProcessorSignature.Uint32 + ExtendedTable[Index].ProcessorFlag
> +        + ExtendedTable[Index].Checksum != Sum32)) {
> +      //
> +      // The extended table entry is valid when the summation of Processor Signature, Processor Flags
> +      // and Checksum equal to the coresponding summation from primary header. Because:
> +      //    CalculateSum32 (Header + Update Binary) == 0
> +      //    CalculateSum32 (Header + Update Binary)
> +      //        - (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)
> +      //        + (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum) == 0
> +      // So,
> +      //    (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)
> +      //     == (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum)
> +      //
> +      continue;
> +    }
> +    Match = IsProcessorMatchedMicrocode (
> +              ExtendedTable[Index].ProcessorSignature.Uint32,
> +              ExtendedTable[Index].ProcessorFlag,
> +              MicrocodeCpuIds,
> +              MicrocodeCpuIdCount
> +              );
> +    if (Match) {
> +      return TRUE;
> +    }
> +  }
> +  return FALSE;
> +}
> diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> new file mode 100644
> index 0000000000..c6f8f52e95
> --- /dev/null
> +++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> @@ -0,0 +1,32 @@
> +##  @file
> +#   Library for microcode verification and load.
> +#
> +#  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010006
> +  BASE_NAME                      = MicrocodeLib
> +  FILE_GUID                      = EB8C72BC-8A48-4F80-996B-E52F68416D57
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MicrocodeLib
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 EBC
> +#
> +
> +[Sources.common]
> +  MicrocodeLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index a639ce5412..62acb291f3 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -1,7 +1,7 @@
>  ## @file  UefiCpuPkg.dec
>  # This Package provides UEFI compatible CPU modules and libraries.
>  #
> -# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -59,6 +59,9 @@ [LibraryClasses.IA32, LibraryClasses.X64]
>    ##  @libraryclass  Provides function to get CPU cache information.
>    CpuCacheInfoLib|Include/Library/CpuCacheInfoLib.h
>  
> +  ##  @libraryclass  Provides function for loading microcode.
> +  MicrocodeLib|Include/Library/MicrocodeLib.h
> +
>  [Guids]
>    gUefiCpuPkgTokenSpaceGuid      = { 0xac05bf33, 0x995a, 0x4ed4, { 0xaa, 0xb8, 0xef, 0x7a, 0xe8, 0xf, 0x5c, 0xb0 }}
>    gMsegSmramGuid                 = { 0x5802bce4, 0xeeee, 0x4e33, { 0xa1, 0x30, 0xeb, 0xad, 0x27, 0xf0, 0xe4, 0x39 }}
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
> index 98c4c53465..b932cf63ec 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -60,6 +60,7 @@ [LibraryClasses]
>    PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
>    TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>    VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>  
>  [LibraryClasses.common.SEC]
>    PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
> 


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

* Re: [edk2-devel] [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files.
  2021-04-02  5:58 ` [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files Ni, Ray
@ 2021-04-07 13:05   ` Laszlo Ersek
  0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2021-04-07 13:05 UTC (permalink / raw)
  To: devel, ray.ni; +Cc: Ard Biesheuvel, Jordan Justen

On 04/02/21 07:58, Ni, Ray wrote:
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc | 1 +
>  OvmfPkg/Bhyve/BhyveX64.dsc   | 1 +
>  OvmfPkg/OvmfPkgIa32.dsc      | 1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc   | 1 +
>  OvmfPkg/OvmfPkgX64.dsc       | 1 +
>  OvmfPkg/OvmfXen.dsc          | 1 +
>  6 files changed, 6 insertions(+)
> 

With the BZ ref added:

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

Thanks
Laszlo


> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index de21312e6f..cdb29d5314 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -153,6 +153,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 52cbed9c2e..7d9e880400 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -152,6 +152,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 396159ebe2..1730b6558b 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -158,6 +158,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 21d58dca98..78a559da0d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -162,6 +162,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2d61926dab..a7d747f6b4 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -162,6 +162,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 0336a74e70..86abe277c3 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -151,6 +151,7 @@ [LibraryClasses]
>    OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
>    SerialPortLib|PcAtChipsetPkg/Library/SerialIoLib/SerialIoLib.inf
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
>    UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
>    UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
> 


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

* Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code
  2021-04-02  5:58 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code Ni, Ray
@ 2021-04-07 13:08   ` Laszlo Ersek
  2021-04-08 14:24   ` Dong, Eric
  1 sibling, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2021-04-07 13:08 UTC (permalink / raw)
  To: devel, ray.ni; +Cc: Eric Dong, Rahul Kumar

On 04/02/21 07:58, Ni, Ray wrote:
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
>  4 files changed, 96 insertions(+), 391 deletions(-)

With the BZ ref added:

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

One observation: this patch will affect all platforms (in edk2-platforms
too, for example), that consume "PeiMpInitLib.inf" or
"DxeMpInitLib.inf". The MicrocodeLib class is new (from patch#1), so all
affected platform DSCs will need new resolutions (patches similar to
patch#2).

Thanks,
Laszlo

> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 860a9750e2..d34419c2a5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
>    SynchronizationLib
>    PcdLib
>    VmgExitLib
> +  MicrocodeLib
>  
>  [Protocols]
>    gEfiTimerArchProtocolGuid                     ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 297c2abcd1..105a9f84bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -1,70 +1,16 @@
>  /** @file
>    Implementation of loading microcode on processors.
>  
> -  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
>  
>  #include "MpLib.h"
>  
> -/**
> -  Get microcode update signature of currently loaded microcode update.
> -
> -  @return  Microcode signature.
> -**/
> -UINT32
> -GetCurrentMicrocodeSignature (
> -  VOID
> -  )
> -{
> -  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;
> -
> -  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);
> -  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);
> -  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);
> -  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;
> -}
> -
>  /**
>    Detect whether specified processor can find matching microcode patch and load it.
>  
> -  Microcode Payload as the following format:
> -  +----------------------------------------+------------------+
> -  |          CPU_MICROCODE_HEADER          |                  |
> -  +----------------------------------------+  CheckSum Part1  |
> -  |            Microcode Binary            |                  |
> -  +----------------------------------------+------------------+
> -  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |                  |
> -  +----------------------------------------+  CheckSum Part2  |
> -  |      CPU_MICROCODE_EXTENDED_TABLE      |                  |
> -  |                   ...                  |                  |
> -  +----------------------------------------+------------------+
> -
> -  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.
> -  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount
> -  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.
> -
> -  When we are trying to verify the CheckSum32 with extended table.
> -  We should use the fields of exnteded table to replace the corresponding
> -  fields in CPU_MICROCODE_HEADER structure, and recalculate the
> -  CheckSum32 with CPU_MICROCODE_HEADER + Microcode Binary. We named
> -  it as CheckSum Part3.
> -
> -  The CheckSum Part2 is used to verify the CPU_MICROCODE_EXTENDED_TABLE_HEADER
> -  and CPU_MICROCODE_EXTENDED_TABLE parts. We should make sure CheckSum Part2
> -  is correct before we are going to verify each CPU_MICROCODE_EXTENDED_TABLE.
> -
> -  Only ProcessorSignature, ProcessorFlag and CheckSum are different between
> -  CheckSum Part1 and CheckSum Part3. To avoid multiple computing CheckSum Part3.
> -  Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
> -  When we are going to calculate CheckSum32, just should use the corresponding part
> -  of the ProcessorSignature, ProcessorFlag and CheckSum with in-complete CheckSum32.
> -
> -  Notes: CheckSum32 is not a strong verification.
> -         It does not guarantee that the data has not been modified.
> -         CPU has its own mechanism to verify Microcode Binary part.
> -
>    @param[in]  CpuMpData        The pointer to CPU MP Data structure.
>    @param[in]  ProcessorNumber  The handle number of the processor. The range is
>                                 from 0 to the total number of logical processors
> @@ -76,26 +22,13 @@ MicrocodeDetect (
>    IN UINTN                   ProcessorNumber
>    )
>  {
> -  UINT32                                  ExtendedTableLength;
> -  UINT32                                  ExtendedTableCount;
> -  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;
> -  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;
> -  CPU_MICROCODE_HEADER                    *MicrocodeEntryPoint;
> +  CPU_MICROCODE_HEADER                    *Microcode;
>    UINTN                                   MicrocodeEnd;
> -  UINTN                                   Index;
> -  UINT8                                   PlatformId;
> -  CPUID_VERSION_INFO_EAX                  Eax;
> -  CPU_AP_DATA                             *CpuData;
> -  UINT32                                  CurrentRevision;
> +  CPU_AP_DATA                             *BspData;
>    UINT32                                  LatestRevision;
> -  UINTN                                   TotalSize;
> -  UINT32                                  CheckSum32;
> -  UINT32                                  InCompleteCheckSum32;
> -  BOOLEAN                                 CorrectMicrocode;
> -  VOID                                    *MicrocodeData;
> -  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
> +  CPU_MICROCODE_HEADER                    *LatestMicrocode;
>    UINT32                                  ThreadId;
> -  BOOLEAN                                 IsBspCallIn;
> +  EDKII_PEI_MICROCODE_CPU_ID              MicrocodeCpuId;
>  
>    if (CpuMpData->MicrocodePatchRegionSize == 0) {
>      //
> @@ -104,9 +37,6 @@ MicrocodeDetect (
>      return;
>    }
>  
> -  CurrentRevision = GetCurrentMicrocodeSignature ();
> -  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
> -
>    GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);
>    if (ThreadId != 0) {
>      //
> @@ -115,156 +45,35 @@ MicrocodeDetect (
>      return;
>    }
>  
> -  ExtendedTableLength = 0;
> -  //
> -  // Here data of CPUID leafs have not been collected into context buffer, so
> -  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> -  //
> -  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +  GetProcessorMicrocodeCpuId (&MicrocodeCpuId);
>  
> -  //
> -  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID
> -  //
> -  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> -  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> -
> -
> -  //
> -  // Check whether AP has same processor with BSP.
> -  // If yes, direct use microcode info saved by BSP.
> -  //
> -  if (!IsBspCallIn) {
> +  if (ProcessorNumber != (UINTN) CpuMpData->BspNumber) {
>      //
> -    // Get the CPU data for BSP
> +    // Direct use microcode of BSP if AP is the same as BSP.
> +    // Assume BSP calls this routine() before AP.
>      //
> -    CpuData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);
> -    if ((CpuData->ProcessorSignature == Eax.Uint32) &&
> -        (CpuData->PlatformId == PlatformId) &&
> -        (CpuData->MicrocodeEntryAddr != 0)) {
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *)(UINTN) CpuData->MicrocodeEntryAddr;
> -      MicrocodeData       = (VOID *) (MicrocodeEntryPoint + 1);
> -      LatestRevision      = MicrocodeEntryPoint->UpdateRevision;
> -      goto Done;
> +    BspData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);
> +    if ((BspData->ProcessorSignature == MicrocodeCpuId.ProcessorSignature) &&
> +        (BspData->PlatformId == MicrocodeCpuId.PlatformId) &&
> +        (BspData->MicrocodeEntryAddr != 0)) {
> +      LatestMicrocode = (CPU_MICROCODE_HEADER *)(UINTN) BspData->MicrocodeEntryAddr;
> +      LatestRevision  = LatestMicrocode->UpdateRevision;
> +      goto LoadMicrocode;
>      }
>    }
>  
> -  LatestRevision = 0;
> -  MicrocodeData  = NULL;
> -  MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress + CpuMpData->MicrocodePatchRegionSize);
> -  MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
> +  //
> +  // BSP or AP which is different from BSP runs here
> +  // Use 0 as the starting revision to search for microcode because MicrocodePatchInfo HOB needs
> +  // the latest microcode location even it's loaded to the processor.
> +  //
> +  LatestRevision  = 0;
> +  LatestMicrocode = NULL;
> +  Microcode       = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
> +  MicrocodeEnd    = (UINTN) Microcode + (UINTN) CpuMpData->MicrocodePatchRegionSize;
>  
>    do {
> -    //
> -    // Check if the microcode is for the Cpu and the version is newer
> -    // and the update can be processed on the platform
> -    //
> -    CorrectMicrocode = FALSE;
> -
> -    if (MicrocodeEntryPoint->DataSize == 0) {
> -      TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
> -    } else {
> -      TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;
> -    }
> -
> -    ///
> -    /// 0x0       MicrocodeBegin  MicrocodeEntry  MicrocodeEnd      0xffffffff
> -    /// |--------------|---------------|---------------|---------------|
> -    ///                                 valid TotalSize
> -    /// TotalSize is only valid between 0 and (MicrocodeEnd - MicrocodeEntry).
> -    /// And it should be aligned with 4 bytes.
> -    /// If the TotalSize is invalid, skip 1KB to check next entry.
> -    ///
> -    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
> -         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
> -         (TotalSize & 0x3) != 0
> -       ) {
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
> -      continue;
> -    }
> -
> -    //
> -    // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.
> -    //
> -    InCompleteCheckSum32 = CalculateSum32 (
> -                             (UINT32 *) MicrocodeEntryPoint,
> -                             TotalSize
> -                             );
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;
> -    InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;
> -
> -    if (MicrocodeEntryPoint->HeaderVersion == 0x1) {
> -      //
> -      // It is the microcode header. It is not the padding data between microcode patches
> -      // because the padding data should not include 0x00000001 and it should be the repeated
> -      // byte format (like 0xXYXYXYXY....).
> -      //
> -      if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&
> -          MicrocodeEntryPoint->UpdateRevision > LatestRevision &&
> -          (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))
> -          ) {
> -        //
> -        // Calculate CheckSum Part1.
> -        //
> -        CheckSum32 = InCompleteCheckSum32;
> -        CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;
> -        CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;
> -        CheckSum32 += MicrocodeEntryPoint->Checksum;
> -        if (CheckSum32 == 0) {
> -          CorrectMicrocode = TRUE;
> -        }
> -      } else if ((MicrocodeEntryPoint->DataSize != 0) &&
> -                 (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {
> -        ExtendedTableLength = MicrocodeEntryPoint->TotalSize - (MicrocodeEntryPoint->DataSize +
> -                                sizeof (CPU_MICROCODE_HEADER));
> -        if (ExtendedTableLength != 0) {
> -          //
> -          // Extended Table exist, check if the CPU in support list
> -          //
> -          ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)
> -                                  + MicrocodeEntryPoint->DataSize + sizeof (CPU_MICROCODE_HEADER));
> -          //
> -          // Calculate Extended Checksum
> -          //
> -          if ((ExtendedTableLength % 4) == 0) {
> -            //
> -            // Calculate CheckSum Part2.
> -            //
> -            CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength);
> -            if (CheckSum32 == 0) {
> -              //
> -              // Checksum correct
> -              //
> -              ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;
> -              ExtendedTable      = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);
> -              for (Index = 0; Index < ExtendedTableCount; Index ++) {
> -                //
> -                // Calculate CheckSum Part3.
> -                //
> -                CheckSum32 = InCompleteCheckSum32;
> -                CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;
> -                CheckSum32 += ExtendedTable->ProcessorFlag;
> -                CheckSum32 += ExtendedTable->Checksum;
> -                if (CheckSum32 == 0) {
> -                  //
> -                  // Verify Header
> -                  //
> -                  if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&
> -                      (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {
> -                    //
> -                    // Find one
> -                    //
> -                    CorrectMicrocode = TRUE;
> -                    break;
> -                  }
> -                }
> -                ExtendedTable ++;
> -              }
> -            }
> -          }
> -        }
> -      }
> -    } else {
> +    if (!IsValidMicrocode (Microcode, MicrocodeEnd - (UINTN) Microcode, LatestRevision, &MicrocodeCpuId, 1, TRUE)) {
>        //
>        // It is the padding data between the microcode patches for microcode patches alignment.
>        // Because the microcode patch is the multiple of 1-KByte, the padding data should not
> @@ -272,156 +81,40 @@ MicrocodeDetect (
>        // alignment value should be larger than 1-KByte. We could skip SIZE_1KB padding data to
>        // find the next possible microcode patch header.
>        //
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
> +      Microcode = (CPU_MICROCODE_HEADER *) ((UINTN) Microcode + SIZE_1KB);
>        continue;
>      }
> -    //
> -    // Get the next patch.
> -    //
> -    if (MicrocodeEntryPoint->DataSize == 0) {
> -      TotalSize = 2048;
> -    } else {
> -      TotalSize = MicrocodeEntryPoint->TotalSize;
> -    }
> +    LatestMicrocode = Microcode;
> +    LatestRevision  = LatestMicrocode->UpdateRevision;
>  
> -    if (CorrectMicrocode) {
> -      LatestRevision = MicrocodeEntryPoint->UpdateRevision;
> -      MicrocodeData = (VOID *) ((UINTN) MicrocodeEntryPoint + sizeof (CPU_MICROCODE_HEADER));
> -    }
> -
> -    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);
> -  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> +    Microcode = (CPU_MICROCODE_HEADER *) (((UINTN) Microcode) + GetMicrocodeLength (Microcode));
> +  } while ((UINTN) Microcode < MicrocodeEnd);
>  
> -Done:
> +LoadMicrocode:
>    if (LatestRevision != 0) {
>      //
> -    // Save the detected microcode patch entry address (including the
> -    // microcode patch header) for each processor.
> +    // Save the detected microcode patch entry address (including the microcode
> +    // patch header) for each processor even it's the same as the loaded one.
>      // It will be used when building the microcode patch cache HOB.
>      //
> -    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr =
> -      (UINTN) MicrocodeData -  sizeof (CPU_MICROCODE_HEADER);
> +    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr = (UINTN) LatestMicrocode;
>    }
>  
> -  if (LatestRevision > CurrentRevision) {
> +  if (LatestRevision > GetProcessorMicrocodeSignature ()) {
>      //
>      // BIOS only authenticate updates that contain a numerically larger revision
>      // than the currently loaded revision, where Current Signature < New Update
>      // Revision. A processor with no loaded update is considered to have a
>      // revision equal to zero.
>      //
> -    ASSERT (MicrocodeData != NULL);
> -    AsmWriteMsr64 (
> -        MSR_IA32_BIOS_UPDT_TRIG,
> -        (UINT64) (UINTN) MicrocodeData
> -        );
> -  }
> -  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();
> -}
> -
> -/**
> -  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
> -                                    supported by a microcode patch.
> -  @param[in]  ProcessorFlags        The prcessor flags field value supported by
> -                                    a microcode patch.
> -
> -  @retval TRUE     The specified microcode patch will be loaded.
> -  @retval FALSE    The specified microcode patch will not be loaded.
> -**/
> -BOOLEAN
> -IsProcessorMatchedMicrocodePatch (
> -  IN CPU_MP_DATA                 *CpuMpData,
> -  IN UINT32                      ProcessorSignature,
> -  IN UINT32                      ProcessorFlags
> -  )
> -{
> -  UINTN          Index;
> -  CPU_AP_DATA    *CpuData;
> -
> -  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> -    CpuData = &CpuMpData->CpuData[Index];
> -    if ((ProcessorSignature == CpuData->ProcessorSignature) &&
> -        (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {
> -      return TRUE;
> -    }
> +    LoadMicrocode (LatestMicrocode);
>    }
> -
> -  return FALSE;
> -}
> -
> -/**
> -  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.
> +  // It's possible that the microcode fails to load. Just capture the CPU microcode revision after loading.
>    //
> -  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;
> +  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetProcessorMicrocodeSignature ();
>  }
>  
> -
>  /**
>    Actual worker function that shadows the required microcode patches into memory.
>  
> @@ -491,14 +184,16 @@ ShadowMicrocodePatchByPcd (
>    IN OUT CPU_MP_DATA             *CpuMpData
>    )
>  {
> +  UINTN                                  Index;
>    CPU_MICROCODE_HEADER                   *MicrocodeEntryPoint;
>    UINTN                                  MicrocodeEnd;
> -  UINTN                                  DataSize;
>    UINTN                                  TotalSize;
>    MICROCODE_PATCH_INFO                   *PatchInfoBuffer;
>    UINTN                                  MaxPatchNumber;
>    UINTN                                  PatchCount;
>    UINTN                                  TotalLoadSize;
> +  EDKII_PEI_MICROCODE_CPU_ID             *MicrocodeCpuIds;
> +  BOOLEAN                                Valid;
>  
>    //
>    // Initialize the microcode patch related fields in CpuMpData as the values
> @@ -526,12 +221,34 @@ ShadowMicrocodePatchByPcd (
>      return;
>    }
>  
> +  MicrocodeCpuIds = AllocatePages (
> +                      EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID))
> +                      );
> +  if (MicrocodeCpuIds == NULL) {
> +    FreePool (PatchInfoBuffer);
> +    return;
> +  }
> +
> +  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +    MicrocodeCpuIds[Index].PlatformId         = CpuMpData->CpuData[Index].PlatformId;
> +    MicrocodeCpuIds[Index].ProcessorSignature = CpuMpData->CpuData[Index].ProcessorSignature;
> +  }
> +
>    //
>    // Process the header of each microcode patch within the region.
>    // The purpose is to decide which microcode patch(es) will be loaded into memory.
> +  // Microcode checksum is not verified because it's slow when performing on flash.
>    //
>    do {
> -    if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
> +    Valid = IsValidMicrocode (
> +              MicrocodeEntryPoint,
> +              MicrocodeEnd - (UINTN) MicrocodeEntryPoint,
> +              0,
> +              MicrocodeCpuIds,
> +              CpuMpData->CpuCount,
> +              FALSE
> +              );
> +    if (!Valid) {
>        //
>        // Padding data between the microcode patches, skip 1KB to check next entry.
>        //
> @@ -539,59 +256,44 @@ ShadowMicrocodePatchByPcd (
>        continue;
>      }
>  
> -    DataSize  = MicrocodeEntryPoint->DataSize;
> -    TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
> -    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
> -         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
> -         (DataSize & 0x3) != 0 ||
> -         (TotalSize & (SIZE_1KB - 1)) != 0 ||
> -         TotalSize < DataSize
> -       ) {
> +    PatchCount++;
> +    if (PatchCount > MaxPatchNumber) {
>        //
> -      // Not a valid microcode header, skip 1KB to check next entry.
> +      // Current 'PatchInfoBuffer' cannot hold the information, double the size
> +      // and allocate a new buffer.
>        //
> -      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
> -      continue;
> -    }
> -
> -    if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
> -      PatchCount++;
> -      if (PatchCount > MaxPatchNumber) {
> +      if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {
>          //
> -        // Current 'PatchInfoBuffer' cannot hold the information, double the size
> -        // and allocate a new buffer.
> +        // Overflow check for MaxPatchNumber
>          //
> -        if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {
> -          //
> -          // Overflow check for MaxPatchNumber
> -          //
> -          goto OnExit;
> -        }
> -
> -        PatchInfoBuffer = ReallocatePool (
> -                            MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> -                            2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> -                            PatchInfoBuffer
> -                            );
> -        if (PatchInfoBuffer == NULL) {
> -          goto OnExit;
> -        }
> -        MaxPatchNumber = MaxPatchNumber * 2;
> +        goto OnExit;
>        }
>  
> -      //
> -      // Store the information of this microcode patch
> -      //
> -      PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;
> -      PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;
> -      TotalLoadSize += TotalSize;
> +      PatchInfoBuffer = ReallocatePool (
> +                          MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> +                          2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
> +                          PatchInfoBuffer
> +                          );
> +      if (PatchInfoBuffer == NULL) {
> +        goto OnExit;
> +      }
> +      MaxPatchNumber = MaxPatchNumber * 2;
>      }
>  
> +    TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);
> +
> +    //
> +    // Store the information of this microcode patch
> +    //
> +    PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;
> +    PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;
> +    TotalLoadSize += TotalSize;
> +
>      //
>      // Process the next microcode patch
>      //
> -    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);
> -  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> +    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) ((UINTN) MicrocodeEntryPoint + TotalSize);
> +  } while ((UINTN) MicrocodeEntryPoint < MicrocodeEnd);
>  
>    if (PatchCount != 0) {
>      DEBUG ((
> @@ -607,7 +309,7 @@ OnExit:
>    if (PatchInfoBuffer != NULL) {
>      FreePool (PatchInfoBuffer);
>    }
> -  return;
> +  FreePages (MicrocodeCpuIds, EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID)));
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 66f9eb2304..e88a5355c9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -32,6 +32,7 @@
>  #include <Library/MtrrLib.h>
>  #include <Library/HobLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/MicrocodeLib.h>
>  
>  #include <Guid/MicrocodePatchHob.h>
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 49b0ffe8be..36fcb96b58 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -51,6 +51,7 @@ [LibraryClasses]
>    PeiServicesLib
>    PcdLib
>    VmgExitLib
> +  MicrocodeLib
>  
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
> 


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

* Re: [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib
  2021-04-02  5:58 ` [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib Ni, Ray
@ 2021-04-08  1:56   ` Ma, Maurice
  0 siblings, 0 replies; 14+ messages in thread
From: Ma, Maurice @ 2021-04-08  1:56 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Guo, You, Benjamin

Reviewed-by:  Maurice Ma <maurice.ma@intel.com>

-Maurice

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, April 1, 2021 22:58
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume
> MicrocodeLib
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> ---
>  UefiPayloadPkg/UefiPayloadPkg.dsc | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc
> b/UefiPayloadPkg/UefiPayloadPkg.dsc
> index e3b017858e..37ad5a0ae7 100644
> --- a/UefiPayloadPkg/UefiPayloadPkg.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
> @@ -180,6 +180,7 @@ [LibraryClasses]
>    #
> 
>    MtrrLib|UefiCpuPkg/Library/MtrrLib/MtrrLib.inf
> 
> 
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.in
> f
> 
> +  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> 
> 
> 
>    #
> 
>    # Platform
> 
> --
> 2.27.0.windows.1


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

* Re: [PATCH 1/4] UefiCpuPkg: Add MicrocodeLib for loading microcode
  2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
  2021-04-07 13:05   ` [edk2-devel] " Laszlo Ersek
@ 2021-04-08  2:19   ` Dong, Eric
  1 sibling, 0 replies; 14+ messages in thread
From: Dong, Eric @ 2021-04-08  2:19 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Rahul1

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

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, April 2, 2021 1:58 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH 1/4] UefiCpuPkg: Add MicrocodeLib for loading microcode

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Include/Library/MicrocodeLib.h     | 120 +++++++
 .../Library/MicrocodeLib/MicrocodeLib.c       | 322 ++++++++++++++++++
 .../Library/MicrocodeLib/MicrocodeLib.inf     |  32 ++
 UefiCpuPkg/UefiCpuPkg.dec                     |   5 +-
 UefiCpuPkg/UefiCpuPkg.dsc                     |   1 +
 5 files changed, 479 insertions(+), 1 deletion(-)
 create mode 100644 UefiCpuPkg/Include/Library/MicrocodeLib.h
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
 create mode 100644 UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

diff --git a/UefiCpuPkg/Include/Library/MicrocodeLib.h b/UefiCpuPkg/Include/Library/MicrocodeLib.h
new file mode 100644
index 0000000000..2570c43cce
--- /dev/null
+++ b/UefiCpuPkg/Include/Library/MicrocodeLib.h
@@ -0,0 +1,120 @@
+/** @file

+  Public include file for Microcode library.

+

+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+

+#ifndef __MICROCODE_LIB_H__

+#define __MICROCODE_LIB_H__

+

+#include <Register/Intel/Microcode.h>

+#include <Ppi/ShadowMicrocode.h>

+

+/**

+  Get microcode update signature of currently loaded microcode update.

+

+  @return  Microcode signature.

+**/

+UINT32

+EFIAPI

+GetProcessorMicrocodeSignature (

+  VOID

+  );

+

+/**

+  Get the processor signature and platform ID for current processor.

+

+  @param MicrocodeCpuId  Return the processor signature and platform ID.

+**/

+VOID

+EFIAPI

+GetProcessorMicrocodeCpuId (

+  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId

+  );

+

+/**

+  Return the total size of the microcode entry.

+

+  Logic follows pseudo code in SDM as below:

+

+     N = 512

+     If (Update.DataSize != 00000000H)

+       N = Update.TotalSize / 4

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode  Pointer to the microcode entry.

+

+  @return The microcode total size.

+**/

+UINT32

+EFIAPI

+GetMicrocodeLength (

+  IN CPU_MICROCODE_HEADER *Microcode

+  );

+

+/**

+  Load the microcode to the processor.

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode  Pointer to the microcode entry.

+**/

+VOID

+EFIAPI

+LoadMicrocode (

+  IN CPU_MICROCODE_HEADER *Microcode

+  );

+

+/**

+  Detect whether specified processor can find matching microcode patch and load it.

+

+  Microcode format is as below:

+  +----------------------------------------+-------------------------------------------------+

+  |          CPU_MICROCODE_HEADER          |                                                 |

+  +----------------------------------------+                                                 V

+  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum

+  +----------------------------------------+-------+                                         ^

+  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |

+  +----------------------------------------+       V                                         |

+  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |

+  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |

+  |                   ...                  |       |                                         |

+  +----------------------------------------+-------+-----------------------------------------+

+

+  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.

+  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount

+  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode            Pointer to a microcode entry.

+  @param MicrocodeLength      The total length of the microcode entry.

+  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.

+                              Caller can supply value get from GetProcessorMicrocodeSignature() to check

+                              whether the microcode is newer than loaded one.

+                              Caller can supply 0 to treat any revision (except 0) microcode as valid.

+  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents

+                              a set of processors.

+                              Caller can supply zero-element array to skip the processor signature and

+                              platform ID check.

+  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.

+  @param VerifyChecksum       FALSE to skip all the checksum verifications.

+

+  @retval TRUE  The microcode is valid.

+  @retval FALSE The microcode is invalid.

+**/

+BOOLEAN

+EFIAPI

+IsValidMicrocode (

+  IN CPU_MICROCODE_HEADER       *Microcode,

+  IN UINTN                      MicrocodeLength,

+  IN UINT32                     MinimumRevision,

+  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,

+  IN UINTN                      MicrocodeCpuIdCount,

+  IN BOOLEAN                    VerifyChecksum

+  );

+

+#endif
\ No newline at end of file
diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
new file mode 100644
index 0000000000..03a43fdae7
--- /dev/null
+++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.c
@@ -0,0 +1,322 @@
+/** @file

+  Implementation of MicrocodeLib.

+

+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

+  SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+

+#include <Uefi/UefiBaseType.h>

+#include <Register/Intel/Cpuid.h>

+#include <Register/Intel/ArchitecturalMsr.h>

+#include <Register/Intel/Microcode.h>

+#include <Library/BaseLib.h>

+#include <Library/DebugLib.h>

+#include <Ppi/ShadowMicrocode.h>

+

+/**

+  Get microcode update signature of currently loaded microcode update.

+

+  @return  Microcode signature.

+**/

+UINT32

+EFIAPI

+GetProcessorMicrocodeSignature (

+  VOID

+  )

+{

+  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;

+

+  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);

+  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);

+  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);

+  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;

+}

+

+/**

+  Get the processor signature and platform ID for current processor.

+

+  @param MicrocodeCpuId  Return the processor signature and platform ID.

+**/

+VOID

+EFIAPI

+GetProcessorMicrocodeCpuId (

+  EDKII_PEI_MICROCODE_CPU_ID  *MicrocodeCpuId

+  )

+{

+  MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;

+

+  ASSERT (MicrocodeCpuId != NULL);

+

+  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);

+  MicrocodeCpuId->PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;

+  AsmCpuid (CPUID_VERSION_INFO, &MicrocodeCpuId->ProcessorSignature, NULL, NULL, NULL);

+}

+

+/**

+  Return the total size of the microcode entry.

+

+  Logic follows pseudo code in SDM as below:

+

+     N = 512

+     If (Update.DataSize != 00000000H)

+       N = Update.TotalSize / 4

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode  Pointer to the microcode entry.

+

+  @return The microcode total size.

+**/

+UINT32

+EFIAPI

+GetMicrocodeLength (

+  IN CPU_MICROCODE_HEADER *Microcode

+  )

+{

+  UINT32   TotalSize;

+

+  ASSERT (Microcode != NULL);

+

+  TotalSize = 2048;

+  if (Microcode->DataSize != 0) {

+    TotalSize = Microcode->TotalSize;

+  }

+  return TotalSize;

+}

+

+/**

+  Load the microcode to the processor.

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode  Pointer to the microcode entry.

+**/

+VOID

+EFIAPI

+LoadMicrocode (

+  IN CPU_MICROCODE_HEADER *Microcode

+  )

+{

+  ASSERT (Microcode != NULL);

+

+  AsmWriteMsr64 (MSR_IA32_BIOS_UPDT_TRIG, (UINT64) (UINTN) (Microcode + 1));

+}

+

+/**

+  Determine if a microcode patch matchs the specific processor signature and flag.

+

+  @param[in]  ProcessorSignature    The processor signature field value in a

+                                    microcode patch.

+  @param[in]  ProcessorFlags        The processor flags field value in a

+                                    microcode patch.

+  @param[in]  MicrocodeCpuId        A pointer to an array of EDKII_PEI_MICROCODE_CPU_ID

+                                    structures.

+  @param[in]  MicrocodeCpuIdCount   Number of elements in MicrocodeCpuId array.

+

+  @retval TRUE     The specified microcode patch matches to one of the MicrocodeCpuId.

+  @retval FALSE    The specified microcode patch doesn't match to any of the MicrocodeCpuId.

+**/

+BOOLEAN

+IsProcessorMatchedMicrocode (

+  IN UINT32                           ProcessorSignature,

+  IN UINT32                           ProcessorFlags,

+  IN EDKII_PEI_MICROCODE_CPU_ID       *MicrocodeCpuId,

+  IN UINTN                            MicrocodeCpuIdCount

+  )

+{

+  UINTN          Index;

+

+  if (MicrocodeCpuIdCount == 0) {

+    return TRUE;

+  }

+

+  for (Index = 0; Index < MicrocodeCpuIdCount; Index++) {

+    if ((ProcessorSignature == MicrocodeCpuId[Index].ProcessorSignature) &&

+        (ProcessorFlags & (1 << MicrocodeCpuId[Index].PlatformId)) != 0) {

+      return TRUE;

+    }

+  }

+

+  return FALSE;

+}

+

+/**

+  Detect whether specified processor can find matching microcode patch and load it.

+

+  Microcode format is as below:

+  +----------------------------------------+-------------------------------------------------+

+  |          CPU_MICROCODE_HEADER          |                                                 |

+  +----------------------------------------+                                                 V

+  |              Update Data               |                                               CPU_MICROCODE_HEADER.Checksum

+  +----------------------------------------+-------+                                         ^

+  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |       |                                         |

+  +----------------------------------------+       V                                         |

+  |      CPU_MICROCODE_EXTENDED_TABLE[0]   |  CPU_MICROCODE_EXTENDED_TABLE_HEADER.Checksum   |

+  |      CPU_MICROCODE_EXTENDED_TABLE[1]   |       ^                                         |

+  |                   ...                  |       |                                         |

+  +----------------------------------------+-------+-----------------------------------------+

+

+  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.

+  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount

+  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.

+

+  If Microcode is NULL, then ASSERT.

+

+  @param Microcode            Pointer to a microcode entry.

+  @param MicrocodeLength      The total length of the microcode entry.

+  @param MinimumRevision      The microcode whose revision <= MinimumRevision is treated as invalid.

+                              Caller can supply value get from GetProcessorMicrocodeSignature() to check

+                              whether the microcode is newer than loaded one.

+                              Caller can supply 0 to treat any revision (except 0) microcode as valid.

+  @param MicrocodeCpuIds      Pointer to an array of processor signature and platform ID that represents

+                              a set of processors.

+                              Caller can supply zero-element array to skip the processor signature and

+                              platform ID check.

+  @param MicrocodeCpuIdCount  The number of elements in MicrocodeCpuIds.

+  @param VerifyChecksum       FALSE to skip all the checksum verifications.

+

+  @retval TRUE  The microcode is valid.

+  @retval FALSE The microcode is invalid.

+**/

+BOOLEAN

+EFIAPI

+IsValidMicrocode (

+  IN CPU_MICROCODE_HEADER       *Microcode,

+  IN UINTN                      MicrocodeLength,

+  IN UINT32                     MinimumRevision,

+  IN EDKII_PEI_MICROCODE_CPU_ID *MicrocodeCpuIds,

+  IN UINTN                      MicrocodeCpuIdCount,

+  IN BOOLEAN                    VerifyChecksum

+  )

+{

+  UINTN                                   Index;

+  UINT32                                  DataSize;

+  UINT32                                  TotalSize;

+  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;

+  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;

+  UINT32                                  ExtendedTableLength;

+  UINT32                                  Sum32;

+  BOOLEAN                                 Match;

+

+  ASSERT (Microcode != NULL);

+

+  //

+  // It's invalid when:

+  //   the input microcode buffer is so small that even cannot contain the header.

+  //   the input microcode buffer is so large that exceeds MAX_ADDRESS.

+  //

+  if ((MicrocodeLength < sizeof (CPU_MICROCODE_HEADER)) || (MicrocodeLength > (MAX_ADDRESS - (UINTN) Microcode))) {

+    return FALSE;

+  }

+

+  //

+  // Per SDM, HeaderVersion and LoaderRevision should both be 1.

+  //

+  if ((Microcode->HeaderVersion != 1) || (Microcode->LoaderRevision != 1)) {

+    return FALSE;

+  }

+

+  //

+  // The microcode revision should be larger than the minimum revision.

+  //

+  if (Microcode->UpdateRevision <= MinimumRevision) {

+    return FALSE;

+  }

+

+  DataSize = Microcode->DataSize;

+  if (DataSize == 0) {

+    DataSize = 2000;

+  }

+

+  //

+  // Per SDM, DataSize should be multiple of DWORDs.

+  //

+  if ((DataSize % 4) != 0) {

+    return FALSE;

+  }

+

+  TotalSize = GetMicrocodeLength (Microcode);

+

+  //

+  // Check whether the whole microcode is within the buffer.

+  // TotalSize should be multiple of 1024.

+  //

+  if (((TotalSize % SIZE_1KB) != 0) || (TotalSize > MicrocodeLength)) {

+    return FALSE;

+  }

+

+  //

+  // The summation of all DWORDs in microcode should be zero.

+  //

+  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) Microcode, TotalSize) != 0)) {

+    return FALSE;

+  }

+

+  Sum32 = Microcode->ProcessorSignature.Uint32 + Microcode->ProcessorFlags + Microcode->Checksum;

+

+  //

+  // Check the processor signature and platform ID in the primary header.

+  //

+  Match = IsProcessorMatchedMicrocode (

+            Microcode->ProcessorSignature.Uint32,

+            Microcode->ProcessorFlags,

+            MicrocodeCpuIds,

+            MicrocodeCpuIdCount

+            );

+  if (Match) {

+    return TRUE;

+  }

+

+  ExtendedTableLength = TotalSize - (DataSize + sizeof (CPU_MICROCODE_HEADER));

+  if ((ExtendedTableLength < sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) || ((ExtendedTableLength % 4) != 0)) {

+    return FALSE;

+  }

+  //

+  // Extended Table exist, check if the CPU in support list

+  //

+  ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINTN) (Microcode + 1) + DataSize);

+  if (ExtendedTableHeader->ExtendedSignatureCount > MAX_UINT32 / sizeof (CPU_MICROCODE_EXTENDED_TABLE)) {

+    return FALSE;

+  }

+  if (ExtendedTableHeader->ExtendedSignatureCount * sizeof (CPU_MICROCODE_EXTENDED_TABLE)

+      > ExtendedTableLength - sizeof (CPU_MICROCODE_EXTENDED_TABLE_HEADER)) {

+    return FALSE;

+  }

+  //

+  // Check the extended table checksum

+  //

+  if (VerifyChecksum && (CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength) != 0)) {

+    return FALSE;

+  }

+

+  ExtendedTable = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);

+  for (Index = 0; Index < ExtendedTableHeader->ExtendedSignatureCount; Index ++) {

+    if (VerifyChecksum &&

+        (ExtendedTable[Index].ProcessorSignature.Uint32 + ExtendedTable[Index].ProcessorFlag

+        + ExtendedTable[Index].Checksum != Sum32)) {

+      //

+      // The extended table entry is valid when the summation of Processor Signature, Processor Flags

+      // and Checksum equal to the coresponding summation from primary header. Because:

+      //    CalculateSum32 (Header + Update Binary) == 0

+      //    CalculateSum32 (Header + Update Binary)

+      //        - (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)

+      //        + (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum) == 0

+      // So,

+      //    (Header.ProcessorSignature + Header.ProcessorFlag + Header.Checksum)

+      //     == (Extended.ProcessorSignature + Extended.ProcessorFlag + Extended.Checksum)

+      //

+      continue;

+    }

+    Match = IsProcessorMatchedMicrocode (

+              ExtendedTable[Index].ProcessorSignature.Uint32,

+              ExtendedTable[Index].ProcessorFlag,

+              MicrocodeCpuIds,

+              MicrocodeCpuIdCount

+              );

+    if (Match) {

+      return TRUE;

+    }

+  }

+  return FALSE;

+}

diff --git a/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
new file mode 100644
index 0000000000..c6f8f52e95
--- /dev/null
+++ b/UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
@@ -0,0 +1,32 @@
+##  @file

+#   Library for microcode verification and load.

+#

+#  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

+#

+#  SPDX-License-Identifier: BSD-2-Clause-Patent

+#

+#

+##

+

+[Defines]

+  INF_VERSION                    = 0x00010006

+  BASE_NAME                      = MicrocodeLib

+  FILE_GUID                      = EB8C72BC-8A48-4F80-996B-E52F68416D57

+  MODULE_TYPE                    = BASE

+  VERSION_STRING                 = 1.0

+  LIBRARY_CLASS                  = MicrocodeLib

+

+#

+#  VALID_ARCHITECTURES           = IA32 X64 EBC

+#

+

+[Sources.common]

+  MicrocodeLib.c

+

+[Packages]

+  MdePkg/MdePkg.dec

+  UefiCpuPkg/UefiCpuPkg.dec

+

+[LibraryClasses]

+  BaseLib

+  DebugLib

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index a639ce5412..62acb291f3 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -1,7 +1,7 @@
 ## @file  UefiCpuPkg.dec

 # This Package provides UEFI compatible CPU modules and libraries.

 #

-# Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>

+# Copyright (c) 2007 - 2021, Intel Corporation. All rights reserved.<BR>

 #

 # SPDX-License-Identifier: BSD-2-Clause-Patent

 #

@@ -59,6 +59,9 @@ [LibraryClasses.IA32, LibraryClasses.X64]
   ##  @libraryclass  Provides function to get CPU cache information.

   CpuCacheInfoLib|Include/Library/CpuCacheInfoLib.h

 

+  ##  @libraryclass  Provides function for loading microcode.

+  MicrocodeLib|Include/Library/MicrocodeLib.h

+

 [Guids]

   gUefiCpuPkgTokenSpaceGuid      = { 0xac05bf33, 0x995a, 0x4ed4, { 0xaa, 0xb8, 0xef, 0x7a, 0xe8, 0xf, 0x5c, 0xb0 }}

   gMsegSmramGuid                 = { 0x5802bce4, 0xeeee, 0x4e33, { 0xa1, 0x30, 0xeb, 0xad, 0x27, 0xf0, 0xe4, 0x39 }}

diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 98c4c53465..b932cf63ec 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -60,6 +60,7 @@ [LibraryClasses]
   PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf

   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf

   VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf

+  MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf

 

 [LibraryClasses.common.SEC]

   PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf

-- 
2.27.0.windows.1


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

* Re: [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code
  2021-04-02  5:58 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code Ni, Ray
  2021-04-07 13:08   ` [edk2-devel] " Laszlo Ersek
@ 2021-04-08 14:24   ` Dong, Eric
  1 sibling, 0 replies; 14+ messages in thread
From: Dong, Eric @ 2021-04-08 14:24 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Rahul1

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

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, April 2, 2021 1:58 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |   1 +
 UefiCpuPkg/Library/MpInitLib/Microcode.c      | 484 ++++--------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h          |   1 +
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |   1 +
 4 files changed, 96 insertions(+), 391 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 860a9750e2..d34419c2a5 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
   SynchronizationLib

   PcdLib

   VmgExitLib

+  MicrocodeLib

 

 [Protocols]

   gEfiTimerArchProtocolGuid                     ## SOMETIMES_CONSUMES

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 297c2abcd1..105a9f84bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -1,70 +1,16 @@
 /** @file

   Implementation of loading microcode on processors.

 

-  Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>

+  Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

 

 **/

 

 #include "MpLib.h"

 

-/**

-  Get microcode update signature of currently loaded microcode update.

-

-  @return  Microcode signature.

-**/

-UINT32

-GetCurrentMicrocodeSignature (

-  VOID

-  )

-{

-  MSR_IA32_BIOS_SIGN_ID_REGISTER   BiosSignIdMsr;

-

-  AsmWriteMsr64 (MSR_IA32_BIOS_SIGN_ID, 0);

-  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, NULL);

-  BiosSignIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_BIOS_SIGN_ID);

-  return BiosSignIdMsr.Bits.MicrocodeUpdateSignature;

-}

-

 /**

   Detect whether specified processor can find matching microcode patch and load it.

 

-  Microcode Payload as the following format:

-  +----------------------------------------+------------------+

-  |          CPU_MICROCODE_HEADER          |                  |

-  +----------------------------------------+  CheckSum Part1  |

-  |            Microcode Binary            |                  |

-  +----------------------------------------+------------------+

-  |  CPU_MICROCODE_EXTENDED_TABLE_HEADER   |                  |

-  +----------------------------------------+  CheckSum Part2  |

-  |      CPU_MICROCODE_EXTENDED_TABLE      |                  |

-  |                   ...                  |                  |

-  +----------------------------------------+------------------+

-

-  There may by multiple CPU_MICROCODE_EXTENDED_TABLE in this format.

-  The count of CPU_MICROCODE_EXTENDED_TABLE is indicated by ExtendedSignatureCount

-  of CPU_MICROCODE_EXTENDED_TABLE_HEADER structure.

-

-  When we are trying to verify the CheckSum32 with extended table.

-  We should use the fields of exnteded table to replace the corresponding

-  fields in CPU_MICROCODE_HEADER structure, and recalculate the

-  CheckSum32 with CPU_MICROCODE_HEADER + Microcode Binary. We named

-  it as CheckSum Part3.

-

-  The CheckSum Part2 is used to verify the CPU_MICROCODE_EXTENDED_TABLE_HEADER

-  and CPU_MICROCODE_EXTENDED_TABLE parts. We should make sure CheckSum Part2

-  is correct before we are going to verify each CPU_MICROCODE_EXTENDED_TABLE.

-

-  Only ProcessorSignature, ProcessorFlag and CheckSum are different between

-  CheckSum Part1 and CheckSum Part3. To avoid multiple computing CheckSum Part3.

-  Save an in-complete CheckSum32 from CheckSum Part1 for common parts.

-  When we are going to calculate CheckSum32, just should use the corresponding part

-  of the ProcessorSignature, ProcessorFlag and CheckSum with in-complete CheckSum32.

-

-  Notes: CheckSum32 is not a strong verification.

-         It does not guarantee that the data has not been modified.

-         CPU has its own mechanism to verify Microcode Binary part.

-

   @param[in]  CpuMpData        The pointer to CPU MP Data structure.

   @param[in]  ProcessorNumber  The handle number of the processor. The range is

                                from 0 to the total number of logical processors

@@ -76,26 +22,13 @@ MicrocodeDetect (
   IN UINTN                   ProcessorNumber

   )

 {

-  UINT32                                  ExtendedTableLength;

-  UINT32                                  ExtendedTableCount;

-  CPU_MICROCODE_EXTENDED_TABLE            *ExtendedTable;

-  CPU_MICROCODE_EXTENDED_TABLE_HEADER     *ExtendedTableHeader;

-  CPU_MICROCODE_HEADER                    *MicrocodeEntryPoint;

+  CPU_MICROCODE_HEADER                    *Microcode;

   UINTN                                   MicrocodeEnd;

-  UINTN                                   Index;

-  UINT8                                   PlatformId;

-  CPUID_VERSION_INFO_EAX                  Eax;

-  CPU_AP_DATA                             *CpuData;

-  UINT32                                  CurrentRevision;

+  CPU_AP_DATA                             *BspData;

   UINT32                                  LatestRevision;

-  UINTN                                   TotalSize;

-  UINT32                                  CheckSum32;

-  UINT32                                  InCompleteCheckSum32;

-  BOOLEAN                                 CorrectMicrocode;

-  VOID                                    *MicrocodeData;

-  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;

+  CPU_MICROCODE_HEADER                    *LatestMicrocode;

   UINT32                                  ThreadId;

-  BOOLEAN                                 IsBspCallIn;

+  EDKII_PEI_MICROCODE_CPU_ID              MicrocodeCpuId;

 

   if (CpuMpData->MicrocodePatchRegionSize == 0) {

     //

@@ -104,9 +37,6 @@ MicrocodeDetect (
     return;

   }

 

-  CurrentRevision = GetCurrentMicrocodeSignature ();

-  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;

-

   GetProcessorLocationByApicId (GetInitialApicId (), NULL, NULL, &ThreadId);

   if (ThreadId != 0) {

     //

@@ -115,156 +45,35 @@ MicrocodeDetect (
     return;

   }

 

-  ExtendedTableLength = 0;

-  //

-  // Here data of CPUID leafs have not been collected into context buffer, so

-  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.

-  //

-  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);

+  GetProcessorMicrocodeCpuId (&MicrocodeCpuId);

 

-  //

-  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID

-  //

-  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);

-  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;

-

-

-  //

-  // Check whether AP has same processor with BSP.

-  // If yes, direct use microcode info saved by BSP.

-  //

-  if (!IsBspCallIn) {

+  if (ProcessorNumber != (UINTN) CpuMpData->BspNumber) {

     //

-    // Get the CPU data for BSP

+    // Direct use microcode of BSP if AP is the same as BSP.

+    // Assume BSP calls this routine() before AP.

     //

-    CpuData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);

-    if ((CpuData->ProcessorSignature == Eax.Uint32) &&

-        (CpuData->PlatformId == PlatformId) &&

-        (CpuData->MicrocodeEntryAddr != 0)) {

-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *)(UINTN) CpuData->MicrocodeEntryAddr;

-      MicrocodeData       = (VOID *) (MicrocodeEntryPoint + 1);

-      LatestRevision      = MicrocodeEntryPoint->UpdateRevision;

-      goto Done;

+    BspData = &(CpuMpData->CpuData[CpuMpData->BspNumber]);

+    if ((BspData->ProcessorSignature == MicrocodeCpuId.ProcessorSignature) &&

+        (BspData->PlatformId == MicrocodeCpuId.PlatformId) &&

+        (BspData->MicrocodeEntryAddr != 0)) {

+      LatestMicrocode = (CPU_MICROCODE_HEADER *)(UINTN) BspData->MicrocodeEntryAddr;

+      LatestRevision  = LatestMicrocode->UpdateRevision;

+      goto LoadMicrocode;

     }

   }

 

-  LatestRevision = 0;

-  MicrocodeData  = NULL;

-  MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress + CpuMpData->MicrocodePatchRegionSize);

-  MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;

+  //

+  // BSP or AP which is different from BSP runs here

+  // Use 0 as the starting revision to search for microcode because MicrocodePatchInfo HOB needs

+  // the latest microcode location even it's loaded to the processor.

+  //

+  LatestRevision  = 0;

+  LatestMicrocode = NULL;

+  Microcode       = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;

+  MicrocodeEnd    = (UINTN) Microcode + (UINTN) CpuMpData->MicrocodePatchRegionSize;

 

   do {

-    //

-    // Check if the microcode is for the Cpu and the version is newer

-    // and the update can be processed on the platform

-    //

-    CorrectMicrocode = FALSE;

-

-    if (MicrocodeEntryPoint->DataSize == 0) {

-      TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;

-    } else {

-      TotalSize = sizeof (CPU_MICROCODE_HEADER) + MicrocodeEntryPoint->DataSize;

-    }

-

-    ///

-    /// 0x0       MicrocodeBegin  MicrocodeEntry  MicrocodeEnd      0xffffffff

-    /// |--------------|---------------|---------------|---------------|

-    ///                                 valid TotalSize

-    /// TotalSize is only valid between 0 and (MicrocodeEnd - MicrocodeEntry).

-    /// And it should be aligned with 4 bytes.

-    /// If the TotalSize is invalid, skip 1KB to check next entry.

-    ///

-    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||

-         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||

-         (TotalSize & 0x3) != 0

-       ) {

-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

-      continue;

-    }

-

-    //

-    // Save an in-complete CheckSum32 from CheckSum Part1 for common parts.

-    //

-    InCompleteCheckSum32 = CalculateSum32 (

-                             (UINT32 *) MicrocodeEntryPoint,

-                             TotalSize

-                             );

-    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorSignature.Uint32;

-    InCompleteCheckSum32 -= MicrocodeEntryPoint->ProcessorFlags;

-    InCompleteCheckSum32 -= MicrocodeEntryPoint->Checksum;

-

-    if (MicrocodeEntryPoint->HeaderVersion == 0x1) {

-      //

-      // It is the microcode header. It is not the padding data between microcode patches

-      // because the padding data should not include 0x00000001 and it should be the repeated

-      // byte format (like 0xXYXYXYXY....).

-      //

-      if (MicrocodeEntryPoint->ProcessorSignature.Uint32 == Eax.Uint32 &&

-          MicrocodeEntryPoint->UpdateRevision > LatestRevision &&

-          (MicrocodeEntryPoint->ProcessorFlags & (1 << PlatformId))

-          ) {

-        //

-        // Calculate CheckSum Part1.

-        //

-        CheckSum32 = InCompleteCheckSum32;

-        CheckSum32 += MicrocodeEntryPoint->ProcessorSignature.Uint32;

-        CheckSum32 += MicrocodeEntryPoint->ProcessorFlags;

-        CheckSum32 += MicrocodeEntryPoint->Checksum;

-        if (CheckSum32 == 0) {

-          CorrectMicrocode = TRUE;

-        }

-      } else if ((MicrocodeEntryPoint->DataSize != 0) &&

-                 (MicrocodeEntryPoint->UpdateRevision > LatestRevision)) {

-        ExtendedTableLength = MicrocodeEntryPoint->TotalSize - (MicrocodeEntryPoint->DataSize +

-                                sizeof (CPU_MICROCODE_HEADER));

-        if (ExtendedTableLength != 0) {

-          //

-          // Extended Table exist, check if the CPU in support list

-          //

-          ExtendedTableHeader = (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *) (MicrocodeEntryPoint)

-                                  + MicrocodeEntryPoint->DataSize + sizeof (CPU_MICROCODE_HEADER));

-          //

-          // Calculate Extended Checksum

-          //

-          if ((ExtendedTableLength % 4) == 0) {

-            //

-            // Calculate CheckSum Part2.

-            //

-            CheckSum32 = CalculateSum32 ((UINT32 *) ExtendedTableHeader, ExtendedTableLength);

-            if (CheckSum32 == 0) {

-              //

-              // Checksum correct

-              //

-              ExtendedTableCount = ExtendedTableHeader->ExtendedSignatureCount;

-              ExtendedTable      = (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader + 1);

-              for (Index = 0; Index < ExtendedTableCount; Index ++) {

-                //

-                // Calculate CheckSum Part3.

-                //

-                CheckSum32 = InCompleteCheckSum32;

-                CheckSum32 += ExtendedTable->ProcessorSignature.Uint32;

-                CheckSum32 += ExtendedTable->ProcessorFlag;

-                CheckSum32 += ExtendedTable->Checksum;

-                if (CheckSum32 == 0) {

-                  //

-                  // Verify Header

-                  //

-                  if ((ExtendedTable->ProcessorSignature.Uint32 == Eax.Uint32) &&

-                      (ExtendedTable->ProcessorFlag & (1 << PlatformId)) ) {

-                    //

-                    // Find one

-                    //

-                    CorrectMicrocode = TRUE;

-                    break;

-                  }

-                }

-                ExtendedTable ++;

-              }

-            }

-          }

-        }

-      }

-    } else {

+    if (!IsValidMicrocode (Microcode, MicrocodeEnd - (UINTN) Microcode, LatestRevision, &MicrocodeCpuId, 1, TRUE)) {

       //

       // It is the padding data between the microcode patches for microcode patches alignment.

       // Because the microcode patch is the multiple of 1-KByte, the padding data should not

@@ -272,156 +81,40 @@ MicrocodeDetect (
       // alignment value should be larger than 1-KByte. We could skip SIZE_1KB padding data to

       // find the next possible microcode patch header.

       //

-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

+      Microcode = (CPU_MICROCODE_HEADER *) ((UINTN) Microcode + SIZE_1KB);

       continue;

     }

-    //

-    // Get the next patch.

-    //

-    if (MicrocodeEntryPoint->DataSize == 0) {

-      TotalSize = 2048;

-    } else {

-      TotalSize = MicrocodeEntryPoint->TotalSize;

-    }

+    LatestMicrocode = Microcode;

+    LatestRevision  = LatestMicrocode->UpdateRevision;

 

-    if (CorrectMicrocode) {

-      LatestRevision = MicrocodeEntryPoint->UpdateRevision;

-      MicrocodeData = (VOID *) ((UINTN) MicrocodeEntryPoint + sizeof (CPU_MICROCODE_HEADER));

-    }

-

-    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);

-  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));

+    Microcode = (CPU_MICROCODE_HEADER *) (((UINTN) Microcode) + GetMicrocodeLength (Microcode));

+  } while ((UINTN) Microcode < MicrocodeEnd);

 

-Done:

+LoadMicrocode:

   if (LatestRevision != 0) {

     //

-    // Save the detected microcode patch entry address (including the

-    // microcode patch header) for each processor.

+    // Save the detected microcode patch entry address (including the microcode

+    // patch header) for each processor even it's the same as the loaded one.

     // It will be used when building the microcode patch cache HOB.

     //

-    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr =

-      (UINTN) MicrocodeData -  sizeof (CPU_MICROCODE_HEADER);

+    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr = (UINTN) LatestMicrocode;

   }

 

-  if (LatestRevision > CurrentRevision) {

+  if (LatestRevision > GetProcessorMicrocodeSignature ()) {

     //

     // BIOS only authenticate updates that contain a numerically larger revision

     // than the currently loaded revision, where Current Signature < New Update

     // Revision. A processor with no loaded update is considered to have a

     // revision equal to zero.

     //

-    ASSERT (MicrocodeData != NULL);

-    AsmWriteMsr64 (

-        MSR_IA32_BIOS_UPDT_TRIG,

-        (UINT64) (UINTN) MicrocodeData

-        );

-  }

-  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();

-}

-

-/**

-  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

-                                    supported by a microcode patch.

-  @param[in]  ProcessorFlags        The prcessor flags field value supported by

-                                    a microcode patch.

-

-  @retval TRUE     The specified microcode patch will be loaded.

-  @retval FALSE    The specified microcode patch will not be loaded.

-**/

-BOOLEAN

-IsProcessorMatchedMicrocodePatch (

-  IN CPU_MP_DATA                 *CpuMpData,

-  IN UINT32                      ProcessorSignature,

-  IN UINT32                      ProcessorFlags

-  )

-{

-  UINTN          Index;

-  CPU_AP_DATA    *CpuData;

-

-  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

-    CpuData = &CpuMpData->CpuData[Index];

-    if ((ProcessorSignature == CpuData->ProcessorSignature) &&

-        (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {

-      return TRUE;

-    }

+    LoadMicrocode (LatestMicrocode);

   }

-

-  return FALSE;

-}

-

-/**

-  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.

+  // It's possible that the microcode fails to load. Just capture the CPU microcode revision after loading.

   //

-  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;

+  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetProcessorMicrocodeSignature ();

 }

 

-

 /**

   Actual worker function that shadows the required microcode patches into memory.

 

@@ -491,14 +184,16 @@ ShadowMicrocodePatchByPcd (
   IN OUT CPU_MP_DATA             *CpuMpData

   )

 {

+  UINTN                                  Index;

   CPU_MICROCODE_HEADER                   *MicrocodeEntryPoint;

   UINTN                                  MicrocodeEnd;

-  UINTN                                  DataSize;

   UINTN                                  TotalSize;

   MICROCODE_PATCH_INFO                   *PatchInfoBuffer;

   UINTN                                  MaxPatchNumber;

   UINTN                                  PatchCount;

   UINTN                                  TotalLoadSize;

+  EDKII_PEI_MICROCODE_CPU_ID             *MicrocodeCpuIds;

+  BOOLEAN                                Valid;

 

   //

   // Initialize the microcode patch related fields in CpuMpData as the values

@@ -526,12 +221,34 @@ ShadowMicrocodePatchByPcd (
     return;

   }

 

+  MicrocodeCpuIds = AllocatePages (

+                      EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID))

+                      );

+  if (MicrocodeCpuIds == NULL) {

+    FreePool (PatchInfoBuffer);

+    return;

+  }

+

+  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

+    MicrocodeCpuIds[Index].PlatformId         = CpuMpData->CpuData[Index].PlatformId;

+    MicrocodeCpuIds[Index].ProcessorSignature = CpuMpData->CpuData[Index].ProcessorSignature;

+  }

+

   //

   // Process the header of each microcode patch within the region.

   // The purpose is to decide which microcode patch(es) will be loaded into memory.

+  // Microcode checksum is not verified because it's slow when performing on flash.

   //

   do {

-    if (MicrocodeEntryPoint->HeaderVersion != 0x1) {

+    Valid = IsValidMicrocode (

+              MicrocodeEntryPoint,

+              MicrocodeEnd - (UINTN) MicrocodeEntryPoint,

+              0,

+              MicrocodeCpuIds,

+              CpuMpData->CpuCount,

+              FALSE

+              );

+    if (!Valid) {

       //

       // Padding data between the microcode patches, skip 1KB to check next entry.

       //

@@ -539,59 +256,44 @@ ShadowMicrocodePatchByPcd (
       continue;

     }

 

-    DataSize  = MicrocodeEntryPoint->DataSize;

-    TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;

-    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||

-         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||

-         (DataSize & 0x3) != 0 ||

-         (TotalSize & (SIZE_1KB - 1)) != 0 ||

-         TotalSize < DataSize

-       ) {

+    PatchCount++;

+    if (PatchCount > MaxPatchNumber) {

       //

-      // Not a valid microcode header, skip 1KB to check next entry.

+      // Current 'PatchInfoBuffer' cannot hold the information, double the size

+      // and allocate a new buffer.

       //

-      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);

-      continue;

-    }

-

-    if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {

-      PatchCount++;

-      if (PatchCount > MaxPatchNumber) {

+      if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {

         //

-        // Current 'PatchInfoBuffer' cannot hold the information, double the size

-        // and allocate a new buffer.

+        // Overflow check for MaxPatchNumber

         //

-        if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {

-          //

-          // Overflow check for MaxPatchNumber

-          //

-          goto OnExit;

-        }

-

-        PatchInfoBuffer = ReallocatePool (

-                            MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

-                            2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

-                            PatchInfoBuffer

-                            );

-        if (PatchInfoBuffer == NULL) {

-          goto OnExit;

-        }

-        MaxPatchNumber = MaxPatchNumber * 2;

+        goto OnExit;

       }

 

-      //

-      // Store the information of this microcode patch

-      //

-      PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;

-      PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;

-      TotalLoadSize += TotalSize;

+      PatchInfoBuffer = ReallocatePool (

+                          MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

+                          2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),

+                          PatchInfoBuffer

+                          );

+      if (PatchInfoBuffer == NULL) {

+        goto OnExit;

+      }

+      MaxPatchNumber = MaxPatchNumber * 2;

     }

 

+    TotalSize = GetMicrocodeLength (MicrocodeEntryPoint);

+

+    //

+    // Store the information of this microcode patch

+    //

+    PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;

+    PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;

+    TotalLoadSize += TotalSize;

+

     //

     // Process the next microcode patch

     //

-    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);

-  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));

+    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) ((UINTN) MicrocodeEntryPoint + TotalSize);

+  } while ((UINTN) MicrocodeEntryPoint < MicrocodeEnd);

 

   if (PatchCount != 0) {

     DEBUG ((

@@ -607,7 +309,7 @@ OnExit:
   if (PatchInfoBuffer != NULL) {

     FreePool (PatchInfoBuffer);

   }

-  return;

+  FreePages (MicrocodeCpuIds, EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID)));

 }

 

 /**

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 66f9eb2304..e88a5355c9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -32,6 +32,7 @@
 #include <Library/MtrrLib.h>

 #include <Library/HobLib.h>

 #include <Library/PcdLib.h>

+#include <Library/MicrocodeLib.h>

 

 #include <Guid/MicrocodePatchHob.h>

 

diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 49b0ffe8be..36fcb96b58 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -51,6 +51,7 @@ [LibraryClasses]
   PeiServicesLib

   PcdLib

   VmgExitLib

+  MicrocodeLib

 

 [Pcd]

   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES

-- 
2.27.0.windows.1


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

end of thread, other threads:[~2021-04-08 14:24 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-04-02  5:58 [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Ni, Ray
2021-04-02  5:58 ` [PATCH 1/4] " Ni, Ray
2021-04-07 13:05   ` [edk2-devel] " Laszlo Ersek
2021-04-08  2:19   ` Dong, Eric
2021-04-02  5:58 ` [PATCH 2/4] OvmfPkg: Add MicrocodeLib in DSC files Ni, Ray
2021-04-07 13:05   ` [edk2-devel] " Laszlo Ersek
2021-04-02  5:58 ` [PATCH 3/4] UefiPayloadPkg/UefiPayloadPkg.dsc: Consume MicrocodeLib Ni, Ray
2021-04-08  1:56   ` Ma, Maurice
2021-04-02  5:58 ` [PATCH 4/4] UefiCpuPkg/MpInitLib: Consume MicrocodeLib to remove duplicated code Ni, Ray
2021-04-07 13:08   ` [edk2-devel] " Laszlo Ersek
2021-04-08 14:24   ` Dong, Eric
2021-04-06 12:03 ` [edk2-devel] [PATCH 0/4] UefiCpuPkg: Add MicrocodeLib for loading microcode Laszlo Ersek
2021-04-07  2:43   ` Ni, Ray
2021-04-07 13:04     ` Laszlo Ersek

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