From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=binx.song@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (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 CC46122393648 for ; Wed, 31 Jan 2018 18:03:51 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jan 2018 18:09:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,442,1511856000"; d="scan'208";a="31071095" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 31 Jan 2018 18:09:27 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 31 Jan 2018 18:09:27 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 31 Jan 2018 18:09:26 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.145]) with mapi id 14.03.0319.002; Thu, 1 Feb 2018 10:09:24 +0800 From: "Song, BinX" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Dong, Eric" Thread-Topic: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check Thread-Index: AdOaYTQY+pBf08MTTXCTDz1deLfPb///p5kA//5t4gA= Date: Thu, 1 Feb 2018 02:09:24 +0000 Message-ID: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2848@shsmsx102.ccr.corp.intel.com> References: <559D2DF22BC9A3468B4FA1AA547F0EF1025E2535@shsmsx102.ccr.corp.intel.com> <9ef96ab1-d50b-5f1a-14ff-43b07562a975@redhat.com> In-Reply-To: <9ef96ab1-d50b-5f1a-14ff-43b07562a975@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 02:03:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, Thanks for your comments. Explain the issue first: In CpuCommonFeaturesLib.inf -> CpuCommonFeaturesLib.c -> CpuCommonFeaturesL= ibConstructor() function, it invokes RegisterCpuFeature() to register CPU feature. Some original sour= ce codes is here. if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) { Status =3D RegisterCpuFeature ( "AESNI", AesniGetConfigData, AesniSupport, AesniInitialize, CPU_FEATURE_AESNI, CPU_FEATURE_END ); ASSERT_EFI_ERROR (Status); } if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) { Status =3D 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 =3D 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 =3D 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 p= rompt nothing when checking and sorting them. 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!", C= urrentCpuFeature->FeatureName)); } 2. Will update it in V2 patch. 3. How about add a prefix before the name? RegisterCpuFeaturesLibSortCpuFea= tures() will be unique. 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 >=20 > 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 =3D 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 =3D 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 conflict= ed. > > > > 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 lis= t. > > + > > + @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 =3D BeforeFlag ? CurrentCpuFeature- > >BeforeFeatureBitMask : CurrentCpuFeature->AfterFeatureBitMask; > > + TempEntry =3D GetFirstNode (CpuFeatureList); > > + while (!IsNull (CpuFeatureList, TempEntry)) { > > + TempFeature =3D CPU_FEATURE_ENTRY_FROM_LINK (TempEntry); > > + if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature- > >FeatureMask)){ > > + return TempEntry; > > + } > > + TempEntry =3D TempEntry->ForwardLink; > > + } > > + > > + DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature- > >FeatureName, BeforeFlag ? "before ":"after ", "condition is invalid!\n")= ); >=20 > 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. >=20 > 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. >=20 > 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. >=20 > Another superficial observation below: >=20 > > +/** > > + Check sorted CPU features' relationship, ASSERT invalid one. > > + > > + @param[in] FeatureList The pointer to CPU feature list. > > +**/ > > +VOID > > +CheckCpuFeaturesRelationShip ( >=20 > I don't think we should capitalize "Ship" in this identifier. >=20 > 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? >=20 > Thanks > Laszlo