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

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

[Jian J Wang]

> 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

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

[Jian J Wang]

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

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

[Ard Biesheuvel]

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
>

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

[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
>

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

[Ard Biesheuvel]

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

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

[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

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

[Ard Biesheuvel]

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

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

[Yao, Jiewen]

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

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

[Wang, Jian J]

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

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

[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

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

[Laszlo Ersek]

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

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

[Wang, Jian J]

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

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

[Jian J Wang]

> 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

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

[Laszlo Ersek]

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;
> 

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

[Wang, Jian J]

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;
> >

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

[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

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

[Laszlo Ersek]

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