[edk2-devel] [Patch V2 0/3] Eliminate redundant microcode loading in DXE.

[edk2-devel] [Patch V2 0/3] Eliminate redundant microcode loading in DXE.

[Yuanhao Xie]

The DXE stage's Microcode loading process has been elimincated.
Compare to V1, V2 seperates the patches and adds more comments

xieyuanh (3):
  UefiCpuPkg/MpInitLib:Repeat WakeUpAp of loading microcode,Syncing
    MTRR.
  UefiCpuPkg/MpInitLib: Detect microcode and store MTRR when CpuCount >
    1
  UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

 UefiCpuPkg/Library/MpInitLib/MpLib.c | 66 ++++++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 24 deletions(-)

--
2.39.1.windows.1



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

[edk2-devel] [Patch V2 1/3] UefiCpuPkg/MpInitLib:Repeat WakeUpAp of loading microcode,Syncing MTRR.

[Yuanhao Xie]

This patch replicates the WakeUpAp operation, encompassing both
microcode loading and MTRR synchronization, regardless of whether
MpHandOff is NULL.

The purpose of this patch is to enhance the review process.
The separation in this patch is aimed at facilitating a more
straightforward review, with the ultimate goal of eliminating the
microcode loading functionality for the second time Mp initialization.

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 9a6ec5db5c..e8ffb99874 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2256,9 +2256,11 @@ MpInitLibInitialize (
       // in DXE.
       //
       CpuMpData->InitFlag = ApInitReconfig;
+      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
+    } else {
+      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     }
 
-    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 (#111522): https://edk2.groups.io/g/devel/message/111522
Mute This Topic: https://groups.io/mt/102724546/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [Patch V2 2/3] UefiCpuPkg/MpInitLib: Detect microcode and store MTRR when CpuCount > 1

[Yuanhao Xie]

Detect and apply Microcode on BSP, store BSP's MTRR setting only when
CpuCount > 1.

The purpose of this patch is to enhance the review process.
The separation in this patch is aimed at facilitating a more
straightforward review, with the ultimate goal of eliminating the
microcode loading functionality for the second time Mp initialization

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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index e8ffb99874..bb5d4188f0 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2236,19 +2236,19 @@ MpInitLibInitialize (
     ShadowMicrocodeUpdatePatch (CpuMpData);
   }
 
-  //
-  // 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)
   //
   if (CpuMpData->CpuCount > 1) {
+    //
+    // Detect and apply Microcode on BSP
+    //
+    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
+    //
+    // 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 (#111523): https://edk2.groups.io/g/devel/message/111523
Mute This Topic: https://groups.io/mt/102724547/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Yuanhao Xie]

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

1. Microcode HOB consumption in MP initialization within the DXE phase.
2. Restricting MicrocodeDetect to the PEI phase instead of DXE for BSP.
3. BSP now WakeUpAp only for synchronizing MTRR settings, with
Microcode loading no longer a part of this process.

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 | 54 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index bb5d4188f0..0cf3520f9e 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -434,7 +434,27 @@ ApFuncEnableX2Apic (
 }
 
 /**
-  Do sync on APs.
+  Sync BSP's MTRR table to AP during waking up APs.
+  @param[in, out] Buffer  Pointer to private data buffer.
+**/
+VOID
+EFIAPI
+ApMtrrSync (
+  IN OUT VOID  *Buffer
+  )
+{
+  CPU_MP_DATA  *CpuMpData;
+
+  CpuMpData = (CPU_MP_DATA *)Buffer;
+
+  //
+  // Sync BSP's MTRR table to AP
+  //
+  MtrrSetAllMtrrs (&CpuMpData->MtrrTable);
+}
+
+/**
+  Do sync on APs, includes loading microcode, and sync MTRRs.
 
   @param[in, out] Buffer  Pointer to private data buffer.
 **/
@@ -2224,26 +2244,10 @@ MpInitLibInitialize (
     }
   }
 
-  if (!GetMicrocodePatchInfoFromHob (
-         &CpuMpData->MicrocodePatchAddress,
-         &CpuMpData->MicrocodePatchRegionSize
-         ))
-  {
-    //
-    // The microcode patch information cache HOB does not exist, which means
-    // the microcode patches data has not been loaded into memory yet
-    //
-    ShadowMicrocodeUpdatePatch (CpuMpData);
-  }
-
   //
-  // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
+  // Wakeup APs to do some AP initialize sync.
   //
   if (CpuMpData->CpuCount > 1) {
-    //
-    // Detect and apply Microcode on BSP
-    //
-    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
     //
     // Store BSP's MTRR setting
     //
@@ -2256,8 +2260,20 @@ MpInitLibInitialize (
       // in DXE.
       //
       CpuMpData->InitFlag = ApInitReconfig;
-      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
+      //
+      //  Wakeup APs to sync MTRR settings.
+      //  For the case the PEI and DXE are in different bit mode.
+      //  It is necessary to do the MTRRs syncing.
+      //
+      WakeUpAP (CpuMpData, TRUE, 0, ApMtrrSync, CpuMpData, TRUE);
     } else {
+      //
+      // Detect and apply Microcode on BSP
+      //
+      MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
+      //
+      // Wakeup APs to do some AP initialize sync load microcode and Sync MTRRs
+      //
       WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
     }
 
-- 
2.39.1.windows.1



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

Re: [edk2-devel] [Patch V2 1/3] UefiCpuPkg/MpInitLib:Repeat WakeUpAp of loading microcode,Syncing MTRR.

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Xie, Yuanhao <yuanhao.xie@intel.com>
> Sent: Tuesday, November 21, 2023 3:40 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 V2 1/3] UefiCpuPkg/MpInitLib:Repeat WakeUpAp of loading
> microcode,Syncing MTRR.
> 
> This patch replicates the WakeUpAp operation, encompassing both
> microcode loading and MTRR synchronization, regardless of whether
> MpHandOff is NULL.
> 
> The purpose of this patch is to enhance the review process.
> The separation in this patch is aimed at facilitating a more
> straightforward review, with the ultimate goal of eliminating the
> microcode loading functionality for the second time Mp initialization.
> 
> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 9a6ec5db5c..e8ffb99874 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2256,9 +2256,11 @@ MpInitLibInitialize (
>        // in DXE.
>        //
>        CpuMpData->InitFlag = ApInitReconfig;
> +      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData,
> TRUE);
> +    } else {
> +      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData,
> TRUE);
>      }
> 
> -    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 (#111531): https://edk2.groups.io/g/devel/message/111531
Mute This Topic: https://groups.io/mt/102724546/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 V2 1/3] UefiCpuPkg/MpInitLib:Repeat WakeUpAp of loading microcode,Syncing MTRR.

[Laszlo Ersek]

On 11/21/23 08:39, Yuanhao Xie wrote:
> This patch replicates the WakeUpAp operation, encompassing both
> microcode loading and MTRR synchronization, regardless of whether
> MpHandOff is NULL.
> 
> The purpose of this patch is to enhance the review process.
> The separation in this patch is aimed at facilitating a more
> straightforward review, with the ultimate goal of eliminating the
> microcode loading functionality for the second time Mp initialization.
> 
> 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 9a6ec5db5c..e8ffb99874 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2256,9 +2256,11 @@ MpInitLibInitialize (
>        // in DXE.
>        //
>        CpuMpData->InitFlag = ApInitReconfig;
> +      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> +    } else {
> +      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
>      }
>  
> -    WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
>      //
>      // Wait for all APs finished initialization
>      //

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111617): https://edk2.groups.io/g/devel/message/111617
Mute This Topic: https://groups.io/mt/102724546/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 V2 2/3] UefiCpuPkg/MpInitLib: Detect microcode and store MTRR when CpuCount > 1

[Laszlo Ersek]

On 11/21/23 08:39, Yuanhao Xie wrote:
> Detect and apply Microcode on BSP, store BSP's MTRR setting only when
> CpuCount > 1.
> 
> The purpose of this patch is to enhance the review process.
> The separation in this patch is aimed at facilitating a more
> straightforward review, with the ultimate goal of eliminating the
> microcode loading functionality for the second time Mp initialization
> 
> 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 | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e8ffb99874..bb5d4188f0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2236,19 +2236,19 @@ MpInitLibInitialize (
>      ShadowMicrocodeUpdatePatch (CpuMpData);
>    }
>  
> -  //
> -  // 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)
>    //
>    if (CpuMpData->CpuCount > 1) {
> +    //
> +    // Detect and apply Microcode on BSP
> +    //
> +    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> +    //
> +    // 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

I can't very well gauge the effect that this patch has on
MicrocodeDetect(). MicrocodeDetect() is a function that is not obvious
to me. However, microcode detection is disabled on OVMF, both via HOB
and via PCD (zero region size). So, I have nothing against *restricting*
microcode detection "further".

Regarding MtrrTable: it seems that this field is only consumed in
ApMtrrSync() and ApInitializeSync(). I suppose those functions never run
when CpuMpData->CpuCount is 1. Thus restricting MtrrGetAllMtrrs() should
be fine as well.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111618): https://edk2.groups.io/g/devel/message/111618
Mute This Topic: https://groups.io/mt/102724547/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 V2 2/3] UefiCpuPkg/MpInitLib: Detect microcode and store MTRR when CpuCount > 1

[Laszlo Ersek]

On 11/22/23 18:03, Laszlo Ersek wrote:
> On 11/21/23 08:39, Yuanhao Xie wrote:
>> Detect and apply Microcode on BSP, store BSP's MTRR setting only when
>> CpuCount > 1.
>>
>> The purpose of this patch is to enhance the review process.
>> The separation in this patch is aimed at facilitating a more
>> straightforward review, with the ultimate goal of eliminating the
>> microcode loading functionality for the second time Mp initialization
>>
>> 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 | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> index e8ffb99874..bb5d4188f0 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> @@ -2236,19 +2236,19 @@ MpInitLibInitialize (
>>      ShadowMicrocodeUpdatePatch (CpuMpData);
>>    }
>>  
>> -  //
>> -  // 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)
>>    //
>>    if (CpuMpData->CpuCount > 1) {
>> +    //
>> +    // Detect and apply Microcode on BSP
>> +    //
>> +    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>> +    //
>> +    // 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
> 
> I can't very well gauge the effect that this patch has on
> MicrocodeDetect(). MicrocodeDetect() is a function that is not obvious
> to me. However, microcode detection is disabled on OVMF, both via HOB
> and via PCD (zero region size). So, I have nothing against *restricting*
> microcode detection "further".
> 
> Regarding MtrrTable: it seems that this field is only consumed in
> ApMtrrSync() and ApInitializeSync(). I suppose those functions never run
> when CpuMpData->CpuCount is 1. Thus restricting MtrrGetAllMtrrs() should
> be fine as well.

sorry, small mistake: when writing this, I was already looking at the
final state of the tree. Right after this patch is applied, only
ApInitializeSync() exists!

But that fact only makes my argument stronger -- so my R-b for this
patch stands.

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111619): https://edk2.groups.io/g/devel/message/111619
Mute This Topic: https://groups.io/mt/102724547/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 V2 2/3] UefiCpuPkg/MpInitLib: Detect microcode and store MTRR when CpuCount > 1

[Laszlo Ersek]

On 11/22/23 18:06, Laszlo Ersek wrote:
> On 11/22/23 18:03, Laszlo Ersek wrote:
>> On 11/21/23 08:39, Yuanhao Xie wrote:
>>> Detect and apply Microcode on BSP, store BSP's MTRR setting only when
>>> CpuCount > 1.
>>>
>>> The purpose of this patch is to enhance the review process.
>>> The separation in this patch is aimed at facilitating a more
>>> straightforward review, with the ultimate goal of eliminating the
>>> microcode loading functionality for the second time Mp initialization
>>>
>>> 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 | 18 +++++++++---------
>>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index e8ffb99874..bb5d4188f0 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -2236,19 +2236,19 @@ MpInitLibInitialize (
>>>      ShadowMicrocodeUpdatePatch (CpuMpData);
>>>    }
>>>  
>>> -  //
>>> -  // 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)
>>>    //
>>>    if (CpuMpData->CpuCount > 1) {
>>> +    //
>>> +    // Detect and apply Microcode on BSP
>>> +    //
>>> +    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>>> +    //
>>> +    // 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
>>
>> I can't very well gauge the effect that this patch has on
>> MicrocodeDetect(). MicrocodeDetect() is a function that is not obvious
>> to me. However, microcode detection is disabled on OVMF, both via HOB
>> and via PCD (zero region size). So, I have nothing against *restricting*
>> microcode detection "further".
>>
>> Regarding MtrrTable: it seems that this field is only consumed in
>> ApMtrrSync() and ApInitializeSync(). I suppose those functions never run
>> when CpuMpData->CpuCount is 1.

[*]

> Thus restricting MtrrGetAllMtrrs() should
>> be fine as well.
> 
> sorry, small mistake: when writing this, I was already looking at the
> final state of the tree. Right after this patch is applied, only
> ApInitializeSync() exists!
> 
> But that fact only makes my argument stronger -- so my R-b for this
> patch stands.

My assumption at [*] is correct, in fact; ApInitializeSync() is only
called from the context updated in patch#1! And after patch#3, which
introduces ApMtrrSync(), the same applies to ApMtrrSync() too.

So all is fine.

Thanks
Laszlo

> 
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
> 
> Thanks
> Laszlo



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

[Laszlo Ersek]

On 11/21/23 08:39, Yuanhao Xie wrote:
> The DXE stage's Microcode loading process has been elimincated by:
> 
> 1. Microcode HOB consumption in MP initialization within the DXE phase.
> 2. Restricting MicrocodeDetect to the PEI phase instead of DXE for BSP.
> 3. BSP now WakeUpAp only for synchronizing MTRR settings, with
> Microcode loading no longer a part of this process.
> 
> 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 | 54 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index bb5d4188f0..0cf3520f9e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -434,7 +434,27 @@ ApFuncEnableX2Apic (
>  }
>  
>  /**
> -  Do sync on APs.
> +  Sync BSP's MTRR table to AP during waking up APs.
> +  @param[in, out] Buffer  Pointer to private data buffer.
> +**/
> +VOID
> +EFIAPI
> +ApMtrrSync (
> +  IN OUT VOID  *Buffer
> +  )
> +{
> +  CPU_MP_DATA  *CpuMpData;
> +
> +  CpuMpData = (CPU_MP_DATA *)Buffer;
> +
> +  //
> +  // Sync BSP's MTRR table to AP
> +  //
> +  MtrrSetAllMtrrs (&CpuMpData->MtrrTable);
> +}
> +
> +/**
> +  Do sync on APs, includes loading microcode, and sync MTRRs.
>  
>    @param[in, out] Buffer  Pointer to private data buffer.
>  **/
> @@ -2224,26 +2244,10 @@ MpInitLibInitialize (
>      }
>    }
>  
> -  if (!GetMicrocodePatchInfoFromHob (
> -         &CpuMpData->MicrocodePatchAddress,
> -         &CpuMpData->MicrocodePatchRegionSize
> -         ))
> -  {
> -    //
> -    // The microcode patch information cache HOB does not exist, which means
> -    // the microcode patches data has not been loaded into memory yet
> -    //
> -    ShadowMicrocodeUpdatePatch (CpuMpData);
> -  }
> -

(1) This patch seems great, functionally speaking, but I *think* the
above part (the elimination of the GetMicrocodePatchInfoFromHob() call,
and the dependent ShadowMicrocodeUpdatePatch() call) should be split to
a separate patch -- perhaps before, perhaps after, the rest of this
patch, in the series.

That's because the GetMicrocodePatchInfoFromHob() and
ShadowMicrocodeUpdatePatch() calls are removed *altogether* from the
code in this patch; they are not *moved* to any of the branches below.
Which makes me think that *even without* restricting the
MicrocodeDetect() call below (and calling ApMtrrSync() on the other
branch below), the HOB stuff is just generally superfluous.

- If that's the case, then please split this part out to a new patch in
v3. (Please also don't forget to update the commit message here!) With
that, you can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

to what *remains* from this patch. (I.e. everything except the
GetMicrocodePatchInfoFromHob/ShadowMicrocodeUpdatePatch removal.)

- If I'm wrong, and the GetMicrocodePatchInfoFromHob /
ShadowMicrocodeUpdatePatch removal is an integral part of this patch (if
it cannot be separated, semantically), then I don't understand something
about this patch. But in that case I'd like you to explain the reason
for removing these function calls right in this patch. I might reply
with an R-b to that explanation. (Although I'd probably request that the
explanation be included in the commit message, in v3.)

Thanks!
Laszlo

>    //
> -  // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> +  // Wakeup APs to do some AP initialize sync.
>    //
>    if (CpuMpData->CpuCount > 1) {
> -    //
> -    // Detect and apply Microcode on BSP
> -    //
> -    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>      //
>      // Store BSP's MTRR setting
>      //
> @@ -2256,8 +2260,20 @@ MpInitLibInitialize (
>        // in DXE.
>        //
>        CpuMpData->InitFlag = ApInitReconfig;
> -      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> +      //
> +      //  Wakeup APs to sync MTRR settings.
> +      //  For the case the PEI and DXE are in different bit mode.
> +      //  It is necessary to do the MTRRs syncing.
> +      //
> +      WakeUpAP (CpuMpData, TRUE, 0, ApMtrrSync, CpuMpData, TRUE);
>      } else {
> +      //
> +      // Detect and apply Microcode on BSP
> +      //
> +      MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> +      //
> +      // Wakeup APs to do some AP initialize sync load microcode and Sync MTRRs
> +      //
>        WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
>      }
>  



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

[Yuanhao Xie]

Hi Laszlo,

Thanks a lot for the feedback!

- (1) This patch seems great, functionally speaking, but I *think* the above part (the elimination of the GetMicrocodePatchInfoFromHob() call, and the dependent ShadowMicrocodeUpdatePatch() call) should be split to a separate patch -- perhaps before, perhaps after, the rest of this patch, in the series.

	ShadowMicrocodeUpdatePatch will not be eliminated anymore in V4, but will be done only in PEI phase.  GetMicrocodePatchInfoFromHob can be eliminated. Which I updated in V4 patch 1.

V4  also reconstruct the patch separation:
	Patch 1: GetMicrocodePatchInfoFromHob was removed, this patch no longer use GetMicrocodePatchInfoFromHob but use MpHandOff == NULL to decide is it should detect ShadowMicrocodeUpdatePatch. Instead of repeating WakeUpAp, check CpuMpData->InitFlag within ApInitializeSync() to determine if it is in the DXE stage to decide if it is necessary to eliminate APs' microcode loading.
	Patch 2: BSP stores MTRR settings only when CpuCount >1
	Patch 3: Since only the PEI phase needs to load microcode, the debug log of the microcode is no longer required in the DXE phase.

Regards
Yuanhao

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, November 23, 2023 1:25 AM
To: devel@edk2.groups.io; Xie, Yuanhao <yuanhao.xie@intel.com>
Cc: 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>
Subject: Re: [edk2-devel] [Patch V2 3/3] UefiCpuPkg/MpInitLib: Eliminate redundant microcode loading in DXE.

On 11/21/23 08:39, Yuanhao Xie wrote:
> The DXE stage's Microcode loading process has been elimincated by:
> 
> 1. Microcode HOB consumption in MP initialization within the DXE phase.
> 2. Restricting MicrocodeDetect to the PEI phase instead of DXE for BSP.
> 3. BSP now WakeUpAp only for synchronizing MTRR settings, with 
> Microcode loading no longer a part of this process.
> 
> 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 | 54 
> +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index bb5d4188f0..0cf3520f9e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -434,7 +434,27 @@ ApFuncEnableX2Apic (  }
>  
>  /**
> -  Do sync on APs.
> +  Sync BSP's MTRR table to AP during waking up APs.
> +  @param[in, out] Buffer  Pointer to private data buffer.
> +**/
> +VOID
> +EFIAPI
> +ApMtrrSync (
> +  IN OUT VOID  *Buffer
> +  )
> +{
> +  CPU_MP_DATA  *CpuMpData;
> +
> +  CpuMpData = (CPU_MP_DATA *)Buffer;
> +
> +  //
> +  // Sync BSP's MTRR table to AP
> +  //
> +  MtrrSetAllMtrrs (&CpuMpData->MtrrTable); }
> +
> +/**
> +  Do sync on APs, includes loading microcode, and sync MTRRs.
>  
>    @param[in, out] Buffer  Pointer to private data buffer.
>  **/
> @@ -2224,26 +2244,10 @@ MpInitLibInitialize (
>      }
>    }
>  
> -  if (!GetMicrocodePatchInfoFromHob (
> -         &CpuMpData->MicrocodePatchAddress,
> -         &CpuMpData->MicrocodePatchRegionSize
> -         ))
> -  {
> -    //
> -    // The microcode patch information cache HOB does not exist, which means
> -    // the microcode patches data has not been loaded into memory yet
> -    //
> -    ShadowMicrocodeUpdatePatch (CpuMpData);
> -  }
> -

(1) This patch seems great, functionally speaking, but I *think* the above part (the elimination of the GetMicrocodePatchInfoFromHob() call, and the dependent ShadowMicrocodeUpdatePatch() call) should be split to a separate patch -- perhaps before, perhaps after, the rest of this patch, in the series.

That's because the GetMicrocodePatchInfoFromHob() and
ShadowMicrocodeUpdatePatch() calls are removed *altogether* from the code in this patch; they are not *moved* to any of the branches below.
Which makes me think that *even without* restricting the
MicrocodeDetect() call below (and calling ApMtrrSync() on the other branch below), the HOB stuff is just generally superfluous.

- If that's the case, then please split this part out to a new patch in v3. (Please also don't forget to update the commit message here!) With that, you can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

to what *remains* from this patch. (I.e. everything except the GetMicrocodePatchInfoFromHob/ShadowMicrocodeUpdatePatch removal.)

- If I'm wrong, and the GetMicrocodePatchInfoFromHob / ShadowMicrocodeUpdatePatch removal is an integral part of this patch (if it cannot be separated, semantically), then I don't understand something about this patch. But in that case I'd like you to explain the reason for removing these function calls right in this patch. I might reply with an R-b to that explanation. (Although I'd probably request that the explanation be included in the commit message, in v3.)

Thanks!
Laszlo

>    //
> -  // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> +  // Wakeup APs to do some AP initialize sync.
>    //
>    if (CpuMpData->CpuCount > 1) {
> -    //
> -    // Detect and apply Microcode on BSP
> -    //
> -    MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
>      //
>      // Store BSP's MTRR setting
>      //
> @@ -2256,8 +2260,20 @@ MpInitLibInitialize (
>        // in DXE.
>        //
>        CpuMpData->InitFlag = ApInitReconfig;
> -      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
> +      //
> +      //  Wakeup APs to sync MTRR settings.
> +      //  For the case the PEI and DXE are in different bit mode.
> +      //  It is necessary to do the MTRRs syncing.
> +      //
> +      WakeUpAP (CpuMpData, TRUE, 0, ApMtrrSync, CpuMpData, TRUE);
>      } else {
> +      //
> +      // Detect and apply Microcode on BSP
> +      //
> +      MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> +      //
> +      // Wakeup APs to do some AP initialize sync load microcode and Sync MTRRs
> +      //
>        WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
>      }
>  



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