Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported

["Zeng, Star" <star.zeng@intel.com>]

Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, May 22, 2019 6:32 PM
> To: devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Li, Kevin Y
> <kevin.y.li@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix
> ASSERT if LMCE is supported
> 
> 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 <lersek@redhat.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> > Cc: Kevin Li <kevin.y.li@intel.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> >
> > 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.

Yeah, it is great to learn this.

Thanks,
Star

> 
> 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) ||
> >