[PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Jian J Wang]

> v7:
>   Merge memory map after filtering paging attributes

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.

Jian J Wang (2):
  MdeModulePkg/DxeCore: Filter out all paging capabilities
  UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

 MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
 MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
 UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
 4 files changed, 112 insertions(+), 22 deletions(-)

-- 
2.14.1.windows.1

[PATCH v7 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities

[Jian J Wang]

> v7:
>   Merge memory map after filtering code

Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
set attributes and change memory paging attribute accordingly.
But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
value from Capabilities in GCD memory map. This might cause
boot problems. Clearing all paging related capabilities can
workaround it. The code added in this patch is supposed to
be removed once the usage of EFI_MEMORY_DESCRIPTOR.Attribute
is clarified in UEFI spec and adopted by both EDK-II Core and
all supported OSs.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++++++++++++++
 MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++++++++++++++++
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 1a0babba71..07b86ba696 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2948,4 +2948,22 @@ ApplyMemoryProtectionPolicy (
   IN  UINT64                Length
   );
 
+/**
+  Merge continous memory map entries whose have same attributes.
+
+  @param  MemoryMap       A pointer to the buffer in which firmware places
+                          the current memory map.
+  @param  MemoryMapSize   A pointer to the size, in bytes, of the
+                          MemoryMap buffer. On input, this is the size of
+                          the current memory map.  On output,
+                          it is the size of new memory map after merge.
+  @param  DescriptorSize  Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+VOID
+MergeMemoryMap (
+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
+  IN OUT UINTN                  *MemoryMapSize,
+  IN UINTN                      DescriptorSize
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index f1e4a37f2a..9ff73efdfa 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1687,6 +1687,7 @@ CoreGetMemoryMap (
   EFI_GCD_MAP_ENTRY                 MergeGcdMapEntry;
   EFI_MEMORY_TYPE                   Type;
   EFI_MEMORY_DESCRIPTOR             *MemoryMapStart;
+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEnd;
 
   //
   // Make sure the parameters are valid
@@ -1896,6 +1897,26 @@ CoreGetMemoryMap (
   //
   BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
 
+  //
+  // Note: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
+  //       set attributes and change memory paging attribute accordingly.
+  //       But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
+  //       value from Capabilities in GCD memory map. This might cause
+  //       boot problems. Clearing all paging related capabilities can
+  //       workaround it. Following code is supposed to be removed once
+  //       the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in
+  //       UEFI spec and adopted by both EDK-II Core and all supported
+  //       OSs.
+  //
+  MemoryMapEnd = MemoryMap;
+  MemoryMap = MemoryMapStart;
+  while (MemoryMap < MemoryMapEnd) {
+    MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
+                                      EFI_MEMORY_XP);
+    MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
+  }
+  MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
+
   Status = EFI_SUCCESS;
 
 Done:
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 6cf5edcbe5..75d9b14c1f 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -182,7 +182,6 @@ SortMemoryMap (
                                  it is the size of new memory map after merge.
   @param  DescriptorSize         Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
 **/
-STATIC
 VOID
 MergeMemoryMap (
   IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
-- 
2.14.1.windows.1

Re: [PATCH v7 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities

[Zeng, Star]

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

-----Original Message-----
From: Wang, Jian J 
Sent: Tuesday, November 21, 2017 2:17 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH v7 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities

> v7:
>   Merge memory map after filtering code

Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really set attributes and change memory paging attribute accordingly.
But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by value from Capabilities in GCD memory map. This might cause boot problems. Clearing all paging related capabilities can workaround it. The code added in this patch is supposed to be removed once the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in UEFI spec and adopted by both EDK-II Core and all supported OSs.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++++++++++++++
 MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++++++++++++++++
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h index 1a0babba71..07b86ba696 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2948,4 +2948,22 @@ ApplyMemoryProtectionPolicy (
   IN  UINT64                Length
   );
 
+/**
+  Merge continous memory map entries whose have same attributes.
+
+  @param  MemoryMap       A pointer to the buffer in which firmware places
+                          the current memory map.
+  @param  MemoryMapSize   A pointer to the size, in bytes, of the
+                          MemoryMap buffer. On input, this is the size of
+                          the current memory map.  On output,
+                          it is the size of new memory map after merge.
+  @param  DescriptorSize  Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+VOID
+MergeMemoryMap (
+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
+  IN OUT UINTN                  *MemoryMapSize,
+  IN UINTN                      DescriptorSize
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index f1e4a37f2a..9ff73efdfa 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1687,6 +1687,7 @@ CoreGetMemoryMap (
   EFI_GCD_MAP_ENTRY                 MergeGcdMapEntry;
   EFI_MEMORY_TYPE                   Type;
   EFI_MEMORY_DESCRIPTOR             *MemoryMapStart;
+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEnd;
 
   //
   // Make sure the parameters are valid @@ -1896,6 +1897,26 @@ CoreGetMemoryMap (
   //
   BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
 
+  //
+  // Note: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really
+  //       set attributes and change memory paging attribute accordingly.
+  //       But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by
+  //       value from Capabilities in GCD memory map. This might cause
+  //       boot problems. Clearing all paging related capabilities can
+  //       workaround it. Following code is supposed to be removed once
+  //       the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in
+  //       UEFI spec and adopted by both EDK-II Core and all supported
+  //       OSs.
+  //
+  MemoryMapEnd = MemoryMap;
+  MemoryMap = MemoryMapStart;
+  while (MemoryMap < MemoryMapEnd) {
+    MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
+                                      EFI_MEMORY_XP);
+    MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);  }  
+ MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
+
   Status = EFI_SUCCESS;
 
 Done:
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 6cf5edcbe5..75d9b14c1f 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -182,7 +182,6 @@ SortMemoryMap (
                                  it is the size of new memory map after merge.
   @param  DescriptorSize         Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
 **/
-STATIC
 VOID
 MergeMemoryMap (
   IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
--
2.14.1.windows.1

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

[Jian J Wang]

> v7:
>    No change.

> v6:
>    Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP

> 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: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 21 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 309f448a83..db41fa401f 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -769,6 +769,20 @@ AssignMemoryPageAttributes (
   return Status;
 }
 
+/**
+ Check if Execute Disable feature is enabled or not.
+**/
+BOOLEAN
+IsExecuteDisableEnabled (
+  VOID
+  )
+{
+  MSR_CORE_IA32_EFER_REGISTER    MsrEfer;
+
+  MsrEfer.Uint64 = AsmReadMsr64 (MSR_IA32_EFER);
+  return (MsrEfer.Bits.NXE == 1);
+}
+
 /**
   Update GCD memory space attributes according to current page table setup.
 **/
@@ -790,7 +804,7 @@ RefreshGcdMemoryAttributesFromPaging (
   UINT64                              PageStartAddress;
   UINT64                              Attributes;
   UINT64                              Capabilities;
-  BOOLEAN                             DoUpdate;
+  UINT64                              NewAttributes;
   UINTN                               Index;
 
   //
@@ -802,17 +816,50 @@ RefreshGcdMemoryAttributesFromPaging (
 
   GetCurrentPagingContext (&PagingContext);
 
-  DoUpdate      = FALSE;
-  Capabilities  = 0;
-  Attributes    = 0;
-  BaseAddress   = 0;
-  PageLength    = 0;
+  Attributes      = 0;
+  NewAttributes   = 0;
+  BaseAddress     = 0;
+  PageLength      = 0;
+
+  if (IsExecuteDisableEnabled ()) {
+    Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
+  } else {
+    Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP;
+  }
 
   for (Index = 0; Index < NumberOfDescriptors; Index++) {
     if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
       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 | Capabilities
+                    );
+    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, MemorySpaceMap[Index].BaseAddress,
+        MemorySpaceMap[Index].BaseAddress + MemorySpaceMap[Index].Length - 1,
+        MemorySpaceMap[Index].Capabilities,
+        MemorySpaceMap[Index].Capabilities | Capabilities
+        ));
+      continue;
+    }
+
     if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
       //
       // Current memory space starts at a new page. Resetting PageLength will
@@ -826,7 +873,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 +891,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

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> 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.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this
because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this
because I've put a lot of BZ writeup, and patch review and testing
effort for this series, and I'd like the commit log to reflect that.


Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a
separate patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
Reviewed-by tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't
think I can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it
will provide the desired result for the code as well.

Thanks
Laszlo

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Zeng, Star]

How about we have the v6 patch series in first with the feedback from Jiewen (about comments) and you (about MemoryMapStart) addressed?

Then we can have a separated patch for the merging.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, November 21, 2017 9:38 PM
To: Wang, Jian J <jian.j.wang@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

Jian,

On 11/21/17 07:17, Jian J Wang wrote:
>> v7:
>>   Merge memory map after filtering paging attributes
> 
> 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.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 
>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>  4 files changed, 112 insertions(+), 22 deletions(-)
> 

I don't have capacity to retest and re-review the series.

Considering the following two options, I like none of them:

(1) Version 7 is merged with my feedback tags from v6. I don't like this because I didn't review or test version 7.

(2) Version 7 is merged without my feedback tags. I don't like this because I've put a lot of BZ writeup, and patch review and testing effort for this series, and I'd like the commit log to reflect that.


Instead, I would like to request the following, for v8:

Please submit a series that consists of three patches:

- patch v8 1/3: identical to v6 1/2, except for the code comment update,
- patch v8 2/3: identical to v6 2/2,
- patch v8 3/3: please implement the merging of the memory map as a separate patch.

Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my Reviewed-by tags, from v6.

Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can find the capacity for that at the moment.

This approach will correctly reflect all the work done thus far, and it will provide the desired result for the code as well.

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

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Yao, Jiewen]

I am OK on that.

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, November 22, 2017 3:57 PM
> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> How about we have the v6 patch series in first with the feedback from Jiewen
> (about comments) and you (about MemoryMapStart) addressed?
> 
> Then we can have a separated patch for the merging.
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
> >> v7:
> >>   Merge memory map after filtering paging attributes
> >
> > 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.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94
> +++++++++++++++++++++-------
> >  4 files changed, 112 insertions(+), 22 deletions(-)
> >
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this because I
> didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this because I've
> put a lot of BZ writeup, and patch review and testing effort for this series, and I'd
> like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a separate
> patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can
> find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it will provide
> the desired result for the code as well.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Laszlo Ersek]

On 11/22/17 08:56, Zeng, Star wrote:
> How about we have the v6 patch series in first with the feedback from Jiewen (about comments) and you (about MemoryMapStart) addressed?
> 
> Then we can have a separated patch for the merging.

Good idea!

Thanks!
Laszlo


> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
>>> v7:
>>>   Merge memory map after filtering paging attributes
>>
>> 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.
>>
>> Jian J Wang (2):
>>   MdeModulePkg/DxeCore: Filter out all paging capabilities
>>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
>>
>>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
>>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
>>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
>>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++-------
>>  4 files changed, 112 insertions(+), 22 deletions(-)
>>
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this because I didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this because I've put a lot of BZ writeup, and patch review and testing effort for this series, and I'd like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a separate patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't think I can find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it will provide the desired result for the code as well.
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Sorry just see this email. I just replied another one. Great to know it works for both of us.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 22, 2017 5:05 PM
> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> On 11/22/17 08:56, Zeng, Star wrote:
> > How about we have the v6 patch series in first with the feedback from Jiewen
> (about comments) and you (about MemoryMapStart) addressed?
> >
> > Then we can have a separated patch for the merging.
> 
> Good idea!
> 
> Thanks!
> Laszlo
> 
> 
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> > Sent: Tuesday, November 21, 2017 9:38 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> >
> > Jian,
> >
> > On 11/21/17 07:17, Jian J Wang wrote:
> >>> v7:
> >>>   Merge memory map after filtering paging attributes
> >>
> >> 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.
> >>
> >> Jian J Wang (2):
> >>   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >>   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >>
> >>  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >>  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >>  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >>  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++--
> -----
> >>  4 files changed, 112 insertions(+), 22 deletions(-)
> >>
> >
> > I don't have capacity to retest and re-review the series.
> >
> > Considering the following two options, I like none of them:
> >
> > (1) Version 7 is merged with my feedback tags from v6. I don't like this because
> I didn't review or test version 7.
> >
> > (2) Version 7 is merged without my feedback tags. I don't like this because I've
> put a lot of BZ writeup, and patch review and testing effort for this series, and
> I'd like the commit log to reflect that.
> >
> >
> > Instead, I would like to request the following, for v8:
> >
> > Please submit a series that consists of three patches:
> >
> > - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> > - patch v8 2/3: identical to v6 2/2,
> > - patch v8 3/3: please implement the merging of the memory map as a
> separate patch.
> >
> > Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> >
> > Patch v8 3/3 should be reviewed / tested separately by others. I don't think I
> can find the capacity for that at the moment.
> >
> > This approach will correctly reflect all the work done thus far, and it will
> provide the desired result for the code as well.
> >
> > Thanks
> > Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >

Re: [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory map

[Wang, Jian J]

Laszlo,

Thanks for the comments. Sorry for the commit log which doesn't meet the requirement.
I appreciate everything you did to this patch series. It's not intended to ignore them in log.
I think I just need more time to get used to the commit conventions.

I've explained the reason why v7 doesn't need a re-test in another email. But I understand
it if you insist re-test is necessary. Star and Jiewen have given a proposal, similar to yours
but putting the "merge" into a new patch instead of in this series. I think it's feasible. Let me
know your opinion.

Again, thanks for all your valuable comments and test efforts on this series and all others.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 9:38 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v7 0/2] Fix multiple entries of RT_CODE in memory
> map
> 
> Jian,
> 
> On 11/21/17 07:17, Jian J Wang wrote:
> >> v7:
> >>   Merge memory map after filtering paging attributes
> >
> > 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.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeCore: Filter out all paging capabilities
> >   UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> >
> >  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c             | 21 +++++++
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c             | 94 +++++++++++++++++++++----
> ---
> >  4 files changed, 112 insertions(+), 22 deletions(-)
> >
> 
> I don't have capacity to retest and re-review the series.
> 
> Considering the following two options, I like none of them:
> 
> (1) Version 7 is merged with my feedback tags from v6. I don't like this
> because I didn't review or test version 7.
> 
> (2) Version 7 is merged without my feedback tags. I don't like this
> because I've put a lot of BZ writeup, and patch review and testing
> effort for this series, and I'd like the commit log to reflect that.
> 
> 
> Instead, I would like to request the following, for v8:
> 
> Please submit a series that consists of three patches:
> 
> - patch v8 1/3: identical to v6 1/2, except for the code comment update,
> - patch v8 2/3: identical to v6 2/2,
> - patch v8 3/3: please implement the merging of the memory map as a
> separate patch.
> 
> Patches v8 1/3 and 2/3 should include *both* my Tested-by *and* my
> Reviewed-by tags, from v6.
> 
> Patch v8 3/3 should be reviewed / tested separately by others. I don't
> think I can find the capacity for that at the moment.
> 
> This approach will correctly reflect all the work done thus far, and it
> will provide the desired result for the code as well.
> 
> Thanks
> Laszlo