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

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

Thanks for catching them. There'll be v5 today:)

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 09, 2017 10:13 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 v3] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> 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;
> >