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=binx.song@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 CE7C12215BD8C for ; Tue, 30 Jan 2018 23:36:12 -0800 (PST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2018 23:41:47 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,438,1511856000"; d="zip'48?scan'48,48,208,223";a="14767683" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga006.jf.intel.com with ESMTP; 30 Jan 2018 23:41:47 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 30 Jan 2018 23:41:46 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.127]) with mapi id 14.03.0319.002; Wed, 31 Jan 2018 15:41:45 +0800 From: "Song, BinX" To: "edk2-devel@lists.01.org" CC: "lersek@redhat.com" , "Dong, Eric" Thread-Topic: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check Thread-Index: AdOaYTQY+pBf08MTTXCTDz1deLfPbwABZcRw Date: Wed, 31 Jan 2018 07:41:44 +0000 Message-ID: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2567@shsmsx102.ccr.corp.intel.com> References: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> In-Reply-To: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.23 Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Jan 2018 07:36:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi All, Attached my test case F.Y.R. Best Regards, Bell Song > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Song, BinX > Sent: Wednesday, January 31, 2018 3:01 PM > To: edk2-devel@lists.01.org > Cc: lersek@redhat.com; Dong, Eric > Subject: [edk2] [PATCH] UefiCpuPkg: Enhance CPU feature dependency > check >=20 > Current CPU feature dependency check will hang on when meet below or > similar case: > if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) { > Status =3D RegisterCpuFeature ( > "AESNI", > AesniGetConfigData, > AesniSupport, > AesniInitialize, > CPU_FEATURE_AESNI, > CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE, > CPU_FEATURE_END > ); > ASSERT_EFI_ERROR (Status); > } > if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) { > Status =3D RegisterCpuFeature ( > "MWAIT", > NULL, > MonitorMwaitSupport, > MonitorMwaitInitialize, > CPU_FEATURE_MWAIT, > CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE, > CPU_FEATURE_END > ); > ASSERT_EFI_ERROR (Status); > } >=20 > Solution is to separate current CPU feature dependency check into > sort and check two parts. >=20 > Sort function: > According to CPU feature's dependency, sort all CPU features. > Later dependency will override previous dependency if they are conflicted= . >=20 > Check function: > Check sorted CPU features' relationship, ASSERT invalid relationship. >=20 > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Bell Song > --- > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271 > ++++++++++++++++++++- > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 7 + > .../RegisterCpuFeaturesLib.c | 130 +--------- > 3 files changed, 278 insertions(+), 130 deletions(-) >=20 > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index 4d75c07..2fd0d5f 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor ( > } >=20 > /** > + From FeatureBitMask, find the right feature entry in CPU feature list. > + > + @param[in] FeatureList The pointer to CPU feature list. > + @param[in] CurrentFeature The pointer to current CPU feature. > + @param[in] BeforeFlag TRUE: BeforeFeatureBitMask; FALSE: > AfterFeatureBitMask. > + > + @return The pointer to right CPU feature entry. > +**/ > +LIST_ENTRY * > +FindFeatureInList( > + IN LIST_ENTRY *CpuFeatureList, > + IN CPU_FEATURES_ENTRY *CurrentCpuFeature, > + IN BOOLEAN BeforeFlag > + ) > +{ > + LIST_ENTRY *TempEntry; > + CPU_FEATURES_ENTRY *TempFeature; > + UINT8 *FeatureBitMask; > + > + FeatureBitMask =3D BeforeFlag ? CurrentCpuFeature- > >BeforeFeatureBitMask : CurrentCpuFeature->AfterFeatureBitMask; > + TempEntry =3D GetFirstNode (CpuFeatureList); > + while (!IsNull (CpuFeatureList, TempEntry)) { > + TempFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (TempEntry); > + if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature->FeatureMask)){ > + return TempEntry; > + } > + TempEntry =3D TempEntry->ForwardLink; > + } > + > + DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature- > >FeatureName, BeforeFlag ? "before ":"after ", "condition is invalid!\n")= ); > + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", > CurrentCpuFeature->FeatureName)); > + ASSERT (FALSE); > + > + return NULL; > +} > + > +/** > + In CPU feature list, check if one entry is before another entry. > + > + @param[in] FeatureList The pointer to CPU feature list. > + @param[in] OneEntry The pointer to current CPU feature entry. > + @param[in] AnotherEntry The pointer to checked CPU feature entry. > + > + @return TRUE One entry is before another entry. > + @return FALSE One entry is NOT before another entry. > +**/ > +BOOLEAN > +CheckEntryBeforeEntry( > + IN LIST_ENTRY *CpuFeatureList, > + IN LIST_ENTRY *OneEntry, > + IN LIST_ENTRY *AnotherEntry > + ) > +{ > + LIST_ENTRY *TempEntry; > + > + TempEntry =3D OneEntry; > + while (!IsNull (CpuFeatureList, TempEntry)) { > + if (IsNull (AnotherEntry, TempEntry)) { > + return TRUE; > + } > + TempEntry =3D TempEntry->ForwardLink; > + } > + return FALSE; > +} > + > +/** > + Check sorted CPU features' relationship, ASSERT invalid one. > + > + @param[in] FeatureList The pointer to CPU feature list. > +**/ > +VOID > +CheckCpuFeaturesRelationShip ( > + IN LIST_ENTRY *FeatureList > + ) > +{ > + LIST_ENTRY *CurrentEntry; > + CPU_FEATURES_ENTRY *CurrentFeature; > + LIST_ENTRY *CheckEntry; > + CPU_FEATURES_ENTRY *CheckFeature; > + > + // > + // From head to tail, one by one to check all CPU features. > + // > + CurrentEntry =3D GetFirstNode (FeatureList); > + while (!IsNull (FeatureList, CurrentEntry)) { > + CurrentFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CurrentEntry); > + ASSERT (CurrentFeature->Sorted); > + if (CurrentFeature->BeforeAll) { > + CheckEntry =3D CurrentEntry->BackLink; > + while (!IsNull (FeatureList, CheckEntry)) { > + CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > + // > + // Check all features before this entry, > + // ASSERT when feature has no BeforeAll flag. > + // > + if (!CheckFeature->BeforeAll){ > + DEBUG ((DEBUG_ERROR, "Error: Feature %a before all is invalid!= \n", > CurrentFeature->FeatureName)); > + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", > CurrentFeature->FeatureName)); > + ASSERT (FALSE); > + } > + CheckEntry =3D CheckEntry->BackLink; > + } > + } > + > + if (CurrentFeature->AfterAll) { > + CheckEntry =3D CurrentEntry->ForwardLink; > + while (!IsNull (FeatureList, CheckEntry)) { > + CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > + // > + // Check all features after this entry, > + // ASSERT when feature has no AfterAll flag. > + // > + if(!CheckFeature->AfterAll){ > + DEBUG ((DEBUG_ERROR, "Error: Feature %a after all is invalid!\= n", > CurrentFeature->FeatureName)); > + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", > CurrentFeature->FeatureName)); > + ASSERT (FALSE); > + } > + CheckEntry =3D CheckEntry->ForwardLink; > + } > + } > + > + if (CurrentFeature->BeforeFeatureBitMask !=3D NULL) { > + // > + // Get correct feature entry in feature list. > + // > + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, TRU= E); > + // > + // ASSERT when current feature's relationship is invalid. > + // > + if (!CheckEntryBeforeEntry(FeatureList, CurrentEntry, CheckEntry))= { > + CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > + DEBUG ((DEBUG_ERROR, "Error: Feature %a before %a is invalid!\n"= , > CurrentFeature->FeatureName, CheckFeature->FeatureName)); > + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", > CurrentFeature->FeatureName)); > + ASSERT (FALSE); > + } > + } > + > + if (CurrentFeature->AfterFeatureBitMask !=3D NULL) { > + // > + // Get correct feature entry in feature list. > + // > + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, FAL= SE); > + // > + // ASSERT when current feature's relationship is invalid. > + // > + if (CheckEntryBeforeEntry(FeatureList, CurrentEntry, CheckEntry)) = { > + CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > + DEBUG ((DEBUG_ERROR, "Error: Feature %a after %a is invalid!\n", > CurrentFeature->FeatureName, CheckFeature->FeatureName)); > + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", > CurrentFeature->FeatureName)); > + ASSERT (FALSE); > + } > + } > + // > + // Check next feature entry. > + // > + CurrentEntry =3D CurrentEntry->ForwardLink; > + } > +} > + > +/** > + According to CPU feature's dependency, sort all CPU features. > + Later dependency will override previous dependency if they are conflic= ted. > + > + @param[in] FeatureList The pointer to CPU feature list. > +**/ > +VOID > +SortCpuFeatures ( > + IN LIST_ENTRY *FeatureList > + ) > +{ > + LIST_ENTRY *CurrentEntry; > + CPU_FEATURES_ENTRY *CurrentFeature; > + LIST_ENTRY *CheckEntry; > + LIST_ENTRY *TempEntry; > + BOOLEAN Swapped; > + > + // > + // Initinate all CPU features' Sorted =3D FALSE. > + // > + CurrentEntry =3D GetFirstNode (FeatureList); > + while (!IsNull (FeatureList, CurrentEntry)) { > + CurrentFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CurrentEntry); > + CurrentFeature->Sorted =3D FALSE; > + CurrentEntry =3D CurrentEntry->ForwardLink; > + } > + > + // > + // From head to tail, one by one to sort all CPU features. > + // > + CurrentEntry =3D GetFirstNode (FeatureList); > + while (!IsNull (FeatureList, CurrentEntry)) { > + Swapped =3D FALSE; > + CurrentFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CurrentEntry); > + // > + // Keep previous feature entry. When current feature entry is swappe= d, > + // check current feature entry position again. > + // > + TempEntry =3D CurrentEntry->BackLink; > + if (!CurrentFeature->Sorted) { > + if (CurrentFeature->BeforeAll) { > + RemoveEntryList (CurrentEntry); > + InsertHeadList (FeatureList, CurrentEntry); > + Swapped =3D TRUE; > + } > + > + if (CurrentFeature->AfterAll) { > + RemoveEntryList (CurrentEntry); > + InsertTailList (FeatureList, CurrentEntry); > + Swapped =3D TRUE; > + } > + > + if (CurrentFeature->BeforeFeatureBitMask !=3D NULL) { > + // > + // Get correct feature entry in feature list. > + // > + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, T= RUE); > + // > + // Swap them if need > + // > + if (!CheckEntryBeforeEntry(FeatureList, CurrentEntry, CheckEntry= )) { > + RemoveEntryList (CheckEntry); > + InsertHeadList (CurrentEntry, CheckEntry); > + Swapped =3D TRUE; > + } > + } > + > + if (CurrentFeature->AfterFeatureBitMask !=3D NULL) { > + // > + // Get correct feature entry in feature list. > + // > + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, F= ALSE); > + // > + // Swap them if need. > + // > + if (CheckEntryBeforeEntry(FeatureList, CurrentEntry, CheckEntry)= ) { > + RemoveEntryList (CheckEntry); > + InsertTailList (CurrentEntry, CheckEntry); > + Swapped =3D TRUE; > + } > + } > + // > + // This feature has been sorted. > + // > + CurrentFeature->Sorted =3D TRUE; > + if (Swapped) { > + // > + // Check current entry position again. > + // > + CurrentEntry =3D TempEntry->ForwardLink; > + } else { > + // > + // Check next feature entry. > + // > + CurrentEntry =3D CurrentEntry->ForwardLink; > + } > + } else { > + // > + // Check next feature entry. > + // > + CurrentEntry =3D CurrentEntry->ForwardLink; > + } > + } > +} > + > +/** > Analysis register CPU features on each processor and save CPU setting = in > CPU register table. >=20 > @param[in] NumberOfCpus Number of processor in system > @@ -466,7 +731,11 @@ AnalysisProcessorFeatures ( > // > SetCapabilityPcd (CpuFeaturesData->CapabilityPcd); > SetSettingPcd (CpuFeaturesData->SettingPcd); > - > + // > + // Sort and check CPU feature list > + // > + SortCpuFeatures(&CpuFeaturesData->FeatureList); > + CheckCpuFeaturesRelationShip(&CpuFeaturesData->FeatureList); > // > // Dump the last CPU feature list > // > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > index 69b4121..550036b 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > @@ -49,6 +49,7 @@ typedef struct { > VOID *ConfigData; > BOOLEAN BeforeAll; > BOOLEAN AfterAll; > + BOOLEAN Sorted; > } CPU_FEATURES_ENTRY; >=20 > typedef struct { > @@ -190,4 +191,10 @@ DumpCpuFeature ( > IN CPU_FEATURES_ENTRY *CpuFeature > ); >=20 > +BOOLEAN > +IsBitMaskMatchCheck ( > + IN UINT8 *FeatureMask, > + IN UINT8 *DependentBitMask > + ); > + > #endif > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > index dd6a82b..a2dfcf7 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > @@ -113,131 +113,6 @@ IsBitMaskMatchCheck ( > } >=20 > /** > - Checks and adjusts CPU features order per dependency relationship. > - > - @param[in] FeatureList Pointer to CPU feature list > -**/ > -VOID > -CheckCpuFeaturesDependency ( > - IN LIST_ENTRY *FeatureList > - ) > -{ > - LIST_ENTRY *CurrentEntry; > - CPU_FEATURES_ENTRY *CpuFeature; > - LIST_ENTRY *CheckEntry; > - CPU_FEATURES_ENTRY *CheckFeature; > - BOOLEAN Swapped; > - LIST_ENTRY *TempEntry; > - > - CurrentEntry =3D GetFirstNode (FeatureList); > - while (!IsNull (FeatureList, CurrentEntry)) { > - Swapped =3D FALSE; > - CpuFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CurrentEntry); > - if (CpuFeature->BeforeAll) { > - // > - // Check all features dispatched before this entry > - // > - CheckEntry =3D GetFirstNode (FeatureList); > - while (CheckEntry !=3D CurrentEntry) { > - CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > - if (!CheckFeature->BeforeAll) { > - // > - // If this feature has no BeforeAll flag and is dispatched bef= ore > CpuFeature, > - // insert currentEntry before Checked feature > - // > - RemoveEntryList (CurrentEntry); > - InsertTailList (CheckEntry, CurrentEntry); > - Swapped =3D TRUE; > - break; > - } > - CheckEntry =3D CheckEntry->ForwardLink; > - } > - if (Swapped) { > - continue; > - } > - } > - > - if (CpuFeature->AfterAll) { > - // > - // Check all features dispatched after this entry > - // > - CheckEntry =3D GetNextNode (FeatureList, CurrentEntry); > - while (!IsNull (FeatureList, CheckEntry)) { > - CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > - if (!CheckFeature->AfterAll) { > - // > - // If this feature has no AfterAll flag and is dispatched afte= r CpuFeature, > - // insert currentEntry after Checked feature > - // > - TempEntry =3D GetNextNode (FeatureList, CurrentEntry); > - RemoveEntryList (CurrentEntry); > - InsertHeadList (CheckEntry, CurrentEntry); > - CurrentEntry =3D TempEntry; > - Swapped =3D TRUE; > - break; > - } > - CheckEntry =3D CheckEntry->ForwardLink; > - } > - if (Swapped) { > - continue; > - } > - } > - > - if (CpuFeature->BeforeFeatureBitMask !=3D NULL) { > - // > - // Check all features dispatched before this entry > - // > - CheckEntry =3D GetFirstNode (FeatureList); > - while (CheckEntry !=3D CurrentEntry) { > - CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > - if (IsBitMaskMatchCheck (CheckFeature->FeatureMask, CpuFeature- > >BeforeFeatureBitMask)) { > - // > - // If there is dependency, swap them > - // > - RemoveEntryList (CurrentEntry); > - InsertTailList (CheckEntry, CurrentEntry); > - Swapped =3D TRUE; > - break; > - } > - CheckEntry =3D CheckEntry->ForwardLink; > - } > - if (Swapped) { > - continue; > - } > - } > - > - if (CpuFeature->AfterFeatureBitMask !=3D NULL) { > - // > - // Check all features dispatched after this entry > - // > - CheckEntry =3D GetNextNode (FeatureList, CurrentEntry); > - while (!IsNull (FeatureList, CheckEntry)) { > - CheckFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry); > - if (IsBitMaskMatchCheck (CheckFeature->FeatureMask, CpuFeature- > >AfterFeatureBitMask)) { > - // > - // If there is dependency, swap them > - // > - TempEntry =3D GetNextNode (FeatureList, CurrentEntry); > - RemoveEntryList (CurrentEntry); > - InsertHeadList (CheckEntry, CurrentEntry); > - CurrentEntry =3D TempEntry; > - Swapped =3D TRUE; > - break; > - } > - CheckEntry =3D CheckEntry->ForwardLink; > - } > - if (Swapped) { > - continue; > - } > - } > - // > - // No swap happened, check the next feature > - // > - CurrentEntry =3D CurrentEntry->ForwardLink; > - } > -} > - > -/** > Worker function to register CPU Feature. >=20 > @param[in] CpuFeature Pointer to CPU feature entry > @@ -334,10 +209,7 @@ RegisterCpuFeatureWorker ( > FreePool (CpuFeature->FeatureMask); > FreePool (CpuFeature); > } > - // > - // Verify CPU features dependency can change CPU feature order > - // > - CheckCpuFeaturesDependency (&CpuFeaturesData->FeatureList); > + > return RETURN_SUCCESS; > } >=20 > -- > 2.10.2.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel