* [PATCH v1 0/4] Microcode related optimizations
@ 2019-12-24 1:36 Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info Wu, Hao A
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 1:36 UTC (permalink / raw)
To: devel
Cc: Hao A Wu, Eric Dong, Ray Ni, Laszlo Ersek, Star Zeng, Siyuan Fu,
Michael D Kinney
Series is also available at:
https://github.com/hwu25/edk2/tree/mpinitlib_opt_v1
This series will introduce a couple of optimizations to the MpInitLib with
regard to microcode:
A. Reduce the copy size when loading the microcode patches data from flash
into memory;
B. Produce a HOB to contain microcode patches information for subsequent
consumers of the microcode patches during the boot flow.
Uni-test done for the series:
A. System boot to OS/Shell successfully on a real platform;
B. Add debug message to verify the same microcode patch is applied to each
processor before and after the series.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Hao A Wu (4):
UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info
UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
UefiCpuPkg: Add definitions for EDKII microcode patch HOB
UefiCpuPkg/MpInitLib: Produce EDKII microcode patch HOB
UefiCpuPkg/UefiCpuPkg.dec | 3 +
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
UefiCpuPkg/Include/Guid/MicrocodePatchHob.h | 50 ++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 50 +++-
UefiCpuPkg/Library/MpInitLib/Microcode.c | 251 +++++++++++++++++++-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 102 ++++----
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 55 +++++
7 files changed, 446 insertions(+), 66 deletions(-)
create mode 100644 UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
--
2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info
2019-12-24 1:36 [PATCH v1 0/4] Microcode related optimizations Wu, Hao A
@ 2019-12-24 1:36 ` Wu, Hao A
2019-12-24 2:51 ` Ni, Ray
2019-12-24 1:36 ` [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Wu, Hao A
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 1:36 UTC (permalink / raw)
To: devel
Cc: Hao A Wu, Eric Dong, Ray Ni, Laszlo Ersek, Star Zeng, Siyuan Fu,
Michael D Kinney
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
This commit will collect the CPUID and Platform ID information for each
processor within system. They will be stored in the CPU_AP_DATA structure.
These information will be used in the next commit to decide whether a
microcode patch will be loaded into memory.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 2 ++
UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 8fa07b12c5..4440dc2701 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -122,6 +122,8 @@ typedef struct {
UINT64 CurrentTime;
UINT64 TotalTime;
EFI_EVENT WaitEvent;
+ UINT32 ProcessorSignature;
+ UINT8 PlatformId;
} CPU_AP_DATA;
//
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d32adf0780..d5077e080e 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -548,7 +548,8 @@ InitializeApData (
IN UINT64 ApTopOfStack
)
{
- CPU_INFO_IN_HOB *CpuInfoInHob;
+ CPU_INFO_IN_HOB *CpuInfoInHob;
+ MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
@@ -559,6 +560,17 @@ InitializeApData (
CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ? TRUE : FALSE;
+ PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
+ CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
+
+ AsmCpuid (
+ CPUID_VERSION_INFO,
+ &CpuMpData->CpuData[ProcessorNumber].ProcessorSignature,
+ NULL,
+ NULL,
+ NULL
+ );
+
InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
}
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
2019-12-24 1:36 [PATCH v1 0/4] Microcode related optimizations Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info Wu, Hao A
@ 2019-12-24 1:36 ` Wu, Hao A
2019-12-24 3:32 ` Ni, Ray
2019-12-24 1:36 ` [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce " Wu, Hao A
3 siblings, 1 reply; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 1:36 UTC (permalink / raw)
To: devel
Cc: Hao A Wu, Eric Dong, Ray Ni, Laszlo Ersek, Star Zeng, Siyuan Fu,
Michael D Kinney
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
This commit will attempt to reduce the copy size when loading the
microcode patches data from flash into memory.
Such optimization is done by a pre-process of the microcode patch headers
(on flash). A microcode patch will be loaded into memory only when the
below 2 criteria are met:
A. With a microcode patch header (which means the data is not padding data
between microcode patches);
B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header match
at least one processor within system.
Criterion B will require all the processors to be woken up once to collect
their CPUID and Platform ID information. Hence, this commit will move the
copy, detect and apply of microcode patch on BSP and APs after all the
processors have been woken up.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++
UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++
UefiCpuPkg/Library/MpInitLib/MpLib.c | 82 ++-----
3 files changed, 283 insertions(+), 58 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 4440dc2701..56b0df664a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -44,6 +44,20 @@
#define CPU_SWITCH_STATE_LOADED 2
//
+// Default maximum number of entries to store the microcode patches information
+//
+#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8
+
+//
+// Data structure for microcode patch information
+//
+typedef struct {
+ UINTN Address;
+ UINTN Size;
+ UINTN AlignedSize;
+} MICROCODE_PATCH_INFO;
+
+//
// CPU exchange information for switch BSP
//
typedef struct {
@@ -576,6 +590,16 @@ MicrocodeDetect (
);
/**
+ Load the required microcode patches data into memory.
+
+ @param[in, out] CpuMpData The pointer to CPU MP Data structure.
+**/
+VOID
+LoadMicrocodePatch (
+ IN OUT CPU_MP_DATA *CpuMpData
+ );
+
+/**
Detect whether Mwait-monitor feature is supported.
@retval TRUE Mwait-monitor feature is supported.
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 199b1f23ce..68088b26a5 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -331,3 +331,238 @@ Done:
MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags, (UINTN) MicrocodeData, LatestRevision));
}
}
+
+/**
+ Actual worker function that loads the required microcode patches into memory.
+
+ @param[in, out] CpuMpData The pointer to CPU MP Data structure.
+ @param[in] PatchInfoBuffer The pointer to an array of information on
+ the microcode patches that will be loaded
+ into memory.
+ @param[in] PatchNumber The number of microcode patches that will
+ be loaded into memory.
+ @param[in] TotalLoadSize The total size of all the microcode
+ patches to be loaded.
+**/
+VOID
+LoadMicrocodePatchWorker (
+ IN OUT CPU_MP_DATA *CpuMpData,
+ IN MICROCODE_PATCH_INFO *PatchInfoBuffer,
+ IN UINTN PatchNumber,
+ IN UINTN TotalLoadSize
+ )
+{
+ UINTN Index;
+ VOID *MicrocodePatchInRam;
+ UINT8 *Walker;
+
+ ASSERT ((PatchInfoBuffer != NULL) && (PatchNumber != 0));
+
+ MicrocodePatchInRam = AllocatePages (EFI_SIZE_TO_PAGES (TotalLoadSize));
+ if (MicrocodePatchInRam == NULL) {
+ return;
+ }
+
+ //
+ // Load all the required microcode patches into memory
+ //
+ for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchNumber; Index++) {
+ CopyMem (
+ Walker,
+ (VOID *) PatchInfoBuffer[Index].Address,
+ PatchInfoBuffer[Index].Size
+ );
+
+ if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Size) {
+ //
+ // Zero-fill the padding area
+ //
+ ZeroMem (
+ Walker + PatchInfoBuffer[Index].Size,
+ PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
+ );
+ }
+
+ Walker += PatchInfoBuffer[Index].AlignedSize;
+ }
+
+ //
+ // Update the microcode patch related fields in CpuMpData
+ //
+ CpuMpData->MicrocodePatchAddress = (UINTN) MicrocodePatchInRam;
+ CpuMpData->MicrocodePatchRegionSize = TotalLoadSize;
+
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: Required microcode patches have been loaded at 0x%lx, with size 0x%lx.\n",
+ __FUNCTION__, CpuMpData->MicrocodePatchAddress, CpuMpData->MicrocodePatchRegionSize
+ ));
+
+ return;
+}
+
+/**
+ Load the required microcode patches data into memory.
+
+ @param[in, out] CpuMpData The pointer to CPU MP Data structure.
+**/
+VOID
+LoadMicrocodePatch (
+ IN OUT CPU_MP_DATA *CpuMpData
+ )
+{
+ CPU_MICROCODE_HEADER *MicrocodeEntryPoint;
+ UINTN MicrocodeEnd;
+ UINTN DataSize;
+ UINTN TotalSize;
+ MICROCODE_PATCH_INFO *PatchInfoBuffer;
+ UINTN MaxPatchNumber;
+ UINTN PatchNumber;
+ UINTN TotalLoadSize;
+ UINT32 ProcessorSignature;
+ UINT32 ProcessorFlags;
+ UINTN Index;
+ CPU_AP_DATA *CpuData;
+ BOOLEAN NeedLoad;
+
+ //
+ // Initialize the microcode patch related fields in CpuMpData as the values
+ // specified by the PCD pair. If the microcode patches are loaded into memory,
+ // these fields will be updated.
+ //
+ CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
+ CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
+
+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
+ MicrocodeEnd = (UINTN) MicrocodeEntryPoint +
+ (UINTN) CpuMpData->MicrocodePatchRegionSize;
+ if ((MicrocodeEntryPoint == NULL) || ((UINTN) MicrocodeEntryPoint == MicrocodeEnd)) {
+ //
+ // There is no microcode patches
+ //
+ return;
+ }
+
+ PatchNumber = 0;
+ MaxPatchNumber = DEFAULT_MAX_MICROCODE_PATCH_NUM;
+ TotalLoadSize = 0;
+ PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
+ if (PatchInfoBuffer == NULL) {
+ return;
+ }
+
+ //
+ // Process the header of each microcode patch within the region.
+ // The purpose is to decide which microcode patch(es) will be loaded into memory.
+ //
+ do {
+ if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
+ //
+ // Padding data between the microcode patches, skip 1KB to check next entry.
+ //
+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
+ continue;
+ }
+
+ DataSize = MicrocodeEntryPoint->DataSize;
+ if (DataSize == 0) {
+ TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
+ } else {
+ TotalSize = sizeof (CPU_MICROCODE_HEADER) + DataSize;
+ }
+
+ if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
+ ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
+ (TotalSize & 0x3) != 0
+ ) {
+ //
+ // Not a valid microcode header, skip 1KB to check next entry.
+ //
+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
+ continue;
+ }
+
+ TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+
+ //
+ // Check the 'ProcessorSignature' & 'ProcessorFlags' of this microcode patch
+ // with the processors' CPUID & PlatformID to decide if it will be copied
+ // into memory
+ //
+ ProcessorSignature = MicrocodeEntryPoint->ProcessorSignature.Uint32;
+ ProcessorFlags = MicrocodeEntryPoint->ProcessorFlags;
+ NeedLoad = FALSE;
+ for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+ CpuData = &CpuMpData->CpuData[Index];
+ if ((ProcessorSignature == CpuData->ProcessorSignature) &&
+ (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {
+ NeedLoad = TRUE;
+ break;
+ }
+ }
+
+ if (NeedLoad) {
+ PatchNumber++;
+ if (PatchNumber >= MaxPatchNumber) {
+ //
+ // Current 'PatchInfoBuffer' cannot hold the information, double the size
+ // and allocate a new buffer.
+ //
+ 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;
+ }
+
+ //
+ // Store the information of this microcode patch
+ //
+ if (TotalSize > MAX_UINTN - TotalLoadSize ||
+ ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
+ goto OnExit;
+ }
+ PatchInfoBuffer[PatchNumber - 1].Address = (UINTN) MicrocodeEntryPoint;
+ PatchInfoBuffer[PatchNumber - 1].Size = TotalSize;
+ PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE (TotalSize, SIZE_1KB);
+ TotalLoadSize += PatchInfoBuffer[PatchNumber - 1].AlignedSize;
+ }
+
+ //
+ // Process the next microcode patch
+ //
+ MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + TotalSize);
+ } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
+
+ if (PatchNumber == 0) {
+ //
+ // No patch needs to be loaded
+ //
+ goto OnExit;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: 0x%x microcode patches will be loaded into memory, with size 0x%x.\n",
+ __FUNCTION__, PatchNumber, TotalLoadSize
+ ));
+
+ LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber, TotalLoadSize);
+
+OnExit:
+ if (PatchInfoBuffer != NULL) {
+ FreePool (PatchInfoBuffer);
+ }
+ return;
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d5077e080e..199468156b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -628,10 +628,6 @@ ApWakeupFunction (
ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
//
- // Do some AP initialize sync
- //
- ApInitializeSync (CpuMpData);
- //
// CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
// to initialize AP in InitConfig path.
// NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
@@ -1615,7 +1611,6 @@ MpInitLibInitialize (
UINTN ApResetVectorSize;
UINTN BackupBufferAddr;
UINTN ApIdtBase;
- VOID *MicrocodePatchInRam;
OldCpuMpData = GetCpuMpDataFromGuidedHob ();
if (OldCpuMpData == NULL) {
@@ -1683,39 +1678,7 @@ MpInitLibInitialize (
CpuMpData->SwitchBspFlag = FALSE;
CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
CpuMpData->CpuInfoInHob = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
- if (OldCpuMpData == NULL) {
- CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
- //
- // If platform has more than one CPU, relocate microcode to memory to reduce
- // loading microcode time.
- //
- MicrocodePatchInRam = NULL;
- if (MaxLogicalProcessorNumber > 1) {
- MicrocodePatchInRam = AllocatePages (
- EFI_SIZE_TO_PAGES (
- (UINTN)CpuMpData->MicrocodePatchRegionSize
- )
- );
- }
- if (MicrocodePatchInRam == NULL) {
- //
- // there is only one processor, or no microcode patch is available, or
- // memory allocation failed
- //
- CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
- } else {
- //
- // there are multiple processors, and a microcode patch is available, and
- // memory allocation succeeded
- //
- CopyMem (
- MicrocodePatchInRam,
- (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress),
- (UINTN)CpuMpData->MicrocodePatchRegionSize
- );
- CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam;
- }
- }else {
+ if (OldCpuMpData != NULL) {
CpuMpData->MicrocodePatchRegionSize = OldCpuMpData->MicrocodePatchRegionSize;
CpuMpData->MicrocodePatchAddress = OldCpuMpData->MicrocodePatchAddress;
}
@@ -1762,10 +1725,6 @@ MpInitLibInitialize (
(UINT32 *)(MonitorBuffer + MonitorFilterSize * Index);
}
//
- // Load Microcode on BSP
- //
- MicrocodeDetect (CpuMpData, TRUE);
- //
// Store BSP's MTRR setting
//
MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
@@ -1781,6 +1740,11 @@ MpInitLibInitialize (
//
CollectProcessorCount (CpuMpData);
}
+
+ //
+ // Load required microcode patches data into memory
+ //
+ LoadMicrocodePatch (CpuMpData);
} else {
//
// APs have been wakeup before, just get the CPU Information
@@ -1788,7 +1752,6 @@ MpInitLibInitialize (
//
CpuMpData->CpuCount = OldCpuMpData->CpuCount;
CpuMpData->BspNumber = OldCpuMpData->BspNumber;
- CpuMpData->InitFlag = ApInitReconfig;
CpuMpData->CpuInfoInHob = OldCpuMpData->CpuInfoInHob;
CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
@@ -1797,21 +1760,24 @@ MpInitLibInitialize (
CpuMpData->CpuData[Index].ApFunction = 0;
CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
}
- if (MaxLogicalProcessorNumber > 1) {
- //
- // Wakeup APs to do some AP initialize sync
- //
- WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
- //
- // Wait for all APs finished initialization
- //
- while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
- CpuPause ();
- }
- CpuMpData->InitFlag = ApInitDone;
- for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
- SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
- }
+ }
+
+ //
+ // Detect and apply Microcode on BSP
+ //
+ MicrocodeDetect (CpuMpData, TRUE);
+
+ //
+ // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
+ //
+ if (CpuMpData->CpuCount > 1) {
+ WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
+ while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
+ CpuPause ();
+ }
+
+ for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+ SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
}
}
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB
2019-12-24 1:36 [PATCH v1 0/4] Microcode related optimizations Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Wu, Hao A
@ 2019-12-24 1:36 ` Wu, Hao A
2019-12-24 3:48 ` Ni, Ray
2019-12-24 8:15 ` Ni, Ray
2019-12-24 1:36 ` [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce " Wu, Hao A
3 siblings, 2 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 1:36 UTC (permalink / raw)
To: devel
Cc: Hao A Wu, Eric Dong, Ray Ni, Laszlo Ersek, Star Zeng, Siyuan Fu,
Michael D Kinney
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
This commit will add the definitions for EDKII microcode patch HOB.
The intention of adding this HOB is to provide a scheme to store the below
information:
A. The base address and size of the microcode patches that are being
loaded (from flash) into memory;
B. The information of applied microcode patch for each processor within
the system.
The producer of the HOB will be the UefiCpuPkg/MpInitLib (where the load,
detect and apply of the microcode happen). The consumer of the HOB can be
modules that want to detect/apply the microcode patch by themselves again
later during the boot flow.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
UefiCpuPkg/UefiCpuPkg.dec | 3 ++
UefiCpuPkg/Include/Guid/MicrocodePatchHob.h | 50 ++++++++++++++++++++
2 files changed, 53 insertions(+)
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 797f948631..45b267ac61 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -63,6 +63,9 @@ [Guids]
## Include/Guid/CpuFeaturesInitDone.h
gEdkiiCpuFeaturesInitDoneGuid = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98, 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
+ ## Include/Guid/MicrocodePatchHob.h
+ gEdkiiMicrocodePatchHobGuid = { 0xd178f11d, 0x8716, 0x418e, { 0xa1, 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
+
[Protocols]
## Include/Protocol/SmmCpuService.h
gEfiSmmCpuServiceProtocolGuid = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
diff --git a/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
new file mode 100644
index 0000000000..3667fc3786
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
@@ -0,0 +1,50 @@
+/** @file
+ The microcode patch HOB is used to store the information of:
+ A. Base address and size of the loaded microcode patches data;
+ B. Applied microcode patch for each processor within system.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD License
+ which accompanies this distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php.
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _MICROCODE_PATCH_HOB_H_
+#define _MICROCODE_PATCH_HOB_H_
+
+extern EFI_GUID gEdkiiMicrocodePatchHobGuid;
+
+//
+// The EDKII microcode patch HOB will be produced by MpInitLib and it can be
+// consumed by modules that want to detect/apply microcode patches.
+//
+typedef struct {
+ //
+ // The base address of the microcode patches data after being loaded into
+ // memory.
+ //
+ UINT64 MicrocodePatchAddress;
+ //
+ // The total size of the loaded microcode patches.
+ //
+ UINT64 MicrocodePatchRegionSize;
+ //
+ // The number of processors within the system.
+ //
+ UINT32 ProcessorNumber;
+ //
+ // An array with 'ProcessorNumber' elements that stores the offset (with
+ // regard to 'MicrocodePatchAddress') of the applied microcode patch for each
+ // processor.
+ // If no microcode patch is applied for certain processor, the relating
+ // element will be set to MAX_UINT64.
+ //
+ UINT64 DetectedPatchOffset[0];
+} EDKII_MICROCODE_PATCH_HOB;
+
+#endif
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode patch HOB
2019-12-24 1:36 [PATCH v1 0/4] Microcode related optimizations Wu, Hao A
` (2 preceding siblings ...)
2019-12-24 1:36 ` [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB Wu, Hao A
@ 2019-12-24 1:36 ` Wu, Hao A
2019-12-24 3:54 ` Ni, Ray
3 siblings, 1 reply; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 1:36 UTC (permalink / raw)
To: devel
Cc: Hao A Wu, Eric Dong, Ray Ni, Laszlo Ersek, Star Zeng, Siyuan Fu,
Michael D Kinney
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
This commit will update the MpInitLib to:
A. Collect the base address and size information after microcode patches
being loaded into memory;
B. Collect the applied microcode patch for each processor within system;
C. Based on the collected information, produce the EDKII microcode patch
HOB.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---
UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 +++++++--
UefiCpuPkg/Library/MpInitLib/Microcode.c | 16 ++++--
UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++-
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 55 ++++++++++++++++++++
5 files changed, 96 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 1538185ef9..326703cc9a 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -63,3 +63,4 @@ [Pcd]
[Guids]
gEdkiiS3SmmInitDoneGuid
+ gEdkiiMicrocodePatchHobGuid
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 56b0df664a..fb251d7aef 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -138,6 +138,7 @@ typedef struct {
EFI_EVENT WaitEvent;
UINT32 ProcessorSignature;
UINT8 PlatformId;
+ UINT64 MicrocodeData;
} CPU_AP_DATA;
//
@@ -580,13 +581,15 @@ CheckAndUpdateApsStatus (
/**
Detect whether specified processor can find matching microcode patch and load it.
- @param[in] CpuMpData The pointer to CPU MP Data structure.
- @param[in] IsBspCallIn Indicate whether the caller is BSP or not.
+ @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
+ minus 1.
**/
VOID
MicrocodeDetect (
IN CPU_MP_DATA *CpuMpData,
- IN BOOLEAN IsBspCallIn
+ IN UINTN ProcessorNumber
);
/**
@@ -619,5 +622,20 @@ EnableDebugAgent (
VOID
);
+/**
+ Find the current Processor number by APIC ID.
+
+ @param[in] CpuMpData Pointer to PEI CPU MP Data
+ @param[out] ProcessorNumber Return the pocessor number found
+
+ @retval EFI_SUCCESS ProcessorNumber is found and returned.
+ @retval EFI_NOT_FOUND ProcessorNumber is not found.
+**/
+EFI_STATUS
+GetProcessorNumber (
+ IN CPU_MP_DATA *CpuMpData,
+ OUT UINTN *ProcessorNumber
+ );
+
#endif
diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 68088b26a5..bbc40f81bf 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -65,13 +65,15 @@ GetCurrentMicrocodeSignature (
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] IsBspCallIn Indicate whether the caller is BSP or not.
+ @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
+ minus 1.
**/
VOID
MicrocodeDetect (
IN CPU_MP_DATA *CpuMpData,
- IN BOOLEAN IsBspCallIn
+ IN UINTN ProcessorNumber
)
{
UINT32 ExtendedTableLength;
@@ -93,6 +95,7 @@ MicrocodeDetect (
MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
UINT32 ProcessorFlags;
UINT32 ThreadId;
+ BOOLEAN IsBspCallIn;
//
// set ProcessorFlags to suppress incorrect compiler/analyzer warnings
@@ -107,6 +110,7 @@ MicrocodeDetect (
}
CurrentRevision = GetCurrentMicrocodeSignature ();
+ IsBspCallIn = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
if (CurrentRevision != 0 && !IsBspCallIn) {
//
// Skip loading microcode if it has been loaded successfully
@@ -316,6 +320,12 @@ Done:
DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not match \
loaded microcode signature [0x%08x]\n", CurrentRevision, LatestRevision));
ReleaseSpinLock(&CpuMpData->MpLock);
+ } else {
+ //
+ // Save the detected microcode patch address for each processor.
+ // It will be used when building the microcode patch cache HOB.
+ //
+ CpuMpData->CpuData[ProcessorNumber].MicrocodeData = (UINTN) MicrocodeData;
}
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 199468156b..8f4b2b1973 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -399,12 +399,16 @@ ApInitializeSync (
)
{
CPU_MP_DATA *CpuMpData;
+ UINTN ProcessorNumber;
+ EFI_STATUS Status;
CpuMpData = (CPU_MP_DATA *) Buffer;
+ Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
+ ASSERT_EFI_ERROR (Status);
//
// Load microcode on AP
//
- MicrocodeDetect (CpuMpData, FALSE);
+ MicrocodeDetect (CpuMpData, ProcessorNumber);
//
// Sync BSP's MTRR table to AP
//
@@ -1765,7 +1769,7 @@ MpInitLibInitialize (
//
// Detect and apply Microcode on BSP
//
- MicrocodeDetect (CpuMpData, TRUE);
+ MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
//
// Wakeup APs to do some AP initialize sync (Microcode & MTRR)
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 3999603c3e..977818fda4 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -9,6 +9,7 @@
#include "MpLib.h"
#include <Library/PeiServicesLib.h>
#include <Guid/S3SmmInitDone.h>
+#include <Guid/MicrocodePatchHob.h>
/**
S3 SMM Init Done notification function.
@@ -291,6 +292,59 @@ CheckAndUpdateApsStatus (
}
/**
+ Build the microcode patch HOB that contains the base address and size of the
+ microcode patch stored in the memory.
+
+ @param[in] CpuMpData Pointer to the CPU_MP_DATA structure.
+
+**/
+VOID
+BuildMicrocodeCacheHob (
+ IN CPU_MP_DATA *CpuMpData
+ )
+{
+ EDKII_MICROCODE_PATCH_HOB *MicrocodeHob;
+ UINTN HobDataLength;
+ UINT32 Index;
+
+ HobDataLength = sizeof (EDKII_MICROCODE_PATCH_HOB) +
+ sizeof (UINT64) * CpuMpData->CpuCount;
+
+ MicrocodeHob = AllocatePool (HobDataLength);
+ if (MicrocodeHob == NULL) {
+ ASSERT (FALSE);
+ return;
+ }
+
+ //
+ // Store the information of the memory region that holds the microcode patches.
+ //
+ MicrocodeHob->MicrocodePatchAddress = CpuMpData->MicrocodePatchAddress;
+ MicrocodeHob->MicrocodePatchRegionSize = CpuMpData->MicrocodePatchRegionSize;
+
+ //
+ // Store the detected microcode patch for each processor as well.
+ //
+ MicrocodeHob->ProcessorNumber = CpuMpData->CpuCount;
+ for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+ if (CpuMpData->CpuData[Index].MicrocodeData != 0) {
+ MicrocodeHob->DetectedPatchOffset[Index] = CpuMpData->CpuData[Index].MicrocodeData -
+ CpuMpData->MicrocodePatchAddress;
+ } else {
+ MicrocodeHob->DetectedPatchOffset[Index] = MAX_UINT64;
+ }
+ }
+
+ BuildGuidDataHob (
+ &gEdkiiMicrocodePatchHobGuid,
+ MicrocodeHob,
+ HobDataLength
+ );
+
+ return;
+}
+
+/**
Initialize global data for MP support.
@param[in] CpuMpData The pointer to CPU MP Data structure.
@@ -303,6 +357,7 @@ InitMpGlobalData (
EFI_STATUS Status;
SaveCpuMpData (CpuMpData);
+ BuildMicrocodeCacheHob (CpuMpData);
///
/// Install Notify
--
2.12.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info
2019-12-24 1:36 ` [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info Wu, Hao A
@ 2019-12-24 2:51 ` Ni, Ray
0 siblings, 0 replies; 14+ messages in thread
From: Ni, Ray @ 2019-12-24 2:51 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID &
> Platform ID info
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
>
> This commit will collect the CPUID and Platform ID information for each
> processor within system. They will be stored in the CPU_AP_DATA structure.
>
> These information will be used in the next commit to decide whether a
> microcode patch will be loaded into memory.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 2 ++
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 8fa07b12c5..4440dc2701 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -122,6 +122,8 @@ typedef struct {
> UINT64 CurrentTime;
> UINT64 TotalTime;
> EFI_EVENT WaitEvent;
> + UINT32 ProcessorSignature;
> + UINT8 PlatformId;
> } CPU_AP_DATA;
>
> //
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d32adf0780..d5077e080e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -548,7 +548,8 @@ InitializeApData (
> IN UINT64 ApTopOfStack
> )
> {
> - CPU_INFO_IN_HOB *CpuInfoInHob;
> + CPU_INFO_IN_HOB *CpuInfoInHob;
> + MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
>
> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData-
> >CpuInfoInHob;
> CpuInfoInHob[ProcessorNumber].InitialApicId = GetInitialApicId ();
> @@ -559,6 +560,17 @@ InitializeApData (
> CpuMpData->CpuData[ProcessorNumber].Waiting = FALSE;
> CpuMpData->CpuData[ProcessorNumber].CpuHealthy = (BistData == 0) ?
> TRUE : FALSE;
>
> + PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> + CpuMpData->CpuData[ProcessorNumber].PlatformId = (UINT8)
> PlatformIdMsr.Bits.PlatformId;
> +
> + AsmCpuid (
> + CPUID_VERSION_INFO,
> + &CpuMpData->CpuData[ProcessorNumber].ProcessorSignature,
> + NULL,
> + NULL,
> + NULL
> + );
> +
> InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
> SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle);
> }
> --
> 2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
2019-12-24 1:36 ` [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Wu, Hao A
@ 2019-12-24 3:32 ` Ni, Ray
2019-12-24 5:48 ` Wu, Hao A
0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2019-12-24 3:32 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
6 minor comments.
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> loading microcode patches
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
>
> This commit will attempt to reduce the copy size when loading the
> microcode patches data from flash into memory.
>
> Such optimization is done by a pre-process of the microcode patch headers
> (on flash). A microcode patch will be loaded into memory only when the
> below 2 criteria are met:
>
> A. With a microcode patch header (which means the data is not padding data
> between microcode patches);
> B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header match
> at least one processor within system.
>
> Criterion B will require all the processors to be woken up once to collect
> their CPUID and Platform ID information. Hence, this commit will move the
> copy, detect and apply of microcode patch on BSP and APs after all the
> processors have been woken up.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++
> UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 82 ++-----
> 3 files changed, 283 insertions(+), 58 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 4440dc2701..56b0df664a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -44,6 +44,20 @@
> #define CPU_SWITCH_STATE_LOADED 2
>
> //
> +// Default maximum number of entries to store the microcode patches
> information
> +//
> +#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8
> +
> +//
> +// Data structure for microcode patch information
> +//
> +typedef struct {
> + UINTN Address;
> + UINTN Size;
> + UINTN AlignedSize;
> +} MICROCODE_PATCH_INFO;
> +
> +//
> // CPU exchange information for switch BSP
> //
> typedef struct {
> @@ -576,6 +590,16 @@ MicrocodeDetect (
> );
>
> /**
> + Load the required microcode patches data into memory.
> +
> + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> +**/
> +VOID
> +LoadMicrocodePatch (
> + IN OUT CPU_MP_DATA *CpuMpData
> + );
> +
> +/**
> Detect whether Mwait-monitor feature is supported.
>
> @retval TRUE Mwait-monitor feature is supported.
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 199b1f23ce..68088b26a5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -331,3 +331,238 @@ Done:
> MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
> (UINTN) MicrocodeData, LatestRevision));
> }
> }
> +
> +/**
> + Actual worker function that loads the required microcode patches into
> memory.
> +
> + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> + @param[in] PatchInfoBuffer The pointer to an array of information on
> + the microcode patches that will be loaded
> + into memory.
> + @param[in] PatchNumber The number of microcode patches that
> will
> + be loaded into memory.
> + @param[in] TotalLoadSize The total size of all the microcode
> + patches to be loaded.
> +**/
> +VOID
> +LoadMicrocodePatchWorker (
> + IN OUT CPU_MP_DATA *CpuMpData,
> + IN MICROCODE_PATCH_INFO *PatchInfoBuffer,
> + IN UINTN PatchNumber,
1. "PatchInfoBuffer" -> "Patches"?
"PatchNumber" -> "PatchCount"?
> + //
> + // Load all the required microcode patches into memory
> + //
> + for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchNumber;
> Index++) {
> + CopyMem (
> + Walker,
> + (VOID *) PatchInfoBuffer[Index].Address,
> + PatchInfoBuffer[Index].Size
> + );
> +
> + if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Size) {
2. This if-check is not needed because AlignedSize always >= Size.
Below ZeroMem() will directly return due to the 2nd parameter is 0.
> + //
> + // Zero-fill the padding area
> + //
> + ZeroMem (
> + Walker + PatchInfoBuffer[Index].Size,
> + PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
> + );
> + }
> +
> + if (TotalSize > MAX_UINTN - TotalLoadSize ||
> + ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
> + goto OnExit;
> + }
3. ALIGN_VALUE (TotalSize , SIZE_1KB) always >= TotalSize.
So can we only check
ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize
?
> + PatchInfoBuffer[PatchNumber - 1].Address = (UINTN)
> MicrocodeEntryPoint;
> + PatchInfoBuffer[PatchNumber - 1].Size = TotalSize;
> + PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE
> (TotalSize, SIZE_1KB);
4. "PatchNumber" -> "PatchCount"?
> + TotalLoadSize += PatchInfoBuffer[PatchNumber - 1].AlignedSize;
> + }
> +
> + //
> + // Process the next microcode patch
> + //
> + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + TotalSize);
> + } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> +
> + if (PatchNumber == 0) {
> + //
> + // No patch needs to be loaded
> + //
> + goto OnExit;
5. This "goto" is not necessary. You can call below two lines when PatchCount != 0.
Less "goto" is better.
> + }
> +
> + DEBUG ((
> + DEBUG_INFO,
> + "%a: 0x%x microcode patches will be loaded into memory, with size
> 0x%x.\n",
> + __FUNCTION__, PatchNumber, TotalLoadSize
> + ));
> +
> + LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber,
> TotalLoadSize);
> +
> +OnExit:
> + if (PatchInfoBuffer != NULL) {
> + FreePool (PatchInfoBuffer);
> + }
> + return;
> +}
> @@ -1788,7 +1752,6 @@ MpInitLibInitialize (
> //
> CpuMpData->CpuCount = OldCpuMpData->CpuCount;
> CpuMpData->BspNumber = OldCpuMpData->BspNumber;
> - CpuMpData->InitFlag = ApInitReconfig;
6. Do you think that ApInitReconfig definition can be removed?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB
2019-12-24 1:36 ` [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB Wu, Hao A
@ 2019-12-24 3:48 ` Ni, Ray
2019-12-24 5:48 ` Wu, Hao A
2019-12-24 8:15 ` Ni, Ray
1 sibling, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2019-12-24 3:48 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
> + //
> + // The number of processors within the system.
> + //
> + UINT32 ProcessorNumber;
1. Number is a bit confusing here. I also provided comments to other patches.
We could "ProcessorCount" here.
Number can be either count of items, or an index to a specific item.
> + //
> + // An array with 'ProcessorNumber' elements that stores the offset (with
> + // regard to 'MicrocodePatchAddress') of the applied microcode patch for
> each
> + // processor.
> + // If no microcode patch is applied for certain processor, the relating
> + // element will be set to MAX_UINT64.
> + //
> + UINT64 DetectedPatchOffset[0];
2. "ProcessorSpecificPatchOffset"?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode patch HOB
2019-12-24 1:36 ` [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce " Wu, Hao A
@ 2019-12-24 3:54 ` Ni, Ray
2019-12-25 5:42 ` Wu, Hao A
0 siblings, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2019-12-24 3:54 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
MicrocodeDetect() is called in DXE phase as well.
But it doesn't use the new MicrocodePatch HOB to speed up the
microcode detection. Why?
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> patch HOB
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
>
> This commit will update the MpInitLib to:
>
> A. Collect the base address and size information after microcode patches
> being loaded into memory;
> B. Collect the applied microcode patch for each processor within system;
> C. Based on the collected information, produce the EDKII microcode patch
> HOB.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 +++++++--
> UefiCpuPkg/Library/MpInitLib/Microcode.c | 16 ++++--
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++-
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 55 ++++++++++++++++++++
> 5 files changed, 96 insertions(+), 8 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 1538185ef9..326703cc9a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -63,3 +63,4 @@ [Pcd]
>
> [Guids]
> gEdkiiS3SmmInitDoneGuid
> + gEdkiiMicrocodePatchHobGuid
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 56b0df664a..fb251d7aef 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -138,6 +138,7 @@ typedef struct {
> EFI_EVENT WaitEvent;
> UINT32 ProcessorSignature;
> UINT8 PlatformId;
> + UINT64 MicrocodeData;
> } CPU_AP_DATA;
>
> //
> @@ -580,13 +581,15 @@ CheckAndUpdateApsStatus (
> /**
> Detect whether specified processor can find matching microcode patch and
> load it.
>
> - @param[in] CpuMpData The pointer to CPU MP Data structure.
> - @param[in] IsBspCallIn Indicate whether the caller is BSP or not.
> + @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
> + minus 1.
> **/
> VOID
> MicrocodeDetect (
> IN CPU_MP_DATA *CpuMpData,
> - IN BOOLEAN IsBspCallIn
> + IN UINTN ProcessorNumber
> );
>
> /**
> @@ -619,5 +622,20 @@ EnableDebugAgent (
> VOID
> );
>
> +/**
> + Find the current Processor number by APIC ID.
> +
> + @param[in] CpuMpData Pointer to PEI CPU MP Data
> + @param[out] ProcessorNumber Return the pocessor number found
> +
> + @retval EFI_SUCCESS ProcessorNumber is found and returned.
> + @retval EFI_NOT_FOUND ProcessorNumber is not found.
> +**/
> +EFI_STATUS
> +GetProcessorNumber (
> + IN CPU_MP_DATA *CpuMpData,
> + OUT UINTN *ProcessorNumber
> + );
> +
> #endif
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 68088b26a5..bbc40f81bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -65,13 +65,15 @@ GetCurrentMicrocodeSignature (
> 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] IsBspCallIn Indicate whether the caller is BSP or not.
> + @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
> + minus 1.
> **/
> VOID
> MicrocodeDetect (
> IN CPU_MP_DATA *CpuMpData,
> - IN BOOLEAN IsBspCallIn
> + IN UINTN ProcessorNumber
> )
> {
> UINT32 ExtendedTableLength;
> @@ -93,6 +95,7 @@ MicrocodeDetect (
> MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
> UINT32 ProcessorFlags;
> UINT32 ThreadId;
> + BOOLEAN IsBspCallIn;
>
> //
> // set ProcessorFlags to suppress incorrect compiler/analyzer warnings
> @@ -107,6 +110,7 @@ MicrocodeDetect (
> }
>
> CurrentRevision = GetCurrentMicrocodeSignature ();
> + IsBspCallIn = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> TRUE : FALSE;
> if (CurrentRevision != 0 && !IsBspCallIn) {
> //
> // Skip loading microcode if it has been loaded successfully
> @@ -316,6 +320,12 @@ Done:
> DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does
> not match \
> loaded microcode signature [0x%08x]\n", CurrentRevision,
> LatestRevision));
> ReleaseSpinLock(&CpuMpData->MpLock);
> + } else {
> + //
> + // Save the detected microcode patch address for each processor.
> + // It will be used when building the microcode patch cache HOB.
> + //
> + CpuMpData->CpuData[ProcessorNumber].MicrocodeData = (UINTN)
> MicrocodeData;
> }
> }
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 199468156b..8f4b2b1973 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -399,12 +399,16 @@ ApInitializeSync (
> )
> {
> CPU_MP_DATA *CpuMpData;
> + UINTN ProcessorNumber;
> + EFI_STATUS Status;
>
> CpuMpData = (CPU_MP_DATA *) Buffer;
> + Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
> + ASSERT_EFI_ERROR (Status);
> //
> // Load microcode on AP
> //
> - MicrocodeDetect (CpuMpData, FALSE);
> + MicrocodeDetect (CpuMpData, ProcessorNumber);
> //
> // Sync BSP's MTRR table to AP
> //
> @@ -1765,7 +1769,7 @@ MpInitLibInitialize (
> //
> // Detect and apply Microcode on BSP
> //
> - MicrocodeDetect (CpuMpData, TRUE);
> + MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>
> //
> // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3999603c3e..977818fda4 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -9,6 +9,7 @@
> #include "MpLib.h"
> #include <Library/PeiServicesLib.h>
> #include <Guid/S3SmmInitDone.h>
> +#include <Guid/MicrocodePatchHob.h>
>
> /**
> S3 SMM Init Done notification function.
> @@ -291,6 +292,59 @@ CheckAndUpdateApsStatus (
> }
>
> /**
> + Build the microcode patch HOB that contains the base address and size of
> the
> + microcode patch stored in the memory.
> +
> + @param[in] CpuMpData Pointer to the CPU_MP_DATA structure.
> +
> +**/
> +VOID
> +BuildMicrocodeCacheHob (
> + IN CPU_MP_DATA *CpuMpData
> + )
> +{
> + EDKII_MICROCODE_PATCH_HOB *MicrocodeHob;
> + UINTN HobDataLength;
> + UINT32 Index;
> +
> + HobDataLength = sizeof (EDKII_MICROCODE_PATCH_HOB) +
> + sizeof (UINT64) * CpuMpData->CpuCount;
> +
> + MicrocodeHob = AllocatePool (HobDataLength);
> + if (MicrocodeHob == NULL) {
> + ASSERT (FALSE);
> + return;
> + }
> +
> + //
> + // Store the information of the memory region that holds the microcode
> patches.
> + //
> + MicrocodeHob->MicrocodePatchAddress = CpuMpData-
> >MicrocodePatchAddress;
> + MicrocodeHob->MicrocodePatchRegionSize = CpuMpData-
> >MicrocodePatchRegionSize;
> +
> + //
> + // Store the detected microcode patch for each processor as well.
> + //
> + MicrocodeHob->ProcessorNumber = CpuMpData->CpuCount;
> + for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> + if (CpuMpData->CpuData[Index].MicrocodeData != 0) {
> + MicrocodeHob->DetectedPatchOffset[Index] = CpuMpData-
> >CpuData[Index].MicrocodeData -
> + CpuMpData->MicrocodePatchAddress;
> + } else {
> + MicrocodeHob->DetectedPatchOffset[Index] = MAX_UINT64;
> + }
> + }
> +
> + BuildGuidDataHob (
> + &gEdkiiMicrocodePatchHobGuid,
> + MicrocodeHob,
> + HobDataLength
> + );
> +
> + return;
> +}
> +
> +/**
> Initialize global data for MP support.
>
> @param[in] CpuMpData The pointer to CPU MP Data structure.
> @@ -303,6 +357,7 @@ InitMpGlobalData (
> EFI_STATUS Status;
>
> SaveCpuMpData (CpuMpData);
> + BuildMicrocodeCacheHob (CpuMpData);
>
> ///
> /// Install Notify
> --
> 2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches
2019-12-24 3:32 ` Ni, Ray
@ 2019-12-24 5:48 ` Wu, Hao A
0 siblings, 0 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 5:48 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:32 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> loading microcode patches
>
> 6 minor comments.
Hello Ray,
Thanks for the feedbacks.
For 1,2,4,5, I will update the patch to address your comments.
For 3, my concern is that:
ALIGN_VALUE (TotalSize , SIZE_1KB)
might have a chance to overflow, how about changing the logic to:
if (TotalSize > ALIGN_VALUE (TotalSize, SIZE_1KB) ||
ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
goto OnExit;
}
For 6, the 'ApInitReconfig' flag value is still being used in function
ResetProcessorToIdleState().
Also, I found that there are several places in MpInitLib that use:
CpuMpData->InitFlag != ApInitDone
CpuMpData->InitFlag != ApInitConfig
In some if statements.
So I prefer the evaluation of the removal of the 'ApInitReconfig' flag value to
be handled in another separate patch series. Do you agree?
Best Regards,
Hao Wu
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Tuesday, December 24, 2019 9:37 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> > <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Subject: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> > loading microcode patches
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
> >
> > This commit will attempt to reduce the copy size when loading the
> > microcode patches data from flash into memory.
> >
> > Such optimization is done by a pre-process of the microcode patch headers
> > (on flash). A microcode patch will be loaded into memory only when the
> > below 2 criteria are met:
> >
> > A. With a microcode patch header (which means the data is not padding data
> > between microcode patches);
> > B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header match
> > at least one processor within system.
> >
> > Criterion B will require all the processors to be woken up once to collect
> > their CPUID and Platform ID information. Hence, this commit will move the
> > copy, detect and apply of microcode patch on BSP and APs after all the
> > processors have been woken up.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++
> > UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 82 ++-----
> > 3 files changed, 283 insertions(+), 58 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 4440dc2701..56b0df664a 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -44,6 +44,20 @@
> > #define CPU_SWITCH_STATE_LOADED 2
> >
> > //
> > +// Default maximum number of entries to store the microcode patches
> > information
> > +//
> > +#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8
> > +
> > +//
> > +// Data structure for microcode patch information
> > +//
> > +typedef struct {
> > + UINTN Address;
> > + UINTN Size;
> > + UINTN AlignedSize;
> > +} MICROCODE_PATCH_INFO;
> > +
> > +//
> > // CPU exchange information for switch BSP
> > //
> > typedef struct {
> > @@ -576,6 +590,16 @@ MicrocodeDetect (
> > );
> >
> > /**
> > + Load the required microcode patches data into memory.
> > +
> > + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> > +**/
> > +VOID
> > +LoadMicrocodePatch (
> > + IN OUT CPU_MP_DATA *CpuMpData
> > + );
> > +
> > +/**
> > Detect whether Mwait-monitor feature is supported.
> >
> > @retval TRUE Mwait-monitor feature is supported.
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index 199b1f23ce..68088b26a5 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -331,3 +331,238 @@ Done:
> > MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
> > (UINTN) MicrocodeData, LatestRevision));
> > }
> > }
> > +
> > +/**
> > + Actual worker function that loads the required microcode patches into
> > memory.
> > +
> > + @param[in, out] CpuMpData The pointer to CPU MP Data structure.
> > + @param[in] PatchInfoBuffer The pointer to an array of information
> on
> > + the microcode patches that will be loaded
> > + into memory.
> > + @param[in] PatchNumber The number of microcode patches that
> > will
> > + be loaded into memory.
> > + @param[in] TotalLoadSize The total size of all the microcode
> > + patches to be loaded.
> > +**/
> > +VOID
> > +LoadMicrocodePatchWorker (
> > + IN OUT CPU_MP_DATA *CpuMpData,
> > + IN MICROCODE_PATCH_INFO *PatchInfoBuffer,
> > + IN UINTN PatchNumber,
>
> 1. "PatchInfoBuffer" -> "Patches"?
> "PatchNumber" -> "PatchCount"?
>
> > + //
> > + // Load all the required microcode patches into memory
> > + //
> > + for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchNumber;
> > Index++) {
> > + CopyMem (
> > + Walker,
> > + (VOID *) PatchInfoBuffer[Index].Address,
> > + PatchInfoBuffer[Index].Size
> > + );
> > +
> > + if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Size) {
>
> 2. This if-check is not needed because AlignedSize always >= Size.
> Below ZeroMem() will directly return due to the 2nd parameter is 0.
>
> > + //
> > + // Zero-fill the padding area
> > + //
> > + ZeroMem (
> > + Walker + PatchInfoBuffer[Index].Size,
> > + PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
> > + );
> > + }
> > +
>
> > + if (TotalSize > MAX_UINTN - TotalLoadSize ||
> > + ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
> > + goto OnExit;
> > + }
> 3. ALIGN_VALUE (TotalSize , SIZE_1KB) always >= TotalSize.
> So can we only check
> ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize
> ?
>
>
> > + PatchInfoBuffer[PatchNumber - 1].Address = (UINTN)
> > MicrocodeEntryPoint;
> > + PatchInfoBuffer[PatchNumber - 1].Size = TotalSize;
> > + PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE
> > (TotalSize, SIZE_1KB);
>
> 4. "PatchNumber" -> "PatchCount"?
>
> > + TotalLoadSize += PatchInfoBuffer[PatchNumber - 1].AlignedSize;
> > + }
> > +
> > + //
> > + // Process the next microcode patch
> > + //
> > + MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> > MicrocodeEntryPoint) + TotalSize);
> > + } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> > +
> > + if (PatchNumber == 0) {
> > + //
> > + // No patch needs to be loaded
> > + //
> > + goto OnExit;
>
> 5. This "goto" is not necessary. You can call below two lines when PatchCount !=
> 0.
> Less "goto" is better.
>
> > + }
> > +
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "%a: 0x%x microcode patches will be loaded into memory, with size
> > 0x%x.\n",
> > + __FUNCTION__, PatchNumber, TotalLoadSize
> > + ));
> > +
> > + LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber,
> > TotalLoadSize);
> > +
> > +OnExit:
> > + if (PatchInfoBuffer != NULL) {
> > + FreePool (PatchInfoBuffer);
> > + }
> > + return;
> > +}
>
> > @@ -1788,7 +1752,6 @@ MpInitLibInitialize (
> > //
> > CpuMpData->CpuCount = OldCpuMpData->CpuCount;
> > CpuMpData->BspNumber = OldCpuMpData->BspNumber;
> > - CpuMpData->InitFlag = ApInitReconfig;
>
> 6. Do you think that ApInitReconfig definition can be removed?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB
2019-12-24 3:48 ` Ni, Ray
@ 2019-12-24 5:48 ` Wu, Hao A
0 siblings, 0 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-24 5:48 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
Agree with both 1 & 2. Will update patch to address them.
Best Regards,
Hao Wu
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:48 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode
> patch HOB
>
> > + //
> > + // The number of processors within the system.
> > + //
> > + UINT32 ProcessorNumber;
> 1. Number is a bit confusing here. I also provided comments to other patches.
> We could "ProcessorCount" here.
> Number can be either count of items, or an index to a specific item.
>
>
> > + //
> > + // An array with 'ProcessorNumber' elements that stores the offset (with
> > + // regard to 'MicrocodePatchAddress') of the applied microcode patch for
> > each
> > + // processor.
> > + // If no microcode patch is applied for certain processor, the relating
> > + // element will be set to MAX_UINT64.
> > + //
> > + UINT64 DetectedPatchOffset[0];
>
> 2. "ProcessorSpecificPatchOffset"?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB
2019-12-24 1:36 ` [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB Wu, Hao A
2019-12-24 3:48 ` Ni, Ray
@ 2019-12-24 8:15 ` Ni, Ray
2019-12-25 5:42 ` Wu, Hao A
1 sibling, 1 reply; 14+ messages in thread
From: Ni, Ray @ 2019-12-24 8:15 UTC (permalink / raw)
To: Wu, Hao A, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
I suggest to let the DetectedPatchOffset points to the microcode header instead of microcode data.
Consumers can use the offset to get more information from header than just the microcode data.
> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode
> patch HOB
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
>
> This commit will add the definitions for EDKII microcode patch HOB.
>
> The intention of adding this HOB is to provide a scheme to store the below
> information:
>
> A. The base address and size of the microcode patches that are being
> loaded (from flash) into memory;
> B. The information of applied microcode patch for each processor within
> the system.
>
> The producer of the HOB will be the UefiCpuPkg/MpInitLib (where the load,
> detect and apply of the microcode happen). The consumer of the HOB can
> be
> modules that want to detect/apply the microcode patch by themselves again
> later during the boot flow.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> UefiCpuPkg/UefiCpuPkg.dec | 3 ++
> UefiCpuPkg/Include/Guid/MicrocodePatchHob.h | 50
> ++++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 797f948631..45b267ac61 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -63,6 +63,9 @@ [Guids]
> ## Include/Guid/CpuFeaturesInitDone.h
> gEdkiiCpuFeaturesInitDoneGuid = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98,
> 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
>
> + ## Include/Guid/MicrocodePatchHob.h
> + gEdkiiMicrocodePatchHobGuid = { 0xd178f11d, 0x8716, 0x418e, { 0xa1,
> 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
> +
> [Protocols]
> ## Include/Protocol/SmmCpuService.h
> gEfiSmmCpuServiceProtocolGuid = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94,
> 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
> diff --git a/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> new file mode 100644
> index 0000000000..3667fc3786
> --- /dev/null
> +++ b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> @@ -0,0 +1,50 @@
> +/** @file
> + The microcode patch HOB is used to store the information of:
> + A. Base address and size of the loaded microcode patches data;
> + B. Applied microcode patch for each processor within system.
> +
> + Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> + This program and the accompanying materials
> + are licensed and made available under the terms and conditions of the BSD
> License
> + which accompanies this distribution. The full text of the license may be
> found at
> + http://opensource.org/licenses/bsd-license.php.
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef _MICROCODE_PATCH_HOB_H_
> +#define _MICROCODE_PATCH_HOB_H_
> +
> +extern EFI_GUID gEdkiiMicrocodePatchHobGuid;
> +
> +//
> +// The EDKII microcode patch HOB will be produced by MpInitLib and it can
> be
> +// consumed by modules that want to detect/apply microcode patches.
> +//
> +typedef struct {
> + //
> + // The base address of the microcode patches data after being loaded into
> + // memory.
> + //
> + UINT64 MicrocodePatchAddress;
> + //
> + // The total size of the loaded microcode patches.
> + //
> + UINT64 MicrocodePatchRegionSize;
> + //
> + // The number of processors within the system.
> + //
> + UINT32 ProcessorNumber;
> + //
> + // An array with 'ProcessorNumber' elements that stores the offset (with
> + // regard to 'MicrocodePatchAddress') of the applied microcode patch for
> each
> + // processor.
> + // If no microcode patch is applied for certain processor, the relating
> + // element will be set to MAX_UINT64.
> + //
> + UINT64 DetectedPatchOffset[0];
> +} EDKII_MICROCODE_PATCH_HOB;
> +
> +#endif
> --
> 2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode patch HOB
2019-12-24 3:54 ` Ni, Ray
@ 2019-12-25 5:42 ` Wu, Hao A
0 siblings, 0 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-25 5:42 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:54 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> patch HOB
>
> MicrocodeDetect() is called in DXE phase as well.
> But it doesn't use the new MicrocodePatch HOB to speed up the
> microcode detection. Why?
I found that within function MicrocodeDetect(), it has logic to check whether
a microcode patch has already been applied on a processor previously:
if (CurrentRevision != 0 && !IsBspCallIn) {
//
// Skip loading microcode if it has been loaded successfully
//
return;
}
My take is that if the microcode patch detection and application have been done
once in the PEI phase, the above check can ensure such process will not be
repeated again in the DXE phase.
Best Regards,
Hao Wu
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Tuesday, December 24, 2019 9:37 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> > <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Subject: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> > patch HOB
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
> >
> > This commit will update the MpInitLib to:
> >
> > A. Collect the base address and size information after microcode patches
> > being loaded into memory;
> > B. Collect the applied microcode patch for each processor within system;
> > C. Based on the collected information, produce the EDKII microcode patch
> > HOB.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 1 +
> > UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 +++++++--
> > UefiCpuPkg/Library/MpInitLib/Microcode.c | 16 ++++--
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 ++-
> > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 55 ++++++++++++++++++++
> > 5 files changed, 96 insertions(+), 8 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> > b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> > index 1538185ef9..326703cc9a 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> > @@ -63,3 +63,4 @@ [Pcd]
> >
> > [Guids]
> > gEdkiiS3SmmInitDoneGuid
> > + gEdkiiMicrocodePatchHobGuid
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 56b0df664a..fb251d7aef 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -138,6 +138,7 @@ typedef struct {
> > EFI_EVENT WaitEvent;
> > UINT32 ProcessorSignature;
> > UINT8 PlatformId;
> > + UINT64 MicrocodeData;
> > } CPU_AP_DATA;
> >
> > //
> > @@ -580,13 +581,15 @@ CheckAndUpdateApsStatus (
> > /**
> > Detect whether specified processor can find matching microcode patch and
> > load it.
> >
> > - @param[in] CpuMpData The pointer to CPU MP Data structure.
> > - @param[in] IsBspCallIn Indicate whether the caller is BSP or not.
> > + @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
> > + minus 1.
> > **/
> > VOID
> > MicrocodeDetect (
> > IN CPU_MP_DATA *CpuMpData,
> > - IN BOOLEAN IsBspCallIn
> > + IN UINTN ProcessorNumber
> > );
> >
> > /**
> > @@ -619,5 +622,20 @@ EnableDebugAgent (
> > VOID
> > );
> >
> > +/**
> > + Find the current Processor number by APIC ID.
> > +
> > + @param[in] CpuMpData Pointer to PEI CPU MP Data
> > + @param[out] ProcessorNumber Return the pocessor number found
> > +
> > + @retval EFI_SUCCESS ProcessorNumber is found and returned.
> > + @retval EFI_NOT_FOUND ProcessorNumber is not found.
> > +**/
> > +EFI_STATUS
> > +GetProcessorNumber (
> > + IN CPU_MP_DATA *CpuMpData,
> > + OUT UINTN *ProcessorNumber
> > + );
> > +
> > #endif
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index 68088b26a5..bbc40f81bf 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -65,13 +65,15 @@ GetCurrentMicrocodeSignature (
> > 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] IsBspCallIn Indicate whether the caller is BSP or not.
> > + @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
> > + minus 1.
> > **/
> > VOID
> > MicrocodeDetect (
> > IN CPU_MP_DATA *CpuMpData,
> > - IN BOOLEAN IsBspCallIn
> > + IN UINTN ProcessorNumber
> > )
> > {
> > UINT32 ExtendedTableLength;
> > @@ -93,6 +95,7 @@ MicrocodeDetect (
> > MSR_IA32_PLATFORM_ID_REGISTER PlatformIdMsr;
> > UINT32 ProcessorFlags;
> > UINT32 ThreadId;
> > + BOOLEAN IsBspCallIn;
> >
> > //
> > // set ProcessorFlags to suppress incorrect compiler/analyzer warnings
> > @@ -107,6 +110,7 @@ MicrocodeDetect (
> > }
> >
> > CurrentRevision = GetCurrentMicrocodeSignature ();
> > + IsBspCallIn = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> > TRUE : FALSE;
> > if (CurrentRevision != 0 && !IsBspCallIn) {
> > //
> > // Skip loading microcode if it has been loaded successfully
> > @@ -316,6 +320,12 @@ Done:
> > DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does
> > not match \
> > loaded microcode signature [0x%08x]\n", CurrentRevision,
> > LatestRevision));
> > ReleaseSpinLock(&CpuMpData->MpLock);
> > + } else {
> > + //
> > + // Save the detected microcode patch address for each processor.
> > + // It will be used when building the microcode patch cache HOB.
> > + //
> > + CpuMpData->CpuData[ProcessorNumber].MicrocodeData = (UINTN)
> > MicrocodeData;
> > }
> > }
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 199468156b..8f4b2b1973 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -399,12 +399,16 @@ ApInitializeSync (
> > )
> > {
> > CPU_MP_DATA *CpuMpData;
> > + UINTN ProcessorNumber;
> > + EFI_STATUS Status;
> >
> > CpuMpData = (CPU_MP_DATA *) Buffer;
> > + Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
> > + ASSERT_EFI_ERROR (Status);
> > //
> > // Load microcode on AP
> > //
> > - MicrocodeDetect (CpuMpData, FALSE);
> > + MicrocodeDetect (CpuMpData, ProcessorNumber);
> > //
> > // Sync BSP's MTRR table to AP
> > //
> > @@ -1765,7 +1769,7 @@ MpInitLibInitialize (
> > //
> > // Detect and apply Microcode on BSP
> > //
> > - MicrocodeDetect (CpuMpData, TRUE);
> > + MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> >
> > //
> > // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > index 3999603c3e..977818fda4 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > @@ -9,6 +9,7 @@
> > #include "MpLib.h"
> > #include <Library/PeiServicesLib.h>
> > #include <Guid/S3SmmInitDone.h>
> > +#include <Guid/MicrocodePatchHob.h>
> >
> > /**
> > S3 SMM Init Done notification function.
> > @@ -291,6 +292,59 @@ CheckAndUpdateApsStatus (
> > }
> >
> > /**
> > + Build the microcode patch HOB that contains the base address and size of
> > the
> > + microcode patch stored in the memory.
> > +
> > + @param[in] CpuMpData Pointer to the CPU_MP_DATA structure.
> > +
> > +**/
> > +VOID
> > +BuildMicrocodeCacheHob (
> > + IN CPU_MP_DATA *CpuMpData
> > + )
> > +{
> > + EDKII_MICROCODE_PATCH_HOB *MicrocodeHob;
> > + UINTN HobDataLength;
> > + UINT32 Index;
> > +
> > + HobDataLength = sizeof (EDKII_MICROCODE_PATCH_HOB) +
> > + sizeof (UINT64) * CpuMpData->CpuCount;
> > +
> > + MicrocodeHob = AllocatePool (HobDataLength);
> > + if (MicrocodeHob == NULL) {
> > + ASSERT (FALSE);
> > + return;
> > + }
> > +
> > + //
> > + // Store the information of the memory region that holds the microcode
> > patches.
> > + //
> > + MicrocodeHob->MicrocodePatchAddress = CpuMpData-
> > >MicrocodePatchAddress;
> > + MicrocodeHob->MicrocodePatchRegionSize = CpuMpData-
> > >MicrocodePatchRegionSize;
> > +
> > + //
> > + // Store the detected microcode patch for each processor as well.
> > + //
> > + MicrocodeHob->ProcessorNumber = CpuMpData->CpuCount;
> > + for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> > + if (CpuMpData->CpuData[Index].MicrocodeData != 0) {
> > + MicrocodeHob->DetectedPatchOffset[Index] = CpuMpData-
> > >CpuData[Index].MicrocodeData -
> > + CpuMpData->MicrocodePatchAddress;
> > + } else {
> > + MicrocodeHob->DetectedPatchOffset[Index] = MAX_UINT64;
> > + }
> > + }
> > +
> > + BuildGuidDataHob (
> > + &gEdkiiMicrocodePatchHobGuid,
> > + MicrocodeHob,
> > + HobDataLength
> > + );
> > +
> > + return;
> > +}
> > +
> > +/**
> > Initialize global data for MP support.
> >
> > @param[in] CpuMpData The pointer to CPU MP Data structure.
> > @@ -303,6 +357,7 @@ InitMpGlobalData (
> > EFI_STATUS Status;
> >
> > SaveCpuMpData (CpuMpData);
> > + BuildMicrocodeCacheHob (CpuMpData);
> >
> > ///
> > /// Install Notify
> > --
> > 2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB
2019-12-24 8:15 ` Ni, Ray
@ 2019-12-25 5:42 ` Wu, Hao A
0 siblings, 0 replies; 14+ messages in thread
From: Wu, Hao A @ 2019-12-25 5:42 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Laszlo Ersek, Zeng, Star, Fu, Siyuan,
Kinney, Michael D
> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 4:16 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode
> patch HOB
>
> I suggest to let the DetectedPatchOffset points to the microcode header instead
> of microcode data.
> Consumers can use the offset to get more information from header than just
> the microcode data.
Yes.
I will handle this as well in the next version of the series.
Best Regards,
Hao Wu
>
> > -----Original Message-----
> > From: Wu, Hao A <hao.a.wu@intel.com>
> > Sent: Tuesday, December 24, 2019 9:37 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> > Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> > <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Subject: [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode
> > patch HOB
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
> >
> > This commit will add the definitions for EDKII microcode patch HOB.
> >
> > The intention of adding this HOB is to provide a scheme to store the below
> > information:
> >
> > A. The base address and size of the microcode patches that are being
> > loaded (from flash) into memory;
> > B. The information of applied microcode patch for each processor within
> > the system.
> >
> > The producer of the HOB will be the UefiCpuPkg/MpInitLib (where the load,
> > detect and apply of the microcode happen). The consumer of the HOB can
> > be
> > modules that want to detect/apply the microcode patch by themselves again
> > later during the boot flow.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> > UefiCpuPkg/UefiCpuPkg.dec | 3 ++
> > UefiCpuPkg/Include/Guid/MicrocodePatchHob.h | 50
> > ++++++++++++++++++++
> > 2 files changed, 53 insertions(+)
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > index 797f948631..45b267ac61 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > @@ -63,6 +63,9 @@ [Guids]
> > ## Include/Guid/CpuFeaturesInitDone.h
> > gEdkiiCpuFeaturesInitDoneGuid = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98,
> > 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
> >
> > + ## Include/Guid/MicrocodePatchHob.h
> > + gEdkiiMicrocodePatchHobGuid = { 0xd178f11d, 0x8716, 0x418e, { 0xa1,
> > 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
> > +
> > [Protocols]
> > ## Include/Protocol/SmmCpuService.h
> > gEfiSmmCpuServiceProtocolGuid = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94,
> > 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
> > diff --git a/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> > b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> > new file mode 100644
> > index 0000000000..3667fc3786
> > --- /dev/null
> > +++ b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> > @@ -0,0 +1,50 @@
> > +/** @file
> > + The microcode patch HOB is used to store the information of:
> > + A. Base address and size of the loaded microcode patches data;
> > + B. Applied microcode patch for each processor within system.
> > +
> > + Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > + This program and the accompanying materials
> > + are licensed and made available under the terms and conditions of the BSD
> > License
> > + which accompanies this distribution. The full text of the license may be
> > found at
> > + http://opensource.org/licenses/bsd-license.php.
> > +
> > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > BASIS,
> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef _MICROCODE_PATCH_HOB_H_
> > +#define _MICROCODE_PATCH_HOB_H_
> > +
> > +extern EFI_GUID gEdkiiMicrocodePatchHobGuid;
> > +
> > +//
> > +// The EDKII microcode patch HOB will be produced by MpInitLib and it can
> > be
> > +// consumed by modules that want to detect/apply microcode patches.
> > +//
> > +typedef struct {
> > + //
> > + // The base address of the microcode patches data after being loaded into
> > + // memory.
> > + //
> > + UINT64 MicrocodePatchAddress;
> > + //
> > + // The total size of the loaded microcode patches.
> > + //
> > + UINT64 MicrocodePatchRegionSize;
> > + //
> > + // The number of processors within the system.
> > + //
> > + UINT32 ProcessorNumber;
> > + //
> > + // An array with 'ProcessorNumber' elements that stores the offset (with
> > + // regard to 'MicrocodePatchAddress') of the applied microcode patch for
> > each
> > + // processor.
> > + // If no microcode patch is applied for certain processor, the relating
> > + // element will be set to MAX_UINT64.
> > + //
> > + UINT64 DetectedPatchOffset[0];
> > +} EDKII_MICROCODE_PATCH_HOB;
> > +
> > +#endif
> > --
> > 2.12.0.windows.1
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-12-25 5:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-24 1:36 [PATCH v1 0/4] Microcode related optimizations Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 1/4] UefiCpuPkg/MpInitLib: Collect processors' CPUID & Platform ID info Wu, Hao A
2019-12-24 2:51 ` Ni, Ray
2019-12-24 1:36 ` [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Wu, Hao A
2019-12-24 3:32 ` Ni, Ray
2019-12-24 5:48 ` Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 3/4] UefiCpuPkg: Add definitions for EDKII microcode patch HOB Wu, Hao A
2019-12-24 3:48 ` Ni, Ray
2019-12-24 5:48 ` Wu, Hao A
2019-12-24 8:15 ` Ni, Ray
2019-12-25 5:42 ` Wu, Hao A
2019-12-24 1:36 ` [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce " Wu, Hao A
2019-12-24 3:54 ` Ni, Ray
2019-12-25 5:42 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox