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


Zeng, Star
 

Laszlo,

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@...]
Sent: Wednesday, May 22, 2019 6:32 PM
To: devel@edk2.groups.io; Zeng, Star <star.zeng@...>
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Kumar,
Chandana C <chandana.c.kumar@...>; Li, Kevin Y
<kevin.y.li@...>
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@...>
Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Chandana Kumar <chandana.c.kumar@...>
Cc: Kevin Li <kevin.y.li@...>
Signed-off-by: Star Zeng <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.
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) ||

Join devel@edk2.groups.io to automatically receive all group messages.