* [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map
@ 2017-11-16 7:26 Jian J Wang
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Jian J Wang @ 2017-11-16 7:26 UTC (permalink / raw)
To: edk2-devel
> v6
> a. Add workaround in core to filter out all paging related capabilities.
> This is to fix boot issue in Fedora 26 and Windows Server 2016.
> b. Add code to check if EFI_MEMORY_XP should be added for GCD memory map
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/Mem/Page.c | 17 ++++++++
UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
2 files changed, 90 insertions(+), 21 deletions(-)
--
2.14.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 7:26 [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
@ 2017-11-16 7:26 ` Jian J Wang
2017-11-16 9:24 ` Ard Biesheuvel
` (2 more replies)
2017-11-16 7:27 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-20 21:08 ` [PATCH v6 0/2] " Laszlo Ersek
2 siblings, 3 replies; 17+ messages in thread
From: Jian J Wang @ 2017-11-16 7:26 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Star Zeng, Laszlo Ersek, Ard Biesheuvel
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>
---
MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index c9219cc068..783b576e35 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
//
BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
+ //
+ // WORKAROUND: 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.
+ //
+ while (MemoryMapStart < MemoryMap) {
+ MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
+ EFI_MEMORY_XP);
+ MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
+ }
+
Status = EFI_SUCCESS;
Done:
--
2.14.1.windows.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-16 7:26 [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
@ 2017-11-16 7:27 ` Jian J Wang
2017-11-20 20:31 ` Laszlo Ersek
2017-11-22 7:54 ` Zeng, Star
2017-11-20 21:08 ` [PATCH v6 0/2] " Laszlo Ersek
2 siblings, 2 replies; 17+ messages in thread
From: Jian J Wang @ 2017-11-16 7:27 UTC (permalink / raw)
To: edk2-devel; +Cc: Eric Dong, Jiewen Yao, Star Zeng, Laszlo Ersek, Ard Biesheuvel
> 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>
---
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 d312eb66f8..3297c1900b 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] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
@ 2017-11-16 9:24 ` Ard Biesheuvel
2017-11-16 9:28 ` Zeng, Star
2017-11-17 1:37 ` Yao, Jiewen
2017-11-20 20:23 ` Laszlo Ersek
2 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-16 9:24 UTC (permalink / raw)
To: Jian J Wang; +Cc: edk2-devel@lists.01.org, Jiewen Yao, Star Zeng, Laszlo Ersek
On 16 November 2017 at 07:26, Jian J Wang <jian.j.wang@intel.com> wrote:
> 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>
> ---
> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index c9219cc068..783b576e35 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> //
> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>
> + //
> + // WORKAROUND: 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.
> + //
> + while (MemoryMapStart < MemoryMap) {
> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
> + EFI_MEMORY_XP);
Why is EFI_MEMORY_WP missing here?
> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
> + }
> +
> Status = EFI_SUCCESS;
>
> Done:
> --
> 2.14.1.windows.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 9:24 ` Ard Biesheuvel
@ 2017-11-16 9:28 ` Zeng, Star
2017-11-16 9:29 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-11-16 9:28 UTC (permalink / raw)
To: Ard Biesheuvel, Wang, Jian J
Cc: edk2-devel@lists.01.org, Yao, Jiewen, Laszlo Ersek, Zeng, Star
Ard,
EFI_MEMORY_WP is for cache.
UefiSpec.h
//
// Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected physical memory
// protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
//
#define EFI_MEMORY_WP 0x0000000000001000ULL
Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Thursday, November 16, 2017 5:25 PM
To: 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>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
On 16 November 2017 at 07:26, Jian J Wang <jian.j.wang@intel.com> wrote:
> 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>
> ---
> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index c9219cc068..783b576e35 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> //
> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>
> + //
> + // WORKAROUND: 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.
> + //
> + while (MemoryMapStart < MemoryMap) {
> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
> + EFI_MEMORY_XP);
Why is EFI_MEMORY_WP missing here?
> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size); }
> +
> Status = EFI_SUCCESS;
>
> Done:
> --
> 2.14.1.windows.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 9:28 ` Zeng, Star
@ 2017-11-16 9:29 ` Ard Biesheuvel
2017-11-16 9:48 ` Zeng, Star
0 siblings, 1 reply; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-16 9:29 UTC (permalink / raw)
To: Zeng, Star
Cc: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org, Yao, Jiewen
On 16 November 2017 at 09:28, Zeng, Star <star.zeng@intel.com> wrote:
> Ard,
>
> EFI_MEMORY_WP is for cache.
>
> UefiSpec.h
> //
> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as write-protected physical memory
> // protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
> //
> #define EFI_MEMORY_WP 0x0000000000001000ULL
>
Yes, but that was a change in v2.5, before that it was a permission
attribute. So it should be filtered from the OS visible memory map as
well.
> Thanks,
> Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, November 16, 2017 5:25 PM
> To: 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>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
>
> On 16 November 2017 at 07:26, Jian J Wang <jian.j.wang@intel.com> wrote:
>> 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>
>> ---
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
>> index c9219cc068..783b576e35 100644
>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
>> //
>> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>>
>> + //
>> + // WORKAROUND: 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.
>> + //
>> + while (MemoryMapStart < MemoryMap) {
>> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
>> + EFI_MEMORY_XP);
>
> Why is EFI_MEMORY_WP missing here?
>
>> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size); }
>> +
>> Status = EFI_SUCCESS;
>>
>> Done:
>> --
>> 2.14.1.windows.1
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 9:29 ` Ard Biesheuvel
@ 2017-11-16 9:48 ` Zeng, Star
2017-11-16 16:06 ` Ard Biesheuvel
0 siblings, 1 reply; 17+ messages in thread
From: Zeng, Star @ 2017-11-16 9:48 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org, Yao, Jiewen,
Zeng, Star
As I remember UEFI 2.5 clarified this and added EFI_MEMORY_RO was because EFI_MEMORY_WP had been typically used for cache even before UEFI 2.5.
And I do not think this patch should filter out EFI_MEMORY_WP since this patch is to filter out new paging bits caused by 14dde9e903bb9a719ebb8f3381da72b19509bc36 [MdeModulePkg/Core: Fix out-of-sync issue in GCD].
Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Thursday, November 16, 2017 5:30 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
On 16 November 2017 at 09:28, Zeng, Star <star.zeng@intel.com> wrote:
> Ard,
>
> EFI_MEMORY_WP is for cache.
>
> UefiSpec.h
> //
> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as
> write-protected physical memory // protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
> //
> #define EFI_MEMORY_WP 0x0000000000001000ULL
>
Yes, but that was a change in v2.5, before that it was a permission attribute. So it should be filtered from the OS visible memory map as well.
> Thanks,
> Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, November 16, 2017 5:25 PM
> To: 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>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
> paging capabilities
>
> On 16 November 2017 at 07:26, Jian J Wang <jian.j.wang@intel.com> wrote:
>> 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>
>> ---
>> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
>> index c9219cc068..783b576e35 100644
>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
>> //
>> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>>
>> + //
>> + // WORKAROUND: 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.
>> + //
>> + while (MemoryMapStart < MemoryMap) {
>> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
>> + EFI_MEMORY_XP);
>
> Why is EFI_MEMORY_WP missing here?
>
>> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
>> + }
>> +
>> Status = EFI_SUCCESS;
>>
>> Done:
>> --
>> 2.14.1.windows.1
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 9:48 ` Zeng, Star
@ 2017-11-16 16:06 ` Ard Biesheuvel
0 siblings, 0 replies; 17+ messages in thread
From: Ard Biesheuvel @ 2017-11-16 16:06 UTC (permalink / raw)
To: Zeng, Star; +Cc: edk2-devel@lists.01.org, Laszlo Ersek, Yao, Jiewen
On 16 November 2017 at 09:48, Zeng, Star <star.zeng@intel.com> wrote:
> As I remember UEFI 2.5 clarified this and added EFI_MEMORY_RO was because EFI_MEMORY_WP had been typically used for cache even before UEFI 2.5.
>
> And I do not think this patch should filter out EFI_MEMORY_WP since this patch is to filter out new paging bits caused by 14dde9e903bb9a719ebb8f3381da72b19509bc36 [MdeModulePkg/Core: Fix out-of-sync issue in GCD].
>
Ah ok. If the bit does not get copied from the GCD map then there is
no need to filter it here.
>
> Thanks,
> Star
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, November 16, 2017 5:30 PM
> To: Zeng, Star <star.zeng@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
>
> On 16 November 2017 at 09:28, Zeng, Star <star.zeng@intel.com> wrote:
>> Ard,
>>
>> EFI_MEMORY_WP is for cache.
>>
>> UefiSpec.h
>> //
>> // Note: UEFI spec 2.5 and following: use EFI_MEMORY_RO as
>> write-protected physical memory // protection attribute. Also, EFI_MEMORY_WP means cacheability attribute.
>> //
>> #define EFI_MEMORY_WP 0x0000000000001000ULL
>>
>
> Yes, but that was a change in v2.5, before that it was a permission attribute. So it should be filtered from the OS visible memory map as well.
>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Thursday, November 16, 2017 5:25 PM
>> To: 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>; Laszlo Ersek <lersek@redhat.com>
>> Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
>> paging capabilities
>>
>> On 16 November 2017 at 07:26, Jian J Wang <jian.j.wang@intel.com> wrote:
>>> 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>
>>> ---
>>> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> index c9219cc068..783b576e35 100644
>>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
>>> //
>>> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>>>
>>> + //
>>> + // WORKAROUND: 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.
>>> + //
>>> + while (MemoryMapStart < MemoryMap) {
>>> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
>>> + EFI_MEMORY_XP);
>>
>> Why is EFI_MEMORY_WP missing here?
>>
>>> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
>>> + }
>>> +
>>> Status = EFI_SUCCESS;
>>>
>>> Done:
>>> --
>>> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
2017-11-16 9:24 ` Ard Biesheuvel
@ 2017-11-17 1:37 ` Yao, Jiewen
2017-11-17 2:48 ` Wang, Jian J
2017-11-20 20:23 ` Laszlo Ersek
2 siblings, 1 reply; 17+ messages in thread
From: Yao, Jiewen @ 2017-11-17 1:37 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Zeng, Star, Laszlo Ersek, Ard Biesheuvel
HI
I have 2 comments:
1) I do not think we need mention: WORKAROUND.
I suggest we just use "NOTE".
We have similar example before, see MdePkg\Library\BasePeCoffLib\BasePeCoff.c
//
// NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic value
// in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
// Magic value in the OptionalHeader is EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
// then override the returned value to EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
//
2) I agree with Star. I think we should merge the final result.
The suggestion before is: *Keep current UEFI memory map unchanged.*
Changing it brings lots of risk without validating all UEFI OS.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 16, 2017 3:27 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 v6 1/2] MdeModulePkg/DxeCore: 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>
> ---
> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index c9219cc068..783b576e35 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> //
> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>
> + //
> + // WORKAROUND: 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.
> + //
> + while (MemoryMapStart < MemoryMap) {
> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> EFI_MEMORY_RO |
> + EFI_MEMORY_XP);
> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart,
> Size);
> + }
> +
> Status = EFI_SUCCESS;
>
> Done:
> --
> 2.14.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-17 1:37 ` Yao, Jiewen
@ 2017-11-17 2:48 ` Wang, Jian J
2017-11-22 7:30 ` Zeng, Star
0 siblings, 1 reply; 17+ messages in thread
From: Wang, Jian J @ 2017-11-17 2:48 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel@lists.01.org
Cc: Zeng, Star, Laszlo Ersek, Ard Biesheuvel, Kinney, Michael D
1) Sure. I'll update the wording.
2) I still think this is just a workaround . In the long run, I don't think the merge is a good idea. It looks like it will "fix" more issues, but actually it just "hide" them and would cause more and more workaround in the future. Anyway, if no one else has objections, I'll update the code.
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, November 17, 2017 9:37 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging
> capabilities
>
> HI
> I have 2 comments:
>
> 1) I do not think we need mention: WORKAROUND.
> I suggest we just use "NOTE".
>
> We have similar example before, see
> MdePkg\Library\BasePeCoffLib\BasePeCoff.c
> //
> // NOTE: Some versions of Linux ELILO for Itanium have an incorrect magic
> value
> // in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> // Magic value in the OptionalHeader is
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> // then override the returned value to
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> //
>
> 2) I agree with Star. I think we should merge the final result.
> The suggestion before is: *Keep current UEFI memory map unchanged.*
> Changing it brings lots of risk without validating all UEFI OS.
>
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 16, 2017 3:27 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 v6 1/2] MdeModulePkg/DxeCore: 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>
> > ---
> > MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index c9219cc068..783b576e35 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> > //
> > BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
> >
> > + //
> > + // WORKAROUND: 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.
> > + //
> > + while (MemoryMapStart < MemoryMap) {
> > + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> > EFI_MEMORY_RO |
> > + EFI_MEMORY_XP);
> > + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart,
> > Size);
> > + }
> > +
> > Status = EFI_SUCCESS;
> >
> > Done:
> > --
> > 2.14.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
2017-11-16 9:24 ` Ard Biesheuvel
2017-11-17 1:37 ` Yao, Jiewen
@ 2017-11-20 20:23 ` Laszlo Ersek
2017-11-21 6:29 ` Wang, Jian J
2 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-20 20:23 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Star Zeng, Ard Biesheuvel
On 11/16/17 08:26, Jian J Wang wrote:
> 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>
> ---
> MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index c9219cc068..783b576e35 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> //
> BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
>
> + //
> + // WORKAROUND: 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.
> + //
> + while (MemoryMapStart < MemoryMap) {
> + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
> + EFI_MEMORY_XP);
> + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
> + }
> +
> Status = EFI_SUCCESS;
>
> Done:
>
OK, let me check if I understand the discussion thus far, for this patch:
- Ard asked about EFI_MEMORY_WP, but clearing that bit is not necessary
because CpuDxe does not add it anyway, in the GCD memory space map.
- The code comment might be updated (according to Jiewen's suggestion)
before pushing the patch. I don't have any particular opinion about the
comment.
- If I understand correctly, Jiewen agrees with applying both patches in
this series.
I have one superficial comment on this patch: in the CoreGetMemoryMap()
function, we keep "MemoryMapStart" unchanged (after the initial
assignment), and keep incrementing "MemoryMap". At the location where
the new code is being added, "MemoryMap" points one past the last
descriptor, and "MemoryMapStart" still points to the first one.
In order to keep this property for possible future "scans" of the full
map, I would prefer keeping "MemoryMapStart" unchanged in this location,
and using an independent loop variable.
... On the other hand, we can easily restore "MemoryMapStart", should it
be necessary, with the help of "BufferSize". So, I guess the function
does not become more difficult to work with after this patch.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(I hope that Star and/or Jiewen will also R-b this patch, possibly with
the updated code comment.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-16 7:27 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
@ 2017-11-20 20:31 ` Laszlo Ersek
2017-11-21 6:51 ` Wang, Jian J
2017-11-22 7:54 ` Zeng, Star
1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-20 20:31 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Jiewen Yao, Eric Dong, Star Zeng, Ard Biesheuvel
On 11/16/17 08:27, Jian J Wang wrote:
>> v6:
>> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
Another change relative to v5 is the fixing of the first DEBUG_WARN
message -- in my v5 review I had missed that the DEBUG_WARN arguments
didn't match the preceding gDS->SetMemorySpaceCapabilities() arguments.
Yet another change that could have been (maybe) possible for this patch
is to replace the remaining occurrences of EFI_MEMORY_PAGETYPE_MASK with
"Capabilities". Namely, in v6, the attributes are enforced on a mask
that is possibly wider than supported by the hardware (i.e., in case NX
is not supported).
However, this should not be a functionality problem, because with NX
unavailable, the GetAttributesFromPageEntry() function should never
return EFI_MEMORY_XP. Thus, the "wider than needed" attribute setting
will just clear EFI_MEMORY_XP.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(I hope that Star and/or Jiewen will also R-b this patch.)
In addition, I will follow up with test results for the series.
Thanks
Laszlo
>> 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>
> ---
> 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 d312eb66f8..3297c1900b 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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map
2017-11-16 7:26 [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
2017-11-16 7:27 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
@ 2017-11-20 21:08 ` Laszlo Ersek
2 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-11-20 21:08 UTC (permalink / raw)
To: Jian J Wang; +Cc: edk2-devel
On 11/16/17 08:26, Jian J Wang wrote:
>> v6
>> a. Add workaround in core to filter out all paging related capabilities.
>> This is to fix boot issue in Fedora 26 and Windows Server 2016.
>> b. Add code to check if EFI_MEMORY_XP should be added for GCD memory map
>
> 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/Mem/Page.c | 17 ++++++++
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 94 +++++++++++++++++++++++++++++++---------
> 2 files changed, 90 insertions(+), 21 deletions(-)
>
Series
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-20 20:23 ` Laszlo Ersek
@ 2017-11-21 6:29 ` Wang, Jian J
0 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-21 6:29 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Zeng, Star, Ard Biesheuvel
Thanks for the comments. V7 patch has added one more variable so that
MemoryMapStart will be kept intact.
V7 added code to merge memory map after filtering. I have verified that
the output keeps the same as v6 on both OVMF and our real platform
(default platform configuration). I think it may be not necessary to validate
all OS boot again. But if you want and have time, you can do it anyway.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 4:23 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
> paging capabilities
>
> On 11/16/17 08:26, Jian J Wang wrote:
> > 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>
> > ---
> > MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index c9219cc068..783b576e35 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> > //
> > BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
> >
> > + //
> > + // WORKAROUND: 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.
> > + //
> > + while (MemoryMapStart < MemoryMap) {
> > + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> EFI_MEMORY_RO |
> > + EFI_MEMORY_XP);
> > + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size);
> > + }
> > +
> > Status = EFI_SUCCESS;
> >
> > Done:
> >
>
> OK, let me check if I understand the discussion thus far, for this patch:
>
> - Ard asked about EFI_MEMORY_WP, but clearing that bit is not necessary
> because CpuDxe does not add it anyway, in the GCD memory space map.
>
> - The code comment might be updated (according to Jiewen's suggestion)
> before pushing the patch. I don't have any particular opinion about the
> comment.
>
> - If I understand correctly, Jiewen agrees with applying both patches in
> this series.
>
>
> I have one superficial comment on this patch: in the CoreGetMemoryMap()
> function, we keep "MemoryMapStart" unchanged (after the initial
> assignment), and keep incrementing "MemoryMap". At the location where
> the new code is being added, "MemoryMap" points one past the last
> descriptor, and "MemoryMapStart" still points to the first one.
>
> In order to keep this property for possible future "scans" of the full
> map, I would prefer keeping "MemoryMapStart" unchanged in this location,
> and using an independent loop variable.
>
> ... On the other hand, we can easily restore "MemoryMapStart", should it
> be necessary, with the help of "BufferSize". So, I guess the function
> does not become more difficult to work with after this patch.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (I hope that Star and/or Jiewen will also R-b this patch, possibly with
> the updated code comment.)
>
> Thanks,
> Laszlo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-20 20:31 ` Laszlo Ersek
@ 2017-11-21 6:51 ` Wang, Jian J
0 siblings, 0 replies; 17+ messages in thread
From: Wang, Jian J @ 2017-11-21 6:51 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Dong, Eric, Zeng, Star, Ard Biesheuvel
You're right about XP/NX in this function. But from DxeCore or other drivers
perspective, they have no knowledge of current capability of NX. I think it's the
responsibility of cpu driver to add/remove it for the sake of GCD.
Actually Star has filed a bz to use GCD service instead of CPU arch protocol to
change memory attributes in DxeCore (like enforce image protection). If the
knowledge between GCD and cpu mismatch, the GCD may not do right thing
upon requests. For example, if we always add XP to capability but current cpu
doesn't support it, the DxeCore or other drivers may still try to enable image
protection which won't take into effect actually.
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 21, 2017 4:32 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
>
> On 11/16/17 08:27, Jian J Wang wrote:
> >> v6:
> >> Add ExecuteDisable feature check to include/exclude EFI_MEMORY_XP
>
> Another change relative to v5 is the fixing of the first DEBUG_WARN
> message -- in my v5 review I had missed that the DEBUG_WARN arguments
> didn't match the preceding gDS->SetMemorySpaceCapabilities() arguments.
>
> Yet another change that could have been (maybe) possible for this patch
> is to replace the remaining occurrences of EFI_MEMORY_PAGETYPE_MASK with
> "Capabilities". Namely, in v6, the attributes are enforced on a mask
> that is possibly wider than supported by the hardware (i.e., in case NX
> is not supported).
>
> However, this should not be a functionality problem, because with NX
> unavailable, the GetAttributesFromPageEntry() function should never
> return EFI_MEMORY_XP. Thus, the "wider than needed" attribute setting
> will just clear EFI_MEMORY_XP.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> (I hope that Star and/or Jiewen will also R-b this patch.)
>
> In addition, I will follow up with test results for the series.
>
> Thanks
> Laszlo
>
> >> 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>
> > ---
> > 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 d312eb66f8..3297c1900b 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;
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
2017-11-17 2:48 ` Wang, Jian J
@ 2017-11-22 7:30 ` Zeng, Star
0 siblings, 0 replies; 17+ messages in thread
From: Zeng, Star @ 2017-11-22 7:30 UTC (permalink / raw)
To: Wang, Jian J, Yao, Jiewen, edk2-devel@lists.01.org
Cc: Laszlo Ersek, Ard Biesheuvel, Kinney, Michael D, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com> if the feedback from Jiewen (about comments) and Laszlo (about MemoryMapStart) has been addressed, and the merging will be done in a separated patch.
Thanks,
Star
-----Original Message-----
From: Wang, Jian J
Sent: Friday, November 17, 2017 10:49 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities
1) Sure. I'll update the wording.
2) I still think this is just a workaround . In the long run, I don't think the merge is a good idea. It looks like it will "fix" more issues, but actually it just "hide" them and would cause more and more workaround in the future. Anyway, if no one else has objections, I'll update the code.
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Friday, November 17, 2017 9:37 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: RE: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all
> paging capabilities
>
> HI
> I have 2 comments:
>
> 1) I do not think we need mention: WORKAROUND.
> I suggest we just use "NOTE".
>
> We have similar example before, see
> MdePkg\Library\BasePeCoffLib\BasePeCoff.c
> //
> // NOTE: Some versions of Linux ELILO for Itanium have an incorrect
> magic value
> // in the PE/COFF Header. If the MachineType is Itanium(IA64) and the
> // Magic value in the OptionalHeader is
> EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC
> // then override the returned value to
> EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC
> //
>
> 2) I agree with Star. I think we should merge the final result.
> The suggestion before is: *Keep current UEFI memory map unchanged.*
> Changing it brings lots of risk without validating all UEFI OS.
>
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 16, 2017 3:27 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 v6 1/2] MdeModulePkg/DxeCore: 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>
> > ---
> > MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index c9219cc068..783b576e35 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1829,6 +1829,23 @@ CoreGetMemoryMap (
> > //
> > BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart);
> >
> > + //
> > + // WORKAROUND: 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.
> > + //
> > + while (MemoryMapStart < MemoryMap) {
> > + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> > EFI_MEMORY_RO |
> > + EFI_MEMORY_XP);
> > + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart,
> > Size);
> > + }
> > +
> > Status = EFI_SUCCESS;
> >
> > Done:
> > --
> > 2.14.1.windows.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
2017-11-16 7:27 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-20 20:31 ` Laszlo Ersek
@ 2017-11-22 7:54 ` Zeng, Star
1 sibling, 0 replies; 17+ messages in thread
From: Zeng, Star @ 2017-11-22 7:54 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Dong, Eric, Yao, Jiewen, Laszlo Ersek, Ard Biesheuvel, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Thursday, November 16, 2017 3:27 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric <eric.dong@intel.com>; 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 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
> 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>
---
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 d312eb66f8..3297c1900b 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] 17+ messages in thread
end of thread, other threads:[~2017-11-22 7:50 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-16 7:26 [PATCH v6 0/2] Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-16 7:26 ` [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities Jian J Wang
2017-11-16 9:24 ` Ard Biesheuvel
2017-11-16 9:28 ` Zeng, Star
2017-11-16 9:29 ` Ard Biesheuvel
2017-11-16 9:48 ` Zeng, Star
2017-11-16 16:06 ` Ard Biesheuvel
2017-11-17 1:37 ` Yao, Jiewen
2017-11-17 2:48 ` Wang, Jian J
2017-11-22 7:30 ` Zeng, Star
2017-11-20 20:23 ` Laszlo Ersek
2017-11-21 6:29 ` Wang, Jian J
2017-11-16 7:27 ` [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-20 20:31 ` Laszlo Ersek
2017-11-21 6:51 ` Wang, Jian J
2017-11-22 7:54 ` Zeng, Star
2017-11-20 21:08 ` [PATCH v6 0/2] " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox