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

["Dong, Eric" <eric.dong@intel.com>]

Hi Leif,

Thanks for your comments. Because I must check in the code today, I will change the code when I push the change.

Thanks,
Eric

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Saturday, November 10, 2018 6:00 PM
> To: Dong, Eric <eric.dong@intel.com>
> Cc: Laszlo Ersek (lersek@redhat.com) <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; afish@apple.com; Ni, Ruiyu
> <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order.
> 
> 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/RegisterCpuFeature
> > > > +++ s.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/RegisterCpuFeature
> > > > +++ sLib
> > > > +++ .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