[PATCH v1 0/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

[PATCH v1 0/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

[Takuto Naito]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3325

This patch fixes the problems of AsmReadMsr64/AsmWriteMsr64 for the GCC toolchain
introduced when RegisterFilterLib support was added.

Patch v1 branch: https://github.com/naitaku/edk2/tree/patch/fix_read_msr_with_gcc

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>

Takuto Naito (1):
  Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

 MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH v1 1/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

[Takuto Naito]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3325

1. AsmReadMsr64() in X64/GccInlinePriv.c
AsmReadMsr64 can return uninitialized value if FilterBeforeMsrRead
returns False. This causes build error with the CLANG toolchain.

2. AsmWriteMsr64() in X64/GccInlinePriv.c
In the case that FilterBeforeMsrWrite changes Value and returns True,
The original Value, not the changed Value, is written to the MSR.
This behavior is different from the one of AsmWriteMsr64() in
X64/WriteMsr64.c for the MSFT toolchain.

Signed-off-by: Takuto Naito <naitaku@gmail.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
index e4920f2116..244bd62ee6 100644
--- a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
+++ b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
@@ -80,7 +80,7 @@ AsmReadMsr64 (
   }
   FilterAfterMsrRead (Index, &Value);
 
-  return (((UINT64)HighData) << 32) | LowData;
+  return Value;
 }
 
 /**
@@ -111,11 +111,10 @@ AsmWriteMsr64 (
   UINT32 HighData;
   BOOLEAN Flag;
 
-  LowData  = (UINT32)(Value);
-  HighData = (UINT32)(Value >> 32);
-
   Flag = FilterBeforeMsrWrite (Index, &Value);
   if (Flag) {
+    LowData  = (UINT32)(Value);
+    HighData = (UINT32)(Value >> 32);
     __asm__ __volatile__ (
       "wrmsr"
       :
-- 
2.31.1

回复: [edk2-devel] [PATCH v1 1/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

[gaoliming]

Naito:
  The fix is correct. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

Thanks
Liming 
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Takuto Naito
> 发送时间: 2021年4月12日 23:07
> 收件人: devel@edk2.groups.io
> 抄送: Takuto Naito <naitaku@gmail.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>
> 主题: [edk2-devel] [PATCH v1 1/1] Fix AsmReadMsr64() and AsmWriteMsr64()
> with GCC toolchain
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3325
> 
> 1. AsmReadMsr64() in X64/GccInlinePriv.c
> AsmReadMsr64 can return uninitialized value if FilterBeforeMsrRead
> returns False. This causes build error with the CLANG toolchain.
> 
> 2. AsmWriteMsr64() in X64/GccInlinePriv.c
> In the case that FilterBeforeMsrWrite changes Value and returns True,
> The original Value, not the changed Value, is written to the MSR.
> This behavior is different from the one of AsmWriteMsr64() in
> X64/WriteMsr64.c for the MSFT toolchain.
> 
> Signed-off-by: Takuto Naito <naitaku@gmail.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> index e4920f2116..244bd62ee6 100644
> --- a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> +++ b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> @@ -80,7 +80,7 @@ AsmReadMsr64 (
>    }
>    FilterAfterMsrRead (Index, &Value);
> 
> -  return (((UINT64)HighData) << 32) | LowData;
> +  return Value;
>  }
> 
>  /**
> @@ -111,11 +111,10 @@ AsmWriteMsr64 (
>    UINT32 HighData;
>    BOOLEAN Flag;
> 
> -  LowData  = (UINT32)(Value);
> -  HighData = (UINT32)(Value >> 32);
> -
>    Flag = FilterBeforeMsrWrite (Index, &Value);
>    if (Flag) {
> +    LowData  = (UINT32)(Value);
> +    HighData = (UINT32)(Value >> 32);
>      __asm__ __volatile__ (
>        "wrmsr"
>        :
> --
> 2.31.1
> 
> 
> 
> 
> 

回复: [edk2-devel] [PATCH v1 1/1] Fix AsmReadMsr64() and AsmWriteMsr64() with GCC toolchain

[gaoliming]

Create PR https://github.com/tianocore/edk2/pull/1562 for it. 

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 gaoliming
> 发送时间: 2021年4月13日 9:16
> 收件人: devel@edk2.groups.io; naitaku@gmail.com
> 抄送: 'Michael D Kinney' <michael.d.kinney@intel.com>; 'Zhiguang Liu'
> <zhiguang.liu@intel.com>
> 主题: 回复: [edk2-devel] [PATCH v1 1/1] Fix AsmReadMsr64() and
> AsmWriteMsr64() with GCC toolchain
> 
> Naito:
>   The fix is correct. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> 
> Thanks
> Liming
> > -----邮件原件-----
> > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Takuto
> Naito
> > 发送时间: 2021年4月12日 23:07
> > 收件人: devel@edk2.groups.io
> > 抄送: Takuto Naito <naitaku@gmail.com>; Michael D Kinney
> > <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> > Zhiguang Liu <zhiguang.liu@intel.com>
> > 主题: [edk2-devel] [PATCH v1 1/1] Fix AsmReadMsr64() and
> AsmWriteMsr64()
> > with GCC toolchain
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3325
> >
> > 1. AsmReadMsr64() in X64/GccInlinePriv.c
> > AsmReadMsr64 can return uninitialized value if FilterBeforeMsrRead
> > returns False. This causes build error with the CLANG toolchain.
> >
> > 2. AsmWriteMsr64() in X64/GccInlinePriv.c
> > In the case that FilterBeforeMsrWrite changes Value and returns True,
> > The original Value, not the changed Value, is written to the MSR.
> > This behavior is different from the one of AsmWriteMsr64() in
> > X64/WriteMsr64.c for the MSFT toolchain.
> >
> > Signed-off-by: Takuto Naito <naitaku@gmail.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >  MdePkg/Library/BaseLib/X64/GccInlinePriv.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> > b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> > index e4920f2116..244bd62ee6 100644
> > --- a/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> > +++ b/MdePkg/Library/BaseLib/X64/GccInlinePriv.c
> > @@ -80,7 +80,7 @@ AsmReadMsr64 (
> >    }
> >    FilterAfterMsrRead (Index, &Value);
> >
> > -  return (((UINT64)HighData) << 32) | LowData;
> > +  return Value;
> >  }
> >
> >  /**
> > @@ -111,11 +111,10 @@ AsmWriteMsr64 (
> >    UINT32 HighData;
> >    BOOLEAN Flag;
> >
> > -  LowData  = (UINT32)(Value);
> > -  HighData = (UINT32)(Value >> 32);
> > -
> >    Flag = FilterBeforeMsrWrite (Index, &Value);
> >    if (Flag) {
> > +    LowData  = (UINT32)(Value);
> > +    HighData = (UINT32)(Value >> 32);
> >      __asm__ __volatile__ (
> >        "wrmsr"
> >        :
> > --
> > 2.31.1
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
>