From: Laszlo Ersek <lersek@redhat.com>
To: Jian J Wang <jian.j.wang@intel.com>, edk2-devel@lists.01.org
Cc: Jiewen Yao <jiewen.yao@intel.com>, Eric Dong <eric.dong@intel.com>
Subject: Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Date: Thu, 9 Nov 2017 15:12:49 +0100 [thread overview]
Message-ID: <5d4b6d41-c348-9175-ac0a-5ddb0d58c565@redhat.com> (raw)
In-Reply-To: <20171109013958.15832-1-jian.j.wang@intel.com>
Hi Jian,
this is v4, but the subject says v3 :) If you post a new version, please
make sure that it says "v5" in the subject.
The logic looks OK to me; I've got some comments on style:
On 11/09/17 02:39, Jian J Wang wrote:
>> 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: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@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..a1d804caed 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 (&PagingContext);
>
> - 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: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + Index, BaseAddress, BaseAddress + Length - 1,
(1) I think you forgot about my note that Index (which is of type UINTN)
should not be printed with "%d". Instead, (UINT64)Index should be
printed with "%lu".
> + 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 - %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;
(2) To my understanding, the edk2 coding style wants us to keep the
bitwise-and operator ("&") at the end of the line.
> + Status = gDS->SetMemorySpaceAttributes (
> + BaseAddress,
> + Length,
> + NewAttributes
> + );
> + ASSERT_EFI_ERROR (Status);
> + DEBUG ((
> + DEBUG_INFO,
> + "Updated memory space attribute: [%d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> + Index, BaseAddress, BaseAddress + Length - 1,
(3) Same comment as (1).
Thanks,
Laszlo
> + MemorySpaceMap[Index].Attributes,
> + NewAttributes
> + ));
> }
>
> PageLength -= Length;
>
next prev parent reply other threads:[~2017-11-09 14:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-09 1:39 [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-09 14:12 ` Laszlo Ersek [this message]
2017-11-10 0:22 ` Wang, Jian J
-- strict thread matches above, loose matches on Subject: below --
2017-11-08 10:52 Jian J Wang
2017-11-08 14:36 ` Laszlo Ersek
2017-11-09 0:51 ` Wang, Jian J
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5d4b6d41-c348-9175-ac0a-5ddb0d58c565@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox