* Re: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
[not found] ` <69d53dbbfe4bb2fdd27d5098850a9e91a43d63bb.1635405564.git.longlong.yang@intel.com>
@ 2021-11-02 1:55 ` Ni, Ray
[not found] ` <16B397E9723CF0A5.13015@groups.io>
1 sibling, 0 replies; 4+ messages in thread
From: Ni, Ray @ 2021-11-02 1:55 UTC (permalink / raw)
To: Yang, Longlong, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1, Yao, Jiewen, Xu, Min M, Zhang, Qi1
Longlong,
Your code creates a big buffer that holds microcode data for all threads.
MicrocodeCpu[i] = MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[i]
BigBuffer = GetMicrocodeBuffer (MicrocodeOfCpu[0]) + GetMicrocodeBuffer (MicrocodeOfCpu[1]) + ...
HashValue = Hash (BigBuffer)
I am not sure if we can do like below:
BigBuffer = <Entire microcode buffer pointed by MicrocodePatchHob->MicrocodePatchAddress> + <array content of MicrocodePatchHob->ProcessorSpecificPatchOffset[]>
HashValue = Hash (BigBuffer)
The second approach doesn't require sorting, one-by-one-copying.
Thanks,
Ray
-----Original Message-----
From: Yang, Longlong <longlong.yang@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.yang@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3683
TCG specification says BIOS should extend measurement of microcode to TPM.
However, reference BIOS is not doing this. This patch consumes gEdkiiMicrocodePatchHobGuid to checkout all applied microcode patches, then all applied microcode patches are packed in order to form a single binary blob which is measured with event type EV_CPU_MICROCODE to PCR[1] in TPM.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min M Xu <min.m.xu@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Signed-off-by: Longlong Yang <longlong.yang@intel.com>
---
.../MicrocodeMeasurementDxe.c | 254 ++++++++++++++++++
.../MicrocodeMeasurementDxe.inf | 58 ++++
.../MicrocodeMeasurementDxe.uni | 15 ++
| 12 +
UefiCpuPkg/UefiCpuPkg.dsc | 2 +
5 files changed, 341 insertions(+)
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
new file mode 100644
index 000000000000..1898a2bff023
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
@@ -0,0 +1,254 @@
+/** @file
+ This driver measures Microcode Patches to TPM.
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <IndustryStandard/UefiTcgPlatform.h>
+#include <Guid/EventGroup.h>
+#include <Guid/MicrocodePatchHob.h>
+#include <Library/DebugLib.h>
+#include <Library/UefiDriverEntryPoint.h> #include <Library/UefiLib.h>
+#include <Library/BaseLib.h> #include <Library/BaseMemoryLib.h>
+#include <Library/MemoryAllocationLib.h> #include
+<Library/UefiBootServicesTableLib.h>
+#include <Library/PrintLib.h>
+#include <Library/SortLib.h>
+#include <Library/HobLib.h>
+#include <Library/MicrocodeLib.h>
+#include <Library/TpmMeasurementLib.h>
+
+
+#define CPU_MICROCODE_MEASUREMENT_DESCRIPTION "Microcode Measurement"
+#define CPU_MICROCODE_MEASUREMENT_EVENT_LOG_DESCRIPTION_LEN
+sizeof(CPU_MICROCODE_MEASUREMENT_DESCRIPTION)
+
+#pragma pack(1)
+typedef struct {
+ UINTN Address;
+ UINTN Size;
+} MICROCODE_PATCH_TYPE;
+
+typedef struct {
+ UINT8 Description[CPU_MICROCODE_MEASUREMENT_EVENT_LOG_DESCRIPTION_LEN];
+ UINTN NumberOfMicrocodePatchesMeasured;
+ UINTN SizeOfMicrocodePatchesMeasured;
+} CPU_MICROCODE_MEASUREMENT_EVENT_LOG;
+#pragma pack()
+
+STATIC BOOLEAN mMicrocodeMeasured = FALSE;
+
+/**
+ The function is called by PerformQuickSort to compare the order of
+ addresses of two microcode patch in RAM.
+
+ @param[in] MicrocodePatch1 The pointer to the first microcode patch type structure.
+ @param[in] MicrocodePatch2 The pointer to the second microcode patch type structure.
+
+ @return 0 The address of MicrocodePatch1 in RAM equal to that of MicrocodePatch2.
+ @return <0 The address of MicrocodePatch1 in RAM is less than that of MicrocodePatch2.
+ @return >0 The address of MicrocodePatch1 in RAM is greater than that of MicrocodePatch2.
+**/
+INTN
+EFIAPI
+MicrocodePatchesListSortFunction (
+ IN CONST VOID *MicrocodePatch1,
+ IN CONST VOID *MicrocodePatch2
+ )
+{
+ return ((MICROCODE_PATCH_TYPE*)MicrocodePatch2)->Address -
+((MICROCODE_PATCH_TYPE*)MicrocodePatch1)->Address;
+}
+
+/**
+ Callback function, called after signaling of the Ready to Boot Event.
+ Measure microcode patches binary blob with event type
+EV_CPU_MICROCODE
+ to PCR[1] in TPM.
+
+ @param[in] Event Event whose notification function is being invoked.
+ @param[in] Context Pointer to the notification function's context.
+
+**/
+VOID
+EFIAPI
+MeasureMicrocodePatches (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+ UINT32 PCRIndex;
+ UINT32 EventType;
+ CPU_MICROCODE_MEASUREMENT_EVENT_LOG EventLog;
+ UINT32 EventLogSize;
+ EFI_HOB_GUID_TYPE *GuidHob;
+ EDKII_MICROCODE_PATCH_HOB *MicrocodePatchHob;
+ UINTN SumOfAllPatchesSizeInMicrocodePatchHob;
+ UINT32 Index;
+ MICROCODE_PATCH_TYPE *MicrocodePatchesList;
+ UINTN LastPackedMicrocodeAddress;
+ UINT8 *MicrocodePatchesBlob;
+ UINT64 MicrocodePatchesBlobSize;
+
+
+ PCRIndex = 1;
+ EventType = EV_CPU_MICROCODE;
+ AsciiSPrint (
+ (CHAR8 *)EventLog.Description,
+ CPU_MICROCODE_MEASUREMENT_EVENT_LOG_DESCRIPTION_LEN,
+ CPU_MICROCODE_MEASUREMENT_DESCRIPTION
+ );
+ EventLog.NumberOfMicrocodePatchesMeasured = 0;
+ EventLog.SizeOfMicrocodePatchesMeasured = 0;
+ EventLogSize = sizeof (CPU_MICROCODE_MEASUREMENT_EVENT_LOG);
+ SumOfAllPatchesSizeInMicrocodePatchHob = 0;
+ LastPackedMicrocodeAddress = 0;
+ MicrocodePatchesBlob = NULL;
+ MicrocodePatchesBlobSize = 0;
+
+
+ if (TRUE == mMicrocodeMeasured) {
+ DEBUG((DEBUG_INFO, "INFO: mMicrocodeMeasured = TRUE, Skip.\n"));
+ return;
+ }
+
+ GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid); if (NULL
+ == GuidHob) {
+ DEBUG((DEBUG_ERROR, "ERROR: GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid) failed.\n"));
+ return;
+ }
+
+ MicrocodePatchHob = GET_GUID_HOB_DATA (GuidHob); DEBUG ((DEBUG_INFO,
+ "INFO: Got MicrocodePatchHob with microcode patches starting
+ address:0x%x, microcode patches region size:0x%x, processor
+ count:0x%x\n", MicrocodePatchHob->MicrocodePatchAddress,
+ MicrocodePatchHob->MicrocodePatchRegionSize,
+ MicrocodePatchHob->ProcessorCount));
+
+ //
+ // Extract all microcode patches to a list from MicrocodePatchHob //
+ MicrocodePatchesList = AllocatePool (MicrocodePatchHob->ProcessorCount
+ * sizeof (MICROCODE_PATCH_TYPE)); if (NULL == MicrocodePatchesList) {
+ DEBUG ((DEBUG_ERROR, "ERROR: AllocatePool to MicrocodePatchesList Failed!\n"));
+ return;
+ }
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ if (MAX_UINT64 == MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]) {
+ //
+ // If no microcode patch was found in a slot, set the address of the microcode patch
+ // in that slot to MAX_UINTN, and the size to 0, thus indicates no patch in that slot.
+ //
+ MicrocodePatchesList[Index].Address = MAX_UINTN;
+ MicrocodePatchesList[Index].Size = 0;
+
+ DEBUG ((DEBUG_INFO, "INFO: Processor#%d: detected no microcode patch\n", Index));
+ } else {
+ MicrocodePatchesList[Index].Address = (UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]);
+ MicrocodePatchesList[Index].Size = ((CPU_MICROCODE_HEADER*)((UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index])))->TotalSize;
+ SumOfAllPatchesSizeInMicrocodePatchHob +=
+ MicrocodePatchesList[Index].Size;
+
+ DEBUG ((DEBUG_INFO, "INFO: Processor#%d: Microcode patch address: 0x%x, size: 0x%x\n", Index, MicrocodePatchesList[Index].Address, MicrocodePatchesList[Index].Size));
+ }
+ }
+
+ //
+ // The order matters when packing all applied microcode patches to a single binary blob.
+ // Therefore it is a must to do sorting before packing.
+ // NOTE: We assumed that the order of address of every microcode
+ patch in RAM is the same // with the order of those in the Microcode
+ Firmware Volume. If any future updates made // this assumption untenable, then there needs a new solution to measure microcode patches.
+ //
+ PerformQuickSort (
+ MicrocodePatchesList,
+ MicrocodePatchHob->ProcessorCount,
+ sizeof (MICROCODE_PATCH_TYPE),
+ MicrocodePatchesListSortFunction
+ );
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ DEBUG ((DEBUG_INFO, "INFO: After sorting: Processor#%d: Microcode
+ patch address: 0x%x, size: 0x%x\n", Index,
+ MicrocodePatchesList[Index].Address,
+ MicrocodePatchesList[Index].Size));
+ }
+
+ MicrocodePatchesBlob = AllocateZeroPool
+ (SumOfAllPatchesSizeInMicrocodePatchHob);
+ if (NULL == MicrocodePatchesBlob) {
+ DEBUG ((DEBUG_ERROR, "ERROR: AllocateZeroPool to MicrocodePatchesBlob failed!\n"));
+ FreePool (MicrocodePatchesList);
+ return;
+ }
+
+ //
+ // LastPackedMicrocodeAddress is used to skip duplicate microcode patch.
+ //
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ if (MicrocodePatchesList[Index].Address != LastPackedMicrocodeAddress &&
+ MicrocodePatchesList[Index].Address != MAX_UINTN) {
+
+ CopyMem (
+ (VOID *)(MicrocodePatchesBlob + MicrocodePatchesBlobSize),
+ (VOID *)(MicrocodePatchesList[Index].Address),
+ (UINTN)(MicrocodePatchesList[Index].Size)
+ );
+ MicrocodePatchesBlobSize += MicrocodePatchesList[Index].Size;
+ LastPackedMicrocodeAddress = MicrocodePatchesList[Index].Address;
+ EventLog.NumberOfMicrocodePatchesMeasured += 1;
+ EventLog.SizeOfMicrocodePatchesMeasured += MicrocodePatchesList[Index].Size;
+
+ }
+ }
+
+ if (0 == MicrocodePatchesBlobSize) {
+ DEBUG ((DEBUG_INFO, "INFO: No microcode patch was ever applied!"));
+ FreePool (MicrocodePatchesList);
+ FreePool (MicrocodePatchesBlob);
+ return;
+ }
+
+ Status = TpmMeasureAndLogData (
+ PCRIndex, // PCRIndex
+ EventType, // EventType
+ &EventLog, // EventLog
+ EventLogSize, // LogLen
+ MicrocodePatchesBlob, // HashData
+ MicrocodePatchesBlobSize // HashDataLen
+ );
+ if (!EFI_ERROR (Status)) {
+ mMicrocodeMeasured = TRUE;
+ gBS->CloseEvent (Event);
+ } else {
+ FreePool (MicrocodePatchesList);
+ FreePool (MicrocodePatchesBlob);
+ DEBUG ((DEBUG_ERROR, "ERROR: TpmMeasureAndLogData failed with
+ %a!\n", Status)); }
+
+ return;
+}
+
+/**
+
+ Driver to produce microcode measurement.
+
+ @param ImageHandle Module's image handle
+ @param SystemTable Pointer of EFI_SYSTEM_TABLE
+
+ @return EFI_SUCCESS This function always complete successfully.
+
+**/
+EFI_STATUS
+EFIAPI
+MicrocodeMeasurementDriverEntryPoint (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_EVENT Event;
+
+ //
+ // Measure Microcode patches
+ //
+ EfiCreateEventReadyToBootEx (
+ TPL_CALLBACK,
+ MeasureMicrocodePatches,
+ NULL,
+ &Event
+ );
+
+ return EFI_SUCCESS;
+}
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
new file mode 100644
index 000000000000..4b03339b431b
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
@@ -0,0 +1,58 @@
+## @file
+# This driver measures microcode patches to TPM.
+#
+# This driver consumes gEdkiiMicrocodePatchHobGuid, packs all unique #
+microcode patch found in gEdkiiMicrocodePatchHobGuid to a binary blob,
+# and measures the binary blob to TPM.
+#
+# Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> # #
+SPDX-License-Identifier: BSD-2-Clause-Patent # ##
+
+[Defines]
+ INF_VERSION = 0x00010005
+ BASE_NAME = MicrocodeMeasurementDxe
+ MODULE_UNI_FILE = MicrocodeMeasurementDxe.uni
+ FILE_GUID = 0A32A803-ACDF-4C89-8293-91011548CD91
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = MicrocodeMeasurementDriverEntryPoint
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARCH64
+#
+
+[Sources]
+ MicrocodeMeasurementDxe.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
+ UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+ UefiBootServicesTableLib
+ MemoryAllocationLib
+ BaseMemoryLib
+ BaseLib
+ UefiLib
+ UefiDriverEntryPoint
+ DebugLib
+ PrintLib
+ SortLib
+ HobLib
+ MicrocodeLib
+ TpmMeasurementLib
+
+[Guids]
+ gEdkiiMicrocodePatchHobGuid ## CONSUMES ## HOB
+
+[UserExtensions.TianoCore."ExtraFiles"]
+ MicrocodeMeasurementDxeExtra.uni
+
+[Depex]
+ TRUE
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
new file mode 100644
index 000000000000..5a21e955fbbf
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
@@ -0,0 +1,15 @@
+// /** @file
+// This driver measures microcode patches to TPM.
+//
+// This driver consumes gEdkiiMicrocodePatchHobGuid, packs all uniquemicrocode patch found in gEdkiiMicrocodePatchHobGuid to a binary blob, and measures the binary blob to TPM.
+//
+// Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> //
+// SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "This driver measures Microcode Patches to TPM."
+
+#string STR_MODULE_DESCRIPTION #language en-US "This driver consumes gEdkiiMicrocodePatchHobGuid, packs all microcode patch found in gEdkiiMicrocodePatchHobGuid to a binary blob, and measure the binary blob to TPM."
--git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
new file mode 100644
index 000000000000..6990cee8c6fd
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
@@ -0,0 +1,12 @@
+// /** @file
+// MicrocodeMeasurementDxe Localized Strings and Content
+//
+// Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// **/
+
+#string STR_PROPERTIES_MODULE_NAME
+#language en-US
+"Microcode Patches Measurement DXE Driver"
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 870b45284087..06d55f780a9c 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -61,6 +61,7 @@
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+ SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
[LibraryClasses.common.SEC]
PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
@@ -119,6 +120,7 @@
UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf
UefiCpuPkg/Library/CpuCacheInfoLib/PeiCpuCacheInfoLib.inf
UefiCpuPkg/Library/CpuCacheInfoLib/DxeCpuCacheInfoLib.inf
+ UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
[Components.IA32, Components.X64]
UefiCpuPkg/CpuDxe/CpuDxe.inf
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
[not found] ` <16B397E9723CF0A5.13015@groups.io>
@ 2021-11-02 3:30 ` Ni, Ray
2021-11-02 5:26 ` Yang, Longlong
2021-11-05 3:26 ` Yang, Longlong
0 siblings, 2 replies; 4+ messages in thread
From: Ni, Ray @ 2021-11-02 3:30 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, Yang, Longlong
Cc: Dong, Eric, Kumar, Rahul1, Yao, Jiewen, Xu, Min M, Zhang, Qi1
Just offline discussed with Longlong, measuring the entire microcode buffer might spend more time comparing to only measuring the applied microcode, when the platform firmware includes lots of microcode.
10 comments embedded in code change in below.
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, November 2, 2021 9:55 AM
To: Yang, Longlong <longlong.yang@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Longlong,
Your code creates a big buffer that holds microcode data for all threads.
MicrocodeCpu[i] = MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[i]
BigBuffer = GetMicrocodeBuffer (MicrocodeOfCpu[0]) + GetMicrocodeBuffer (MicrocodeOfCpu[1]) + ...
HashValue = Hash (BigBuffer)
I am not sure if we can do like below:
BigBuffer = <Entire microcode buffer pointed by MicrocodePatchHob->MicrocodePatchAddress> + <array content of MicrocodePatchHob->ProcessorSpecificPatchOffset[]>
HashValue = Hash (BigBuffer)
The second approach doesn't require sorting, one-by-one-copying.
Thanks,
Ray
-----Original Message-----
From: Yang, Longlong <longlong.yang@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.yang@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3683
TCG specification says BIOS should extend measurement of microcode to TPM.
However, reference BIOS is not doing this. This patch consumes gEdkiiMicrocodePatchHobGuid to checkout all applied microcode patches, then all applied microcode patches are packed in order to form a single binary blob which is measured with event type EV_CPU_MICROCODE to PCR[1] in TPM.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min M Xu <min.m.xu@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Signed-off-by: Longlong Yang <longlong.yang@intel.com>
---
.../MicrocodeMeasurementDxe.c | 254 ++++++++++++++++++
.../MicrocodeMeasurementDxe.inf | 58 ++++
.../MicrocodeMeasurementDxe.uni | 15 ++
| 12 +
UefiCpuPkg/UefiCpuPkg.dsc | 2 +
5 files changed, 341 insertions(+)
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
new file mode 100644
index 000000000000..1898a2bff023
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
@@ -0,0 +1,254 @@
+/** @file
+
+
+ if (TRUE == mMicrocodeMeasured) {
1. Remove "TRUE == " please
2. Can you please duplicate the MicrocodePatchHob->ProcessorSpecificPatchOffset in a new array and sort the "PatchOffset" before calculating the total microcode size?
This avoids big memory consumption in many-core platforms.
+
+ //
+ // Extract all microcode patches to a list from MicrocodePatchHob //
+ MicrocodePatchesList = AllocatePool (MicrocodePatchHob->ProcessorCount
+ * sizeof (MICROCODE_PATCH_TYPE)); if (NULL == MicrocodePatchesList) {
+ DEBUG ((DEBUG_ERROR, "ERROR: AllocatePool to MicrocodePatchesList Failed!\n"));
+ return;
+ }
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ if (MAX_UINT64 == MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]) {
+ //
+ // If no microcode patch was found in a slot, set the address of the microcode patch
+ // in that slot to MAX_UINTN, and the size to 0, thus indicates no patch in that slot.
+ //
+ MicrocodePatchesList[Index].Address = MAX_UINTN;
+ MicrocodePatchesList[Index].Size = 0;
+
+ DEBUG ((DEBUG_INFO, "INFO: Processor#%d: detected no microcode patch\n", Index));
+ } else {
+ MicrocodePatchesList[Index].Address = (UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]);
+ MicrocodePatchesList[Index].Size = ((CPU_MICROCODE_HEADER*)((UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index])))->TotalSize;
3. Can you please use GetMicrocodeLength() from MicrocodeLib?
+ PerformQuickSort (
+ MicrocodePatchesList,
+ MicrocodePatchHob->ProcessorCount,
+ sizeof (MICROCODE_PATCH_TYPE),
+ MicrocodePatchesListSortFunction
+ );
4. Can you please use QuickSort() in BaseLib? This avoids UefiCpuPkg depends on MdeModulePkg.
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ DEBUG ((DEBUG_INFO, "INFO: After sorting: Processor#%d: Microcode
+ patch address: 0x%x, size: 0x%x\n", Index,
+ MicrocodePatchesList[Index].Address,
+ MicrocodePatchesList[Index].Size));
+ }
5. There are lots of debug messages in this module. Please review them and think about what are necessary. Try to remove some unnecessary messages.
+ //
+ // LastPackedMicrocodeAddress is used to skip duplicate microcode patch.
6. You might need a "LastPatchOffset" to skip duplicate the PatchOffset after sorting.
+
+ if (0 == MicrocodePatchesBlobSize) {
+ DEBUG ((DEBUG_INFO, "INFO: No microcode patch was ever applied!"));
+ FreePool (MicrocodePatchesList);
+ FreePool (MicrocodePatchesBlob);
+ return;
+ }
7. Please confirm with Jiewen or Qi whether no measurement is fine if there is no microcode.
+
+ Status = TpmMeasureAndLogData (
+ PCRIndex, // PCRIndex
+ EventType, // EventType
+ &EventLog, // EventLog
+ EventLogSize, // LogLen
+ MicrocodePatchesBlob, // HashData
+ MicrocodePatchesBlobSize // HashDataLen
+ );
+ if (!EFI_ERROR (Status)) {
+ mMicrocodeMeasured = TRUE;
+ gBS->CloseEvent (Event);
8. I think if you CloseEvent() there is no need to use mMicrocodeMeasured flag because the event won't be signaled again.
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARCH64
9. Can you just list "IA32" and "X64"? The microcode HOB doesn't apply to ARM. EBC can be added to the supported list if we verified it works.
VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+ SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
10. No need the above SortLib.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
2021-11-02 3:30 ` [edk2-devel] " Ni, Ray
@ 2021-11-02 5:26 ` Yang, Longlong
2021-11-05 3:26 ` Yang, Longlong
1 sibling, 0 replies; 4+ messages in thread
From: Yang, Longlong @ 2021-11-02 5:26 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1, Yao, Jiewen, Xu, Min M, Zhang, Qi1
Hi Ray
It is nice of you to review and give the brilliant comments, I will check and refine them one by one.
Thank you Ray!
BRs
Longlong
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, November 2, 2021 11:30 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Yang, Longlong <longlong.yang@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Just offline discussed with Longlong, measuring the entire microcode buffer might spend more time comparing to only measuring the applied microcode, when the platform firmware includes lots of microcode.
10 comments embedded in code change in below.
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, November 2, 2021 9:55 AM
To: Yang, Longlong <longlong.yang@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Longlong,
Your code creates a big buffer that holds microcode data for all threads.
MicrocodeCpu[i] = MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[i]
BigBuffer = GetMicrocodeBuffer (MicrocodeOfCpu[0]) + GetMicrocodeBuffer (MicrocodeOfCpu[1]) + ...
HashValue = Hash (BigBuffer)
I am not sure if we can do like below:
BigBuffer = <Entire microcode buffer pointed by MicrocodePatchHob->MicrocodePatchAddress> + <array content of MicrocodePatchHob->ProcessorSpecificPatchOffset[]>
HashValue = Hash (BigBuffer)
The second approach doesn't require sorting, one-by-one-copying.
Thanks,
Ray
-----Original Message-----
From: Yang, Longlong <longlong.yang@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.yang@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3683
TCG specification says BIOS should extend measurement of microcode to TPM.
However, reference BIOS is not doing this. This patch consumes gEdkiiMicrocodePatchHobGuid to checkout all applied microcode patches, then all applied microcode patches are packed in order to form a single binary blob which is measured with event type EV_CPU_MICROCODE to PCR[1] in TPM.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min M Xu <min.m.xu@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Signed-off-by: Longlong Yang <longlong.yang@intel.com>
---
.../MicrocodeMeasurementDxe.c | 254 ++++++++++++++++++
.../MicrocodeMeasurementDxe.inf | 58 ++++
.../MicrocodeMeasurementDxe.uni | 15 ++
| 12 +
UefiCpuPkg/UefiCpuPkg.dsc | 2 +
5 files changed, 341 insertions(+)
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
new file mode 100644
index 000000000000..1898a2bff023
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
@@ -0,0 +1,254 @@
+/** @file
+
+
+ if (TRUE == mMicrocodeMeasured) {
1. Remove "TRUE == " please
2. Can you please duplicate the MicrocodePatchHob->ProcessorSpecificPatchOffset in a new array and sort the "PatchOffset" before calculating the total microcode size?
This avoids big memory consumption in many-core platforms.
+
+ //
+ // Extract all microcode patches to a list from MicrocodePatchHob //
+ MicrocodePatchesList = AllocatePool (MicrocodePatchHob->ProcessorCount
+ * sizeof (MICROCODE_PATCH_TYPE)); if (NULL == MicrocodePatchesList) {
+ DEBUG ((DEBUG_ERROR, "ERROR: AllocatePool to MicrocodePatchesList Failed!\n"));
+ return;
+ }
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ if (MAX_UINT64 == MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]) {
+ //
+ // If no microcode patch was found in a slot, set the address of the microcode patch
+ // in that slot to MAX_UINTN, and the size to 0, thus indicates no patch in that slot.
+ //
+ MicrocodePatchesList[Index].Address = MAX_UINTN;
+ MicrocodePatchesList[Index].Size = 0;
+
+ DEBUG ((DEBUG_INFO, "INFO: Processor#%d: detected no microcode patch\n", Index));
+ } else {
+ MicrocodePatchesList[Index].Address = (UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]);
+ MicrocodePatchesList[Index].Size = ((CPU_MICROCODE_HEADER*)((UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index])))->TotalSize;
3. Can you please use GetMicrocodeLength() from MicrocodeLib?
+ PerformQuickSort (
+ MicrocodePatchesList,
+ MicrocodePatchHob->ProcessorCount,
+ sizeof (MICROCODE_PATCH_TYPE),
+ MicrocodePatchesListSortFunction
+ );
4. Can you please use QuickSort() in BaseLib? This avoids UefiCpuPkg depends on MdeModulePkg.
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ DEBUG ((DEBUG_INFO, "INFO: After sorting: Processor#%d: Microcode
+ patch address: 0x%x, size: 0x%x\n", Index,
+ MicrocodePatchesList[Index].Address,
+ MicrocodePatchesList[Index].Size));
+ }
5. There are lots of debug messages in this module. Please review them and think about what are necessary. Try to remove some unnecessary messages.
+ //
+ // LastPackedMicrocodeAddress is used to skip duplicate microcode patch.
6. You might need a "LastPatchOffset" to skip duplicate the PatchOffset after sorting.
+
+ if (0 == MicrocodePatchesBlobSize) {
+ DEBUG ((DEBUG_INFO, "INFO: No microcode patch was ever applied!"));
+ FreePool (MicrocodePatchesList);
+ FreePool (MicrocodePatchesBlob);
+ return;
+ }
7. Please confirm with Jiewen or Qi whether no measurement is fine if there is no microcode.
+
+ Status = TpmMeasureAndLogData (
+ PCRIndex, // PCRIndex
+ EventType, // EventType
+ &EventLog, // EventLog
+ EventLogSize, // LogLen
+ MicrocodePatchesBlob, // HashData
+ MicrocodePatchesBlobSize // HashDataLen
+ );
+ if (!EFI_ERROR (Status)) {
+ mMicrocodeMeasured = TRUE;
+ gBS->CloseEvent (Event);
8. I think if you CloseEvent() there is no need to use mMicrocodeMeasured flag because the event won't be signaled again.
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARCH64
9. Can you just list "IA32" and "X64"? The microcode HOB doesn't apply to ARM. EBC can be added to the supported list if we verified it works.
VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+ SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
10. No need the above SortLib.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
2021-11-02 3:30 ` [edk2-devel] " Ni, Ray
2021-11-02 5:26 ` Yang, Longlong
@ 2021-11-05 3:26 ` Yang, Longlong
1 sibling, 0 replies; 4+ messages in thread
From: Yang, Longlong @ 2021-11-05 3:26 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul1, Yao, Jiewen, Xu, Min M, Zhang, Qi1
Thank you Ray for your kind and patient feedbacks and advices.
I checked all 10 comments one by one and you could see my responses inline in below code change.
I am testing new patch, will send to community soon if all tests pass.
BRs
Longlong
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, November 2, 2021 11:30 AM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Yang, Longlong <longlong.yang@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Just offline discussed with Longlong, measuring the entire microcode buffer might spend more time comparing to only measuring the applied microcode, when the platform firmware includes lots of microcode.
10 comments embedded in code change in below.
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
Sent: Tuesday, November 2, 2021 9:55 AM
To: Yang, Longlong <longlong.yang@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
Longlong,
Your code creates a big buffer that holds microcode data for all threads.
MicrocodeCpu[i] = MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[i]
BigBuffer = GetMicrocodeBuffer (MicrocodeOfCpu[0]) + GetMicrocodeBuffer (MicrocodeOfCpu[1]) + ...
HashValue = Hash (BigBuffer)
I am not sure if we can do like below:
BigBuffer = <Entire microcode buffer pointed by MicrocodePatchHob->MicrocodePatchAddress> + <array content of MicrocodePatchHob->ProcessorSpecificPatchOffset[]>
HashValue = Hash (BigBuffer)
The second approach doesn't require sorting, one-by-one-copying.
Thanks,
Ray
-----Original Message-----
From: Yang, Longlong <longlong.yang@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.yang@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Zhang, Qi1 <qi1.zhang@intel.com>
Subject: [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3683
TCG specification says BIOS should extend measurement of microcode to TPM.
However, reference BIOS is not doing this. This patch consumes gEdkiiMicrocodePatchHobGuid to checkout all applied microcode patches, then all applied microcode patches are packed in order to form a single binary blob which is measured with event type EV_CPU_MICROCODE to PCR[1] in TPM.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Min M Xu <min.m.xu@intel.com>
Cc: Qi Zhang <qi1.zhang@intel.com>
Signed-off-by: Longlong Yang <longlong.yang@intel.com>
---
.../MicrocodeMeasurementDxe.c | 254 ++++++++++++++++++
.../MicrocodeMeasurementDxe.inf | 58 ++++
.../MicrocodeMeasurementDxe.uni | 15 ++
| 12 +
UefiCpuPkg/UefiCpuPkg.dsc | 2 +
5 files changed, 341 insertions(+)
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.inf
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.uni
create mode 100644 UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxeExtra.uni
diff --git a/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
new file mode 100644
index 000000000000..1898a2bff023
--- /dev/null
+++ b/UefiCpuPkg/MicrocodeMeasurementDxe/MicrocodeMeasurementDxe.c
@@ -0,0 +1,254 @@
+/** @file
+
+
+ if (TRUE == mMicrocodeMeasured) {
1. Remove "TRUE == " please
[longlong] The mMicrocodeMeasured flag and this check are removed in new implementation.
2. Can you please duplicate the MicrocodePatchHob->ProcessorSpecificPatchOffset in a new array and sort the "PatchOffset" before calculating the total microcode size?
This avoids big memory consumption in many-core platforms.
[longlong] Fixed in new implementation.
+
+ //
+ // Extract all microcode patches to a list from MicrocodePatchHob //
+ MicrocodePatchesList = AllocatePool (MicrocodePatchHob->ProcessorCount
+ * sizeof (MICROCODE_PATCH_TYPE)); if (NULL == MicrocodePatchesList) {
+ DEBUG ((DEBUG_ERROR, "ERROR: AllocatePool to MicrocodePatchesList Failed!\n"));
+ return;
+ }
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ if (MAX_UINT64 == MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]) {
+ //
+ // If no microcode patch was found in a slot, set the address of the microcode patch
+ // in that slot to MAX_UINTN, and the size to 0, thus indicates no patch in that slot.
+ //
+ MicrocodePatchesList[Index].Address = MAX_UINTN;
+ MicrocodePatchesList[Index].Size = 0;
+
+ DEBUG ((DEBUG_INFO, "INFO: Processor#%d: detected no microcode patch\n", Index));
+ } else {
+ MicrocodePatchesList[Index].Address = (UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index]);
+ MicrocodePatchesList[Index].Size = ((CPU_MICROCODE_HEADER*)((UINTN)(MicrocodePatchHob->MicrocodePatchAddress + MicrocodePatchHob->ProcessorSpecificPatchOffset[Index])))->TotalSize;
3. Can you please use GetMicrocodeLength() from MicrocodeLib?
[longlong] Fixed in new implementation.
+ PerformQuickSort (
+ MicrocodePatchesList,
+ MicrocodePatchHob->ProcessorCount,
+ sizeof (MICROCODE_PATCH_TYPE),
+ MicrocodePatchesListSortFunction
+ );
4. Can you please use QuickSort() in BaseLib? This avoids UefiCpuPkg depends on MdeModulePkg.
[longlong] Fixed in new implementation.
+ for (Index = 0; Index < MicrocodePatchHob->ProcessorCount; Index++) {
+ DEBUG ((DEBUG_INFO, "INFO: After sorting: Processor#%d: Microcode
+ patch address: 0x%x, size: 0x%x\n", Index,
+ MicrocodePatchesList[Index].Address,
+ MicrocodePatchesList[Index].Size));
+ }
5. There are lots of debug messages in this module. Please review them and think about what are necessary. Try to remove some unnecessary messages.
[longlong] Checked and removed some unnecessary messages in new implementation.
+ //
+ // LastPackedMicrocodeAddress is used to skip duplicate microcode patch.
6. You might need a "LastPatchOffset" to skip duplicate the PatchOffset after sorting.
[longlong] Advice accepted. "LastPatchOffset" is used to skip duplicate the PatchOffset after sorting in new implementation.
+
+ if (0 == MicrocodePatchesBlobSize) {
+ DEBUG ((DEBUG_INFO, "INFO: No microcode patch was ever applied!"));
+ FreePool (MicrocodePatchesList);
+ FreePool (MicrocodePatchesBlob);
+ return;
+ }
7. Please confirm with Jiewen or Qi whether no measurement is fine if there is no microcode.
[longlong] Confirmed with Qi, he prefers no measurement if there is no microcode.
+
+ Status = TpmMeasureAndLogData (
+ PCRIndex, // PCRIndex
+ EventType, // EventType
+ &EventLog, // EventLog
+ EventLogSize, // LogLen
+ MicrocodePatchesBlob, // HashData
+ MicrocodePatchesBlobSize // HashDataLen
+ );
+ if (!EFI_ERROR (Status)) {
+ mMicrocodeMeasured = TRUE;
+ gBS->CloseEvent (Event);
8. I think if you CloseEvent() there is no need to use mMicrocodeMeasured flag because the event won't be signaled again.
[longlong] The mMicrocodeMeasured flag is removed in new implementation.
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARCH64
9. Can you just list "IA32" and "X64"? The microcode HOB doesn't apply to ARM. EBC can be added to the supported list if we verified it works.
[longlong] After following a template to create the inf file, I ignored this comment, Sorry for that. Will check other comments as well and fix it in new implementation.
VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
+ SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
10. No need the above SortLib.
[longlong] SortLib is deleted in new implementation.
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-05 3:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1635405564.git.longlong.yang@intel.com>
[not found] ` <69d53dbbfe4bb2fdd27d5098850a9e91a43d63bb.1635405564.git.longlong.yang@intel.com>
2021-11-02 1:55 ` [PATCH 1/1] UefiCpuPkg: Extend measurement of microcode patches to TPM Ni, Ray
[not found] ` <16B397E9723CF0A5.13015@groups.io>
2021-11-02 3:30 ` [edk2-devel] " Ni, Ray
2021-11-02 5:26 ` Yang, Longlong
2021-11-05 3:26 ` Yang, Longlong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox