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 (#111621): https://edk2.groups.io/g/devel/message/111621
Mute This Topic: https://groups.io/mt/102724548/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
