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

["Ni, Ray" <ray.ni@intel.com>]

Hao,
I submitted a new Bugzilla to capture some potential enhancement to this patch.
https://bugzilla.tianocore.org/show_bug.cgi?id=2519

Your patch can fix the issue, but I think we need some cleanup to avoid potential bugs in future.

Thanks,
Ray

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, February 6, 2020 9:40 AM
> To: Fu, Siyuan <siyuan.fu@intel.com>; Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect()
> 
> Thanks Siyuan and Eric,
> 
> The patch has been pushed via commit a9e3458ba7.
> 
> Best Regards,
> Hao Wu
> 
> 
> > -----Original Message-----
> > From: Dong, Eric
> > Sent: Tuesday, February 04, 2020 9:47 PM
> > To: devel@edk2.groups.io; Wu, Hao A
> > Cc: Ni, Ray; Laszlo Ersek; Fu, Siyuan; Kinney, Michael D
> > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get
> > CPUID & PlatformID in MicrocodeDetect()
> >
> > Reviewed-by: Eric Dong <eric.dong@intel.com>
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao
> > A
> > Sent: Monday, February 3, 2020 8:35 AM
> > To: devel@edk2.groups.io
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Fu, Siyuan
> > <siyuan.fu@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID &
> > PlatformID in MicrocodeDetect()
> >
> > 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(-)
> >
> > 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.
> > --
> > 2.12.0.windows.1
> >
> >
> >