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

[Laszlo Ersek <lersek@redhat.com>]

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.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@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.)

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 (&PagingContext, BaseAddress, &PageAttribute);
> @@ -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 (&PagingContext, BaseAddress, &PageAttribute);
> @@ -846,7 +847,7 @@ 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;
> +          Capabilities |= Attributes;
>          } else {
>            DoUpdate = FALSE;
>          }
> @@ -854,8 +855,20 @@ RefreshGcdMemoryAttributesFromPaging (
>
>        Length = MIN (PageLength, MemorySpaceLength);
>        if (DoUpdate) {
> -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> +        Status = gDS->SetMemorySpaceCapabilities (
> +                        MemorySpaceMap[Index].BaseAddress,
> +                        MemorySpaceMap[Index].Length,
> +                        Capabilities
> +                        );
> +        ASSERT_EFI_ERROR (Status);
> +
> +        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));

What do you think?


Meta comment: can you please CC me on the next version of the patch (if
you send one)? Looks like I'm now a Reviewer for UefiCpuPkg :) , I just
have to commit the patch for "Maintainers.txt".

In addition, if you send a v2, please locate the web link for it in the
mailing list archive:

  https://lists.01.org/pipermail/edk2-devel/2017-October/thread.html

and add the link to the Bugzilla, in a new comment -- this way someone
who looks at the bugzilla can see all the versions and the discussion
threads.

Thanks!
Laszlo