From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.43; helo=mga05.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 EE13621168233 for ; Fri, 9 Nov 2018 06:55:15 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Nov 2018 06:55:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,483,1534834800"; d="scan'208";a="102997235" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga002.fm.intel.com with ESMTP; 09 Nov 2018 06:55:15 -0800 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 9 Nov 2018 06:55:15 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 9 Nov 2018 06:55:14 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.199]) with mapi id 14.03.0415.000; Fri, 9 Nov 2018 22:55:13 +0800 From: "Ni, Ruiyu" To: "Dong, Eric" , "edk2-devel@lists.01.org" CC: Laszlo Ersek Thread-Topic: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. Thread-Index: AQHUd+v9A65SxQx/t0KfCwSDPljUBaVHiGKA Date: Fri, 9 Nov 2018 14:55:12 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BEFCE90@SHSMSX104.ccr.corp.intel.com> References: <20181109052041.14816-1-eric.dong@intel.com> In-Reply-To: <20181109052041.14816-1-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmNhMmEwOTQtNDMxOS00MjI5LThiYzEtODZjMThiNmVmYmQ0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiNXRKd2F5dlwvUjBibUZRVEZsdTNOWnM2Wk9FVmpVZjlyMHUrcGdoRkxZYllqQjBWanRrcDBNdFFrME9vUENvWE0ifQ== 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: Fri, 09 Nov 2018 14:55:16 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 Er= ic > 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 Order. >=20 > 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. >=20 > V1 Changes: > 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. >=20 > 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. >=20 > Root cause is driver ignores the dependence for this case, so > threads may execute not follow the dependence order. >=20 > Fix this issue by change code logic to adjust feature position > for CPU features which has dependence relationship. >=20 > 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(-) >=20 > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitial= ize.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index 4bed0ce3a4..69e2c04daf 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -534,6 +534,28 @@ DumpRegisterTableOnProcessor ( > } > } >=20 > +/** > + 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); > +} > + > /** > Analysis register CPU features on each processor and save CPU setting = in CPU > register table. >=20 > @@ -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; >=20 > CpuFeaturesData =3D GetCpuFeaturesData (); > CpuFeaturesData->CapabilityPcd =3D AllocatePool (CpuFeaturesData- > >BitMaskSize); > @@ -634,14 +658,9 @@ AnalysisProcessorFeatures ( > // > CpuInfo =3D &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; > 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, CpuFeaturesDat= a- > >SettingPcd)) { > Status =3D CpuFeatureInOrder->InitializeFunc (ProcessorNumber, C= puInfo, > CpuFeatureInOrder->ConfigData, TRUE); > @@ -674,31 +693,43 @@ AnalysisProcessorFeatures ( > } >=20 > if (Success) { > - // > - // If feature has dependence with the next feature (ONLY care > 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 here. > + // > BeforeDep =3D DetectFeatureScope (CpuFeatureInOrder, TRUE, > NextCpuFeatureInOrder->FeatureMask); > AfterDep =3D DetectFeatureScope (NextCpuFeatureInOrder, FALSE= , > CpuFeatureInOrder->FeatureMask); > + // > + // Check whether next feature has After type dependence with n= ot > neighborhood CPU > + // Features in former CPU features. > + // > + NoneNeibAfterDep =3D > DetectNoneNeighborhoodFeatureScope(NextCpuFeatureInOrder, FALSE, > &CpuInitOrder->OrderList); > } else { > - BeforeDep =3D DetectFeatureScope (CpuFeatureInOrder, TRUE, NUL= L); > - 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); > - } > } >=20 > - Entry =3D Entry->ForwardLink; > - NextEntry =3D Entry->ForwardLink; > + Entry =3D Entry->ForwardLink; > } >=20 > // > diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeature= s.h > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > index 4898a80827..cf3da84837 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > @@ -207,6 +207,22 @@ DetectFeatureScope ( > IN UINT8 *NextCpuFeatureMask > ); >=20 > +/** > + Return feature dependence result. > + > + @param[in] CpuFeature Pointer to CPU feature. > + @param[in] Before Check before dependence or after. > + @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. >=20 > 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/RegisterCpuFeaturesLib.c > @@ -112,6 +112,75 @@ IsBitMaskMatchCheck ( > return FALSE; > } >=20 > +/** > + Try to find the specify cpu featuren in former/after feature list. > + > + @param[in] FeatureList Pointer to dependent CPU feature list > + @param[in] CurrentEntry Pointer to current CPU feature entry. > + @param[in] SearchFormer Find in former feature or after feature= s. > + @param[in] FeatureMask Pointer to CPU feature bit mask > + > + @retval TRUE The feature bit mask is in dependent CPU feature bit mas= k > 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. >=20 > @@ -178,6 +247,59 @@ DetectFeatureScope ( > return NoneDepType; > } >=20 > +/** > + Return feature dependence result. > + > + @param[in] CpuFeature Pointer to CPU feature. > + @param[in] Before Check before dependence or after. > + @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, CpuFea= ture- > >PackageBeforeFeatureBitMask)) { > + return PackageDepType; > + } > + > + if ((CpuFeature->CoreBeforeFeatureBitMask !=3D NULL) && > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, CpuFea= ture- > >CoreBeforeFeatureBitMask)) { > + return CoreDepType; > + } > + > + if ((CpuFeature->BeforeFeatureBitMask !=3D NULL) && > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, CpuFea= ture- > >BeforeFeatureBitMask)) { > + return ThreadDepType; > + } > + > + return NoneDepType; > + } > + > + if ((CpuFeature->PackageAfterFeatureBitMask !=3D NULL) && > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, CpuFeatur= e- > >PackageAfterFeatureBitMask)) { > + return PackageDepType; > + } > + > + if ((CpuFeature->CoreAfterFeatureBitMask !=3D NULL) && > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, CpuFeatur= e- > >CoreAfterFeatureBitMask)) { > + return CoreDepType; > + } > + > + if ((CpuFeature->AfterFeatureBitMask !=3D NULL) && > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, CpuFeatur= e- > >AfterFeatureBitMask)) { > + return ThreadDepType; > + } > + > + return NoneDepType; > +} > + > /** > Base on dependence relationship to asjust feature dependence. >=20 > -- > 2.15.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel