Re: [PATCH] UefiCpuPkg/MtrrLib: Revert "Skip MSR access when the pair is invalid"

["Dong, Eric" <eric.dong@intel.com>]

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