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

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, October 26, 2017 6:08 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
> 
> 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.)
> 

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 (&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?
> 

You have a good catch on the fix. I do considered it during the coding and
struggled for a while. From paging perspective, it's not wrong to add those 
capabilities to all memory address range under paging, even for IO. But
from usage perspective, maybe letting developers add capabilities before 
changing page attributes is a good reminder for them to do it cautiously.

I don't have strong opinion on it actually because I can't foresee the long-term 
impacts. Please allow me to consult more experts for this.

> 
> 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.
> 

Sure. I think we welcome more maintainers :-)

> Thanks!
> Laszlo