public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order.
@ 2018-11-07  8:26 Eric Dong
  2018-11-07 13:41 ` Laszlo Ersek
  2018-11-07 15:35 ` Ni, Ruiyu
  0 siblings, 2 replies; 9+ messages in thread
From: Eric Dong @ 2018-11-07  8:26 UTC (permalink / raw)
  To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni

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.

Change-Id: I86171cb1dbf44a2f6fd8d5d2209cafee9451b866
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 .../RegisterCpuFeaturesLib.c                       | 62 ++++++++++++++++++++--
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 394695baf2..31a44b6bad 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -271,6 +271,10 @@ AdjustEntry (
     PreviousEntry = GetNextNode (FeatureList, FindEntry);
   }
 
+  if (PreviousEntry == CurrentEntry) {
+    return;
+  }
+
   CurrentFeature  = CPU_FEATURE_ENTRY_FROM_LINK (CurrentEntry);
   RemoveEntryList (CurrentEntry);
 
@@ -389,6 +393,56 @@ InsertToAfterEntry (
   return Swapped;
 }
 
+/**
+  Checks and adjusts current CPU features per dependency relationship.
+
+  @param[in]  FeatureList        Pointer to CPU feature list
+  @param[in]  CurrentEntry       Pointer to current checked CPU feature
+  @param[in]  FeatureMask        The feature bit mask.
+  @param[in]  Before             The dependence is before type or after type.
+
+  @retval     return Swapped info.
+**/
+BOOLEAN
+AdjustToAllEntry (
+  IN LIST_ENTRY              *FeatureList,
+  IN LIST_ENTRY              *CurrentEntry,
+  IN UINT8                   *FeatureMask,
+  IN BOOLEAN                 Before
+  )
+{
+  LIST_ENTRY                 *CheckEntry;
+  CPU_FEATURES_ENTRY         *CheckFeature;
+  BOOLEAN                    Swapped;
+  LIST_ENTRY                 *NextEntry;
+
+  Swapped = FALSE;
+  //
+  // Record neighbor for later compre.
+  //
+  NextEntry = CurrentEntry->ForwardLink;
+  //
+  // Check all features in current list.
+  //
+  CheckEntry = GetFirstNode (FeatureList);
+  while (!IsNull (FeatureList, CheckEntry)) {
+    CheckFeature = CPU_FEATURE_ENTRY_FROM_LINK (CheckEntry);
+    if (IsBitMaskMatchCheck (CheckFeature->FeatureMask, FeatureMask)) {
+      AdjustEntry (FeatureList, CheckEntry, CurrentEntry, Before);
+      //
+      // Base on former record neighbor to detect whether current entry
+      // adjust the position.
+      // Current Entry position maybe changed in AdjustEntry function.
+      //
+      Swapped = (NextEntry != CurrentEntry->ForwardLink);
+      break;
+    }
+    CheckEntry = CheckEntry->ForwardLink;
+  }
+
+  return Swapped;
+}
+
 /**
   Checks and adjusts CPU features order per dependency relationship.
 
@@ -479,28 +533,28 @@ CheckCpuFeaturesDependency (
     }
 
     if (CpuFeature->CoreBeforeFeatureBitMask != NULL) {
-      Swapped = InsertToBeforeEntry (FeatureList, CurrentEntry, CpuFeature->CoreBeforeFeatureBitMask);
+      Swapped = AdjustToAllEntry (FeatureList, CurrentEntry, CpuFeature->CoreBeforeFeatureBitMask, TRUE);
       if (Swapped) {
         continue;
       }
     }
 
     if (CpuFeature->CoreAfterFeatureBitMask != NULL) {
-      Swapped = InsertToAfterEntry (FeatureList, CurrentEntry, CpuFeature->CoreAfterFeatureBitMask);
+      Swapped = AdjustToAllEntry (FeatureList, CurrentEntry, CpuFeature->CoreAfterFeatureBitMask, FALSE);
       if (Swapped) {
         continue;
       }
     }
 
     if (CpuFeature->PackageBeforeFeatureBitMask != NULL) {
-      Swapped = InsertToBeforeEntry (FeatureList, CurrentEntry, CpuFeature->PackageBeforeFeatureBitMask);
+      Swapped = AdjustToAllEntry (FeatureList, CurrentEntry, CpuFeature->PackageBeforeFeatureBitMask, TRUE);
       if (Swapped) {
         continue;
       }
     }
 
     if (CpuFeature->PackageAfterFeatureBitMask != NULL) {
-      Swapped = InsertToAfterEntry (FeatureList, CurrentEntry, CpuFeature->PackageAfterFeatureBitMask);
+      Swapped = AdjustToAllEntry (FeatureList, CurrentEntry, CpuFeature->PackageAfterFeatureBitMask, FALSE);
       if (Swapped) {
         continue;
       }
-- 
2.15.0.windows.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order.
@ 2018-11-09  5:20 Eric Dong
  2018-11-09 14:55 ` Ni, Ruiyu
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Dong @ 2018-11-09  5:20 UTC (permalink / raw)
  To: edk2-devel; +Cc: Laszlo Ersek, Ruiyu Ni

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 <lersek@redhat.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 .../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



^ permalink raw reply related	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-11-12  9:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-07  8:26 [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order Eric Dong
2018-11-07 13:41 ` Laszlo Ersek
2018-11-07 15:35 ` Ni, Ruiyu
  -- strict thread matches above, loose matches on Subject: below --
2018-11-09  5:20 Eric Dong
2018-11-09 14:55 ` Ni, Ruiyu
2018-11-10  3:16   ` Dong, Eric
2018-11-10 10:00     ` Leif Lindholm
2018-11-11  2:01       ` Dong, Eric
2018-11-12  9:42         ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox