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 8E4AD2215BD9C for ; Wed, 31 Jan 2018 01:38:43 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 788D23D943; Wed, 31 Jan 2018 09:44:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id 83A4060C96; Wed, 31 Jan 2018 09:44:17 +0000 (UTC) To: "Song, BinX" , "edk2-devel@lists.01.org" Cc: "Dong, Eric" References: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <9ef96ab1-d50b-5f1a-14ff-43b07562a975@redhat.com> Date: Wed, 31 Jan 2018 10:44:16 +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: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 31 Jan 2018 09:44:18 +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: Wed, 31 Jan 2018 09:38:44 -0000 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit 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