Got it. Reviewed-by: Siyuan Fu <siyuan...@intel.com>
> -----Original Message----- > From: Wu, Hao A <hao.a...@intel.com> > Sent: 2020年2月3日 9:03 > To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Laszlo > Ersek <ler...@redhat.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get > CPUID & PlatformID in MicrocodeDetect() > > > -----Original Message----- > > From: Fu, Siyuan > > Sent: Monday, February 03, 2020 8:55 AM > > To: devel@edk2.groups.io; Wu, Hao A > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D > > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get > CPUID > > & PlatformID in MicrocodeDetect() > > > > Hi, Hao > > > > If CPUID and PlatformID will always be collected in MicrocodeDetect(), is > the > > change in 999463 redundant now? Should we remove that code? > > > I think the changes are still needed. > > The CPUID and PlatformID information stored in the 'CPU_AP_DATA' > structure are > needed during the pre-process of the microcode patch headers for deciding > whether the microcode patch data should be loaded into memory. > > Best Regards, > Hao Wu > > > > > > Best Regards > > Siyuan > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, > Hao > > > A > > > Sent: 2020年2月3日 8:35 > > > To: devel@edk2.groups.io > > > Cc: Wu, Hao A <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; > Ni, > > > Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Fu, Siyuan > > > <siyuan...@intel.com>; Kinney, Michael D <michael.d.kin...@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.d...@intel.com> > > > Cc: Ray Ni <ray...@intel.com> > > > Cc: Laszlo Ersek <ler...@redhat.com> > > > Cc: Siyuan Fu <siyuan...@intel.com> > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > Signed-off-by: Hao A Wu <hao.a...@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 > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53637): https://edk2.groups.io/g/devel/message/53637 Mute This Topic: https://groups.io/mt/70934372/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
