[edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[Yuanhao Xie]

The DXE stage's Microcode loading process has been eliminated.
Store BSP's MTRR setting only when CpuCount>1.
Extract Dump Microcode Revision as a function

Compare with V3, this version updates the comments,
Move GetMicrocodePatchInfoFromHob removal from patch 3 to patch 1

on
xieyuanh (3):
  UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.
  UefiCpuPkg/MpInitLib: Store MTRRs settings only when  CpuCount>1
  UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.

 UefiCpuPkg/Library/MpInitLib/Microcode.c | 85 +++++++++++++++++++++++++++++++++++++++----------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 81 ++++++++++++++++++++++++++++++++-------------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h     | 31 ++++++++++---------------------
 3 files changed, 81 insertions(+), 116 deletions(-)

--
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111686): https://edk2.groups.io/g/devel/message/111686
Mute This Topic: https://groups.io/mt/102775770/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V4 1/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[Yuanhao Xie]

The DXE stage's Microcode loading process has been elimincated by:

1. Let ShadowMicrocodeUpdatePatch and MicrocodeDetect for BSP performed
only during the PEI phase. DXE skip those actions.
2. BSP in DXE WakeUpAp only for synchronizing MTRR settings, not loading
microcode.

Synchronizing the MTRRs setting to the APs is always essential.
The modification of the MTRRs on the BSP may happens during
timing in the early DXE phase, while the CpuMp PPI service and
the CpuMp Protocol both not available.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 46 ----------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 39 ++++++++++++++++++++++++---------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h     | 21 ---------------------
 3 files changed, 24 insertions(+), 82 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 11720560af..8faa93024d 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -338,49 +338,3 @@ ShadowMicrocodeUpdatePatch (
     ShadowMicrocodePatchByPcd (CpuMpData);
   }
 }
-
-/**
-  Get the cached microcode patch base address and size from the microcode patch
-  information cache HOB.
-
-  @param[out] Address       Base address of the microcode patches data.
-                            It will be updated if the microcode patch
-                            information cache HOB is found.
-  @param[out] RegionSize    Size of the microcode patches data.
-                            It will be updated if the microcode patch
-                            information cache HOB is found.
-
-  @retval  TRUE     The microcode patch information cache HOB is found.
-  @retval  FALSE    The microcode patch information cache HOB is not found.
-
-**/
-BOOLEAN
-GetMicrocodePatchInfoFromHob (
-  UINT64  *Address,
-  UINT64  *RegionSize
-  )
-{
-  EFI_HOB_GUID_TYPE          *GuidHob;
-  EDKII_MICROCODE_PATCH_HOB  *MicrocodePathHob;
-
-  GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid);
-  if (GuidHob == NULL) {
-    DEBUG ((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n", __func__));
-    return FALSE;
-  }
-
-  MicrocodePathHob = GET_GUID_HOB_DATA (GuidHob);
-
-  *Address    = MicrocodePathHob->MicrocodePatchAddress;
-  *RegionSize = MicrocodePathHob->MicrocodePatchRegionSize;
-
-  DEBUG ((
-    DEBUG_INFO,
-    "%a: MicrocodeBase = 0x%lx, MicrocodeSize = 0x%lx\n",
-    __func__,
-    *Address,
-    *RegionSize
-    ));
-
-  return TRUE;
-}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 9a6ec5db5c..1fd6440a53 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -451,12 +451,20 @@ ApInitializeSync (
   CpuMpData = (CPU_MP_DATA *)Buffer;
   Status    = GetProcessorNumber (CpuMpData, &ProcessorNumber);
   ASSERT_EFI_ERROR (Status);
+  ASSERT (CpuMpData->InitFlag == ApInitReconfig || CpuMpData->InitFlag == ApInitDone);
+  if (CpuMpData->InitFlag != ApInitReconfig) {
+    //
+    // Load microcode on AP for PEI phase.
+    // During the DXE phase, it cannot omitted.
+    //
+    MicrocodeDetect (CpuMpData, ProcessorNumber);
+  }
+
   //
-  // Load microcode on AP
-  //
-  MicrocodeDetect (CpuMpData, ProcessorNumber);
-  //
-  // Sync BSP's MTRR table to AP
+  // Synchronizing the MTRRs setting to the APs is always essential.
+  // The modification of the MTRRs on the BSP may happens during
+  // timing in the early DXE phase, while the CpuMp PPI service and
+  // the CpuMp Protocol are both not available.
   //
   MtrrSetAllMtrrs (&CpuMpData->MtrrTable);
 }
@@ -2224,29 +2232,25 @@ MpInitLibInitialize (
     }
   }
 
-  if (!GetMicrocodePatchInfoFromHob (
-         &CpuMpData->MicrocodePatchAddress,
-         &CpuMpData->MicrocodePatchRegionSize
-         ))
-  {
+  if (MpHandOff == NULL) {
     //
     // The microcode patch information cache HOB does not exist, which means
     // the microcode patches data has not been loaded into memory yet
     //
     ShadowMicrocodeUpdatePatch (CpuMpData);
+    //
+    // Detect and apply Microcode on BSP
+    //
+    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
   }
 
-  //
-  // Detect and apply Microcode on BSP
-  //
-  MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
   //
   // Store BSP's MTRR setting
   //
   MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
 
   //
-  // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
+  // Wakeup APs to do some AP initialize sync (MTRR and/or Microcode).
   //
   if (CpuMpData->CpuCount > 1) {
     if (MpHandOff != NULL) {
@@ -2258,6 +2262,11 @@ MpInitLibInitialize (
       CpuMpData->InitFlag = ApInitReconfig;
     }
 
+    //
+    // Wake up APs to perform AP initialization synchronization.
+    // 1. For the PEI stage, load microcode and synchronize MTRRs,
+    // 2. For the DXE phase, only synchronize MTRRs.
+    //
     WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     //
     // Wait for all APs finished initialization
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 763db4963d..01b5b9c113 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -744,27 +744,6 @@ ShadowMicrocodeUpdatePatch (
   IN OUT CPU_MP_DATA  *CpuMpData
   );
 
-/**
-  Get the cached microcode patch base address and size from the microcode patch
-  information cache HOB.
-
-  @param[out] Address       Base address of the microcode patches data.
-                            It will be updated if the microcode patch
-                            information cache HOB is found.
-  @param[out] RegionSize    Size of the microcode patches data.
-                            It will be updated if the microcode patch
-                            information cache HOB is found.
-
-  @retval  TRUE     The microcode patch information cache HOB is found.
-  @retval  FALSE    The microcode patch information cache HOB is not found.
-
-**/
-BOOLEAN
-GetMicrocodePatchInfoFromHob (
-  UINT64  *Address,
-  UINT64  *RegionSize
-  );
-
 /**
   Detect whether Mwait-monitor feature is supported.
 
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111687): https://edk2.groups.io/g/devel/message/111687
Mute This Topic: https://groups.io/mt/102775771/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V4 2/3] UefiCpuPkg/MpInitLib: Store MTRRs settings only when CpuCount>1

[Yuanhao Xie]

Store BSP's MTRR setting only when CpuCount>1.

Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 1fd6440a53..4dac931fa5 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2244,15 +2244,14 @@ MpInitLibInitialize (
     MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
   }
 
-  //
-  // Store BSP's MTRR setting
-  //
-  MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
-
   //
   // Wakeup APs to do some AP initialize sync (MTRR and/or Microcode).
   //
   if (CpuMpData->CpuCount > 1) {
+    //
+    // Store BSP's MTRR setting
+    //
+    MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
     if (MpHandOff != NULL) {
       //
       // Only needs to use this flag for DXE phase to update the wake up
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111688): https://edk2.groups.io/g/devel/message/111688
Mute This Topic: https://groups.io/mt/102775772/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V4 3/3] UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.

[Yuanhao Xie]

There is no functional changes, only extract DumpMicrocodeRevision since
only in PEI BSP will detect, apply microcode, and APs will sync
microcode.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 39 +++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 33 ++++-----------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h     | 10 ++++++++++
 3 files changed, 53 insertions(+), 29 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 8faa93024d..947caee97a 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -321,6 +321,45 @@ OnExit:
   FreePages (MicrocodeCpuIds, EFI_SIZE_TO_PAGES (CpuMpData->CpuCount * sizeof (EDKII_PEI_MICROCODE_CPU_ID)));
 }
 
+/**
+    Dump the microcode revision for each core.
+
+    @param[in] CpuMpData          Pointer to CPU MP Data
+  **/
+VOID
+DumpMicrocodeRevision (
+  IN CPU_MP_DATA  *CpuMpData
+  )
+{
+  UINT32           ThreadId;
+  UINT32           ExpectedMicrocodeRevision;
+  CPU_INFO_IN_HOB  *CpuInfoInHob;
+  UINTN            Index;
+
+  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
+  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+    GetProcessorLocationByApicId (CpuInfoInHob[Index].InitialApicId, NULL, NULL, &ThreadId);
+    if (ThreadId == 0) {
+      //
+      // MicrocodeDetect() loads microcode in first thread of each core, so,
+      // CpuMpData->CpuData[Index].MicrocodeEntryAddr is initialized only for first thread of each core.
+      //
+      ExpectedMicrocodeRevision = 0;
+      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
+        ExpectedMicrocodeRevision = ((CPU_MICROCODE_HEADER *)(UINTN)CpuMpData->CpuData[Index].MicrocodeEntryAddr)->UpdateRevision;
+      }
+
+      DEBUG ((
+        DEBUG_INFO,
+        "CPU[%04d]: Microcode revision = %08x, expected = %08x\n",
+        Index,
+        CpuMpData->CpuData[Index].MicrocodeRevision,
+        ExpectedMicrocodeRevision
+        ));
+    }
+  }
+}
+
 /**
   Shadow the required microcode patches data into memory.
 
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 4dac931fa5..785042a8d7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2283,37 +2283,12 @@ MpInitLibInitialize (
     }
   }
 
-  //
-  // Dump the microcode revision for each core.
-  //
-  DEBUG_CODE_BEGIN ();
-  UINT32  ThreadId;
-  UINT32  ExpectedMicrocodeRevision;
-
-  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
-  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
-    GetProcessorLocationByApicId (CpuInfoInHob[Index].InitialApicId, NULL, NULL, &ThreadId);
-    if (ThreadId == 0) {
-      //
-      // MicrocodeDetect() loads microcode in first thread of each core, so,
-      // CpuMpData->CpuData[Index].MicrocodeEntryAddr is initialized only for first thread of each core.
-      //
-      ExpectedMicrocodeRevision = 0;
-      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
-        ExpectedMicrocodeRevision = ((CPU_MICROCODE_HEADER *)(UINTN)CpuMpData->CpuData[Index].MicrocodeEntryAddr)->UpdateRevision;
-      }
-
-      DEBUG ((
-        DEBUG_INFO,
-        "CPU[%04d]: Microcode revision = %08x, expected = %08x\n",
-        Index,
-        CpuMpData->CpuData[Index].MicrocodeRevision,
-        ExpectedMicrocodeRevision
-        ));
-    }
+  if (MpHandOff == NULL) {
+    DEBUG_CODE (
+      DumpMicrocodeRevision (CpuMpData);
+      );
   }
 
-  DEBUG_CODE_END ();
   //
   // Initialize global data for MP support
   //
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 01b5b9c113..fd302e6845 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -734,6 +734,16 @@ MicrocodeDetect (
   IN UINTN        ProcessorNumber
   );
 
+/**
+    Dump the microcode revision for each core.
+
+    @param[in] CpuMpData          Pointer to CPU MP Data
+  **/
+VOID
+DumpMicrocodeRevision (
+  IN CPU_MP_DATA  *CpuMpData
+  );
+
 /**
   Shadow the required microcode patches data into memory.
 
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111689): https://edk2.groups.io/g/devel/message/111689
Mute This Topic: https://groups.io/mt/102775773/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yuanhao
> Xie
> Sent: Friday, November 24, 2023 10:57 AM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>
> Subject: [edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate
> redundant microcode loading in DXE.
> 
> The DXE stage's Microcode loading process has been eliminated.
> Store BSP's MTRR setting only when CpuCount>1.
> Extract Dump Microcode Revision as a function
> 
> Compare with V3, this version updates the comments,
> Move GetMicrocodePatchInfoFromHob removal from patch 3 to patch 1
> 
> on
> xieyuanh (3):
>   UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.
>   UefiCpuPkg/MpInitLib: Store MTRRs settings only when  CpuCount>1
>   UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.
> 
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 85
> +++++++++++++++++++++++++++++++++++++++--------------------------------------
> --------
>  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 81
> ++++++++++++++++++++++++++++++++-------------------------------------------------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h     | 31
> ++++++++++---------------------
>  3 files changed, 81 insertions(+), 116 deletions(-)
> 
> --
> 2.39.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111693): https://edk2.groups.io/g/devel/message/111693
Mute This Topic: https://groups.io/mt/102775770/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[Laszlo Ersek]

On 11/24/23 04:45, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>

Please go ahead with merging this. The patch set has changed so much
since v2 (which I last reviewed) that I can't perform an incremental
review, and I can't review this from zero at the moment.

Thanks
Laszlo

> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yuanhao
>> Xie
>> Sent: Friday, November 24, 2023 10:57 AM
>> To: devel@edk2.groups.io
>> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>
>> Subject: [edk2-devel] [Patch V4 0/3] UefiCpuPkg/MpInitLib: Eliminate
>> redundant microcode loading in DXE.
>>
>> The DXE stage's Microcode loading process has been eliminated.
>> Store BSP's MTRR setting only when CpuCount>1.
>> Extract Dump Microcode Revision as a function
>>
>> Compare with V3, this version updates the comments,
>> Move GetMicrocodePatchInfoFromHob removal from patch 3 to patch 1
>>
>> on
>> xieyuanh (3):
>>   UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.
>>   UefiCpuPkg/MpInitLib: Store MTRRs settings only when  CpuCount>1
>>   UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.
>>
>>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 85
>> +++++++++++++++++++++++++++++++++++++++--------------------------------------
>> --------
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 81
>> ++++++++++++++++++++++++++++++++-------------------------------------------------
>>  UefiCpuPkg/Library/MpInitLib/MpLib.h     | 31
>> ++++++++++---------------------
>>  3 files changed, 81 insertions(+), 116 deletions(-)
>>
>> --
>> 2.39.1.windows.1
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112325): https://edk2.groups.io/g/devel/message/112325
Mute This Topic: https://groups.io/mt/102775770/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-