From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7A1C1223CCEE9 for ; Thu, 1 Feb 2018 05:10:16 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1AA142821A; Thu, 1 Feb 2018 13:15:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-206.rdu2.redhat.com [10.10.121.206]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1AD09620AB; Thu, 1 Feb 2018 13:15:50 +0000 (UTC) To: "Song, BinX" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" References: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> <9ef96ab1-d50b-5f1a-14ff-43b07562a975@redhat.com> <559D2DF22BC9A3468B4FA1AA547F0EF1025E2848@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <372be680-648a-b67a-98c0-90ec5c1b83c0@redhat.com> Date: Thu, 1 Feb 2018 14:15:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2848@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 01 Feb 2018 13:15:52 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Feb 2018 13:10:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; edk2-devel@lists.01.org >> Cc: Dong, Eric >> 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 >>> Cc: Laszlo Ersek >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Bell Song >>> --- >>> .../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