From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 22 May 2019 03:32:36 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EE0603079B86; Wed, 22 May 2019 10:32:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-230.rdu2.redhat.com [10.10.120.230]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4724E608C2; Wed, 22 May 2019 10:32:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported To: devel@edk2.groups.io, star.zeng@intel.com Cc: Eric Dong , Ray Ni , Chandana Kumar , Kevin Li References: <20190522101751.89952-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 22 May 2019 12:32:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190522101751.89952-1-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Wed, 22 May 2019 10:32:33 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/22/19 12:17, Zeng, Star wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1829 > > There will be ASSERT if LMCE is supported as below. > DXE_ASSERT!: [CpuFeaturesDxe] > XXX\UefiCpuPkg\Library\CpuCommonFeaturesLib\MachineCheck.c (342): > ConfigData != ((void *) 0) > > The code should get Config Data and FeatureControlGetConfigData > could be used. > > This issue is there since the code was added at the commit below. > > Revision: 3d6275c1137c9633ce24e31522b71105367bd6a0 > Date: 2017/8/4 8:46:41 > UefiCpuPkg CpuCommonFeaturesLib: Enable LMCE feature. > > The commits below are also related to move the code. > > Revision: 023387144299741d727521b425ef443438aecc1f > Date: 2017/9/1 10:12:38 > UefiCpuPkg/Lmce.c Remove useless file. > > Revision: 306a5bcc6b0170d28b0db10bd359817bb4b1db9f > Date: 2017/8/17 11:40:38 > UefiCpuPkg/CpuCommonFeaturesLib: Merge machine check code to same file. > > So, the code should not be tested at all on a platform > that supports LMCE. > > BTW: A typo in LmceInitialize is also fixed. > The typo is introduced by the commit below. > > Revision: d28daaddb3e732468e930a809d3d3943a5de9558 > Date: 2018/10/17 9:24:05 > UefiCpuPkg/CpuCommonFeaturesLib: Register MSR base on scope Info. > > Cc: Laszlo Ersek > Cc: Eric Dong > Cc: Ray Ni > Cc: Chandana Kumar > Cc: Kevin Li > Signed-off-by: Star Zeng > > Notes: > v2: Based on Laszlo's great feedback. > - Update Ray's name and email address to match Maintainers.txt. > - Remove "Change-Id" line that was added unintended. > - Add more code history. > > --- [*] > UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c | 2 +- > UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Thanks for the update. Just one more quick note: - if you want to include the v2 changes in the commit message, then please describe them above your Signed-off-by (in the normal commit message body). - If you don't want the v2 notes to be present in the final commit message, then please add them between the "---" mark, and the diffstat. I marked the location above with [*]. Then "git am" will automatically ignore the notes. No need to resubmit now, it's just a hint for the future. Thanks Laszlo > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > index 738b57dc87f9..9ddc6ce9d476 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > @@ -214,7 +214,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_LMCE)) { > Status = RegisterCpuFeature ( > "LMCE", > - NULL, > + FeatureControlGetConfigData, > LmceSupport, > LmceInitialize, > CPU_FEATURE_LMCE, > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > index 9ee559130080..2528e0044ecb 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > @@ -322,7 +322,7 @@ LmceInitialize ( > MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > > // > - // The scope of FastStrings bit in the MSR_IA32_MISC_ENABLE is core for below processor type, only program > + // The scope of LcmeOn bit in the MSR_IA32_MISC_ENABLE is core for below processor type, only program > // MSR_IA32_MISC_ENABLE for thread 0 in each core. > // > if (IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo->DisplayModel) || >