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.126; helo=mga18.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 B3DBA2117CE9F for ; Thu, 8 Nov 2018 21:20:45 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Nov 2018 21:20:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,482,1534834800"; d="scan'208";a="272619003" Received: from ydong10-win10.ccr.corp.intel.com ([10.239.9.125]) by orsmga005.jf.intel.com with ESMTP; 08 Nov 2018 21:20:43 -0800 From: Eric Dong To: edk2-devel@lists.01.org Cc: Laszlo Ersek , Ruiyu Ni Date: Fri, 9 Nov 2018 13:20:41 +0800 Message-Id: <20181109052041.14816-1-eric.dong@intel.com> X-Mailer: git-send-email 2.15.0.windows.1 Subject: [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 05:20:45 -0000 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: 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/CpuFeaturesInitialize.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); +} + /** 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 = GetCpuFeaturesData (); CpuFeaturesData->CapabilityPcd = AllocatePool (CpuFeaturesData->BitMaskSize); @@ -634,14 +658,9 @@ AnalysisProcessorFeatures ( // CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; Entry = GetFirstNode (&CpuInitOrder->OrderList); - NextEntry = Entry->ForwardLink; while (!IsNull (&CpuInitOrder->OrderList, Entry)) { CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry); - if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { - NextCpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (NextEntry); - } else { - NextCpuFeatureInOrder = NULL; - } + Success = FALSE; if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask, CpuFeaturesData->SettingPcd)) { Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber, CpuInfo, CpuFeatureInOrder->ConfigData, TRUE); @@ -674,31 +693,43 @@ AnalysisProcessorFeatures ( } 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 != NULL) { + NextEntry = Entry->ForwardLink; + if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { + NextCpuFeatureInOrder = 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 = DetectFeatureScope (CpuFeatureInOrder, TRUE, NextCpuFeatureInOrder->FeatureMask); AfterDep = DetectFeatureScope (NextCpuFeatureInOrder, FALSE, CpuFeatureInOrder->FeatureMask); + // + // Check whether next feature has After type dependence with not neighborhood CPU + // Features in former CPU features. + // + NoneNeibAfterDep = DetectNoneNeighborhoodFeatureScope(NextCpuFeatureInOrder, FALSE, &CpuInitOrder->OrderList); } else { - BeforeDep = DetectFeatureScope (CpuFeatureInOrder, TRUE, NULL); - AfterDep = NoneDepType; + BeforeDep = NoneDepType; + AfterDep = NoneDepType; + NoneNeibAfterDep = 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 = DetectNoneNeighborhoodFeatureScope(CpuFeatureInOrder, TRUE, &CpuInitOrder->OrderList); + + // + // Get the biggest dependence and add semaphore for it. + // PackageDepType > CoreDepType > ThreadDepType > NoneDepType. + // + BeforeDep = 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 = Entry->ForwardLink; - NextEntry = Entry->ForwardLink; + Entry = 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/RegisterCpuFeatures.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 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. 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; } +/** + 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 features. + @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 = CurrentEntry->BackLink; + if (IsNull (FeatureList, NextEntry)) { + return FALSE; + } + + NextEntry = NextEntry->BackLink; + if (IsNull (FeatureList, NextEntry)) { + return FALSE; + } + + NextEntry = CurrentEntry->BackLink->BackLink; + } else { + NextEntry = CurrentEntry->ForwardLink; + if (IsNull (FeatureList, NextEntry)) { + return FALSE; + } + + NextEntry = NextEntry->ForwardLink; + if (IsNull (FeatureList, NextEntry)) { + return FALSE; + } + + NextEntry = CurrentEntry->ForwardLink->ForwardLink; + } + + while (!IsNull (FeatureList, NextEntry)) { + CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (NextEntry); + + if (IsBitMaskMatchCheck (FeatureMask, CpuFeature->FeatureMask)) { + return TRUE; + } + + if (SearchFormer) { + NextEntry = NextEntry->BackLink; + } else { + NextEntry = 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 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 != NULL) && + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, CpuFeature->PackageBeforeFeatureBitMask)) { + return PackageDepType; + } + + if ((CpuFeature->CoreBeforeFeatureBitMask != NULL) && + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, CpuFeature->CoreBeforeFeatureBitMask)) { + return CoreDepType; + } + + if ((CpuFeature->BeforeFeatureBitMask != NULL) && + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, CpuFeature->BeforeFeatureBitMask)) { + return ThreadDepType; + } + + return NoneDepType; + } + + if ((CpuFeature->PackageAfterFeatureBitMask != NULL) && + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, CpuFeature->PackageAfterFeatureBitMask)) { + return PackageDepType; + } + + if ((CpuFeature->CoreAfterFeatureBitMask != NULL) && + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, CpuFeature->CoreAfterFeatureBitMask)) { + return CoreDepType; + } + + if ((CpuFeature->AfterFeatureBitMask != 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