[PATCH 0/4] Quality improvement of MtrrLib

[PATCH 0/4] Quality improvement of MtrrLib

[Ruiyu Ni]

The patch sets fix four issues.
For details, please refer to the invidual patch.
Patch 2/4 fixes the most critical bug.

Ruiyu Ni (4):
  UefiCpuPkg/MtrrLib: Refine the debug messages
  UefiCpuPkg/MtrrLib: Fix bug that may calculate wrong MTRR result
  UefiCpuPkg/MtrrLib: Handle one setting request covering all memory
  UefiCpuPkg/MtrrLib: Correct typo to change vector to vertex

 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 295 ++++++++++++++++++++---------------
 1 file changed, 168 insertions(+), 127 deletions(-)

-- 
2.15.1.windows.2

[PATCH 1/4] UefiCpuPkg/MtrrLib: Refine the debug messages

[Ruiyu Ni]

MtrrSetMemoryAttributesInMtrrSettings() missed the debug messages
of memory attribute request and status. The patch moves all debug
messages from MtrrSetMemoryAttributeInMtrrSettings() to
MtrrSetMemoryAttributesInMtrrSettings() and refines the debug message
to carry more information.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 83 +++++++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index f37b740fdf..b83d768c5f 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 - 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2008 - 2018, 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
@@ -1570,7 +1570,7 @@ MtrrLibCalculateMtrrs (
   //
   VectorCount = VectorIndex + 1;
   DEBUG ((
-    DEBUG_CACHE, "VectorCount (%016lx - %016lx) = %d\n", 
+    DEBUG_CACHE, "  VectorCount (%016lx - %016lx) = %d\n",
     Ranges[0].BaseAddress, Ranges[RangeCount - 1].BaseAddress + Ranges[RangeCount - 1].Length, VectorCount
     ));
   ASSERT (VectorCount < MAX_UINT16);
@@ -2209,6 +2209,7 @@ MtrrSetMemoryAttributesInMtrrSettings (
   MTRR_CONTEXT              MtrrContext;
   BOOLEAN                   MtrrContextValid;
 
+  Status = RETURN_SUCCESS;
   MtrrLibInitializeMtrrMask (&MtrrValidBitsMask, &MtrrValidAddressMask);
 
   //
@@ -2226,24 +2227,48 @@ MtrrSetMemoryAttributesInMtrrSettings (
   Above1MbExist             = FALSE;
   OriginalVariableMtrrCount = 0;
 
+  //
+  // 0. Dump the requests.
+  //
+  DEBUG_CODE (
+    DEBUG ((DEBUG_CACHE, "Mtrr: Set Mem Attribute to %a, ScratchSize = %x%a",
+            (MtrrSetting == NULL) ? "Hardware" : "Buffer", ScratchSize,
+            (RangeCount <= 1) ? "," : "\n"
+            ));
+    for (Index = 0; Index < RangeCount; Index++) {
+      DEBUG ((DEBUG_CACHE, " %a: [%016lx, %016lx)\n",
+              mMtrrMemoryCacheTypeShortName[MIN (Ranges[Index].Type, CacheInvalid)],
+              Ranges[Index].BaseAddress, Ranges[Index].BaseAddress + Ranges[Index].Length
+              ));
+    }
+  );
+
   //
   // 1. Validate the parameters.
   //
+  if (!IsMtrrSupported ()) {
+    Status = RETURN_UNSUPPORTED;
+    goto Exit;
+  }
+
   for (Index = 0; Index < RangeCount; Index++) {
     if (Ranges[Index].Length == 0) {
-      return RETURN_INVALID_PARAMETER;
+      Status = RETURN_INVALID_PARAMETER;
+      goto Exit;
     }
     if (((Ranges[Index].BaseAddress & ~MtrrValidAddressMask) != 0) ||
         ((Ranges[Index].Length & ~MtrrValidAddressMask) != 0)
         ) {
-      return RETURN_UNSUPPORTED;
+      Status = RETURN_UNSUPPORTED;
+      goto Exit;
     }
     if ((Ranges[Index].Type != CacheUncacheable) &&
         (Ranges[Index].Type != CacheWriteCombining) &&
         (Ranges[Index].Type != CacheWriteThrough) &&
         (Ranges[Index].Type != CacheWriteProtected) &&
         (Ranges[Index].Type != CacheWriteBack)) {
-      return RETURN_INVALID_PARAMETER;
+      Status = RETURN_INVALID_PARAMETER;
+      goto Exit;
     }
     if (Ranges[Index].BaseAddress + Ranges[Index].Length > BASE_1MB) {
       Above1MbExist = TRUE;
@@ -2309,7 +2334,7 @@ MtrrSetMemoryAttributesInMtrrSettings (
       if (Status == RETURN_ALREADY_STARTED) {
         Status = RETURN_SUCCESS;
       } else if (Status == RETURN_OUT_OF_RESOURCES) {
-        return Status;
+        goto Exit;
       } else {
         ASSERT_RETURN_ERROR (Status);
         Modified = TRUE;
@@ -2327,7 +2352,7 @@ MtrrSetMemoryAttributesInMtrrSettings (
                  WorkingVariableMtrr, FirmwareVariableMtrrCount + 1, &WorkingVariableMtrrCount
                  );
       if (RETURN_ERROR (Status)) {
-        return Status;
+        goto Exit;
       }
 
       //
@@ -2346,7 +2371,8 @@ MtrrSetMemoryAttributesInMtrrSettings (
       }
 
       if (WorkingVariableMtrrCount > FirmwareVariableMtrrCount) {
-        return RETURN_OUT_OF_RESOURCES;
+        Status = RETURN_OUT_OF_RESOURCES;
+        goto Exit;
       }
 
       //
@@ -2375,7 +2401,7 @@ MtrrSetMemoryAttributesInMtrrSettings (
                Ranges[Index].BaseAddress, Ranges[Index].Length, Ranges[Index].Type
                );
     if (RETURN_ERROR (Status)) {
-      return Status;
+      goto Exit;
     }
   }
 
@@ -2441,7 +2467,12 @@ MtrrSetMemoryAttributesInMtrrSettings (
     }
   }
 
-  return RETURN_SUCCESS;
+Exit:
+  DEBUG ((DEBUG_CACHE, "  Result = %r\n", Status));
+  if (!RETURN_ERROR (Status)) {
+    MtrrDebugPrintAllMtrrsWorker (MtrrSetting);
+  }
+  return Status;
 }
 
 /**
@@ -2475,28 +2506,15 @@ MtrrSetMemoryAttributeInMtrrSettings (
   IN MTRR_MEMORY_CACHE_TYPE  Attribute
   )
 {
-  RETURN_STATUS              Status;
   UINT8                      Scratch[SCRATCH_BUFFER_SIZE];
   UINTN                      ScratchSize;
   MTRR_MEMORY_RANGE          Range;
 
-  if (!IsMtrrSupported ()) {
-    return RETURN_UNSUPPORTED;
-  }
-
   Range.BaseAddress = BaseAddress;
   Range.Length      = Length;
   Range.Type        = Attribute;
   ScratchSize = sizeof (Scratch);
-  Status = MtrrSetMemoryAttributesInMtrrSettings (MtrrSetting, Scratch, &ScratchSize, &Range, 1);
-  DEBUG ((DEBUG_CACHE, "MtrrSetMemoryAttribute(MtrrSettings = %p) %a: [%016lx, %016lx) - %r\n",
-          MtrrSetting,
-          mMtrrMemoryCacheTypeShortName[Attribute], BaseAddress, BaseAddress + Length, Status));
-
-  if (!RETURN_ERROR (Status)) {
-    MtrrDebugPrintAllMtrrsWorker (MtrrSetting);
-  }
-  return Status;
+  return MtrrSetMemoryAttributesInMtrrSettings (MtrrSetting, Scratch, &ScratchSize, &Range, 1);
 }
 
 /**
@@ -2788,6 +2806,7 @@ MtrrDebugPrintAllMtrrsWorker (
     UINT64            MtrrValidBitsMask;
     UINT64            MtrrValidAddressMask;
     UINT32            VariableMtrrCount;
+    BOOLEAN           ContainVariableMtrr;
     MTRR_MEMORY_RANGE Ranges[
       ARRAY_SIZE (mMtrrLibFixedMtrrTable) * sizeof (UINT64) + 2 * ARRAY_SIZE (Mtrrs->Variables.Mtrr) + 1
       ];
@@ -2809,13 +2828,13 @@ MtrrDebugPrintAllMtrrsWorker (
     //
     // Dump RAW MTRR contents
     //
-    DEBUG((DEBUG_CACHE, "MTRR Settings\n"));
-    DEBUG((DEBUG_CACHE, "=============\n"));
-    DEBUG((DEBUG_CACHE, "MTRR Default Type: %016lx\n", Mtrrs->MtrrDefType));
+    DEBUG ((DEBUG_CACHE, "MTRR Settings:\n"));
+    DEBUG ((DEBUG_CACHE, "=============\n"));
+    DEBUG ((DEBUG_CACHE, "MTRR Default Type: %016lx\n", Mtrrs->MtrrDefType));
     for (Index = 0; Index < ARRAY_SIZE (mMtrrLibFixedMtrrTable); Index++) {
-      DEBUG((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index]));
+      DEBUG ((DEBUG_CACHE, "Fixed MTRR[%02d]   : %016lx\n", Index, Mtrrs->Fixed.Mtrr[Index]));
     }
-
+    ContainVariableMtrr = FALSE;
     for (Index = 0; Index < VariableMtrrCount; Index++) {
       if (((MSR_IA32_MTRR_PHYSMASK_REGISTER *)&Mtrrs->Variables.Mtrr[Index].Mask)->Bits.V == 0) {
         //
@@ -2823,18 +2842,22 @@ MtrrDebugPrintAllMtrrsWorker (
         //
         continue;
       }
+      ContainVariableMtrr = TRUE;
       DEBUG ((DEBUG_CACHE, "Variable MTRR[%02d]: Base=%016lx Mask=%016lx\n",
         Index,
         Mtrrs->Variables.Mtrr[Index].Base,
         Mtrrs->Variables.Mtrr[Index].Mask
         ));
     }
+    if (!ContainVariableMtrr) {
+      DEBUG ((DEBUG_CACHE, "Variable MTRR    : None.\n"));
+    }
     DEBUG((DEBUG_CACHE, "\n"));
 
     //
     // Dump MTRR setting in ranges
     //
-    DEBUG((DEBUG_CACHE, "MTRR Ranges\n"));
+    DEBUG((DEBUG_CACHE, "Memory Ranges:\n"));
     DEBUG((DEBUG_CACHE, "====================================\n"));
     MtrrLibInitializeMtrrMask (&MtrrValidBitsMask, &MtrrValidAddressMask);
     Ranges[0].BaseAddress = 0;
-- 
2.15.1.windows.2

[PATCH 2/4] UefiCpuPkg/MtrrLib: Fix bug that may calculate wrong MTRR result

[Ruiyu Ni]

Code forgot to initialize the optional weight between adjacent
vertices. It caused wrong MTRR result was calculated for some
memory settings.

The logic was incorrectly removed when converting from POC
code. The patch adds back the initialization.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 37 ++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index b83d768c5f..566a4cb67b 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -1583,20 +1583,33 @@ MtrrLibCalculateMtrrs (
   Vector[VectorCount - 1].Address = Base1;
 
   Weight = (UINT8 *) &Vector[VectorCount];
-  //
-  // Set mandatory weight between any vector to max
-  // Set optional weight and between any vector and self->self to 0
-  // E.g.:
-  //   00 FF FF FF
-  //   00 00 FF FF
-  //   00 00 00 FF
-  //   00 00 00 00
-  //
   for (VectorIndex = 0; VectorIndex < VectorCount; VectorIndex++) {
+    //
+    // Set optional weight between vertices and self->self to 0
+    //
     SetMem (&Weight[M(VectorIndex, 0)], VectorIndex + 1, 0);
-    if (VectorIndex != VectorCount - 1) {
-      Weight[M (VectorIndex, VectorIndex + 1)] = (DefaultType == Vector[VectorIndex].Type) ? 0 : 1;
-      SetMem (&Weight[M (VectorIndex, VectorIndex + 2)], VectorCount - VectorIndex - 2, MAX_WEIGHT);
+    //
+    // Set mandatory weight between vectors to MAX_WEIGHT
+    //
+    SetMem (&Weight[M (VectorIndex, VectorIndex + 1)], VectorCount - VectorIndex - 1, MAX_WEIGHT);
+
+    // Final result looks like:
+    //   00 FF FF FF
+    //   00 00 FF FF
+    //   00 00 00 FF
+    //   00 00 00 00
+  }
+
+  //
+  // Set mandatory weight and optional weight for adjacent vertices
+  //
+  for (VectorIndex = 0; VectorIndex < VectorCount - 1; VectorIndex++) {
+    if (Vector[VectorIndex].Type != DefaultType) {
+      Weight[M (VectorIndex, VectorIndex + 1)] = 1;
+      Weight[O (VectorIndex, VectorIndex + 1)] = 0;
+    } else {
+      Weight[M (VectorIndex, VectorIndex + 1)] = 0;
+      Weight[O (VectorIndex, VectorIndex + 1)] = 1;
     }
   }
 
-- 
2.15.1.windows.2

[PATCH 3/4] UefiCpuPkg/MtrrLib: Handle one setting request covering all memory

[Ruiyu Ni]

*SetMemoryAttribute*() API cannot handle the setting request that
looks like <0, MAX_ADDRESS, Type>. The buggy parameter checking
logic returns Unsupported for this case.
The patch fixes the checking logic to handle such case.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index 566a4cb67b..54f703606b 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -2270,8 +2270,13 @@ MtrrSetMemoryAttributesInMtrrSettings (
       goto Exit;
     }
     if (((Ranges[Index].BaseAddress & ~MtrrValidAddressMask) != 0) ||
-        ((Ranges[Index].Length & ~MtrrValidAddressMask) != 0)
+        ((((Ranges[Index].BaseAddress + Ranges[Index].Length) & ~MtrrValidAddressMask) != 0) &&
+          (Ranges[Index].BaseAddress + Ranges[Index].Length) != MtrrValidBitsMask + 1)
         ) {
+      //
+      // Either the BaseAddress or the Limit doesn't follow the alignment requirement.
+      // Note: It's still valid if Limit doesn't follow the alignment requirement but equals to MAX Address.
+      //
       Status = RETURN_UNSUPPORTED;
       goto Exit;
     }
-- 
2.15.1.windows.2

[PATCH 4/4] UefiCpuPkg/MtrrLib: Correct typo to change vector to vertex

[Ruiyu Ni]

The patch only change the comments and variable name so
doesn't impact the functionality.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 190 +++++++++++++++++------------------
 1 file changed, 95 insertions(+), 95 deletions(-)

diff --git a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
index 54f703606b..eb46861163 100644
--- a/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
+++ b/UefiCpuPkg/Library/MtrrLib/MtrrLib.c
@@ -1,7 +1,7 @@
 /** @file
   MTRR setting library
 
-  @par Note: 
+  @par Note:
     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.
 
@@ -32,8 +32,8 @@
 #define SCRATCH_BUFFER_SIZE           (4 * SIZE_4KB)
 #define MTRR_LIB_ASSERT_ALIGNED(B, L) ASSERT ((B & ~(L - 1)) == B);
 
-#define M(x,y) ((x) * VectorCount + (y))
-#define O(x,y) ((y) * VectorCount + (x))
+#define M(x,y) ((x) * VertexCount + (y))
+#define O(x,y) ((y) * VertexCount + (x))
 
 //
 // Context to save and restore when MTRRs are programmed
@@ -1142,21 +1142,21 @@ MtrrLibGetNumberOfTypes (
 }
 
 /**
-  Calculate the least MTRR number from vector Start to Stop and update
-  the Previous of all vectors from Start to Stop is updated to reflect
+  Calculate the least MTRR number from vertex Start to Stop and update
+  the Previous of all vertices from Start to Stop is updated to reflect
   how the memory range is covered by MTRR.
 
-  @param VectorCount     The count of vectors in the graph.
-  @param Vector          Array holding all vectors.
-  @param Weight          2-dimention array holding weights between vectors.
-  @param Start           Start vector.
-  @param Stop            Stop vector.
+  @param VertexCount     The count of vertices in the graph.
+  @param Vertices        Array holding all vertices.
+  @param Weight          2-dimention array holding weights between vertices.
+  @param Start           Start vertex.
+  @param Stop            Stop vertex.
   @param IncludeOptional TRUE to count the optional weight.
 **/
 VOID
 MtrrLibCalculateLeastMtrrs (
-  IN UINT16                      VectorCount,
-  IN MTRR_LIB_ADDRESS            *Vector,
+  IN UINT16                      VertexCount,
+  IN MTRR_LIB_ADDRESS            *Vertices,
   IN OUT CONST UINT8             *Weight,
   IN UINT16                      Start,
   IN UINT16                      Stop,
@@ -1170,52 +1170,52 @@ MtrrLibCalculateLeastMtrrs (
   UINT8                          Optional;
 
   for (Index = Start; Index <= Stop; Index++) {
-    Vector[Index].Visited = FALSE;
-    Vector[Index].Previous = VectorCount;
+    Vertices[Index].Visited = FALSE;
+    Vertices[Index].Previous = VertexCount;
     Mandatory = Weight[M(Start,Index)];
-    Vector[Index].Weight = Mandatory;
+    Vertices[Index].Weight = Mandatory;
     if (Mandatory != MAX_WEIGHT) {
       Optional = IncludeOptional ? Weight[O(Start, Index)] : 0;
-      Vector[Index].Weight += Optional;
-      ASSERT (Vector[Index].Weight >= Optional);
+      Vertices[Index].Weight += Optional;
+      ASSERT (Vertices[Index].Weight >= Optional);
     }
   }
 
   MinI = Start;
   MinWeight = 0;
-  while (!Vector[Stop].Visited) {
+  while (!Vertices[Stop].Visited) {
     //
-    // Update the weight from the shortest vector to other unvisited vectors
+    // Update the weight from the shortest vertex to other unvisited vertices
     //
     for (Index = Start + 1; Index <= Stop; Index++) {
-      if (!Vector[Index].Visited) {
+      if (!Vertices[Index].Visited) {
         Mandatory = Weight[M(MinI, Index)];
         if (Mandatory != MAX_WEIGHT) {
           Optional = IncludeOptional ? Weight[O(MinI, Index)] : 0;
-          if (MinWeight + Mandatory + Optional <= Vector[Index].Weight) {
-            Vector[Index].Weight   = MinWeight + Mandatory + Optional;
-            Vector[Index].Previous = MinI; // Previous is Start based.
+          if (MinWeight + Mandatory + Optional <= Vertices[Index].Weight) {
+            Vertices[Index].Weight   = MinWeight + Mandatory + Optional;
+            Vertices[Index].Previous = MinI; // Previous is Start based.
           }
         }
       }
     }
 
     //
-    // Find the shortest vector from Start
+    // Find the shortest vertex from Start
     //
-    MinI      = VectorCount;
+    MinI      = VertexCount;
     MinWeight = MAX_WEIGHT;
     for (Index = Start + 1; Index <= Stop; Index++) {
-      if (!Vector[Index].Visited && MinWeight > Vector[Index].Weight) {
+      if (!Vertices[Index].Visited && MinWeight > Vertices[Index].Weight) {
         MinI      = Index;
-        MinWeight = Vector[Index].Weight;
+        MinWeight = Vertices[Index].Weight;
       }
     }
 
     //
-    // Mark the shortest vector from Start as visited
+    // Mark the shortest vertex from Start as visited
     //
-    Vector[MinI].Visited = TRUE;
+    Vertices[MinI].Visited = TRUE;
   }
 }
 
@@ -1288,17 +1288,17 @@ MtrrLibIsPowerOfTwo (
 }
 
 /**
-  Calculate the subtractive path from vector Start to Stop.
+  Calculate the subtractive path from vertex Start to Stop.
 
   @param DefaultType  Default memory type.
   @param A0           Alignment to use when base address is 0.
   @param Ranges       Array holding memory type settings for all memory regions.
   @param RangeCount   The count of memory ranges the array holds.
-  @param VectorCount  The count of vectors in the graph.
-  @param Vector       Array holding all vectors.
-  @param Weight       2-dimention array holding weights between vectors.
-  @param Start        Start vector.
-  @param Stop         Stop vector.
+  @param VertexCount  The count of vertices in the graph.
+  @param Vertices     Array holding all vertices.
+  @param Weight       2-dimention array holding weights between vertices.
+  @param Start        Start vertex.
+  @param Stop         Stop vertex.
   @param Types        Type bit mask of memory range from Start to Stop.
   @param TypeCount    Number of different memory types from Start to Stop.
   @param Mtrrs        Array holding all MTRR settings.
@@ -1315,8 +1315,8 @@ MtrrLibCalculateSubtractivePath (
   IN UINT64                      A0,
   IN CONST MTRR_MEMORY_RANGE     *Ranges,
   IN UINTN                       RangeCount,
-  IN UINT16                      VectorCount,
-  IN MTRR_LIB_ADDRESS            *Vector,
+  IN UINT16                      VertexCount,
+  IN MTRR_LIB_ADDRESS            *Vertices,
   IN OUT UINT8                   *Weight,
   IN UINT16                      Start,
   IN UINT16                      Stop,
@@ -1342,8 +1342,8 @@ MtrrLibCalculateSubtractivePath (
   MTRR_MEMORY_CACHE_TYPE         LowestType;
   MTRR_MEMORY_CACHE_TYPE         LowestPrecedentType;
 
-  Base   = Vector[Start].Address;
-  Length = Vector[Stop].Address - Base;
+  Base   = Vertices[Start].Address;
+  Length = Vertices[Stop].Address - Base;
 
   LowestType = MtrrLibLowestType (Types);
 
@@ -1404,18 +1404,18 @@ MtrrLibCalculateSubtractivePath (
       // We might use positive or subtractive, depending on which way uses less MTRR
       //
       for (SubStart = Start; SubStart <= Stop; SubStart++) {
-        if (Vector[SubStart].Address == HBase) {
+        if (Vertices[SubStart].Address == HBase) {
           break;
         }
       }
 
       for (SubStop = SubStart; SubStop <= Stop; SubStop++) {
-        if (Vector[SubStop].Address == HBase + HLength) {
+        if (Vertices[SubStop].Address == HBase + HLength) {
           break;
         }
       }
-      ASSERT (Vector[SubStart].Address == HBase);
-      ASSERT (Vector[SubStop].Address == HBase + HLength);
+      ASSERT (Vertices[SubStart].Address == HBase);
+      ASSERT (Vertices[SubStop].Address == HBase + HLength);
 
       if ((TypeCount == 2) || (SubStart == SubStop - 1)) {
         //
@@ -1429,7 +1429,7 @@ MtrrLibCalculateSubtractivePath (
           while (SubStart != SubStop) {
             Status = MtrrLibAppendVariableMtrr (
               Mtrrs, MtrrCapacity, MtrrCount,
-              Vector[SubStart].Address, Vector[SubStart].Length, (MTRR_MEMORY_CACHE_TYPE) Vector[SubStart].Type
+              Vertices[SubStart].Address, Vertices[SubStart].Length, (MTRR_MEMORY_CACHE_TYPE) Vertices[SubStart].Type
             );
             if (RETURN_ERROR (Status)) {
               return Status;
@@ -1439,21 +1439,21 @@ MtrrLibCalculateSubtractivePath (
         }
       } else {
         ASSERT (TypeCount == 3);
-        MtrrLibCalculateLeastMtrrs (VectorCount, Vector, Weight, SubStart, SubStop, TRUE);
+        MtrrLibCalculateLeastMtrrs (VertexCount, Vertices, Weight, SubStart, SubStop, TRUE);
 
         if (Mtrrs == NULL) {
-          Weight[M (Start, Stop)] += Vector[SubStop].Weight;
+          Weight[M (Start, Stop)] += Vertices[SubStop].Weight;
         } else {
           // When we need to collect the optimal path from SubStart to SubStop
           while (SubStop != SubStart) {
             Cur = SubStop;
-            Pre = Vector[Cur].Previous;
+            Pre = Vertices[Cur].Previous;
             SubStop = Pre;
 
             if (Weight[M (Pre, Cur)] != 0) {
               Status = MtrrLibAppendVariableMtrr (
                 Mtrrs, MtrrCapacity, MtrrCount,
-                Vector[Pre].Address, Vector[Cur].Address - Vector[Pre].Address, LowestPrecedentType
+                Vertices[Pre].Address, Vertices[Cur].Address - Vertices[Pre].Address, LowestPrecedentType
               );
               if (RETURN_ERROR (Status)) {
                 return Status;
@@ -1463,7 +1463,7 @@ MtrrLibCalculateSubtractivePath (
               Status = MtrrLibCalculateSubtractivePath (
                 DefaultType, A0,
                 Ranges, RangeCount,
-                VectorCount, Vector, Weight,
+                VertexCount, Vertices, Weight,
                 Pre, Cur, PrecedentTypes, 2,
                 Mtrrs, MtrrCapacity, MtrrCount
               );
@@ -1526,10 +1526,10 @@ MtrrLibCalculateMtrrs (
   UINT64                    Length;
   UINT64                    Alignment;
   UINT64                    SubLength;
-  MTRR_LIB_ADDRESS          *Vector;
+  MTRR_LIB_ADDRESS          *Vectices;
   UINT8                     *Weight;
-  UINT32                    VectorIndex;
-  UINT32                    VectorCount;
+  UINT32                    VertexIndex;
+  UINT32                    VertexCount;
   UINTN                     RequiredScratchSize;
   UINT8                     TypeCount;
   UINT16                    Start;
@@ -1542,10 +1542,10 @@ MtrrLibCalculateMtrrs (
   MTRR_LIB_ASSERT_ALIGNED (Base0, Base1 - Base0);
 
   //
-  // Count the number of vectors.
+  // Count the number of vertices.
   //
-  Vector = (MTRR_LIB_ADDRESS*)Scratch;
-  for (VectorIndex = 0, Index = 0; Index < RangeCount; Index++) {
+  Vectices = (MTRR_LIB_ADDRESS*)Scratch;
+  for (VertexIndex = 0, Index = 0; Index < RangeCount; Index++) {
     Base = Ranges[Index].BaseAddress;
     Length = Ranges[Index].Length;
     while (Length != 0) {
@@ -1554,44 +1554,44 @@ MtrrLibCalculateMtrrs (
       if (SubLength > Length) {
         SubLength = GetPowerOfTwo64 (Length);
       }
-      if (VectorIndex < *ScratchSize / sizeof (*Vector)) {
-        Vector[VectorIndex].Address   = Base;
-        Vector[VectorIndex].Alignment = Alignment;
-        Vector[VectorIndex].Type      = Ranges[Index].Type;
-        Vector[VectorIndex].Length    = SubLength;
+      if (VertexIndex < *ScratchSize / sizeof (*Vectices)) {
+        Vectices[VertexIndex].Address   = Base;
+        Vectices[VertexIndex].Alignment = Alignment;
+        Vectices[VertexIndex].Type      = Ranges[Index].Type;
+        Vectices[VertexIndex].Length    = SubLength;
       }
       Base   += SubLength;
       Length -= SubLength;
-      VectorIndex++;
+      VertexIndex++;
     }
   }
   //
-  // Vector[VectorIndex] = Base1, so whole vector count is (VectorIndex + 1).
+  // Vertices[VertexIndex] = Base1, so whole vertex count is (VertexIndex + 1).
   //
-  VectorCount = VectorIndex + 1;
+  VertexCount = VertexIndex + 1;
   DEBUG ((
-    DEBUG_CACHE, "  VectorCount (%016lx - %016lx) = %d\n",
-    Ranges[0].BaseAddress, Ranges[RangeCount - 1].BaseAddress + Ranges[RangeCount - 1].Length, VectorCount
+    DEBUG_CACHE, "  Count of vertices (%016llx - %016llx) = %d\n",
+    Ranges[0].BaseAddress, Ranges[RangeCount - 1].BaseAddress + Ranges[RangeCount - 1].Length, VertexCount
     ));
-  ASSERT (VectorCount < MAX_UINT16);
+  ASSERT (VertexCount < MAX_UINT16);
 
-  RequiredScratchSize = VectorCount * sizeof (*Vector) + VectorCount * VectorCount * sizeof (*Weight);
+  RequiredScratchSize = VertexCount * sizeof (*Vectices) + VertexCount * VertexCount * sizeof (*Weight);
   if (*ScratchSize < RequiredScratchSize) {
     *ScratchSize = RequiredScratchSize;
     return RETURN_BUFFER_TOO_SMALL;
   }
-  Vector[VectorCount - 1].Address = Base1;
+  Vectices[VertexCount - 1].Address = Base1;
 
-  Weight = (UINT8 *) &Vector[VectorCount];
-  for (VectorIndex = 0; VectorIndex < VectorCount; VectorIndex++) {
+  Weight = (UINT8 *) &Vectices[VertexCount];
+  for (VertexIndex = 0; VertexIndex < VertexCount; VertexIndex++) {
     //
     // Set optional weight between vertices and self->self to 0
     //
-    SetMem (&Weight[M(VectorIndex, 0)], VectorIndex + 1, 0);
+    SetMem (&Weight[M(VertexIndex, 0)], VertexIndex + 1, 0);
     //
-    // Set mandatory weight between vectors to MAX_WEIGHT
+    // Set mandatory weight between vertices to MAX_WEIGHT
     //
-    SetMem (&Weight[M (VectorIndex, VectorIndex + 1)], VectorCount - VectorIndex - 1, MAX_WEIGHT);
+    SetMem (&Weight[M (VertexIndex, VertexIndex + 1)], VertexCount - VertexIndex - 1, MAX_WEIGHT);
 
     // Final result looks like:
     //   00 FF FF FF
@@ -1603,22 +1603,22 @@ MtrrLibCalculateMtrrs (
   //
   // Set mandatory weight and optional weight for adjacent vertices
   //
-  for (VectorIndex = 0; VectorIndex < VectorCount - 1; VectorIndex++) {
-    if (Vector[VectorIndex].Type != DefaultType) {
-      Weight[M (VectorIndex, VectorIndex + 1)] = 1;
-      Weight[O (VectorIndex, VectorIndex + 1)] = 0;
+  for (VertexIndex = 0; VertexIndex < VertexCount - 1; VertexIndex++) {
+    if (Vectices[VertexIndex].Type != DefaultType) {
+      Weight[M (VertexIndex, VertexIndex + 1)] = 1;
+      Weight[O (VertexIndex, VertexIndex + 1)] = 0;
     } else {
-      Weight[M (VectorIndex, VectorIndex + 1)] = 0;
-      Weight[O (VectorIndex, VectorIndex + 1)] = 1;
+      Weight[M (VertexIndex, VertexIndex + 1)] = 0;
+      Weight[O (VertexIndex, VertexIndex + 1)] = 1;
     }
   }
 
   for (TypeCount = 2; TypeCount <= 3; TypeCount++) {
-    for (Start = 0; Start < VectorCount; Start++) {
-      for (Stop = Start + 2; Stop < VectorCount; Stop++) {
-        ASSERT (Vector[Stop].Address > Vector[Start].Address);
-        Length = Vector[Stop].Address - Vector[Start].Address;
-        if (Length > Vector[Start].Alignment) {
+    for (Start = 0; Start < VertexCount; Start++) {
+      for (Stop = Start + 2; Stop < VertexCount; Stop++) {
+        ASSERT (Vectices[Stop].Address > Vectices[Start].Address);
+        Length = Vectices[Stop].Address - Vectices[Start].Address;
+        if (Length > Vectices[Start].Alignment) {
           //
           // Pickup a new Start when [Start, Stop) cannot be described by one MTRR.
           //
@@ -1626,7 +1626,7 @@ MtrrLibCalculateMtrrs (
         }
         if ((Weight[M(Start, Stop)] == MAX_WEIGHT) && MtrrLibIsPowerOfTwo (Length)) {
           if (MtrrLibGetNumberOfTypes (
-                Ranges, RangeCount, Vector[Start].Address, Vector[Stop].Address - Vector[Start].Address, &Type
+                Ranges, RangeCount, Vectices[Start].Address, Vectices[Stop].Address - Vectices[Start].Address, &Type
                 ) == TypeCount) {
             //
             // Update the Weight[Start, Stop] using subtractive path.
@@ -1634,7 +1634,7 @@ MtrrLibCalculateMtrrs (
             MtrrLibCalculateSubtractivePath (
               DefaultType, A0,
               Ranges, RangeCount,
-              (UINT16)VectorCount, Vector, Weight,
+              (UINT16)VertexCount, Vectices, Weight,
               Start, Stop, Type, TypeCount,
               NULL, 0, NULL
               );
@@ -1651,17 +1651,17 @@ MtrrLibCalculateMtrrs (
   }
 
   Status = RETURN_SUCCESS;
-  MtrrLibCalculateLeastMtrrs ((UINT16) VectorCount, Vector, Weight, 0, (UINT16) VectorCount - 1, FALSE);
-  Stop = (UINT16) VectorCount - 1;
+  MtrrLibCalculateLeastMtrrs ((UINT16) VertexCount, Vectices, Weight, 0, (UINT16) VertexCount - 1, FALSE);
+  Stop = (UINT16) VertexCount - 1;
   while (Stop != 0) {
-    Start = Vector[Stop].Previous;
+    Start = Vectices[Stop].Previous;
     TypeCount = MAX_UINT8;
     Type = 0;
     if (Weight[M(Start, Stop)] != 0) {
-      TypeCount = MtrrLibGetNumberOfTypes (Ranges, RangeCount, Vector[Start].Address, Vector[Stop].Address - Vector[Start].Address, &Type);
+      TypeCount = MtrrLibGetNumberOfTypes (Ranges, RangeCount, Vectices[Start].Address, Vectices[Stop].Address - Vectices[Start].Address, &Type);
       Status = MtrrLibAppendVariableMtrr (
         Mtrrs, MtrrCapacity, MtrrCount,
-        Vector[Start].Address, Vector[Stop].Address - Vector[Start].Address, 
+        Vectices[Start].Address, Vectices[Stop].Address - Vectices[Start].Address,
         MtrrLibLowestType (Type)
         );
       if (RETURN_ERROR (Status)) {
@@ -1675,13 +1675,13 @@ MtrrLibCalculateMtrrs (
       //
       if (TypeCount == MAX_UINT8) {
         TypeCount = MtrrLibGetNumberOfTypes (
-                      Ranges, RangeCount, Vector[Start].Address, Vector[Stop].Address - Vector[Start].Address, &Type
+                      Ranges, RangeCount, Vectices[Start].Address, Vectices[Stop].Address - Vectices[Start].Address, &Type
                       );
       }
       Status = MtrrLibCalculateSubtractivePath (
                  DefaultType, A0,
                  Ranges, RangeCount,
-                 (UINT16) VectorCount, Vector, Weight, Start, Stop,
+                 (UINT16) VertexCount, Vectices, Weight, Start, Stop,
                  Type, TypeCount,
                  Mtrrs, MtrrCapacity, MtrrCount
                  );
@@ -1788,7 +1788,7 @@ MtrrLibApplyVariableMtrrs (
   // 2. Set other types than WB or UC
   //
   for (Index = 0; Index < VariableMtrrCount; Index++) {
-    if ((VariableMtrr[Index].Length != 0) && 
+    if ((VariableMtrr[Index].Length != 0) &&
         (VariableMtrr[Index].Type != CacheWriteBack) && (VariableMtrr[Index].Type != CacheUncacheable)) {
       Status = MtrrLibSetMemoryType (
                  Ranges, RangeCapacity, RangeCount,
@@ -1981,7 +1981,7 @@ MtrrLibSetMemoryRanges (
   UINTN                     BiggestScratchSize;
 
   *VariableMtrrCount = 0;
-  
+
   //
   // Since the whole ranges need multiple calls of MtrrLibCalculateMtrrs().
   // Each call needs different scratch buffer size.
-- 
2.15.1.windows.2

Re: [PATCH 0/4] Quality improvement of MtrrLib

[Dong, Eric]

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, January 9, 2018 11:36 AM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 0/4] Quality improvement of MtrrLib

The patch sets fix four issues.
For details, please refer to the invidual patch.
Patch 2/4 fixes the most critical bug.

Ruiyu Ni (4):
  UefiCpuPkg/MtrrLib: Refine the debug messages
  UefiCpuPkg/MtrrLib: Fix bug that may calculate wrong MTRR result
  UefiCpuPkg/MtrrLib: Handle one setting request covering all memory
  UefiCpuPkg/MtrrLib: Correct typo to change vector to vertex

 UefiCpuPkg/Library/MtrrLib/MtrrLib.c | 295 ++++++++++++++++++++---------------
 1 file changed, 168 insertions(+), 127 deletions(-)

-- 
2.15.1.windows.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel