From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web12.4708.1581388290505535791 for ; Mon, 10 Feb 2020 18:31:30 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Feb 2020 18:31:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,427,1574150400"; d="scan'208";a="280813776" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 10 Feb 2020 18:31:29 -0800 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 10 Feb 2020 18:31:29 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.138]) with mapi id 14.03.0439.000; Tue, 11 Feb 2020 10:31:26 +0800 From: "Ni, Ray" To: "Wu, Hao A" , "devel@edk2.groups.io" CC: "Kubacki, Michael A" , "Kinney, Michael D" , "Dong, Eric" , "Laszlo Ersek" Subject: Re: [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA Thread-Topic: [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info between archs in CPU_MP_DATA Thread-Index: AQHV3K2q98wn9RT8GEyfkgu1lrA9mqgVTJtw Date: Tue, 11 Feb 2020 02:31:26 +0000 Deferred-Delivery: Tue, 11 Feb 2020 02:29:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C42CAAC@SHSMSX104.ccr.corp.intel.com> References: <20200206052356.3672-1-hao.a.wu@intel.com> <20200206052356.3672-3-hao.a.wu@intel.com> In-Reply-To: <20200206052356.3672-3-hao.a.wu@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 Reviewed-by: Ray Ni > -----Original Message----- > From: Wu, Hao A > Sent: Thursday, February 6, 2020 1:24 PM > To: devel@edk2.groups.io > Cc: Wu, Hao A ; Kubacki, Michael A > ; Kinney, Michael D > ; Dong, Eric ; Ni, Ray > ; Laszlo Ersek > Subject: [PATCH v1 2/2] UefiCpuPkg/MpInitLib: Not pass microcode info > between archs in CPU_MP_DATA >=20 > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2465 >=20 > Commit 89164babec: > UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice. >=20 > attempted to use 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' > fields to avoid loading the microcode patches data into memory again in > the DXE phase. >=20 > However, the CPU_MP_DATA structure has members with type 'UINTN' or > pointer before the microcode patch related fields. This may cause issues > when PEI and DXE are of different archs (e.g. PEI - IA32, DXE - x64), > since the microcode patch related fields will have different offsets in > the CPU_MP_DATA structure. >=20 > Commit 88bd066166: > UefiCpuPkg/MpInitLib: Relocate microcode patch fields in CPU_MP_DATA >=20 > tried to resolve the above-mentioned issue by relocating the fields > 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress' before members wit= h > different size between different archs. But it failed to take the case of > pre-built binaries (e.g. FSP) into consideration. >=20 > Binaries can be built when the code base had a different version of the > CPU_MP_DATA structure definition. This may cause issues when accessing > these microcode patch related fields, since their offsets are different > (between PEI phase in the binaries and DXE phase in current code > implementation). >=20 > This commit will use the newly introduced EDKII microcode patch HOB > instead for the DXE phase to get the information of the loaded microcode > patches data done in the PEI phase. And the 'MicrocodePatchRegionSize' an= d > 'MicrocodePatchAddress' fields in CPU_MP_DATA will not be used to pass > information between phases. >=20 > For pre-built binaries, they can be classified into 3 types with regard t= o > the time when they are being built: >=20 > A. Before commit 89164babec > (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress= ' > were not being used to skip microcode load in DXE) >=20 > For this case, the EDKII microcode patch HOB will not be produced. This > commit will load the microcode patches data again in DXE. Such behavior i= s > the same with the code base back then. >=20 > B. After commit 89164babec, before commit e1ed55738e > (In other words, 'MicrocodePatchRegionSize' and 'MicrocodePatchAddress= ' > being used to skip microcode load in DXE, but failed to work properly > between differnt archs.) >=20 > For this case, the EDKII microcode patch HOB will not be produced as well= . > This commit will also load the microcode patches data again in DXE. >=20 > But since commit 89164babec failed to keep the detection and application > of microcode patches working properly in DXE after skipping the load, we > fall back to the origin behavior (that is to load the microcode patches > data again in DXE). >=20 > C. After commit e1ed55738e > (In other words, EDKII microcode patch HOB will be produced.) >=20 > For this case, it will have the same behavior with the BIOS built from > the current source codes. >=20 > Cc: Michael Kubacki > Cc: Michael D Kinney > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Hao A Wu > --- > UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 3 +- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 23 +++++++++++ > UefiCpuPkg/Library/MpInitLib/Microcode.c | 43 ++++++++++++++++++++ > UefiCpuPkg/Library/MpInitLib/MpLib.c | 20 +++++---- > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 3 +- > 5 files changed, 80 insertions(+), 12 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > index bf5d18d521..9e6cce0895 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > @@ -1,7 +1,7 @@ > ## @file > # MP Initialize Library instance for DXE driver. > # > -# Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved. > +# Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved. > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > @@ -58,6 +58,7 @@ [Protocols] > [Guids] > gEfiEventExitBootServicesGuid ## CONSUMES ## Event > gEfiEventLegacyBootGuid ## SOMETIMES_CONSUMES #= # Event > + gEdkiiMicrocodePatchHobGuid ## SOMETIMES_CONSUMES #= # > HOB >=20 > [Pcd] > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## > CONSUMES > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h > b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index d7e20f0b74..a6eab5f3d7 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -29,6 +29,8 @@ > #include > #include >=20 > +#include > + > #include >=20 >=20 > @@ -600,6 +602,27 @@ ShadowMicrocodeUpdatePatch ( > ); >=20 > /** > + Get the cached microcode patch base address and size from the microcod= e > patch > + information cache HOB. > + > + @param[out] Address Base address of the microcode patches data. > + It will be updated if the microcode patch > + information cache HOB is found. > + @param[out] RegionSize Size of the microcode patches data. > + It will be updated if the microcode patch > + information cache HOB is found. > + > + @retval TRUE The microcode patch information cache HOB is found. > + @retval FALSE The microcode patch information cache HOB is not fou= nd. > + > +**/ > +BOOLEAN > +GetMicrocodePatchInfoFromHob ( > + UINT64 *Address, > + UINT64 *RegionSize > + ); > + > +/** > Detect whether Mwait-monitor feature is supported. >=20 > @retval TRUE Mwait-monitor feature is supported. > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c > b/UefiCpuPkg/Library/MpInitLib/Microcode.c > index 247f835b09..67e214d463 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c > @@ -739,3 +739,46 @@ ShadowMicrocodeUpdatePatch ( > ShadowMicrocodePatchByPcd (CpuMpData); > } > } > + > +/** > + Get the cached microcode patch base address and size from the microcod= e > patch > + information cache HOB. > + > + @param[out] Address Base address of the microcode patches data. > + It will be updated if the microcode patch > + information cache HOB is found. > + @param[out] RegionSize Size of the microcode patches data. > + It will be updated if the microcode patch > + information cache HOB is found. > + > + @retval TRUE The microcode patch information cache HOB is found. > + @retval FALSE The microcode patch information cache HOB is not fou= nd. > + > +**/ > +BOOLEAN > +GetMicrocodePatchInfoFromHob ( > + UINT64 *Address, > + UINT64 *RegionSize > + ) > +{ > + EFI_HOB_GUID_TYPE *GuidHob; > + EDKII_MICROCODE_PATCH_HOB *MicrocodePathHob; > + > + GuidHob =3D GetFirstGuidHob (&gEdkiiMicrocodePatchHobGuid); > + if (GuidHob =3D=3D NULL) { > + DEBUG((DEBUG_INFO, "%a: Microcode patch cache HOB is not found.\n", > __FUNCTION__)); > + return FALSE; > + } > + > + MicrocodePathHob =3D GET_GUID_HOB_DATA (GuidHob); > + > + *Address =3D MicrocodePathHob->MicrocodePatchAddress; > + *RegionSize =3D MicrocodePathHob->MicrocodePatchRegionSize; > + > + DEBUG(( > + DEBUG_INFO, "%a: MicrocodeBase =3D 0x%lx, MicrocodeSize =3D 0x%lx\n"= , > + __FUNCTION__, *Address, *RegionSize > + )); > + > + return TRUE; > +} > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 855d37ba3e..d0fbc17ce5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1682,10 +1682,6 @@ MpInitLibInitialize ( > CpuMpData->SwitchBspFlag =3D FALSE; > CpuMpData->CpuData =3D (CPU_AP_DATA *) (CpuMpData + 1); > CpuMpData->CpuInfoInHob =3D (UINT64) (UINTN) (CpuMpData->CpuData + > MaxLogicalProcessorNumber); > - if (OldCpuMpData !=3D NULL) { > - CpuMpData->MicrocodePatchRegionSize =3D OldCpuMpData- > >MicrocodePatchRegionSize; > - CpuMpData->MicrocodePatchAddress =3D OldCpuMpData- > >MicrocodePatchAddress; > - } > InitializeSpinLock(&CpuMpData->MpLock); >=20 > // > @@ -1740,11 +1736,6 @@ MpInitLibInitialize ( > // > CollectProcessorCount (CpuMpData); > } > - > - // > - // Load required microcode patches data into memory > - // > - ShadowMicrocodeUpdatePatch (CpuMpData); > } else { > // > // APs have been wakeup before, just get the CPU Information > @@ -1762,6 +1753,17 @@ MpInitLibInitialize ( > } > } >=20 > + if (!GetMicrocodePatchInfoFromHob ( > + &CpuMpData->MicrocodePatchAddress, > + &CpuMpData->MicrocodePatchRegionSize > + )) { > + // > + // The microcode patch information cache HOB does not exist, which m= eans > + // the microcode patches data has not been loaded into memory yet > + // > + ShadowMicrocodeUpdatePatch (CpuMpData); > + } > + > // > // Detect and apply Microcode on BSP > // > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > index 06e3f5d0d3..6ecbed39ec 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c > @@ -1,7 +1,7 @@ > /** @file > MP initialize support functions for PEI phase. >=20 > - Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ > @@ -9,7 +9,6 @@ > #include "MpLib.h" > #include > #include > -#include >=20 > /** > S3 SMM Init Done notification function. > -- > 2.12.0.windows.1