Re: [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()

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

Hello Hao,

On 02/03/20 01:34, Hao A Wu wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2498
> 
> Commit fd30b00707 updated the logic in function MicrocodeDetect() that
> will directly use the CPUID and PlatformID information from the 'CpuData'
> field in the CPU_MP_DATA structure, instead of collecting these
> information for each processor via AsmCpuid() and AsmReadMsr64() calls
> respectively.
> 
> At that moment, this approach worked fine for APs. Since:
> a) When the APs are waken up for the 1st time (1st MpInitLibInitialize()
>    entry at PEI phase), the function InitializeApData() will be called for
>    each AP and the CPUID and PlatformID information will be collected.
> 
> b) During the 2nd entry of MpInitLibInitialize() at DXE phase, when the
>    APs are waken up again, the function InitializeApData() will not be
>    called, which means the CPUID and PlatformID information will not be
>    collected. However, the below logics in MicrocodeDetect() function:
> 
>   CurrentRevision = GetCurrentMicrocodeSignature ();
>   IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ? TRUE : FALSE;
>   if (CurrentRevision != 0 && !IsBspCallIn) {
>     //
>     // Skip loading microcode if it has been loaded successfully
>     //
>     return;
>   }
> 
>    will ensure that the microcode detection and application will be
>    skipped due to the fact that such process has already been done in the
>    PEI phase.
> 
> But after commit 396e791059, which removes the above skip loading logic,
> the CPUID and PlatformID information on APs will be used upon the 2nd
> entry of the MpInitLibInitialize(). But since the CPUID and PlatformID
> information has not been collected, it will bring issue to the microcode
> detection process.
> 
> This commit will update the logic in MicrocodeDetect() back to always
> collecting the CPUID and PlatformID information explicitly.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

It seems like I haven't been involved in reviewing either commit
fd30b0070773 ("UefiCpuPkg/MpInitLib: Remove redundant microcode fields
in CPU_MP_DATA", 2020-01-02) or commit 396e791059f3 ("UefiCpuPkg: Always
load microcode patch on AP processor.", 2020-01-08), so I'd like to
defer on this patch to the other UefiCpuPkg reviewers.

Thanks!
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index b6675b9a60..67e214d463 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -93,6 +93,7 @@ MicrocodeDetect (
>    UINT32                                  InCompleteCheckSum32;
>    BOOLEAN                                 CorrectMicrocode;
>    VOID                                    *MicrocodeData;
> +  MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
>    UINT32                                  ThreadId;
>    BOOLEAN                                 IsBspCallIn;
>  
> @@ -115,8 +116,18 @@ MicrocodeDetect (
>    }
>  
>    ExtendedTableLength = 0;
> -  Eax.Uint32 = CpuMpData->CpuData[ProcessorNumber].ProcessorSignature;
> -  PlatformId = CpuMpData->CpuData[ProcessorNumber].PlatformId;
> +  //
> +  // Here data of CPUID leafs have not been collected into context buffer, so
> +  // GetProcessorCpuid() cannot be used here to retrieve CPUID data.
> +  //
> +  AsmCpuid (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +  //
> +  // The index of platform information resides in bits 50:52 of MSR IA32_PLATFORM_ID
> +  //
> +  PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> +  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> +
>  
>    //
>    // Check whether AP has same processor with BSP.
>