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

["Laszlo Ersek" <lersek@redhat.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-