From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: eric.dong@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Tue, 16 Jul 2019 01:05:14 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2019 01:05:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,497,1557212400"; d="scan'208";a="158070253" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga007.jf.intel.com with ESMTP; 16 Jul 2019 01:05:13 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 16 Jul 2019 01:05:13 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 16 Jul 2019 01:05:12 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 16 Jul 2019 01:05:12 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.3]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.232]) with mapi id 14.03.0439.000; Tue, 16 Jul 2019 16:05:11 +0800 From: "Dong, Eric" To: "Zeng, Star" , "devel@edk2.groups.io" CC: Laszlo Ersek , "Ni, Ray" , "Kumar, Chandana C" , "Li, Kevin Y" Subject: Re: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code Thread-Topic: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code Thread-Index: AQHVOJple9NZLFdGnkS0PamLCZ4Kn6bM3K6g//+FA4CAAIdxQA== Date: Tue, 16 Jul 2019 08:05:10 +0000 Message-ID: References: <20190712101258.17512-1-star.zeng@intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483104036673A@shsmsx102.ccr.corp.intel.com> In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483104036673A@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Done SHA-1: 84a459472075d94963463bffaa5dc6eee47f14c3 * UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin code BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1961 Enhance Ppin code to enable and unlock for TRUE State, and disable and lock for FALSE State. Note: enable and lock could not be set both. According to SDM, once Enable_PPIN is set, attempt to write 1 to LockOut will cause #GP, and writing 1 to LockOut is permitted only if Enable_PPIN is clear. Cc: Laszlo Ersek Cc: Eric Dong Cc: Ray Ni Cc: Chandana Kumar Cc: Kevin Li Signed-off-by: Star Zeng Reviewed-by: Ray Ni Reviewed-by: Eric Dong > -----Original Message----- > From: Zeng, Star > Sent: Tuesday, July 16, 2019 4:00 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Laszlo Ersek ; Ni, Ray ; Kumar, > Chandana C ; Li, Kevin Y > ; Zeng, Star > Subject: RE: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin > code >=20 > Eric, >=20 > Thanks for the comments. > Attach the updated patch, and you may help push it if it is ok. >=20 >=20 > Thanks, > Star >=20 > > -----Original Message----- > > From: Dong, Eric > > Sent: Tuesday, July 16, 2019 3:24 PM > > To: Zeng, Star ; devel@edk2.groups.io > > Cc: Laszlo Ersek ; Ni, Ray ; > > Kumar, Chandana C ; Li, Kevin Y > > > > Subject: RE: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin > > code > > > > Hi Star, > > > > Suggest to add some code comments for the behavior, detail see the > > inline comments. > > with these comments, Reviewed-by: Eric Dong > > > > Thanks, > > Eric > > > -----Original Message----- > > > From: Zeng, Star > > > Sent: Friday, July 12, 2019 6:13 PM > > > To: devel@edk2.groups.io > > > Cc: Zeng, Star ; Laszlo Ersek > > > ; Dong, Eric ; Ni, Ray > > > ; Kumar, Chandana C ; > > > Li, Kevin Y > > > Subject: [PATCH] UefiCpuPkg CpuCommonFeaturesLib: Enhance Ppin > code > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1961 > > > Enhance Ppin code to enable and unlock for TRUE State, and disable > > > and lock for FALSE State. > > > Note: enable and lock could not be set both. > > > > > > Cc: Laszlo Ersek > > > Cc: Eric Dong > > > Cc: Ray Ni > > > Cc: Chandana Kumar > > > Cc: Kevin Li > > > Signed-off-by: Star Zeng > > > --- > > > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 +++++ > > > .../CpuCommonFeaturesLib.c | 2 +- > > > .../Library/CpuCommonFeaturesLib/Ppin.c | 65 +++++++++++++++--= - > - > > > 3 files changed, 70 insertions(+), 12 deletions(-) > > > > > > diff --git > > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > > index 9e784e916a85..8406c6c1619f 100644 > > > --- > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > > +++ > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > > > @@ -863,6 +863,21 @@ FeatureControlGetConfigData ( > > > IN UINTN NumberOfProcessors > > > ); > > > > > > +/** > > > + Prepares for the data used by CPU feature detection and initializa= tion. > > > + > > > + @param[in] NumberOfProcessors The number of CPUs in the > platform. > > > + > > > + @return Pointer to a buffer of CPU related configuration data. > > > + > > > + @note This service could be called by BSP only. > > > +**/ > > > +VOID * > > > +EFIAPI > > > +PpinGetConfigData ( > > > + IN UINTN NumberOfProcessors > > > + ); > > > + > > > /** > > > Detects if Protected Processor Inventory Number feature supported > > > on current > > > processor. > > > diff --git > > > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > > index 7cc692efb649..fd43b8d66290 100644 > > > --- > > > > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > > +++ > > > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > > > @@ -203,7 +203,7 @@ CpuCommonFeaturesLibConstructor ( > > > if (IsCpuFeatureSupported (CPU_FEATURE_PPIN)) { > > > Status =3D RegisterCpuFeature ( > > > "PPIN", > > > - NULL, > > > + PpinGetConfigData, > > > PpinSupport, > > > PpinInitialize, > > > CPU_FEATURE_PPIN, > > > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > > index e8a4de8dcf60..8067cf44d015 100644 > > > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/Ppin.c > > > @@ -8,6 +8,28 @@ > > > > > > #include "CpuCommonFeatures.h" > > > > > > +/** > > > + Prepares for the data used by CPU feature detection and initializa= tion. > > > + > > > + @param[in] NumberOfProcessors The number of CPUs in the > platform. > > > + > > > + @return Pointer to a buffer of CPU related configuration data. > > > + > > > + @note This service could be called by BSP only. > > > +**/ > > > +VOID * > > > +EFIAPI > > > +PpinGetConfigData ( > > > + IN UINTN NumberOfProcessors > > > + ) > > > +{ > > > + VOID *ConfigData; > > > + > > > + ConfigData =3D AllocateZeroPool (sizeof > > > +(MSR_IVY_BRIDGE_PPIN_CTL_REGISTER) * NumberOfProcessors); > > > + ASSERT (ConfigData !=3D NULL); > > > + return ConfigData; > > > +} > > > + > > > /** > > > Detects if Protected Processor Inventory Number feature supported > > > on current > > > processor. > > > @@ -34,6 +56,7 @@ PpinSupport ( > > > ) > > > { > > > MSR_IVY_BRIDGE_PLATFORM_INFO_1_REGISTER PlatformInfo; > > > + MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *MsrPpinCtrl; > > > > > > if ((CpuInfo->DisplayFamily =3D=3D 0x06) && > > > ((CpuInfo->DisplayModel =3D=3D 0x3E) || // Xeon E5 V2 > > > @@ -47,7 +70,12 @@ PpinSupport ( > > > // Check whether platform support this feature. > > > // > > > PlatformInfo.Uint64 =3D AsmReadMsr64 > > > (MSR_IVY_BRIDGE_PLATFORM_INFO_1); > > > - return (PlatformInfo.Bits.PPIN_CAP !=3D 0); > > > + if (PlatformInfo.Bits.PPIN_CAP !=3D 0) { > > > + MsrPpinCtrl =3D (MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *) ConfigDat= a; > > > + ASSERT (MsrPpinCtrl !=3D NULL); > > > + MsrPpinCtrl[ProcessorNumber].Uint64 =3D AsmReadMsr64 > > > (MSR_IVY_BRIDGE_PPIN_CTL); > > > + return TRUE; > > > + } > > > } > > > > > > return FALSE; > > > @@ -73,6 +101,7 @@ PpinSupport ( > > > @retval RETURN_DEVICE_ERROR Device can't change state because it > > > has been > > > locked. > > > > > > + @note This service could be called by BSP only. > > > **/ > > > RETURN_STATUS > > > EFIAPI > > > @@ -83,16 +112,18 @@ PpinInitialize ( > > > IN BOOLEAN State > > > ) > > > { > > > - MSR_IVY_BRIDGE_PPIN_CTL_REGISTER MsrPpinCtrl; > > > + MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *MsrPpinCtrl; > > > + > > > + MsrPpinCtrl =3D (MSR_IVY_BRIDGE_PPIN_CTL_REGISTER *) ConfigData; > > > + ASSERT (MsrPpinCtrl !=3D NULL); > > > > > > // > > > - // Check whether device already lock this register. > > > - // If already locked, just base on the request state and > > > + // Check whether processor already lock this register. > > > + // If already locked, just based on the request state and > > > // the current state to return the status. > > > // > > > - MsrPpinCtrl.Uint64 =3D AsmReadMsr64 (MSR_IVY_BRIDGE_PPIN_CTL); > > > - if (MsrPpinCtrl.Bits.LockOut !=3D 0) { > > > - return MsrPpinCtrl.Bits.Enable_PPIN =3D=3D State ? RETURN_SUCCES= S : > > > RETURN_DEVICE_ERROR; > > > + if (MsrPpinCtrl[ProcessorNumber].Bits.LockOut !=3D 0) { > > > + return MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN =3D=3D Stat= e ? > > > + RETURN_SUCCESS : RETURN_DEVICE_ERROR; > > > } > > > > > > // > > > @@ -106,13 +137,25 @@ PpinInitialize ( > > > return RETURN_SUCCESS; > > > } > > > > > > - CPU_REGISTER_TABLE_WRITE_FIELD ( > > > + if (State) { > > > + // > > > + // Enable and Unlock. > > > + // > > > + MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN =3D 1; > > > + MsrPpinCtrl[ProcessorNumber].Bits.LockOut =3D 0; } else { > > > > 1. I suggest to add some comments about why unlock & enable need to > > set at the same time. > > > > > > > + // > > > + // Disable and Lock. > > > + // > > > + MsrPpinCtrl[ProcessorNumber].Bits.Enable_PPIN =3D 0; > > > + MsrPpinCtrl[ProcessorNumber].Bits.LockOut =3D 1; } > > > > > > 2. Same as comments 1. > > > > Thanks, > > Eric > > > + > > > + CPU_REGISTER_TABLE_WRITE64 ( > > > ProcessorNumber, > > > Msr, > > > MSR_IVY_BRIDGE_PPIN_CTL, > > > - MSR_IVY_BRIDGE_PPIN_CTL_REGISTER, > > > - Bits.Enable_PPIN, > > > - (State) ? 1 : 0 > > > + MsrPpinCtrl[ProcessorNumber].Uint64 > > > ); > > > > > > return RETURN_SUCCESS; > > > -- > > > 2.21.0.windows.1