Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported
Reviewed-by: Eric Dong
> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, May 22, 2019 6:18 PM
> To: devel@edk2.groups.io
> Cc: Zeng, Star ; Laszlo Ersek ;
> Dong, Eric ; Ni, Ray ; Kumar,
> Chandana C ; Li, Kevin Y
>
> Subject: [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is
> supported
>
> 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(-)
>
> 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) ||
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41296): https://edk2.groups.io/g/devel/message/41296
Mute This Topic: https://groups.io/mt/31718348/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported
Laszlo,
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, May 22, 2019 6:32 PM
> To: devel@edk2.groups.io; Zeng, Star
> Cc: Dong, Eric ; Ni, Ray ; Kumar,
> Chandana C ; Li, Kevin Y
>
> 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
> > 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.
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) ||
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41256): https://edk2.groups.io/g/devel/message/41256
Mute This Topic: https://groups.io/mt/31718348/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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
> 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) ||
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41224): https://edk2.groups.io/g/devel/message/41224
Mute This Topic: https://groups.io/mt/31718348/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH V2] UefiCpuPkg CpuCommFeaturesLib: Fix ASSERT if LMCE is supported
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(-)
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)
||
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41222): https://edk2.groups.io/g/devel/message/41222
Mute This Topic: https://groups.io/mt/31718348/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
