Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check

["Song, BinX" <binx.song@intel.com>]

Hi Laszlo,

Thanks for your reply, I have also discussed this patch with Eric and Ray, all comments will be in the V2 patch.

Best Regards,
Bell Song


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, February 1, 2018 9:16 PM
> To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check
> 
> On 02/01/18 03:09, Song, BinX wrote:
> > Hi Laszlo,
> >
> > Thanks for your comments.
> > Explain the issue first:
> > In CpuCommonFeaturesLib.inf -> CpuCommonFeaturesLib.c ->
> CpuCommonFeaturesLibConstructor() function,
> > it invokes RegisterCpuFeature() to register CPU feature. Some original
> source codes is here.
> >   if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
> >     Status = RegisterCpuFeature (
> >                "AESNI",
> >                AesniGetConfigData,
> >                AesniSupport,
> >                AesniInitialize,
> >                CPU_FEATURE_AESNI,
> >                CPU_FEATURE_END
> >                );
> >     ASSERT_EFI_ERROR (Status);
> >   }
> >   if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
> >     Status = RegisterCpuFeature (
> >                "MWAIT",
> >                NULL,
> >                MonitorMwaitSupport,
> >                MonitorMwaitInitialize,
> >                CPU_FEATURE_MWAIT,
> >                CPU_FEATURE_END
> >                );
> >     ASSERT_EFI_ERROR (Status);
> >   }
> >
> > Then I update them to below.
> >   if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
> >     Status = RegisterCpuFeature (
> >                "AESNI",
> >                AesniGetConfigData,
> >                AesniSupport,
> >                AesniInitialize,
> >                CPU_FEATURE_AESNI,
> >                CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
> >                CPU_FEATURE_END
> >                );
> >     ASSERT_EFI_ERROR (Status);
> >   }
> >   if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
> >     Status = RegisterCpuFeature (
> >                "MWAIT",
> >                NULL,
> >                MonitorMwaitSupport,
> >                MonitorMwaitInitialize,
> >                CPU_FEATURE_MWAIT,
> >                CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
> >                CPU_FEATURE_END
> >                );
> >     ASSERT_EFI_ERROR (Status);
> >   }
> > Original function CheckCpuFeaturesDependency() will enter a dead loop
> and prompt nothing when checking and sorting them.
> 
> Ah, I see, so the RegisterCpuFeature() call can add before/after hints
> to the features. And circular dependencies cause an infinite loop currently.
> 
> > I think a better way is to detect this conflicted logic and give some hints to
> user, then assert(false).
> >
> > For your three comments.
> > 1. How about change to this?
> >   if (BeforeFlag) {
> >     DEBUG ((DEBUG_ERROR, "Error: Feature %a before condition is invalid!",
> CurrentCpuFeature->FeatureName));
> >   } else {
> >     DEBUG ((DEBUG_ERROR, "Error: Feature %a after condition is invalid!",
> CurrentCpuFeature->FeatureName));
> >   }
> 
> It's OK to do this as well:
> 
>   DEBUG ((
>     DEBUG_ERROR,
>     "Error: Feature %a %a condition is invalid!\n",
>     CurrentCpuFeature->FeatureName,
>     BeforeFlag ? "before" : "after"
>     ));
> 
> > 2. Will update it in V2 patch.
> > 3. How about add a prefix before the name?
> RegisterCpuFeaturesLibSortCpuFeatures() will be unique.
> 
> Sure.
> 
> Thanks!
> Laszlo
> 
> >
> > Best Regards,
> > Bell Song
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, January 31, 2018 5:44 PM
> >> To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> >> Cc: Dong, Eric <eric.dong@intel.com>
> >> Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency
> check
> >>
> >> On 01/31/18 08:00, Song, BinX wrote:
> >>> Current CPU feature dependency check will hang on when meet below
> or
> >>> similar case:
> >>> if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
> >>>   Status = RegisterCpuFeature (
> >>>              "AESNI",
> >>>              AesniGetConfigData,
> >>>              AesniSupport,
> >>>              AesniInitialize,
> >>>              CPU_FEATURE_AESNI,
> >>>              CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
> >>>              CPU_FEATURE_END
> >>>              );
> >>>   ASSERT_EFI_ERROR (Status);
> >>> }
> >>> if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
> >>>   Status = RegisterCpuFeature (
> >>>              "MWAIT",
> >>>              NULL,
> >>>              MonitorMwaitSupport,
> >>>              MonitorMwaitInitialize,
> >>>              CPU_FEATURE_MWAIT,
> >>>              CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
> >>>              CPU_FEATURE_END
> >>>              );
> >>>   ASSERT_EFI_ERROR (Status);
> >>> }
> >>>
> >>> Solution is to separate current CPU feature dependency check into
> >>> sort and check two parts.
> >>>
> >>> Sort function:
> >>> According to CPU feature's dependency, sort all CPU features.
> >>> Later dependency will override previous dependency if they are
> conflicted.
> >>>
> >>> Check function:
> >>> Check sorted CPU features' relationship, ASSERT invalid relationship.
> >>>
> >>> Cc: Eric Dong <eric.dong@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Bell Song <binx.song@intel.com>
> >>> ---
> >>>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271
> >> ++++++++++++++++++++-
> >>>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   7 +
> >>>  .../RegisterCpuFeaturesLib.c                       | 130 +---------
> >>>  3 files changed, 278 insertions(+), 130 deletions(-)
> >>>
> >>> diff --git
> >> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> >> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> >>> index 4d75c07..2fd0d5f 100644
> >>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> >>> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> >>> @@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor (
> >>>  }
> >>>
> >>>  /**
> >>> +  From FeatureBitMask, find the right feature entry in CPU feature list.
> >>> +
> >>> +  @param[in]  FeatureList    The pointer to CPU feature list.
> >>> +  @param[in]  CurrentFeature The pointer to current CPU feature.
> >>> +  @param[in]  BeforeFlag     TRUE: BeforeFeatureBitMask; FALSE:
> >> AfterFeatureBitMask.
> >>> +
> >>> +  @return  The pointer to right CPU feature entry.
> >>> +**/
> >>> +LIST_ENTRY *
> >>> +FindFeatureInList(
> >>> +  IN LIST_ENTRY              *CpuFeatureList,
> >>> +  IN CPU_FEATURES_ENTRY      *CurrentCpuFeature,
> >>> +  IN BOOLEAN                  BeforeFlag
> >>> +  )
> >>> +{
> >>> +  LIST_ENTRY                 *TempEntry;
> >>> +  CPU_FEATURES_ENTRY         *TempFeature;
> >>> +  UINT8                      *FeatureBitMask;
> >>> +
> >>> +  FeatureBitMask = BeforeFlag ? CurrentCpuFeature-
> >>> BeforeFeatureBitMask : CurrentCpuFeature->AfterFeatureBitMask;
> >>> +  TempEntry = GetFirstNode (CpuFeatureList);
> >>> +  while (!IsNull (CpuFeatureList, TempEntry)) {
> >>> +    TempFeature = CPU_FEATURE_ENTRY_FROM_LINK (TempEntry);
> >>> +    if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature-
> >>> FeatureMask)){
> >>> +      return TempEntry;
> >>> +    }
> >>> +    TempEntry = TempEntry->ForwardLink;
> >>> +  }
> >>> +
> >>> +  DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature-
> >>> FeatureName, BeforeFlag ? "before ":"after ", "condition is invalid!\n"));
> >>
> >> Hi, I skimmed this patch quickly -- I can tell that I can't really tell
> >> what's going on. I don't know how the feature dependencies are defined
> >> in the first place, and what the bug is.
> >>
> >> However, I do see that the above DEBUG macro invocation is incorrect.
> >> The format string has one (1) %a conversion specification, but we pass
> >> three (3) arguments.
> >>
> >> I think the last argument ("condition is invalid!\n") should actually be
> >> part of the format string. And then, the "before"/"after" string has to
> >> be printed somehow as well.
> >>
> >> Another superficial observation below:
> >>
> >>> +/**
> >>> +  Check sorted CPU features' relationship, ASSERT invalid one.
> >>> +
> >>> +  @param[in]  FeatureList  The pointer to CPU feature list.
> >>> +**/
> >>> +VOID
> >>> +CheckCpuFeaturesRelationShip (
> >>
> >> I don't think we should capitalize "Ship" in this identifier.
> >>
> >> Third comment: there are several ways to define "sorting", so I'm not
> >> sure my question applies, but: can we replace the manual sorting with
> >> SortLib?
> >>
> >> Thanks
> >> Laszlo