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

[edk2-devel] [Patch V3 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.
Compare with V2, V3 also Extract Dump Microcode Revision as function

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 | 91 +++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 78 +++++++++++++++++++++++++++++-------------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h     | 31 ++++++++++---------------------
 3 files changed, 84 insertions(+), 116 deletions(-)

--
2.39.1.windows.1



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

[edk2-devel] [Patch V3 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 MTRR table to the AP is always essential.
During the DXE phase, it cannot be omitted like loading microcode,
as the PEI and DXE may be in different bit modes.

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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 9a6ec5db5c..c26a17e1db 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -451,12 +451,19 @@ 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 MTRR table to the AP is always essential.
+  // During the DXE phase, it cannot be omitted like loading microcode,
+  // as the PEI and DXE may be in different bit modes.
   //
   MtrrSetAllMtrrs (&CpuMpData->MtrrTable);
 }
@@ -2224,29 +2231,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 +2261,11 @@ MpInitLibInitialize (
       CpuMpData->InitFlag = ApInitReconfig;
     }
 
+    //
+    // Wake up the AP to perform some AP initialization synchronization.
+    // 1. For PEI stage, load microcode and synchronize MTRR,
+    // 2. For the DXE phase, only synchronize MTRR.
+    //
     WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     //
     // Wait for all APs finished initialization
-- 
2.39.1.windows.1



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

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

[Yuanhao Xie]

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

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 c26a17e1db..538095d3bb 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2243,15 +2243,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 (#111596): https://edk2.groups.io/g/devel/message/111596
Mute This Topic: https://groups.io/mt/102744600/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V3 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 | 91 +++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     | 31 ++-----------------------------
 UefiCpuPkg/Library/MpInitLib/MpLib.h     | 31 ++++++++++---------------------
 3 files changed, 57 insertions(+), 96 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 11720560af..c0ca85543a 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -322,65 +322,64 @@ OnExit:
 }
 
 /**
-  Shadow the required microcode patches data into memory.
+    Dump the microcode revision for each core.
 
-  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
-**/
+    @param[in] CpuMpData          Pointer to CPU MP Data
+  **/
 VOID
-ShadowMicrocodeUpdatePatch (
-  IN OUT CPU_MP_DATA  *CpuMpData
+DumpMicrocodeRevision (
+  IN CPU_MP_DATA  *CpuMpData
   )
 {
-  EFI_STATUS  Status;
+  UINT32           ThreadId;
+  UINT32           ExpectedMicrocodeRevision;
+  CPU_INFO_IN_HOB  *CpuInfoInHob;
+  UINTN            Index;
 
-  Status = PlatformShadowMicrocode (CpuMpData);
-  if (EFI_ERROR (Status)) {
-    ShadowMicrocodePatchByPcd (CpuMpData);
+  //
+  // Dump the microcode revision for each core.
+  //
+  DEBUG_CODE_BEGIN ();
+  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
+        ));
+    }
   }
+
+  DEBUG_CODE_END ();
 }
 
 /**
-  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.
+  Shadow the required microcode patches data into memory.
 
+  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
 **/
-BOOLEAN
-GetMicrocodePatchInfoFromHob (
-  UINT64  *Address,
-  UINT64  *RegionSize
+VOID
+ShadowMicrocodeUpdatePatch (
+  IN OUT CPU_MP_DATA  *CpuMpData
   )
 {
-  EFI_HOB_GUID_TYPE          *GuidHob;
-  EDKII_MICROCODE_PATCH_HOB  *MicrocodePathHob;
+  EFI_STATUS  Status;
 
-  GuidHob = GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid);
-  if (GuidHob == NULL) {
-    DEBUG ((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n", __func__));
-    return FALSE;
+  Status = PlatformShadowMicrocode (CpuMpData);
+  if (EFI_ERROR (Status)) {
+    ShadowMicrocodePatchByPcd (CpuMpData);
   }
-
-  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 538095d3bb..2fd96bf0bc 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2282,37 +2282,10 @@ 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) {
+    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 763db4963d..fd302e6845 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -735,34 +735,23 @@ MicrocodeDetect (
   );
 
 /**
-  Shadow the required microcode patches data into memory.
+    Dump the microcode revision for each core.
 
-  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
-**/
+    @param[in] CpuMpData          Pointer to CPU MP Data
+  **/
 VOID
-ShadowMicrocodeUpdatePatch (
-  IN OUT CPU_MP_DATA  *CpuMpData
+DumpMicrocodeRevision (
+  IN 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.
+  Shadow the required microcode patches data into memory.
 
+  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
 **/
-BOOLEAN
-GetMicrocodePatchInfoFromHob (
-  UINT64  *Address,
-  UINT64  *RegionSize
+VOID
+ShadowMicrocodeUpdatePatch (
+  IN OUT CPU_MP_DATA  *CpuMpData
   );
 
 /**
-- 
2.39.1.windows.1



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

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

[Ni, Ray]

> -  MicrocodeDetect (CpuMpData, ProcessorNumber);
> -  //
> -  // Sync BSP's MTRR table to AP
> +  // Synchronizing the MTRR table to the AP is always essential.
> +  // During the DXE phase, it cannot be omitted like loading microcode,
> +  // as the PEI and DXE may be in different bit modes.


"as the PEI and DXE may be in different bit modes" is not the reason that
MTRR sync needs to be performed in DXE phase.

The reason is some code might change MTRR on BSP only after CpuMp PPI
service is not available and before CpuMp Protocol is available.
That is in early DXE phase before CpuMp protocol is installed.

>    //
>    MtrrSetAllMtrrs (&CpuMpData->MtrrTable);
>  }


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111634): https://edk2.groups.io/g/devel/message/111634
Mute This Topic: https://groups.io/mt/102744598/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 V3 1/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

[Ni, Ray]

> 
> -  if (!GetMicrocodePatchInfoFromHob (
> -         &CpuMpData->MicrocodePatchAddress,
> -         &CpuMpData->MicrocodePatchRegionSize
> -         ))
> -  {

Can you delete the GetMicrocodePathInfoFromHob() in this patch instead of the 3rd patch?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111635): https://edk2.groups.io/g/devel/message/111635
Mute This Topic: https://groups.io/mt/102744598/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 V3 2/3] UefiCpuPkg/MpInitLib: Store MTRRs settings only when CpuCount>1

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Wednesday, November 22, 2023 2:09 PM
> To: devel@edk2.groups.io
> Cc: Xie, Yuanhao <yuanhao.xie@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> Laszlo Ersek <lersek@redhat.com>
> Subject: [Patch V3 2/3] UefiCpuPkg/MpInitLib: Store MTRRs settings only
> when CpuCount>1
> 
> Store BSP's MTRR setting only when CpuCount>1.
> 
> 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 c26a17e1db..538095d3bb 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2243,15 +2243,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 (#111637): https://edk2.groups.io/g/devel/message/111637
Mute This Topic: https://groups.io/mt/102744600/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 V3 3/3] UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.

[Ni, Ray]

>  VOID
> -ShadowMicrocodeUpdatePatch (
> -  IN OUT CPU_MP_DATA  *CpuMpData
> +DumpMicrocodeRevision (
> +  IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  EFI_STATUS  Status;
> +  UINT32           ThreadId;
> +  UINT32           ExpectedMicrocodeRevision;
> +  CPU_INFO_IN_HOB  *CpuInfoInHob;
> +  UINTN            Index;


When DEBUG_CODE is disabled, above local variable declarations will
cause build failure as the compiler only sees the local variable declarations
but cannot see any code that references them.

So, can you remove DEBUG_CODE_BEGIN/END() from DumpMicrocodeRevision()?

Instead, you call it using following way:
DEBUG_CODE (
  DumpMicrocodeRevision ();
  );


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111638): https://edk2.groups.io/g/devel/message/111638
Mute This Topic: https://groups.io/mt/102744601/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 V3 3/3] UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.

[Yuanhao Xie]

Hi Ray,

In v4, DEBUG_CODE_BEGIN/END() from DumpMicrocodeRevision() is removed, and calling is updated as:
DEBUG_CODE (
  DumpMicrocodeRevision ();
  );

Thanks a lot for the feedback!

Regards,
Yunahao
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, November 23, 2023 9:24 AM
To: Xie, Yuanhao <yuanhao.xie@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [Patch V3 3/3] UefiCpuPkg/MpInitLib: Extract Dump Microcode Revision as function.

>  VOID
> -ShadowMicrocodeUpdatePatch (
> -  IN OUT CPU_MP_DATA  *CpuMpData
> +DumpMicrocodeRevision (
> +  IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  EFI_STATUS  Status;
> +  UINT32           ThreadId;
> +  UINT32           ExpectedMicrocodeRevision;
> +  CPU_INFO_IN_HOB  *CpuInfoInHob;
> +  UINTN            Index;


When DEBUG_CODE is disabled, above local variable declarations will cause build failure as the compiler only sees the local variable declarations but cannot see any code that references them.

So, can you remove DEBUG_CODE_BEGIN/END() from DumpMicrocodeRevision()?

Instead, you call it using following way:
DEBUG_CODE (
  DumpMicrocodeRevision ();
  );


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

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

[Yuanhao Xie]

Hi Ray,

In V4, the GetMicrocodePathInfoFromHob() removal is in patch 1 instead of the patch 3.
Comments of MTRRs syncing is a updated.

Thanks
Yuanhao
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, November 23, 2023 9:18 AM
To: Xie, Yuanhao <yuanhao.xie@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [Patch V3 1/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.


> -  MicrocodeDetect (CpuMpData, ProcessorNumber);
> -  //
> -  // Sync BSP's MTRR table to AP
> +  // Synchronizing the MTRR table to the AP is always essential.
> +  // During the DXE phase, it cannot be omitted like loading 
> + microcode,  // as the PEI and DXE may be in different bit modes.


"as the PEI and DXE may be in different bit modes" is not the reason that MTRR sync needs to be performed in DXE phase.

The reason is some code might change MTRR on BSP only after CpuMp PPI service is not available and before CpuMp Protocol is available.
That is in early DXE phase before CpuMp protocol is installed.

>    //
>    MtrrSetAllMtrrs (&CpuMpData->MtrrTable);  }

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Thursday, November 23, 2023 9:19 AM
To: Xie, Yuanhao <yuanhao.xie@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Laszlo Ersek <lersek@redhat.com>
Subject: RE: [Patch V3 1/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

> 
> -  if (!GetMicrocodePatchInfoFromHob (
> -         &CpuMpData->MicrocodePatchAddress,
> -         &CpuMpData->MicrocodePatchRegionSize
> -         ))
> -  {

Can you delete the GetMicrocodePathInfoFromHob() in this patch instead of the 3rd patch?


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