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...@intel.com>
Sent: Tuesday, November 2, 2021 11:30 AM
To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Yang, Longlong
<longlong.y...@intel.com>
Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>;
Yao, Jiewen <jiewen....@intel.com>; Xu, Min M <min.m...@intel.com>; Zhang, Qi1
<qi1.zh...@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.y...@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>;
Yao, Jiewen <jiewen....@intel.com>; Xu, Min M <min.m...@intel.com>; Zhang, Qi1
<qi1.zh...@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.y...@intel.com>
Sent: Thursday, October 28, 2021 3:21 PM
To: devel@edk2.groups.io
Cc: Yang, Longlong <longlong.y...@intel.com>; Dong, Eric <eric.d...@intel.com>;
Ni, Ray <ray...@intel.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; Yao, Jiewen
<jiewen....@intel.com>; Xu, Min M <min.m...@intel.com>; Zhang, Qi1
<qi1.zh...@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.d...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Cc: Rahul Kumar <rahul1.ku...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Min M Xu <min.m...@intel.com>
Cc: Qi Zhang <qi1.zh...@intel.com>
Signed-off-by: Longlong Yang <longlong.y...@intel.com>
---
.../MicrocodeMeasurementDxe.c | 254 ++++++++++++++++++
.../MicrocodeMeasurementDxe.inf | 58 ++++
.../MicrocodeMeasurementDxe.uni | 15 ++
.../MicrocodeMeasurementDxeExtra.uni | 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83103): https://edk2.groups.io/g/devel/message/83103
Mute This Topic: https://groups.io/mt/86757841/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
