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

[Leif Lindholm <leif.lindholm@linaro.org>]

Hi Eric,

I have no concerns over pushing this bugfix.

However, having lookes at it, I do have a couple of style comments.

On Sat, Nov 10, 2018 at 03:16:26AM +0000, Dong, Eric wrote:
> 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 <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Subject: RE: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order.
> > 
> > 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:

The patch version information does not belong in the patch
description. Please include it under "---" and make the description
refer only to what the patch does.

> > > 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);

And please make use of some temporary variables here.
It will make no difference to code generation in any compiler designed
since 1995, but will make it human readable.

/
    Leif

> > > +}
> > > +
> > >  /**
> > >    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