Re: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set memory <1MB to UC

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

Hi Ray,

Sorry for later response.  

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

Thanks,
Eric
> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, January 21, 2019 11:17 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Chiu,
> Chasel <chasel.chiu@intel.com>
> Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set
> memory <1MB to UC
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1481
> 
> Today's MtrrLib contains a bug, for example:
>  when the original cache setting is WB for [0xF_0000, 0xF_8000) and,  a new
> request to set [0xF_0000, 0xF_4000) to WP,  the cache setting for [0xF_4000,
> 0xF_8000) is reset to UC.
> 
> The reason is when MtrrLibSetBelow1MBMemoryAttribute() is called the
> WorkingFixedSettings doesn't contain the actual MSR value stored in
> hardware, but when writing the fixed MTRRs, the code logic assumes
> WorkingFixedSettings contains the actual MSR value.
> 
> The new fix is to change MtrrLibSetBelow1MBMemoryAttribute() to
> calculate the correct ClearMasks[] and OrMasks[], and use them directly
> when writing the fixed MTRRs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 59 +++++++++-------------------
>  1 file changed, 18 insertions(+), 41 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> index 086f7ad8f0..2cf7d092e8 100644
> --- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> +++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
> @@ -5,7 +5,7 @@
>      Most of services in this library instance are suggested to be invoked by BSP
> only,
>      except for MtrrSetAllMtrrs() which is used to sync BSP's MTRR setting to
> APs.
> 
> -  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2008 - 2019, Intel Corporation. All rights
> + reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at @@ -2099,8 +2099,8 @@ MtrrLibSetMemoryRanges (
>    Set the below-1MB memory attribute to fixed MTRR buffer.
>    Modified flag array indicates which fixed MTRR is modified.
> 
> -  @param [in, out] FixedSettings Fixed MTRR buffer.
> -  @param [out]     Modified      Flag array indicating which MTRR is modified.
> +  @param [in, out] ClearMasks    The bits to clear in the fixed MTRR MSR.
> +  @param [in, out] OrMasks       The bits to set in the fixed MTRR MSR.
>    @param [in]      BaseAddress   Base address.
>    @param [in]      Length        Length.
>    @param [in]      Type          Memory type.
> @@ -2111,8 +2111,8 @@ MtrrLibSetMemoryRanges (  **/  RETURN_STATUS
> MtrrLibSetBelow1MBMemoryAttribute (
> -  IN OUT MTRR_FIXED_SETTINGS     *FixedSettings,
> -  OUT BOOLEAN                    *Modified,
> +  IN OUT UINT64                  *ClearMasks,
> +  IN OUT UINT64                  *OrMasks,
>    IN PHYSICAL_ADDRESS            BaseAddress,
>    IN UINT64                      Length,
>    IN MTRR_MEMORY_CACHE_TYPE      Type
> @@ -2122,36 +2122,17 @@ MtrrLibSetBelow1MBMemoryAttribute (
>    UINT32                    MsrIndex;
>    UINT64                    ClearMask;
>    UINT64                    OrMask;
> -  UINT64                    ClearMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)];
> -  UINT64                    OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)];
> -  BOOLEAN                   LocalModified[ARRAY_SIZE (mMtrrLibFixedMtrrTable)];
> 
>    ASSERT (BaseAddress < BASE_1MB);
> 
> -  SetMem (LocalModified, sizeof (LocalModified), FALSE);
> -
> -  //
> -  // (Value & ~0 | 0) still equals to (Value)
> -  //
> -  SetMem (ClearMasks, sizeof (ClearMasks), 0);
> -  SetMem (OrMasks, sizeof (OrMasks), 0);
> -
>    MsrIndex = (UINT32)-1;
>    while ((BaseAddress < BASE_1MB) && (Length != 0)) {
>      Status = MtrrLibProgramFixedMtrr (Type, &BaseAddress, &Length,
> &MsrIndex, &ClearMask, &OrMask);
>      if (RETURN_ERROR (Status)) {
>        return Status;
>      }
> -    ClearMasks[MsrIndex]    = ClearMask;
> -    OrMasks[MsrIndex]       = OrMask;
> -    Modified[MsrIndex]      = TRUE;
> -    LocalModified[MsrIndex] = TRUE;
> -  }
> -
> -  for (MsrIndex = 0; MsrIndex < ARRAY_SIZE (mMtrrLibFixedMtrrTable);
> MsrIndex++) {
> -    if (LocalModified[MsrIndex]) {
> -      FixedSettings->Mtrr[MsrIndex] = (FixedSettings->Mtrr[MsrIndex] &
> ~ClearMasks[MsrIndex]) | OrMasks[MsrIndex];
> -    }
> +    ClearMasks[MsrIndex] = ClearMasks[MsrIndex] | ClearMask;
> +    OrMasks[MsrIndex]    = (OrMasks[MsrIndex] & ~ClearMask) | OrMask;
>    }
>    return RETURN_SUCCESS;
>  }
> @@ -2213,8 +2194,8 @@ MtrrSetMemoryAttributesInMtrrSettings (
>    MTRR_MEMORY_RANGE         WorkingVariableMtrr[ARRAY_SIZE
> (MtrrSetting->Variables.Mtrr)];
>    BOOLEAN                   VariableSettingModified[ARRAY_SIZE (MtrrSetting-
> >Variables.Mtrr)];
> 
> -  BOOLEAN                   FixedSettingsModified[ARRAY_SIZE
> (mMtrrLibFixedMtrrTable)];
> -  MTRR_FIXED_SETTINGS       WorkingFixedSettings;
> +  UINT64                    ClearMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)];
> +  UINT64                    OrMasks[ARRAY_SIZE (mMtrrLibFixedMtrrTable)];
> 
>    MTRR_CONTEXT              MtrrContext;
>    BOOLEAN                   MtrrContextValid;
> @@ -2226,10 +2207,6 @@ MtrrSetMemoryAttributesInMtrrSettings (
>    // TRUE indicating the accordingly Variable setting needs modificaiton in
> OriginalVariableMtrr.
>    //
>    SetMem (VariableSettingModified, ARRAY_SIZE (VariableSettingModified),
> FALSE);
> -  //
> -  // TRUE indicating the accordingly Fixed setting needs modification in
> WorkingFixedSettings.
> -  //
> -  SetMem (FixedSettingsModified, ARRAY_SIZE (FixedSettingsModified),
> FALSE);
> 
>    //
>    // TRUE indicating the caller requests to set variable MTRRs.
> @@ -2405,14 +2382,17 @@ MtrrSetMemoryAttributesInMtrrSettings (
>    //
>    // 3. Apply the below-1MB memory attribute settings.
>    //
> -  ZeroMem (WorkingFixedSettings.Mtrr, sizeof
> (WorkingFixedSettings.Mtrr));
> +  // (Value & ~0 | 0) still equals to (Value)  //  ZeroMem (ClearMasks,
> + sizeof (ClearMasks));  ZeroMem (OrMasks, sizeof (OrMasks));
>    for (Index = 0; Index < RangeCount; Index++) {
>      if (Ranges[Index].BaseAddress >= BASE_1MB) {
>        continue;
>      }
> 
>      Status = MtrrLibSetBelow1MBMemoryAttribute (
> -               &WorkingFixedSettings, FixedSettingsModified,
> +               ClearMasks, OrMasks,
>                 Ranges[Index].BaseAddress, Ranges[Index].Length,
> Ranges[Index].Type
>                 );
>      if (RETURN_ERROR (Status)) {
> @@ -2424,19 +2404,16 @@ MtrrSetMemoryAttributesInMtrrSettings (
>    //
>    // 4. Write fixed MTRRs that have been modified
>    //
> -  for (Index = 0; Index < ARRAY_SIZE (FixedSettingsModified); Index++) {
> -    if (FixedSettingsModified[Index]) {
> +  for (Index = 0; Index < ARRAY_SIZE (ClearMasks); Index++) {
> +    if (ClearMasks[Index] != 0) {
>        if (MtrrSetting != NULL) {
> -        MtrrSetting->Fixed.Mtrr[Index] = WorkingFixedSettings.Mtrr[Index];
> +        MtrrSetting->Fixed.Mtrr[Index] =
> + (MtrrSetting->Fixed.Mtrr[Index] & ~ClearMasks[Index]) |
> + OrMasks[Index];
>        } else {
>          if (!MtrrContextValid) {
>            MtrrLibPreMtrrChange (&MtrrContext);
>            MtrrContextValid = TRUE;
>          }
> -        AsmWriteMsr64 (
> -          mMtrrLibFixedMtrrTable[Index].Msr,
> -          WorkingFixedSettings.Mtrr[Index]
> -        );
> +        AsmMsrAndThenOr64 (mMtrrLibFixedMtrrTable[Index].Msr,
> + ~ClearMasks[Index], OrMasks[Index]);
>        }
>      }
>    }
> --
> 2.20.1.windows.1