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:[email protected]] > Sent: Thursday, February 1, 2018 9:16 PM > To: Song, BinX <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]> > 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:[email protected]] > >> Sent: Wednesday, January 31, 2018 5:44 PM > >> To: Song, BinX <[email protected]>; [email protected] > >> Cc: Dong, Eric <[email protected]> > >> 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 <[email protected]> > >>> Cc: Laszlo Ersek <[email protected]> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Bell Song <[email protected]> > >>> --- > >>> .../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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
