From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B0F5921A07A80 for ; Sat, 10 Nov 2018 18:01:42 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Nov 2018 18:01:42 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,489,1534834800"; d="scan'208";a="107220154" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 10 Nov 2018 18:01:41 -0800 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sat, 10 Nov 2018 18:01:41 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sat, 10 Nov 2018 18:01:41 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.84]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.199]) with mapi id 14.03.0415.000; Sun, 11 Nov 2018 10:01:39 +0800 From: "Dong, Eric" To: Leif Lindholm CC: "Laszlo Ersek (lersek@redhat.com)" , "Kinney, Michael D" , "afish@apple.com" , "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. Thread-Index: AQHUd+v9uWR3/ncmLUGVI9K4Nrbl/6VHAnoAgAFUK3D//+u1gIABkcog Date: Sun, 11 Nov 2018 02:01:38 +0000 Message-ID: References: <20181109052041.14816-1-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5BEFCE90@SHSMSX104.ccr.corp.intel.com> <20181110100005.u2apchcfynttcyhj@bivouac.eciton.net> In-Reply-To: <20181110100005.u2apchcfynttcyhj@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDAwZDk5OTgtM2Y1NC00ZDNhLWExYjYtNDM0MWFlYzIwOTRhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiVXAyZkw2NlRtQ3hBR3gzTGhMaFcyMEN2SUY3ZFh3V2JFQ1wvZktTYjRyTDhCOVwvbnRUb3RMQTF4XC9abnQ4N3Q2TSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 11 Nov 2018 02:01:43 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Leif, Thanks for your comments. Because I must check in the code today, I will ch= ange the code when I push the change. Thanks, Eric > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Saturday, November 10, 2018 6:00 PM > To: Dong, Eric > Cc: Laszlo Ersek (lersek@redhat.com) ; Kinney, Michael= D > ; afish@apple.com; Ni, Ruiyu > ; edk2-devel@lists.01.org > Subject: Re: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Ord= er. >=20 > Hi Eric, >=20 > I have no concerns over pushing this bugfix. >=20 > However, having lookes at it, I do have a couple of style comments. >=20 > On Sat, Nov 10, 2018 at 03:16:26AM +0000, Dong, Eric wrote: > > Hi Stewards: > > > > Since this is a bug fix, and the risk for this release is small. I > > plan to push this change before edk2-stable201811 tag. > > > > If you have any concern, please raise here. > > > > Thanks, > > Eric > > > > > -----Original Message----- > > > From: Ni, Ruiyu > > > Sent: Friday, November 9, 2018 10:55 PM > > > To: Dong, Eric ; edk2-devel@lists.01.org > > > Cc: Laszlo Ersek > > > Subject: RE: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust= Order. > > > > > > Eric, > > > Thanks for fixing the new found issues. > > > > > > Reviewed-by: Ruiyu Ni > > > > > > > -----Original Message----- > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > > > > Behalf Of Eric Dong > > > > Sent: Friday, November 9, 2018 1:21 PM > > > > To: edk2-devel@lists.01.org > > > > Cc: Ni, Ruiyu ; Laszlo Ersek > > > > > > > > Subject: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust O= rder. > > > > > > > > V2 changes: > > > > V1 change has regression which caused by change feature order. > > > > V2 changes logic to detect dependence not only for the > > > > neighborhood features. It need to check all features in the list. > > > > > > > > V1 Changes: >=20 > The patch version information does not belong in the patch description. P= lease > include it under "---" and make the description refer only to what the pa= tch does. >=20 > > > > In current code logic, only adjust feature position if current CPU > > > > feature position not follow the request order. Just like Feature A > > > > need to be executed before feature B, but current feature A > > > > registers after feature B. So code will adjust the position for > > > > feature A, move it to just before feature B. If the position > > > > already met the requirement, code will not adjust the position. > > > > > > > > This logic has issue when met all below cases: > > > > 1. feature A has core or package level dependence with feature B. > > > > 2. feature A is register before feature B. > > > > 3. Also exist other features exist between feature A and B. > > > > > > > > Root cause is driver ignores the dependence for this case, so > > > > threads may execute not follow the dependence order. > > > > > > > > Fix this issue by change code logic to adjust feature position for > > > > CPU features which has dependence relationship. > > > > > > > > Cc: Laszlo Ersek > > > > Cc: Ruiyu Ni > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Eric Dong > > > > --- > > > > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 71 ++++++++-= --- > > > > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 16 +++ > > > > .../RegisterCpuFeaturesLib.c | 122 +++++++++= ++++++++++++ > > > > 3 files changed, 189 insertions(+), 20 deletions(-) > > > > > > > > diff --git > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > > c > > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > > c index 4bed0ce3a4..69e2c04daf 100644 > > > > --- > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > > c > > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitiali= ze. > > > > +++ c > > > > @@ -534,6 +534,28 @@ DumpRegisterTableOnProcessor ( > > > > } > > > > } > > > > > > > > +/** > > > > + Get the biggest dependence type. > > > > + PackageDepType > CoreDepType > ThreadDepType > NoneDepType. > > > > + > > > > + @param[in] BeforeDep Before dependence type. > > > > + @param[in] AfterDep After dependence type. > > > > + @param[in] NoneNeibBeforeDep Before dependence type for not > > > > neighborhood features. > > > > + @param[in] NoneNeibAfterDep After dependence type for not > > > > neighborhood features. > > > > + > > > > + @retval Return the biggest dependence type. > > > > +**/ > > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > > +BiggestDep ( > > > > + IN CPU_FEATURE_DEPENDENCE_TYPE BeforeDep, > > > > + IN CPU_FEATURE_DEPENDENCE_TYPE AfterDep, > > > > + IN CPU_FEATURE_DEPENDENCE_TYPE NoneNeibBeforeDep, > > > > + IN CPU_FEATURE_DEPENDENCE_TYPE NoneNeibAfterDep > > > > + ) > > > > +{ > > > > + return MAX(MAX (MAX(BeforeDep, AfterDep), NoneNeibBeforeDep), > > > > NoneNeibAfterDep); >=20 > And please make use of some temporary variables here. > It will make no difference to code generation in any compiler designed si= nce > 1995, but will make it human readable. >=20 > / > Leif >=20 > > > > +} > > > > + > > > > /** > > > > Analysis register CPU features on each processor and save CPU > > > > setting in CPU register table. > > > > > > > > @@ -558,6 +580,8 @@ AnalysisProcessorFeatures ( > > > > BOOLEAN Success; > > > > CPU_FEATURE_DEPENDENCE_TYPE BeforeDep; > > > > CPU_FEATURE_DEPENDENCE_TYPE AfterDep; > > > > + CPU_FEATURE_DEPENDENCE_TYPE NoneNeibBeforeDep; > > > > + CPU_FEATURE_DEPENDENCE_TYPE NoneNeibAfterDep; > > > > > > > > CpuFeaturesData =3D GetCpuFeaturesData (); > > > > CpuFeaturesData->CapabilityPcd =3D AllocatePool (CpuFeaturesData= - > > > > >BitMaskSize); > > > > @@ -634,14 +658,9 @@ AnalysisProcessorFeatures ( > > > > // > > > > CpuInfo =3D &CpuFeaturesData->InitOrder[ProcessorNumber].CpuIn= fo; > > > > Entry =3D GetFirstNode (&CpuInitOrder->OrderList); > > > > - NextEntry =3D Entry->ForwardLink; > > > > while (!IsNull (&CpuInitOrder->OrderList, Entry)) { > > > > CpuFeatureInOrder =3D CPU_FEATURE_ENTRY_FROM_LINK (Entry); > > > > - if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { > > > > - NextCpuFeatureInOrder =3D CPU_FEATURE_ENTRY_FROM_LINK > > > (NextEntry); > > > > - } else { > > > > - NextCpuFeatureInOrder =3D NULL; > > > > - } > > > > + > > > > Success =3D FALSE; > > > > if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask, > > > > CpuFeaturesData- > > > > >SettingPcd)) { > > > > Status =3D CpuFeatureInOrder->InitializeFunc > > > > (ProcessorNumber, CpuInfo, > > > > CpuFeatureInOrder->ConfigData, TRUE); > > > > @@ -674,31 +693,43 @@ AnalysisProcessorFeatures ( > > > > } > > > > > > > > if (Success) { > > > > - // > > > > - // If feature has dependence with the next feature (ONLY c= are > > > > core/package dependency). > > > > - // and feature initialize succeed, add sync semaphere here= . > > > > - // > > > > - if (NextCpuFeatureInOrder !=3D NULL) { > > > > + NextEntry =3D Entry->ForwardLink; > > > > + if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { > > > > + NextCpuFeatureInOrder =3D CPU_FEATURE_ENTRY_FROM_LINK > > > > (NextEntry); > > > > + > > > > + // > > > > + // If feature has dependence with the next feature > > > > + (ONLY care > > > > core/package dependency). > > > > + // and feature initialize succeed, add sync semaphere he= re. > > > > + // > > > > BeforeDep =3D DetectFeatureScope (CpuFeatureInOrder, > > > > TRUE, > > > > NextCpuFeatureInOrder->FeatureMask); > > > > AfterDep =3D DetectFeatureScope (NextCpuFeatureInOrder, > > > > FALSE, > > > > CpuFeatureInOrder->FeatureMask); > > > > + // > > > > + // Check whether next feature has After type dependence > > > > + with not > > > > neighborhood CPU > > > > + // Features in former CPU features. > > > > + // > > > > + NoneNeibAfterDep =3D > > > > DetectNoneNeighborhoodFeatureScope(NextCpuFeatureInOrder, FALSE, > > > > &CpuInitOrder->OrderList); > > > > } else { > > > > - BeforeDep =3D DetectFeatureScope (CpuFeatureInOrder, TRU= E, NULL); > > > > - AfterDep =3D NoneDepType; > > > > + BeforeDep =3D NoneDepType; > > > > + AfterDep =3D NoneDepType; > > > > + NoneNeibAfterDep =3D NoneDepType; > > > > } > > > > // > > > > - // Assume only one of the depend is valid. > > > > + // Check whether current feature has Before type > > > > + dependence with none > > > > neighborhood > > > > + // CPU features in after Cpu features. > > > > // > > > > - ASSERT (!(BeforeDep > ThreadDepType && AfterDep > > ThreadDepType)); > > > > + NoneNeibBeforeDep =3D > > > > DetectNoneNeighborhoodFeatureScope(CpuFeatureInOrder, TRUE, > > > > &CpuInitOrder->OrderList); > > > > + > > > > + // > > > > + // Get the biggest dependence and add semaphore for it. > > > > + // PackageDepType > CoreDepType > ThreadDepType > > NoneDepType. > > > > + // > > > > + BeforeDep =3D BiggestDep(BeforeDep, AfterDep, > > > > + NoneNeibBeforeDep, > > > > NoneNeibAfterDep); > > > > if (BeforeDep > ThreadDepType) { > > > > CPU_REGISTER_TABLE_WRITE32 (ProcessorNumber, Semaphore, > > > > 0, BeforeDep); > > > > } > > > > - if (AfterDep > ThreadDepType) { > > > > - CPU_REGISTER_TABLE_WRITE32 (ProcessorNumber, Semaphore, = 0, > > > > AfterDep); > > > > - } > > > > } > > > > > > > > - Entry =3D Entry->ForwardLink; > > > > - NextEntry =3D Entry->ForwardLink; > > > > + Entry =3D Entry->ForwardLink; > > > > } > > > > > > > > // > > > > diff --git > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > > index 4898a80827..cf3da84837 100644 > > > > --- > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeature > > > > +++ s.h > > > > @@ -207,6 +207,22 @@ DetectFeatureScope ( > > > > IN UINT8 *NextCpuFeatureMask > > > > ); > > > > > > > > +/** > > > > + Return feature dependence result. > > > > + > > > > + @param[in] CpuFeature Pointer to CPU feature. > > > > + @param[in] Before Check before dependence or aft= er. > > > > + @param[in] FeatureList Pointer to CPU feature list. > > > > + > > > > + @retval return the dependence result. > > > > +**/ > > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > > +DetectNoneNeighborhoodFeatureScope ( > > > > + IN CPU_FEATURES_ENTRY *CpuFeature, > > > > + IN BOOLEAN Before, > > > > + IN LIST_ENTRY *FeatureList > > > > + ); > > > > + > > > > /** > > > > Programs registers for the calling processor. > > > > > > > > diff --git > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib > > > > .c > > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib > > > > .c index 394695baf2..ed8d526325 100644 > > > > --- > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib > > > > .c > > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeature > > > > +++ sLib > > > > +++ .c > > > > @@ -112,6 +112,75 @@ IsBitMaskMatchCheck ( > > > > return FALSE; > > > > } > > > > > > > > +/** > > > > + Try to find the specify cpu featuren in former/after feature lis= t. > > > > + > > > > + @param[in] FeatureList Pointer to dependent CPU feature = list > > > > + @param[in] CurrentEntry Pointer to current CPU feature en= try. > > > > + @param[in] SearchFormer Find in former feature or after f= eatures. > > > > + @param[in] FeatureMask Pointer to CPU feature bit mask > > > > + > > > > + @retval TRUE The feature bit mask is in dependent CPU feature > > > > + bit mask > > > > buffer. > > > > + @retval FALSE The feature bit mask is not in dependent CPU > > > > + feature bit mask > > > > buffer. > > > > +**/ > > > > +BOOLEAN > > > > +FindSpecifyFeature ( > > > > + IN LIST_ENTRY *FeatureList, > > > > + IN LIST_ENTRY *CurrentEntry, > > > > + IN BOOLEAN SearchFormer, > > > > + IN UINT8 *FeatureMask > > > > + ) > > > > +{ > > > > + CPU_FEATURES_ENTRY *CpuFeature; > > > > + LIST_ENTRY *NextEntry; > > > > + > > > > + // > > > > + // Check whether exist the not neighborhood entry first. > > > > + // If not exist, return FALSE means not found status. > > > > + // > > > > + if (SearchFormer) { > > > > + NextEntry =3D CurrentEntry->BackLink; > > > > + if (IsNull (FeatureList, NextEntry)) { > > > > + return FALSE; > > > > + } > > > > + > > > > + NextEntry =3D NextEntry->BackLink; > > > > + if (IsNull (FeatureList, NextEntry)) { > > > > + return FALSE; > > > > + } > > > > + > > > > + NextEntry =3D CurrentEntry->BackLink->BackLink; } else { > > > > + NextEntry =3D CurrentEntry->ForwardLink; > > > > + if (IsNull (FeatureList, NextEntry)) { > > > > + return FALSE; > > > > + } > > > > + > > > > + NextEntry =3D NextEntry->ForwardLink; > > > > + if (IsNull (FeatureList, NextEntry)) { > > > > + return FALSE; > > > > + } > > > > + > > > > + NextEntry =3D CurrentEntry->ForwardLink->ForwardLink; > > > > + } > > > > + > > > > + while (!IsNull (FeatureList, NextEntry)) { > > > > + CpuFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (NextEntry); > > > > + > > > > + if (IsBitMaskMatchCheck (FeatureMask, CpuFeature->FeatureMask)= ) { > > > > + return TRUE; > > > > + } > > > > + > > > > + if (SearchFormer) { > > > > + NextEntry =3D NextEntry->BackLink; > > > > + } else { > > > > + NextEntry =3D NextEntry->ForwardLink; > > > > + } > > > > + } > > > > + > > > > + return FALSE; > > > > +} > > > > + > > > > /** > > > > Return feature dependence result. > > > > > > > > @@ -178,6 +247,59 @@ DetectFeatureScope ( > > > > return NoneDepType; > > > > } > > > > > > > > +/** > > > > + Return feature dependence result. > > > > + > > > > + @param[in] CpuFeature Pointer to CPU feature. > > > > + @param[in] Before Check before dependence or aft= er. > > > > + @param[in] FeatureList Pointer to CPU feature list. > > > > + > > > > + @retval return the dependence result. > > > > +**/ > > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > > +DetectNoneNeighborhoodFeatureScope ( > > > > + IN CPU_FEATURES_ENTRY *CpuFeature, > > > > + IN BOOLEAN Before, > > > > + IN LIST_ENTRY *FeatureList > > > > + ) > > > > +{ > > > > + if (Before) { > > > > + if ((CpuFeature->PackageBeforeFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > > +CpuFeature- > > > > >PackageBeforeFeatureBitMask)) { > > > > + return PackageDepType; > > > > + } > > > > + > > > > + if ((CpuFeature->CoreBeforeFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > > + CpuFeature- > > > > >CoreBeforeFeatureBitMask)) { > > > > + return CoreDepType; > > > > + } > > > > + > > > > + if ((CpuFeature->BeforeFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > > + CpuFeature- > > > > >BeforeFeatureBitMask)) { > > > > + return ThreadDepType; > > > > + } > > > > + > > > > + return NoneDepType; > > > > + } > > > > + > > > > + if ((CpuFeature->PackageAfterFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > > + CpuFeature- > > > > >PackageAfterFeatureBitMask)) { > > > > + return PackageDepType; > > > > + } > > > > + > > > > + if ((CpuFeature->CoreAfterFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > > + CpuFeature- > > > > >CoreAfterFeatureBitMask)) { > > > > + return CoreDepType; > > > > + } > > > > + > > > > + if ((CpuFeature->AfterFeatureBitMask !=3D NULL) && > > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > > + CpuFeature- > > > > >AfterFeatureBitMask)) { > > > > + return ThreadDepType; > > > > + } > > > > + > > > > + return NoneDepType; > > > > +} > > > > + > > > > /** > > > > Base on dependence relationship to asjust feature dependence. > > > > > > > > -- > > > > 2.15.0.windows.1 > > > > > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel