1. Patchew
  2. EDK2
  3. View series

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

Jian J Wang posted 1 patch 6 years, 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +++++++++++++++++++++++++++++-----------
1 file changed, 50 insertions(+), 19 deletions(-)
[edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Posted by Jian J Wang 6 years, 5 months ago 
> v5:
>    Coding style clean-up

> v4:
> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>    a logic hole
> b. Add warning message if failed to update capability
> c. Add local variable to hold new attributes to make code cleaner

> v3:
> a. Add comment to explain more on updating memory capabilities
> b. Fix logic hole in updating attributes
> c. Instead of checking illegal memory space address and size, use return
>    status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>    cannot be updated with new capabilities.

> 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 | 69 +++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..61537838b7 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64                              BaseAddress;
   UINT64                              PageStartAddress;
   UINT64                              Attributes;
-  UINT64                              Capabilities;
-  BOOLEAN                             DoUpdate;
+  UINT64                              NewAttributes;
   UINTN                               Index;
 
   //
@@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext (&PagingContext);
 
-  DoUpdate      = FALSE;
-  Capabilities  = 0;
   Attributes    = 0;
+  NewAttributes = 0;
   BaseAddress   = 0;
   PageLength    = 0;
 
@@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
       continue;
     }
 
+    //
+    // Sync the actual paging related capabilities back to GCD service first.
+    // As a side effect (good one), this can also help to avoid unnecessary
+    // memory map entries due to the different capabilities of the same type
+    // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
+    // which could cause boot failure of some old Linux distro (before v4.3).
+    //
+    Status = gDS->SetMemorySpaceCapabilities (
+                    MemorySpaceMap[Index].BaseAddress,
+                    MemorySpaceMap[Index].Length,
+                    MemorySpaceMap[Index].Capabilities |
+                    EFI_MEMORY_PAGETYPE_MASK
+                    );
+    if (EFI_ERROR (Status)) {
+      //
+      // If we cannot udpate the capabilities, we cannot update its
+      // attributes either. So just simply skip current block of memory.
+      //
+      DEBUG ((
+        DEBUG_WARN,
+        "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+        (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+        MemorySpaceMap[Index].Capabilities,
+        MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
+        ));
+      continue;
+    }
+
     if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
       //
       // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
       PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
     }
 
-    // Sync real page attributes to GCD
+    //
+    // Sync actual page attributes to GCD
+    //
     BaseAddress       = MemorySpaceMap[Index].BaseAddress;
     MemorySpaceLength = MemorySpaceMap[Index].Length;
     while (MemorySpaceLength > 0) {
@@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
         PageStartAddress  = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
         PageLength        = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
         Attributes        = GetAttributesFromPageEntry (PageEntry);
-
-        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;
-        }
       }
 
       Length = MIN (PageLength, MemorySpaceLength);
-      if (DoUpdate) {
-        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
-        DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
-                             Index, BaseAddress, BaseAddress + Length - 1,
-                             MemorySpaceMap[Index].Attributes, Attributes));
+      if (Attributes != (MemorySpaceMap[Index].Attributes &
+                         EFI_MEMORY_PAGETYPE_MASK)) {
+        NewAttributes = (MemorySpaceMap[Index].Attributes &
+                         ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
+        Status = gDS->SetMemorySpaceAttributes (
+                        BaseAddress,
+                        Length,
+                        NewAttributes
+                        );
+        ASSERT_EFI_ERROR (Status);
+        DEBUG ((
+          DEBUG_INFO,
+          "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
+          (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
+          MemorySpaceMap[Index].Attributes,
+          NewAttributes
+          ));
       }
 
       PageLength        -= Length;
-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Posted by Wang, Jian J 6 years, 5 months ago 
Sorry guys. I did update the title but v5 is still missing. A new one will be sent out.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, November 10, 2017 8:42 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] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> > v5:
> >    Coding style clean-up
> 
> > v4:
> > a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >    a logic hole
> > b. Add warning message if failed to update capability
> > c. Add local variable to hold new attributes to make code cleaner
> 
> > v3:
> > a. Add comment to explain more on updating memory capabilities
> > b. Fix logic hole in updating attributes
> > c. Instead of checking illegal memory space address and size, use return
> >    status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >    cannot be updated with new capabilities.
> 
> > 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 | 69 +++++++++++++++++++++++++++++--
> ---------
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..61537838b7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
>    UINT64                              BaseAddress;
>    UINT64                              PageStartAddress;
>    UINT64                              Attributes;
> -  UINT64                              Capabilities;
> -  BOOLEAN                             DoUpdate;
> +  UINT64                              NewAttributes;
>    UINTN                               Index;
> 
>    //
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> 
>    GetCurrentPagingContext (&PagingContext);
> 
> -  DoUpdate      = FALSE;
> -  Capabilities  = 0;
>    Attributes    = 0;
> +  NewAttributes = 0;
>    BaseAddress   = 0;
>    PageLength    = 0;
> 
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
>        continue;
>      }
> 
> +    //
> +    // Sync the actual paging related capabilities back to GCD service first.
> +    // As a side effect (good one), this can also help to avoid unnecessary
> +    // memory map entries due to the different capabilities of the same type
> +    // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +    // which could cause boot failure of some old Linux distro (before v4.3).
> +    //
> +    Status = gDS->SetMemorySpaceCapabilities (
> +                    MemorySpaceMap[Index].BaseAddress,
> +                    MemorySpaceMap[Index].Length,
> +                    MemorySpaceMap[Index].Capabilities |
> +                    EFI_MEMORY_PAGETYPE_MASK
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // If we cannot udpate the capabilities, we cannot update its
> +      // attributes either. So just simply skip current block of memory.
> +      //
> +      DEBUG ((
> +        DEBUG_WARN,
> +        "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
> +        (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +        MemorySpaceMap[Index].Capabilities,
> +        MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> +        ));
> +      continue;
> +    }
> +
>      if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>        //
>        // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
>        PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>      }
> 
> -    // Sync real page attributes to GCD
> +    //
> +    // Sync actual page attributes to GCD
> +    //
>      BaseAddress       = MemorySpaceMap[Index].BaseAddress;
>      MemorySpaceLength = MemorySpaceMap[Index].Length;
>      while (MemorySpaceLength > 0) {
> @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
>          PageStartAddress  = (*PageEntry) &
> (UINT64)PageAttributeToMask(PageAttribute);
>          PageLength        = PageAttributeToLength (PageAttribute) - (BaseAddress -
> PageStartAddress);
>          Attributes        = GetAttributesFromPageEntry (PageEntry);
> -
> -        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;
> -        }
>        }
> 
>        Length = MIN (PageLength, MemorySpaceLength);
> -      if (DoUpdate) {
> -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> -        DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx -
>  %016lx (%08lx -> %08lx)\r\n",
> -                             Index, BaseAddress, BaseAddress + Length - 1,
> -                             MemorySpaceMap[Index].Attributes, Attributes));
> +      if (Attributes != (MemorySpaceMap[Index].Attributes &
> +                         EFI_MEMORY_PAGETYPE_MASK)) {
> +        NewAttributes = (MemorySpaceMap[Index].Attributes &
> +                         ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> +        Status = gDS->SetMemorySpaceAttributes (
> +                        BaseAddress,
> +                        Length,
> +                        NewAttributes
> +                        );
> +        ASSERT_EFI_ERROR (Status);
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> +          (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +          MemorySpaceMap[Index].Attributes,
> +          NewAttributes
> +          ));
>        }
> 
>        PageLength        -= Length;
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.