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 <[email protected]>; [email protected] > Cc: Laszlo Ersek <[email protected]> > Subject: RE: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. > > Eric, > Thanks for fixing the new found issues. > > Reviewed-by: Ruiyu Ni <[email protected]> > > > -----Original Message----- > > From: edk2-devel [mailto:[email protected]] On Behalf Of > > Eric Dong > > Sent: Friday, November 9, 2018 1:21 PM > > To: [email protected] > > Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek <[email protected]> > > Subject: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. > > > > 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 <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <[email protected]> > > --- > > .../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 > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
