public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ruiyu Ni <ruiyu.ni@intel.com>
To: edk2-devel@lists.01.org
Cc: Ray Ni <ray.ni@intel.com>, Eric Dong <eric.dong@intel.com>,
	Chasel Chiu <chasel.chiu@intel.com>
Subject: [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set memory <1MB to UC
Date: Mon, 21 Jan 2019 23:16:43 +0800	[thread overview]
Message-ID: <20190121151643.47824-1-ruiyu.ni@intel.com> (raw)

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



             reply	other threads:[~2019-01-21 15:14 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-21 15:16 Ruiyu Ni [this message]
2019-01-22  1:17 ` [PATCH] UefiCpuPkg/MtrrLib: Fix a bug that may wrongly set memory <1MB to UC Chiu, Chasel
2019-02-19  5:33 ` Dong, Eric

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190121151643.47824-1-ruiyu.ni@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox