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 <ler...@redhat.com>
Sent: Thursday, November 23, 2023 1:25 AM
To: devel@edk2.groups.io; Xie, Yuanhao <yuanhao....@intel.com>
Cc: Ni, Ray <ray...@intel.com>; Dong, Eric <eric.d...@intel.com>; Kumar, Rahul
R <rahul.r.ku...@intel.com>; Tom Lendacky <thomas.lenda...@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...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Yuanhao Xie <yuanhao....@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
