public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when the pair is invalid"
@ 2018-09-25  5:22 Ruiyu Ni
  2018-09-25  8:14 ` Dong, Eric
  0 siblings, 1 reply; 2+ messages in thread
From: Ruiyu Ni @ 2018-09-25  5:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Michael D Kinney, Eric Dong, Sean Brogan

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

The patch reverts 9c8c4478cfcacaf5fd60b75ff78d26732d93a5b8
"UefiCpuPkg/MtrrLib: Skip Base MSR access when the pair is invalid".

Microsoft Windows will report an error in event manager if MTRR
usage is different across hibernate even when the difference is
in an non valid MTRR pair. This seems like a bug in Windows but
for compatibility and servicing reasons we think a change in UEFI
would wise.
A Windows change has already been submitted for the next iteration
(2019 time frame).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index dfce9a996b..086f7ad8f0 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -449,13 +449,10 @@ MtrrGetVariableMtrrWorker (
 
   for (Index = 0; Index < VariableMtrrCount; Index++) {
     if (MtrrSetting == NULL) {
-      VariableSettings->Mtrr[Index].Mask = AsmReadMsr64 (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1));
-      //
-      // Skip to read the Base MSR when the Mask.V is not set.
-      //
-      if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&VariableSettings->Mtrr[Index].Mask)->Bits.V != 0) {
-        VariableSettings->Mtrr[Index].Base = AsmReadMsr64 (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1));
-      }
+      VariableSettings->Mtrr[Index].Base =
+        AsmReadMsr64 (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1));
+      VariableSettings->Mtrr[Index].Mask =
+        AsmReadMsr64 (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1));
     } else {
       VariableSettings->Mtrr[Index].Base = MtrrSetting->Variables.Mtrr[Index].Base;
       VariableSettings->Mtrr[Index].Mask = MtrrSetting->Variables.Mtrr[Index].Mask;
@@ -2604,14 +2601,14 @@ MtrrSetVariableMtrrWorker (
   ASSERT (VariableMtrrCount <= ARRAY_SIZE (VariableSettings->Mtrr));
 
   for (Index = 0; Index < VariableMtrrCount; Index++) {
-    //
-    // Mask MSR is always updated since caller might need to invalidate the MSR pair.
-    // Base MSR is skipped when Mask.V is not set.
-    //
-    AsmWriteMsr64 (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1), VariableSettings->Mtrr[Index].Mask);
-    if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&VariableSettings->Mtrr[Index].Mask)->Bits.V != 0) {
-      AsmWriteMsr64 (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1), VariableSettings->Mtrr[Index].Base);
-    }
+    AsmWriteMsr64 (
+      MSR_IA32_MTRR_PHYSBASE0 + (Index << 1),
+      VariableSettings->Mtrr[Index].Base
+      );
+    AsmWriteMsr64 (
+      MSR_IA32_MTRR_PHYSMASK0 + (Index << 1),
+      VariableSettings->Mtrr[Index].Mask
+      );
   }
 }
 
@@ -2868,7 +2865,7 @@ MtrrDebugPrintAllMtrrsWorker (
     }
     ContainVariableMtrr = FALSE;
     for (Index = 0; Index < VariableMtrrCount; Index++) {
-      if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) {
+      if ((Mtrrs->Variables.Mtrr[Index].Mask & BIT11) == 0) {
         //
         // If mask is not valid, then do not display range
         //
-- 
2.16.1.windows.1



^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when the pair is invalid"
  2018-09-25  5:22 [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when the pair is invalid" Ruiyu Ni
@ 2018-09-25  8:14 ` Dong, Eric
  0 siblings, 0 replies; 2+ messages in thread
From: Dong, Eric @ 2018-09-25  8:14 UTC (permalink / raw)
  To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Kinney, Michael D

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Tuesday, September 25, 2018 1:22 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when
> the pair is invalid"
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1187
> 
> The patch reverts 9c8c4478cfcacaf5fd60b75ff78d26732d93a5b8
> "UefiCpuPkg/MtrrLib: Skip Base MSR access when the pair is invalid".
> 
> Microsoft Windows will report an error in event manager if MTRR usage is
> different across hibernate even when the difference is in an non valid MTRR
> pair. This seems like a bug in Windows but for compatibility and servicing
> reasons we think a change in UEFI would wise.
> A Windows change has already been submitted for the next iteration
> (2019 time frame).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index dfce9a996b..086f7ad8f0 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -449,13 +449,10 @@ MtrrGetVariableMtrrWorker (
> 
>    for (Index = 0; Index < VariableMtrrCount; Index++) {
>      if (MtrrSetting == NULL) {
> -      VariableSettings->Mtrr[Index].Mask = AsmReadMsr64
> (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1));
> -      //
> -      // Skip to read the Base MSR when the Mask.V is not set.
> -      //
> -      if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&VariableSettings-
> >Mtrr[Index].Mask)->Bits.V != 0) {
> -        VariableSettings->Mtrr[Index].Base = AsmReadMsr64
> (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1));
> -      }
> +      VariableSettings->Mtrr[Index].Base =
> +        AsmReadMsr64 (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1));
> +      VariableSettings->Mtrr[Index].Mask =
> +        AsmReadMsr64 (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1));
>      } else {
>        VariableSettings->Mtrr[Index].Base = MtrrSetting-
> >Variables.Mtrr[Index].Base;
>        VariableSettings->Mtrr[Index].Mask = MtrrSetting-
> >Variables.Mtrr[Index].Mask;
> @@ -2604,14 +2601,14 @@ MtrrSetVariableMtrrWorker (
>    ASSERT (VariableMtrrCount <= ARRAY_SIZE (VariableSettings->Mtrr));
> 
>    for (Index = 0; Index < VariableMtrrCount; Index++) {
> -    //
> -    // Mask MSR is always updated since caller might need to invalidate the
> MSR pair.
> -    // Base MSR is skipped when Mask.V is not set.
> -    //
> -    AsmWriteMsr64 (MSR_IA32_MTRR_PHYSMASK0 + (Index << 1),
> VariableSettings->Mtrr[Index].Mask);
> -    if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&VariableSettings-
> >Mtrr[Index].Mask)->Bits.V != 0) {
> -      AsmWriteMsr64 (MSR_IA32_MTRR_PHYSBASE0 + (Index << 1),
> VariableSettings->Mtrr[Index].Base);
> -    }
> +    AsmWriteMsr64 (
> +      MSR_IA32_MTRR_PHYSBASE0 + (Index << 1),
> +      VariableSettings->Mtrr[Index].Base
> +      );
> +    AsmWriteMsr64 (
> +      MSR_IA32_MTRR_PHYSMASK0 + (Index << 1),
> +      VariableSettings->Mtrr[Index].Mask
> +      );
>    }
>  }
> 
> @@ -2868,7 +2865,7 @@ MtrrDebugPrintAllMtrrsWorker (
>      }
>      ContainVariableMtrr = FALSE;
>      for (Index = 0; Index < VariableMtrrCount; Index++) {
> -      if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs-
> >Variables.Mtrr[Index].Mask)->Bits.V == 0) {
> +      if ((Mtrrs->Variables.Mtrr[Index].Mask & BIT11) == 0) {
>          //
>          // If mask is not valid, then do not display range
>          //
> --
> 2.16.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-09-25  8:14 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-25  5:22 [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when the pair is invalid" Ruiyu Ni
2018-09-25  8:14 ` Dong, Eric

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox