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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.