* [PATCH v8 1/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-23 2:12 [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
@ 2017-11-23 2:12 ` Jian J Wang
2017-11-23 2:12 ` [PATCH v8 2/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
2017-11-23 16:45 ` [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Jian J Wang @ 2017-11-23 2:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao, Star Zeng, Laszlo Ersek, Ard Biesheuvel
> v7/v8:
> 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: Star Zeng <star.zeng@intel.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 76f44f9bd1..9658ed74c5 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v8 2/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-23 2:12 [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-23 2:12 ` [PATCH v8 1/2] UefiCpuPkg/CpuDxe: " Jian J Wang
@ 2017-11-23 2:12 ` Jian J Wang
2017-11-23 16:45 ` [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Jian J Wang @ 2017-11-23 2:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Star Zeng, Laszlo Ersek, Ard Biesheuvel
> v8:
> Remove merge code but keep code to remain MemoryMapStart
> v7:
> Merge memory map after filtering code
> v6:
> Add code filter out all paging capabilities
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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/Core/Dxe/Mem/Page.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 2034b64cd7..962ae90d3d 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,25 @@ 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);
+ }
+
Status = EFI_SUCCESS;
Done:
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map
2017-11-23 2:12 [PATCH v8 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-23 2:12 ` [PATCH v8 1/2] UefiCpuPkg/CpuDxe: " Jian J Wang
2017-11-23 2:12 ` [PATCH v8 2/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
@ 2017-11-23 16:45 ` Laszlo Ersek
2 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2017-11-23 16:45 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
On 11/23/17 03:12, Jian J Wang wrote:
>> v8:
>> Remove merge code but keep code to remain MemoryMapStart
>
>> v7:
>> Merge memory map after filtering paging attributes
>
>> 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
>
> Laszlo did a thorough test on OVMF emulated platform. The details can be found
> at https://bugzilla.tianocore.org/show_bug.cgi?id=753#c10
>
> 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>
>
> Jian J Wang (2):
> UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> MdeModulePkg/DxeCore: Filter out all paging capabilities
>
> MdeModulePkg/Core/Dxe/Mem/Page.c | 20 +++++++++
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
> 2 files changed, 93 insertions(+), 21 deletions(-)
>
Many thanks, Jian; this version looks great. From my side, please feel
free to push it.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 4+ messages in thread