Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 08, 2017 10:37 PM
> To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.d...@intel.com>; Yao, Jiewen <jiewen....@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.d...@intel.com>
> > Cc: Jiewen Yao <jiewen....@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.w...@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;
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
