From: "Wang, Jian J" <jian.j.wang@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Dong, Eric" <eric.dong@intel.com>, "Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Date: Thu, 9 Nov 2017 00:51:11 +0000 [thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624CAA81B@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <d7020291-ef95-bdce-99d1-abc722ce0306@redhat.com>
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 08, 2017 10:37 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in
> memory map
>
> On 11/08/17 11:52, Jian J Wang wrote:
> >> 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 | 48
> +++++++++++++++++++++++++++++++---------
> > 1 file changed, 38 insertions(+), 10 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index d312eb66f8..455c713dfc 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > UINT64 BaseAddress;
> > UINT64 PageStartAddress;
> > UINT64 Attributes;
> > - UINT64 Capabilities;
> > BOOLEAN DoUpdate;
> > UINTN Index;
> >
> > @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
> > GetCurrentPagingContext (&PagingContext);
> >
> > DoUpdate = FALSE;
> > - Capabilities = 0;
> > Attributes = 0;
> > BaseAddress = 0;
> > PageLength = 0;
> > @@ -813,6 +811,27 @@ 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.
> > + //
>
> (1) Can you perhaps add a DEBUG_WARN here?
Sure.
>
> > + continue;
> > + }
> > +
> > if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
> > //
> > // Current memory space starts at a new page. Resetting PageLength will
> > @@ -826,7 +845,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) {
> > @@ -845,8 +866,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;
> > }
>
> (2) To me it seems like we can remove the "DoUpdate" local variable
> completely. Below, we can replace the DoUpdate check with the actual
>
> (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK))
>
> check.
>
> The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
> entry's attributes. If they do not match Attributes, we clear the full
> bit-field, and then add Attributes back in. I.e., we set the bit-field
> to the desired Attributes.
>
You're right. Checking the attribute mismatch inside the "if (PageLength == 0)"
still leaves a logic hole there. You suggestion can fix it. Thanks for this catch.
> > @@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
> >
> > 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));
> > + Status = gDS->SetMemorySpaceAttributes (
> > + BaseAddress,
> > + Length,
> > + (MemorySpaceMap[Index].Attributes
> > + & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > + DEBUG ((
> > + DEBUG_INFO,
> > + "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> > + Index, BaseAddress, BaseAddress + Length - 1,
> > + MemorySpaceMap[Index].Attributes,
> > + (MemorySpaceMap[Index].Attributes &
> ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> > + ));
> > }
>
> (3) I suggest introducing a new variable called
> "NewMemorySpaceAttributes", and using that for both the debug message
> and the SetMemorySpaceAttributes() call.
>
I agree.
> (4) Not closely related to this patch, but I'll mention it: the "%d"
> format specifier is not right for printing UINTN values. The
> 32-bit/64-bit clean way to print UINTN is:
>
> - cast the variable to UINT64 explicitly,
> - print it with "%lu".
>
Thanks for pointing this out.
> Thanks!
> Laszlo
>
> >
> > PageLength -= Length;
> >
next prev parent reply other threads:[~2017-11-09 0:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-08 10:52 [PATCH v3] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-11-08 14:36 ` Laszlo Ersek
2017-11-09 0:51 ` Wang, Jian J [this message]
-- strict thread matches above, loose matches on Subject: below --
2017-11-09 1:39 Jian J Wang
2017-11-09 14:12 ` Laszlo Ersek
2017-11-10 0:22 ` 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=D827630B58408649ACB04F44C510003624CAA81B@SHSMSX103.ccr.corp.intel.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