Thanks for catching them. There'll be v5 today:) > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, November 09, 2017 10:13 PM > To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@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.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 | 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; > >
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
