Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

["Zeng, Star" <star.zeng@intel.com>]

I am ok to this code approach.

Acknowledged-by: Star Zeng <star.zeng@intel.com>

Besides, I think except GCD services, DxeCore should not call gCpu->SetMemoryAttributes() directly, for example ApplyMemoryProtectionPolicy(), it should have no assumption of the CPU code (to set capabilities), and call GCD setcapabilities first, and then call GCD setattributes since 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out-of-sync issue in GCD", otherwise there may be mismatch of page attributes between GCD and gCPU after RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().

Anyway, that could be in a separated patch. :)

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J Wang
Sent: Friday, November 3, 2017 8:57 AM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

> v2
> a. Fix an issue which will cause setting capability failure if size is smaller
>    than a page.

More than one entry of RT_CODE memory might cause boot problem for some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.

More detailed information, please refer to
    https://bugzilla.tianocore.org/show_bug.cgi?id=753

Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..4a7827ebc9 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
   PageLength    = 0;
 
   for (Index = 0; Index < NumberOfDescriptors; Index++) {
-    if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+    if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent
+        || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
+        || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
       continue;
     }
 
@@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
     // Sync real page attributes to GCD
     BaseAddress       = MemorySpaceMap[Index].BaseAddress;
     MemorySpaceLength = MemorySpaceMap[Index].Length;
+    Capabilities      = MemorySpaceMap[Index].Capabilities |
+                        EFI_MEMORY_PAGETYPE_MASK;
+    Status = gDS->SetMemorySpaceCapabilities (
+                    BaseAddress,
+                    MemorySpaceLength,
+                    Capabilities
+                    );
+    ASSERT_EFI_ERROR (Status);
+
     while (MemorySpaceLength > 0) {
       if (PageLength == 0) {
         PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, &PageAttribute); @@ -846,7 +857,6 @@ RefreshGcdMemoryAttributesFromPaging (
         if (Attributes != (MemorySpaceMap[Index].Attributes & EFI_MEMORY_PAGETYPE_MASK)) {
           DoUpdate = TRUE;
           Attributes |= (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK);
-          Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
         } else {
           DoUpdate = FALSE;
         }
@@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
       Length = MIN (PageLength, MemorySpaceLength);
       if (DoUpdate) {
-        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+        Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+        ASSERT_EFI_ERROR (Status);
         DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
                              Index, BaseAddress, BaseAddress + Length - 1,
                              MemorySpaceMap[Index].Attributes, Attributes));
--
2.14.1.windows.1

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