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.20; helo=mga02.intel.com; envelope-from=binx.song@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 047D0223648B5 for ; Tue, 30 Jan 2018 22:55:10 -0800 (PST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2018 23:00:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,438,1511856000"; d="dat'59?zip'59,48?scan'59,48,208,48,59,223";a="30830158" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga002.jf.intel.com with ESMTP; 30 Jan 2018 23:00:45 -0800 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 30 Jan 2018 23:00:45 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 30 Jan 2018 23:00:44 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.145]) with mapi id 14.03.0319.002; Wed, 31 Jan 2018 15:00:43 +0800 From: "Song, BinX" To: "edk2-devel@lists.01.org" CC: "Dong, Eric" , "lersek@redhat.com" Thread-Topic: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check Thread-Index: AdOaYTQY+pBf08MTTXCTDz1deLfPbw== Date: Wed, 31 Jan 2018 07:00:41 +0000 Message-ID: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> 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: [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 06:55:11 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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); } Solution is to separate current CPU feature dependency check into sort and check two parts. Sort function: According to CPU feature's dependency, sort all CPU features. Later dependency will override previous dependency if they are conflicted. Check function: Check sorted CPU features' relationship, ASSERT invalid relationship. 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(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitializ= e.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: AfterFeatu= reBitMask. + + @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->FeatureNam= e, BeforeFlag ? "before ":"after ", "condition is invalid!\n")); + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", CurrentCp= uFeature->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", C= urrentFeature->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", C= urrentFeature->FeatureName)); + ASSERT (FALSE); + } + CheckEntry =3D CheckEntry->ForwardLink; + } + } + + if (CurrentFeature->BeforeFeatureBitMask !=3D NULL) { + // + // Get correct feature entry in feature list. + // + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, TRUE)= ; + // + // 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", Cur= rentFeature->FeatureName)); + ASSERT (FALSE); + } + } + + if (CurrentFeature->AfterFeatureBitMask !=3D NULL) { + // + // Get correct feature entry in feature list. + // + CheckEntry =3D FindFeatureInList (FeatureList, CurrentFeature, FALSE= ); + // + // 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", C= urrentFeature->FeatureName, CheckFeature->FeatureName)); + DEBUG ((DEBUG_ERROR, "Error: Please check %a relationship!\n", Cur= rentFeature->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 conflicte= d. + + @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 swapped, + // 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, TRU= E); + // + // 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, FAL= SE); + // + // 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/RegisterCpuFeaturesL= ib.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 befor= e 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 after = 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->Be= foreFeatureBitMask)) { - // - // 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->Af= terFeatureBitMask)) { - // - // 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 --=20 2.10.2.windows.1