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 <[email protected]> Sent: Tuesday, November 2, 2021 11:30 AM To: [email protected]; Ni, Ray <[email protected]>; Yang, Longlong <[email protected]> Cc: Dong, Eric <[email protected]>; Kumar, Rahul1 <[email protected]>; Yao, Jiewen <[email protected]>; Xu, Min M <[email protected]>; Zhang, Qi1 <[email protected]> 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: [email protected] <[email protected]> On Behalf Of Ni, Ray Sent: Tuesday, November 2, 2021 9:55 AM To: Yang, Longlong <[email protected]>; [email protected] Cc: Dong, Eric <[email protected]>; Kumar, Rahul1 <[email protected]>; Yao, Jiewen <[email protected]>; Xu, Min M <[email protected]>; Zhang, Qi1 <[email protected]> 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 <[email protected]> Sent: Thursday, October 28, 2021 3:21 PM To: [email protected] Cc: Yang, Longlong <[email protected]>; Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Kumar, Rahul1 <[email protected]>; Yao, Jiewen <[email protected]>; Xu, Min M <[email protected]>; Zhang, Qi1 <[email protected]> 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 <[email protected]> Cc: Ray Ni <[email protected]> Cc: Rahul Kumar <[email protected]> Cc: Jiewen Yao <[email protected]> Cc: Min M Xu <[email protected]> Cc: Qi Zhang <[email protected]> Signed-off-by: Longlong Yang <[email protected]> --- .../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 [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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#83403): https://edk2.groups.io/g/devel/message/83403 Mute This Topic: https://groups.io/mt/86757841/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
