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

[Wang, Jian J]

Sorry guys. I did update the title but v5 is still missing. A new one will be 
sent out.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, November 10, 2017 8:42 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen@intel.com>;
> Dong, Eric <eric.d...@intel.com>
> Subject: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
> 
> > v5:
> >Coding style clean-up
> 
> > v4:
> > a. Remove DoUpdate and check attributes mismatch all the time to avoid
> >a logic hole
> > b. Add warning message if failed to update capability
> > c. Add local variable to hold new attributes to make code cleaner
> 
> > v3:
> > a. Add comment to explain more on updating memory capabilities
> > b. Fix logic hole in updating attributes
> > c. Instead of checking illegal memory space address and size, use return
> >status of gDS->SetMemorySpaceCapabilities() to skip memory block which
> >cannot be updated with new capabilities.
> 
> > v2
> > a. Fix an issue which will cause setting capability failure if size is 
> > smaller
> >than a page.
> 
> More than one entry of RT_CODE memory might cause boot problem for some
> old OSs. This patch will fix this issue to keep OS compatibility as much
> as possible.
> 
> More detailed information, please refer to
> https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Jiewen Yao <jiewen@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +--
> -
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..61537838b7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
>UINT64  BaseAddress;
>UINT64  PageStartAddress;
>UINT64  Attributes;
> -  UINT64  Capabilities;
> -  BOOLEAN DoUpdate;
> +  UINT64  NewAttributes;
>UINTN   Index;
> 
>//
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
> 
>GetCurrentPagingContext ();
> 
> -  DoUpdate  = FALSE;
> -  Capabilities  = 0;
>Attributes= 0;
> +  NewAttributes = 0;
>BaseAddress   = 0;
>PageLength= 0;
> 
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
>continue;
>  }
> 
> +//
> +// Sync the actual paging related capabilities back to GCD service first.
> +// As a side effect (good one), this can also help to avoid unnecessary
> +// memory map entries due to the different capabilities of the same type
> +// memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +// which could cause boot failure of some old Linux distro (before v4.3).
> +//
> +Status = gDS->SetMemorySpaceCapabilities (
> +MemorySpaceMap[Index].BaseAddress,
> +MemorySpaceMap[Index].Length,
> +MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK
> +);
> +if (EFI_ERROR (Status)) {
> +  //
> +  // If we cannot udpate the capabilities, we cannot update its
> +  // attributes either. So just simply skip current block of memory.
> +  //
> +  DEBUG ((
> +DEBUG_WARN,
> +"Failed to update capability: [%lu] %016lx - %016lx (%016lx -> 
> %016lx)\r\n",
> +(UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +MemorySpaceMap[Index].Capabilities,
> +MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> +));
> +  continue;
> +}
> +
>  if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>//
>// Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
>PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>  }
> 
> -// Sync real page attributes to GCD
> +//
> +//

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

[Jian J Wang]

> 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 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 69 +---
 1 file changed, 50 insertions(+), 19 deletions(-)

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

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

[Wang, Jian J]

Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, October 26, 2017 6:08 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen@intel.com>; Dong, Eric <eric.d...@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> Hello Jian,
> 
> On 10/23/17 08:50, Jian J Wang wrote:
> > 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.
> >
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Jiewen Yao <jiewen@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Thank you again for the explanation elsewhere in this thread. I filed
> the following TianoCore Bugzilla entry about this issue, and assigned it
> to you:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Can you please read the BZ, and add corrections (in further comments) if
> you think any such are necessary?
> 
> I suggest that the BZ reference be included in the commit message. (If
> there is no v2 necessary for this patch, then Eric can do that as well,
> when he commits / pushes your patch.)
> 

You're welcome. Those information should have been provided at the very 
beginning. 
I think I have a lot to learn from you to do better for open source community. 
Like the
Bugzilla entry. I'd say this is the best description I've ever read.

> I think the patch is good, but I have one technical question below:
> 
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..0802464b9d 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
> >  // Sync real page attributes to GCD
> >  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
> >  MemorySpaceLength = MemorySpaceMap[Index].Length;
> > +Capabilities  = MemorySpaceMap[Index].Capabilities |
> > +EFI_MEMORY_PAGETYPE_MASK;
> > +Status = gDS->SetMemorySpaceCapabilities (
> > +BaseAddress,
> > +MemorySpaceLength,
> > +Capabilities
> > +);
> > +ASSERT_EFI_ERROR (Status);
> > +
> 
> This logic -- i.e. the addition of the EFI_MEMORY_PAGETYPE_MASK
> capabilities -- will be applied to *all* GCD memory space map entries
> that have a type different from "EfiGcdMemoryTypeNonExistent".
> 
> I wonder if that's a good idea -- for example I don't think it makes
> much sense for "EfiGcdMemoryTypeMemoryMappedIo".
> 
> How about the following alternatives:
> 
> (1) *Either* restrict this capability addition to
> EfiGcdMemoryTypeSystemMemory (and maybe some other GCD types as well?),
> 
> (2) *or*, remove this change, and:
> 
> >  while (MemorySpaceLength > 0) {
> >if (PageLength == 0) {
> >  PageEntry = GetPageTableEntry (, BaseAddress,
> );
> > @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
> >  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;
> >  }
> > @@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> >Length = MIN (PageLength, MemorySpaceLength);
> >if (DoUpdate) {
> > -gDS->SetMemorySpaceCapabilities (BaseAddress, Length, 
> > Capabilities);
> > -gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> > +Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> > +ASSERT_EFI_ERROR (Status);
> >  DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx
> - %016lx (%08lx -> %08lx)\r\n",
> >   Index, BaseAddress, BaseAddress + Length - 1,
> >   MemorySpaceMap[Index].Attributes, 
> > Attributes));
> >
> 
> ke

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

[Laszlo Ersek]

Hello Jian,

On 10/23/17 08:50, Jian J Wang wrote:
> 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.
>
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Thank you again for the explanation elsewhere in this thread. I filed
the following TianoCore Bugzilla entry about this issue, and assigned it
to you:

  https://bugzilla.tianocore.org/show_bug.cgi?id=753

Can you please read the BZ, and add corrections (in further comments) if
you think any such are necessary?

I suggest that the BZ reference be included in the commit message. (If
there is no v2 necessary for this patch, then Eric can do that as well,
when he commits / pushes your patch.)

I think the patch is good, but I have one technical question below:

> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..0802464b9d 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
>  // Sync real page attributes to GCD
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
> +Capabilities  = MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK;
> +Status = gDS->SetMemorySpaceCapabilities (
> +BaseAddress,
> +MemorySpaceLength,
> +Capabilities
> +);
> +ASSERT_EFI_ERROR (Status);
> +

This logic -- i.e. the addition of the EFI_MEMORY_PAGETYPE_MASK
capabilities -- will be applied to *all* GCD memory space map entries
that have a type different from "EfiGcdMemoryTypeNonExistent".

I wonder if that's a good idea -- for example I don't think it makes
much sense for "EfiGcdMemoryTypeMemoryMappedIo".

How about the following alternatives:

(1) *Either* restrict this capability addition to
EfiGcdMemoryTypeSystemMemory (and maybe some other GCD types as well?),

(2) *or*, remove this change, and:

>  while (MemorySpaceLength > 0) {
>if (PageLength == 0) {
>  PageEntry = GetPageTableEntry (, BaseAddress, 
> );
> @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
>  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;
>  }
> @@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
>
>Length = MIN (PageLength, MemorySpaceLength);
>if (DoUpdate) {
> -gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> +Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, 
> Attributes);
> +ASSERT_EFI_ERROR (Status);
>  DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
> %016lx (%08lx -> %08lx)\r\n",
>   Index, BaseAddress, BaseAddress + Length - 1,
>   MemorySpaceMap[Index].Attributes, Attributes));
>

keep the SetMemorySpaceCapabilities() call here, but use the following
arguments instead:

  MemorySpaceMap[Index].BaseAddress
  MemorySpaceMap[Index].Length

This will ensure that the capabilities are changed on the *entire*
containing GCD memory space entry, and no entry splitting will take
place.

Yes, it is possible that the SetMemorySpaceCapabilities() function will
be invoked multiple times, on the same GCD memory space entry, but
that's not a problem, IMO. The Capabilities value (bitmask) should be
the exact same.

In fact, you could set Capabilities just before the inner loop, and then
only *grow* it in the inner loop. Something like this:

> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f87c..5a0eb2900cd5 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -829,6 +829,7 @@ RefreshGcdMemoryAttributesFromPaging (
>  // Sync real page attributes to GCD
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
> +Capabilities  = MemorySpaceMap[Index].Capabilities;
>  while (MemorySpaceLength > 0) {
>if (PageLength == 0) {
>  PageEntry = GetPageTableEntry (, BaseAddress, 
> );
> @@ -846,7 +847,7 @@ RefreshGcdMemoryAttributesFromPaging (
>  if 

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

[Laszlo Ersek]

On 10/26/17 03:41, Wang, Jian J wrote:
> Please see my comments below. Thanks.
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, October 25, 2017 8:51 PM
>> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen@intel.com>; Dong, Eric <eric.d...@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> On 10/25/17 03:33, Wang, Jian J wrote:
>>> Hi Laszlo,
>>>
>>> Thanks for the feedback. I'd like to explain a bit more here first and
>>> will update the commit message later.
>>>
>>> The multiple RT_CODE entries issue was reported by LUV test suite from
>>> https://01.org/linux-uefi-validation
>>>
>>> You're right this issue is caused by my commit c1cab54ce57c, which
>>> tried to fix the GCD issue in which you can't set paging related
>>> memory attributes through GCD service API. The reasons are that GCD
>>> will filter out all paging related memory attributes and also, the CPU
>>> driver didn't sync the paging attributes back to GCD. Sorry I don't
>>> know why GCD and cpu driver are implemented that way.
>>>
>>> My previous commit c1cab54ce57c fixed above issues but didn't make
>>> sure that all memory blocks share the same capabilities because I just
>>> added paging related memory capabilities to those memory block having
>>> some paging attributes set. This will in turn cause more than one
>>> RT_CODE entries in memory map because GCD reports memory to OS based
>>> on the memory block capability.
>>>
>>> Why multiple RT_CODE matters? It's because that Linux kernel would
>>> misplace the runtime service code segment and its data segment. What I
>>> know is this Linux issue had been fixed. That's why recent Linux
>>> distro won't encounter problems even if we report multiple RT_CODE
>>> memory to kernel.
>>
>> Ah, OK, I remember now.
>>
>> The point is that separate entries in the UEFI memmap may be mapped by
>> the OS to discontiguous virtual address ranges.
>>
>> However, if we take the UEFI memmap entries that belong to a single
>> runtime DXE driver, and unintentionally split those entries up (by
>> setting distinct capabilities for a subset of their pages), then the
>> runtime driver will break, because the linear address range that the
>> driver expects (from its original loading and relocation) will not be
>> kept linear by the OS.
>>
>>> I'm sorry that I cannot find the specific version of kernel which has
>>> such problem and I can't find any discussion related. Maybe Jiewen can
>>> provide more detailed information.
>>
>> It would be really helpful if you guys could name a guest kernel version
>> (or a GNU/Linux distro release) that is affected by this problem.
>>
> 
> It seems following log history which mentioned the problem.
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0ce3cc008ec04258b6a6314b09f1a6012810881a

Ah, exactly. I vaguely remembered that the same issue had originally
popped up in connection to the properties table.

Commit 0ce3cc008ec0 ("arm64/efi: Fix boot crash by not padding between
EFI_MEMORY_RUNTIME regions", 2015-09-25) was first released as part of
Linux v4.3.

> 
>> Are there perhaps any conditions that this issue depends on, such as
>> PCDs for example? In other words, is it possible that various edk2
>> platform settings (in the DSC) can mask this issue?
>>
> 
> In current code base, following PCDs may cause memory page attribute changes
> 
>   PcdImageProtectionPolicy
>   PcdDxeNxMemoryProtectionPolicy
> 
> Once the heap guard feature is checked in (you may notice my recent patches),
> there're three more PCDs:
> 
>   PcdHeapGuardPropertyMask
>   PcdHeapGuardPoolType
>   PcdHeapGuardPageType
> 
>> Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in
>> Linux) that show the problem, compared to this fix?
>>
> 
> Before this patch (after c1cab54ce57c), the memory map looks like
> 
> RT_DataBE20F000-BE38EFFF 0180 800F
> RT_CodeBE38F000-BE46 00E1 800F
> RT_CodeBE47-BE470FFF 0001 8000400F
> RT_CodeBE471000-BE472FFF 0002 8002000F
> RT_CodeBE473000-BE476FFF 0004 8000400F
> RT_Code000

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

[Wang, Jian J]

Please see my comments below. Thanks.

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, October 25, 2017 8:51 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen@intel.com>; Dong, Eric <eric.d...@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> On 10/25/17 03:33, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> > Thanks for the feedback. I'd like to explain a bit more here first and
> > will update the commit message later.
> >
> > The multiple RT_CODE entries issue was reported by LUV test suite from
> > https://01.org/linux-uefi-validation
> >
> > You're right this issue is caused by my commit c1cab54ce57c, which
> > tried to fix the GCD issue in which you can't set paging related
> > memory attributes through GCD service API. The reasons are that GCD
> > will filter out all paging related memory attributes and also, the CPU
> > driver didn't sync the paging attributes back to GCD. Sorry I don't
> > know why GCD and cpu driver are implemented that way.
> >
> > My previous commit c1cab54ce57c fixed above issues but didn't make
> > sure that all memory blocks share the same capabilities because I just
> > added paging related memory capabilities to those memory block having
> > some paging attributes set. This will in turn cause more than one
> > RT_CODE entries in memory map because GCD reports memory to OS based
> > on the memory block capability.
> >
> > Why multiple RT_CODE matters? It's because that Linux kernel would
> > misplace the runtime service code segment and its data segment. What I
> > know is this Linux issue had been fixed. That's why recent Linux
> > distro won't encounter problems even if we report multiple RT_CODE
> > memory to kernel.
> 
> Ah, OK, I remember now.
> 
> The point is that separate entries in the UEFI memmap may be mapped by
> the OS to discontiguous virtual address ranges.
> 
> However, if we take the UEFI memmap entries that belong to a single
> runtime DXE driver, and unintentionally split those entries up (by
> setting distinct capabilities for a subset of their pages), then the
> runtime driver will break, because the linear address range that the
> driver expects (from its original loading and relocation) will not be
> kept linear by the OS.
> 
> > I'm sorry that I cannot find the specific version of kernel which has
> > such problem and I can't find any discussion related. Maybe Jiewen can
> > provide more detailed information.
> 
> It would be really helpful if you guys could name a guest kernel version
> (or a GNU/Linux distro release) that is affected by this problem.
> 

It seems following log history which mentioned the problem.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?id=0ce3cc008ec04258b6a6314b09f1a6012810881a

> Are there perhaps any conditions that this issue depends on, such as
> PCDs for example? In other words, is it possible that various edk2
> platform settings (in the DSC) can mask this issue?
> 

In current code base, following PCDs may cause memory page attribute changes

  PcdImageProtectionPolicy
  PcdDxeNxMemoryProtectionPolicy

Once the heap guard feature is checked in (you may notice my recent patches),
there're three more PCDs:

  PcdHeapGuardPropertyMask
  PcdHeapGuardPoolType
  PcdHeapGuardPageType

> Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in
> Linux) that show the problem, compared to this fix?
> 

Before this patch (after c1cab54ce57c), the memory map looks like

RT_DataBE20F000-BE38EFFF 0180 800F
RT_CodeBE38F000-BE46 00E1 800F
RT_CodeBE47-BE470FFF 0001 8000400F
RT_CodeBE471000-BE472FFF 0002 8002000F
RT_CodeBE473000-BE476FFF 0004 8000400F
RT_CodeBE477000-BE478FFF 0002 8002000F
RT_CodeBE479000-BE47CFFF 0004 8000400F
RT_CodeBE47D000-BE47 0003 8002000F
RT_CodeBE48-BE483FFF 0004 8000400F
RT_CodeBE484000-BE485FFF 0002 8002000F
RT_CodeBE486000-BE489FFF 0004 8000400F
RT_CodeBE48A000-BE48BFFF 0002 8002000F
RT_CodeBE48C000-BE48EFFF 0003 8000400F

You may notice that there're one RT_Data with 12 RT_Code entries

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

[Laszlo Ersek]

On 10/25/17 03:33, Wang, Jian J wrote:
> Hi Laszlo,
>
> Thanks for the feedback. I'd like to explain a bit more here first and
> will update the commit message later.
>
> The multiple RT_CODE entries issue was reported by LUV test suite from
> https://01.org/linux-uefi-validation
>
> You're right this issue is caused by my commit c1cab54ce57c, which
> tried to fix the GCD issue in which you can't set paging related
> memory attributes through GCD service API. The reasons are that GCD
> will filter out all paging related memory attributes and also, the CPU
> driver didn't sync the paging attributes back to GCD. Sorry I don't
> know why GCD and cpu driver are implemented that way.
>
> My previous commit c1cab54ce57c fixed above issues but didn't make
> sure that all memory blocks share the same capabilities because I just
> added paging related memory capabilities to those memory block having
> some paging attributes set. This will in turn cause more than one
> RT_CODE entries in memory map because GCD reports memory to OS based
> on the memory block capability.
>
> Why multiple RT_CODE matters? It's because that Linux kernel would
> misplace the runtime service code segment and its data segment. What I
> know is this Linux issue had been fixed. That's why recent Linux
> distro won't encounter problems even if we report multiple RT_CODE
> memory to kernel.

Ah, OK, I remember now.

The point is that separate entries in the UEFI memmap may be mapped by
the OS to discontiguous virtual address ranges.

However, if we take the UEFI memmap entries that belong to a single
runtime DXE driver, and unintentionally split those entries up (by
setting distinct capabilities for a subset of their pages), then the
runtime driver will break, because the linear address range that the
driver expects (from its original loading and relocation) will not be
kept linear by the OS.

> I'm sorry that I cannot find the specific version of kernel which has
> such problem and I can't find any discussion related. Maybe Jiewen can
> provide more detailed information.

It would be really helpful if you guys could name a guest kernel version
(or a GNU/Linux distro release) that is affected by this problem.

Are there perhaps any conditions that this issue depends on, such as
PCDs for example? In other words, is it possible that various edk2
platform settings (in the DSC) can mask this issue?

Can you maybe present UEFI memmaps (dumped in the UEFI shell, or in
Linux) that show the problem, compared to this fix?

> This patch will make sure that all memory block share the same paging
> capabilities. Because all memory are actually paged in current EDK2
> (at least IA processors), technically we're capable of setting any
> page of memory to read-only and/or non-executable. I think this fix is
> not only trying to avoid multiple RT_CODE memory map entries but also
> trying to make sure the memory capabilities in GCD service to reflect
> complete status of the real world.

Are you saying that *any* firmware that carries commit c1cab54ce57c
should also receive this patch?

If that's the case, then some kind of "reproducer" would be really nice
-- steps that you can run both with and without this patch, and the
output or the behavior will show the difference.

Thanks!
Laszlo


>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, October 24, 2017 8:20 PM
>> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
>> Cc: Yao, Jiewen <jiewen@intel.com>; Dong, Eric <eric.d...@intel.com>
>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
>> RT_CODE in memory map
>>
>> On 10/23/17 08:50, Jian J Wang wrote:
>>> 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.
>>>
>>> Cc: Eric Dong <eric.d...@intel.com>
>>> Cc: Jiewen Yao <jiewen@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
>>> ---
>>>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> Can you please explain in the commit message; what OSes are affected by
>> this issue, to your knowledge?
>>
>> Also, the code being patched seems to originate from commit c1cab54ce57c
>> ("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes",
>> 2017-09-16). I vaguely recall that this commit was related to your "page
>> 0 protection" work.
>>
>> Can you please explain, in the commit

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

[Wang, Jian J]

Hi Laszlo,

Thanks for the feedback. I'd like to explain a bit more here first and will
update the commit message later.

The multiple RT_CODE entries issue was reported by LUV test suite from
https://01.org/linux-uefi-validation

You're right this issue is caused by my commit c1cab54ce57c, which tried to
fix the GCD issue in which you can't set paging related memory attributes
through GCD service API. The reasons are that GCD will filter out all paging
related memory attributes and also, the CPU driver didn't sync the paging
attributes back to GCD. Sorry I don't know why GCD and cpu driver are
implemented that way.

My previous commit c1cab54ce57c fixed above issues but didn't make sure that
all memory blocks share the same capabilities because I just added paging
related memory capabilities to those memory block having some paging
attributes set. This will in turn cause more than one RT_CODE entries in
memory map because GCD reports memory to OS based on the memory block
capability.

Why multiple RT_CODE matters? It's because that Linux kernel would misplace
the runtime service code segment and its data segment. What I know is this
Linux issue had been fixed. That's why recent Linux distro won't encounter
problems even if we report multiple RT_CODE memory to kernel.

I'm sorry that I cannot find the specific version of kernel which has such
problem and I can't find any discussion related. Maybe Jiewen can provide
more detailed information.

This patch will make sure that all memory block share the same paging
capabilities. Because all memory are actually paged in current EDK2 (at least
IA processors), technically we're capable of setting any page of memory to
read-only and/or non-executable. I think this fix is not only trying to avoid
multiple RT_CODE memory map entries but also trying to make sure the memory
capabilities in GCD service to reflect complete status of the real world.

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, October 24, 2017 8:20 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen@intel.com>; Dong, Eric <eric.d...@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> On 10/23/17 08:50, Jian J Wang wrote:
> > 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.
> >
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Jiewen Yao <jiewen@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Can you please explain in the commit message; what OSes are affected by
> this issue, to your knowledge?
> 
> Also, the code being patched seems to originate from commit c1cab54ce57c
> ("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes",
> 2017-09-16). I vaguely recall that this commit was related to your "page
> 0 protection" work.
> 
> Can you please explain, in the commit message, under what circumstances
> (PCD settings etc) the issue arises, and how we end up with multiple
> RT_CODE entries in the memory map?
> 
> (BTW, multiple RT_CODE entries in the memmap should be perfectly
> normal... So I'm extra curious about the OSes that are not compatible
> with that.)
> 
> Thanks,
> Laszlo
> 
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..0802464b9d 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
> >  // Sync real page attributes to GCD
> >  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
> >  MemorySpaceLength = MemorySpaceMap[Index].Length;
> > +Capabilities  = MemorySpaceMap[Index].Capabilities |
> > +EFI_MEMORY_PAGETYPE_MASK;
> > +Status = gDS->SetMemorySpaceCapabilities (
> > +BaseAddress,
> > +MemorySpaceLength,
> > +Capabilities
> > +);
> > +ASSERT_EFI_ERROR (Status);
> > +
> >  while (MemorySpaceLength > 0) {
> >if (PageLength == 0) {
> >  PageEntry = GetPageTableEntry (, BaseAddress,
> );
> > @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
> >  if (Attributes != (MemorySpaceMap[Index].Attributes &
> EF

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

[Laszlo Ersek]

On 10/23/17 08:50, Jian J Wang wrote:
> 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.
> 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Can you please explain in the commit message; what OSes are affected by
this issue, to your knowledge?

Also, the code being patched seems to originate from commit c1cab54ce57c
("UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes",
2017-09-16). I vaguely recall that this commit was related to your "page
0 protection" work.

Can you please explain, in the commit message, under what circumstances
(PCD settings etc) the issue arises, and how we end up with multiple
RT_CODE entries in the memory map?

(BTW, multiple RT_CODE entries in the memmap should be perfectly
normal... So I'm extra curious about the OSes that are not compatible
with that.)

Thanks,
Laszlo

> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..0802464b9d 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
>  // Sync real page attributes to GCD
>  BaseAddress   = MemorySpaceMap[Index].BaseAddress;
>  MemorySpaceLength = MemorySpaceMap[Index].Length;
> +Capabilities  = MemorySpaceMap[Index].Capabilities |
> +EFI_MEMORY_PAGETYPE_MASK;
> +Status = gDS->SetMemorySpaceCapabilities (
> +BaseAddress,
> +MemorySpaceLength,
> +Capabilities
> +);
> +ASSERT_EFI_ERROR (Status);
> +
>  while (MemorySpaceLength > 0) {
>if (PageLength == 0) {
>  PageEntry = GetPageTableEntry (, BaseAddress, 
> );
> @@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
>  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;
>  }
> @@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>Length = MIN (PageLength, MemorySpaceLength);
>if (DoUpdate) {
> -gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> +Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, 
> Attributes);
> +ASSERT_EFI_ERROR (Status);
>  DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
> %016lx (%08lx -> %08lx)\r\n",
>   Index, BaseAddress, BaseAddress + Length - 1,
>   MemorySpaceMap[Index].Attributes, Attributes));
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Jian J Wang]

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.

Cc: Eric Dong 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index d312eb66f8..0802464b9d 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -829,6 +829,15 @@ RefreshGcdMemoryAttributesFromPaging (
 // Sync real page attributes to GCD
 BaseAddress   = MemorySpaceMap[Index].BaseAddress;
 MemorySpaceLength = MemorySpaceMap[Index].Length;
+Capabilities  = MemorySpaceMap[Index].Capabilities |
+EFI_MEMORY_PAGETYPE_MASK;
+Status = gDS->SetMemorySpaceCapabilities (
+BaseAddress,
+MemorySpaceLength,
+Capabilities
+);
+ASSERT_EFI_ERROR (Status);
+
 while (MemorySpaceLength > 0) {
   if (PageLength == 0) {
 PageEntry = GetPageTableEntry (, BaseAddress, 
);
@@ -846,7 +855,6 @@ RefreshGcdMemoryAttributesFromPaging (
 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;
 }
@@ -854,8 +862,8 @@ RefreshGcdMemoryAttributesFromPaging (
 
   Length = MIN (PageLength, MemorySpaceLength);
   if (DoUpdate) {
-gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
-gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
+Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length, 
Attributes);
+ASSERT_EFI_ERROR (Status);
 DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - 
%016lx (%08lx -> %08lx)\r\n",
  Index, BaseAddress, BaseAddress + Length - 1,
  MemorySpaceMap[Index].Attributes, Attributes));
-- 
2.14.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel