Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order.

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Eric,
Thanks for fixing the new found issues.

Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Eric
> Dong
> Sent: Friday, November 9, 2018 1:21 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> 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 <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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel