From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web11.8615.1581419174050713104 for ; Tue, 11 Feb 2020 03:06:14 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Feb 2020 03:06:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,428,1574150400"; d="scan'208";a="226473541" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 11 Feb 2020 03:06:12 -0800 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 11 Feb 2020 03:06:12 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 11 Feb 2020 03:06:12 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.196]) with mapi id 14.03.0439.000; Tue, 11 Feb 2020 19:06:10 +0800 From: "Ni, Ray" To: "Wu, Hao A" , "Fu, Siyuan" , "Dong, Eric" , "devel@edk2.groups.io" CC: Laszlo Ersek , "Kinney, Michael D" Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect() Thread-Topic: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUID & PlatformID in MicrocodeDetect() Thread-Index: AQHV2inAYmIvLAo0t02HIyaVjjuzyagKiKAAgAJZfACACP8fUA== Date: Tue, 11 Feb 2020 11:06:09 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C42D629@SHSMSX104.ccr.corp.intel.com> References: <20200203003438.6724-1-hao.a.wu@intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hao, I submitted a new Bugzilla to capture some potential enhancement to this p= atch. https://bugzilla.tianocore.org/show_bug.cgi?id=3D2519 Your patch can fix the issue, but I think we need some cleanup to avoid po= tential bugs in future. Thanks, Ray > -----Original Message----- > From: Wu, Hao A > Sent: Thursday, February 6, 2020 9:40 AM > To: Fu, Siyuan ; Dong, Eric ; = devel@edk2.groups.io > Cc: Ni, Ray ; Laszlo Ersek ; Kinney= , Michael D > Subject: RE: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CP= UID & PlatformID in MicrocodeDetect() >=20 > Thanks Siyuan and Eric, >=20 > The patch has been pushed via commit a9e3458ba7. >=20 > Best Regards, > Hao Wu >=20 >=20 > > -----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 > > > > -----Original Message----- > > From: 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 ; Dong, Eric ; > > Ni, Ray ; Laszlo Ersek ; Fu, Siyu= an > > ; Kinney, Michael D > > Subject: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Always get CPUI= D & > > PlatformID in MicrocodeDetect() > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2498 > > > > Commit fd30b00707 updated the logic in function MicrocodeDetect() that > > will directly use the CPUID and PlatformID information from the 'CpuDa= ta' > > field in the CPU_MP_DATA structure, instead of collecting these inform= ation > > for each processor via AsmCpuid() and AsmReadMsr64() calls respectivel= y. > > > > 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 th= e > > APs are waken up again, the function InitializeApData() will not be > > called, which means the CPUID and PlatformID information will not b= e > > collected. However, the below logics in MicrocodeDetect() function: > > > > CurrentRevision =3D GetCurrentMicrocodeSignature (); > > IsBspCallIn =3D (ProcessorNumber =3D=3D (UINTN)CpuMpData->BspNum= ber) ? > > TRUE : FALSE; > > if (CurrentRevision !=3D 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 logi= c, > > 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 microco= de > > detection process. > > > > This commit will update the logic in MicrocodeDetect() back to always > > collecting the CPUID and PlatformID information explicitly. > > > > Cc: Eric Dong > > Cc: Ray Ni > > Cc: Laszlo Ersek > > Cc: Siyuan Fu > > Cc: Michael D Kinney > > Signed-off-by: Hao A Wu > > --- > > 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 =3D 0; > > - Eax.Uint32 =3D CpuMpData- > > >CpuData[ProcessorNumber].ProcessorSignature; > > - PlatformId =3D CpuMpData->CpuData[ProcessorNumber].PlatformId; > > + // > > + // Here data of CPUID leafs have not been collected into context > > + buffer, so // GetProcessorCpuid() cannot be used here to retrieve C= PUID > > 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 =3D AsmReadMsr64 (MSR_IA32_PLATFORM_ID); > > + PlatformId =3D (UINT8) PlatformIdMsr.Bits.PlatformId; > > + > > > > // > > // Check whether AP has same processor with BSP. > > -- > > 2.12.0.windows.1 > > > > > >=20