UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

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

About the IsModified, current function doesn't consider that hardware
also may change the pagetable. The issue is that in the first call of
internal function PageTableLibMapInLevel, the function assume page
table is not changed, and add ASSERT to check. But hardware may change
the page table, which cause the ASSERT happens.
Fix the issue by adding addtional condition to only check if the page
table is changed when the software want to modify the page table.
Also, add more comment to explain this behavior.

Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Crystal Lee <CrystalLee@ami.com.tw>
Cc: Pedro Falcato <pedro.falcato@gmail.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index cd493ad69ccb731af86c2174319bd05696c513d5..e09fa416144858923669f7526d22d56c478ec3a7 100644 (file)
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -81,7 +81,9 @@ typedef enum {
                                  Page table entries that map the linear address range are reset to 0 before set to the new attribute\r
                                  when a new physical base address is set.\r
   @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.\r
-  @param[out]     IsModified     TRUE means page table is modified. FALSE means page table is not modified.\r
+  @param[out]     IsModified     TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.\r
+                                 If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok\r
+                                 because page table can be changed by hardware anytime, and caller don't need to Flush TLB.\r
 \r
   @retval RETURN_UNSUPPORTED        PagingMode is not supported.\r
   @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.\r

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 7c2bd25d22a61b69705c59f95ce73efee8c075d1..6a39ac3830650a3458d81965aa4bace078f6c463 100644 (file)
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -294,7 +294,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
                                     Page table entries that map the linear address range are reset to 0 before set to the new attribute\r
                                     when a new physical base address is set.\r
   @param[in]      Mask              The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.\r
-  @param[out]     IsModified        TRUE means page table is modified. FALSE means page table is not modified.\r
+  @param[in, out] IsModified        Change IsModified to True if page table is modified and input parameter Modify is TRUE.\r
 \r
   @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 0 but some other attributes are provided.\r
   @retval RETURN_INVALID_PARAMETER  For non-present range, Mask->Bits.Present is 1, Attribute->Bits.Present is 1 but some other attributes are not provided.\r
@@ -314,7 +314,7 @@ PageTableLibMapInLevel (
   IN     UINT64              Offset,\r
   IN     IA32_MAP_ATTRIBUTE  *Attribute,\r
   IN     IA32_MAP_ATTRIBUTE  *Mask,\r
-  OUT    BOOLEAN             *IsModified\r
+  IN OUT BOOLEAN             *IsModified\r
   )\r
 {\r
   RETURN_STATUS       Status;\r
@@ -587,7 +587,10 @@ PageTableLibMapInLevel (
         OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;\r
         PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);\r
 \r
-        if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {\r
+        if (Modify && (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64)) {\r
+          //\r
+          // The page table entry can be changed by this function only when Modify is true.\r
+          //\r
           *IsModified = TRUE;\r
         }\r
       }\r
@@ -629,7 +632,10 @@ PageTableLibMapInLevel (
   // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during\r
   // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.\r
   //\r
-  if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {\r
+  if (Modify && (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64)) {\r
+    //\r
+    // The page table entry can be changed by this function only when Modify is true.\r
+    //\r
     *IsModified = TRUE;\r
   }\r
 \r
@@ -654,7 +660,9 @@ PageTableLibMapInLevel (
                                  Page table entries that map the linear address range are reset to 0 before set to the new attribute\r
                                  when a new physical base address is set.\r
   @param[in]      Mask           The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.\r
-  @param[out]     IsModified     TRUE means page table is modified. FALSE means page table is not modified.\r
+  @param[out]     IsModified     TRUE means page table is modified by software or hardware. FALSE means page table is not modified by software.\r
+                                 If the output IsModified is FALSE, there is possibility that the page table is changed by hardware. It is ok\r
+                                 because page table can be changed by hardware anytime, and caller don't need to Flush TLB.\r
 \r
   @retval RETURN_UNSUPPORTED        PagingMode is not supported.\r
   @retval RETURN_INVALID_PARAMETER  PageTable, BufferSize, Attribute or Mask is NULL.\r