public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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: "Yao, Jiewen" <jiewen.yao@intel.com>, "Dong, Eric" <eric.dong@intel.com>
Subject: Re: [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map
Date: Wed, 25 Oct 2017 01:33:10 +0000	[thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624CA4ABF@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <764523da-0d75-d4ac-3121-c8282ada3e8f@redhat.com>

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:lersek@redhat.com]
> Sent: Tuesday, October 24, 2017 8:20 PM
> 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>
> 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.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(-)
> 
> 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 (&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));
> >


  reply	other threads:[~2017-10-25  1:29 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-23  6:50 [PATCH] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map Jian J Wang
2017-10-24 12:20 ` Laszlo Ersek
2017-10-25  1:33   ` Wang, Jian J [this message]
2017-10-25 12:50     ` Laszlo Ersek
2017-10-26  1:41       ` Wang, Jian J
2017-10-26  8:42         ` Laszlo Ersek
2017-10-26 10:07 ` Laszlo Ersek
2017-10-27  1:16   ` Wang, Jian J
  -- strict thread matches above, loose matches on Subject: below --
2017-11-10  0:41 Jian J Wang
2017-11-10  1:04 ` 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=D827630B58408649ACB04F44C510003624CA4ABF@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