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 02:20:49 -0700 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 5BB3B3082E23; Wed, 22 May 2019 09:20:43 +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 D502D619A9; Wed, 22 May 2019 09:20:41 +0000 (UTC) Subject: Re: [PATCH] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported To: Star Zeng , devel@edk2.groups.io Cc: Eric Dong , Ruiyu Ni , Chandana Kumar , Kevin Li References: <20190522032632.89416-1-star.zeng@intel.com> From: "Laszlo Ersek" Message-ID: <3d2869fd-c968-c47b-0487-bab08ddf25cf@redhat.com> Date: Wed, 22 May 2019 11:20:40 +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: <20190522032632.89416-1-star.zeng@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.46]); Wed, 22 May 2019 09:20:43 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/22/19 05:26, Star Zeng 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. > > BTW: A typo in LmceInitialize is also fixed. > > Change-Id: I32b63ba649fc2977e155181a6263009e359742ed > Cc: Laszlo Ersek > Cc: Eric Dong > Cc: Ruiyu Ni > Cc: Chandana Kumar > Cc: Kevin Li > Signed-off-by: Star Zeng > --- > UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c | 2 +- > UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) (1) Please drop the "Change-Id" line from the commit message. (2) Please update Ray's name and email address in the commit message. (See Maintainers.txt.) (3) This looks like a clear bugfix to me, so I'm fine if it's pushed, after Eric or Ray ACK it. (4) It would be really nice if you could identify in either the commit message or the bugzilla the commit that introduced the issue. If this is a regression fix, then I also suggest adding the Keyword "regression" to TianoCore#1829. 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) || >