Some updates below > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, > Jian J > Sent: Wednesday, November 08, 2017 8:11 AM > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com> > Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of > RT_CODE in memory map > > Hi Laszlo, > > > -----Original Message----- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Wednesday, November 08, 2017 1:14 AM > > 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 v2] UefiCpuPkg/CpuDxe: Fix multiple entries of > > RT_CODE in memory map > > > > sorry about the late response > > > > On 11/03/17 01:57, Jian J Wang wrote: > > >> 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 | 18 ++++++++++++++---- > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c > > b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > index d312eb66f8..4a7827ebc9 100644 > > > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c > > > @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging ( > > > PageLength = 0; > > > > > > for (Index = 0; Index < NumberOfDescriptors; Index++) { > > > - if (MemorySpaceMap[Index].GcdMemoryType == > > EfiGcdMemoryTypeNonExistent) { > > > + if (MemorySpaceMap[Index].GcdMemoryType == > > EfiGcdMemoryTypeNonExistent > > > + || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0 > > > + || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) { > > > continue; > > > } > > > > When exactly do the new conditions match? > > > > I thought the base addresses and the lengths in the GCD memory space map > > are all page aligned. Is that not the case? > > > > If these conditions are just a sanity check (i.e. we never expect them > > to fire), then should we perpahs turn them into ASSERT()s? > > > > I found that there's a mmio entry in memory map on OVMF which has size > less than a page. I didn't encounter this before. Maybe some recent changes > in other part of EDKII caused this situation. So ASSERT is not enough. >
I changed my original fix in v2 to not check the Address and Size. Instead, I'll use the Status of gDS->SetMemorySpaceCapabilities() to skip those memory block which cannot be updated with new capabilities. This can avoid the assumption that only the address and size will cause the calling failure. And I found a logic hole in code. You'll find new changes in v3 patch. > > > > > > @@ -829,6 +831,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); > > > + > > > > OK, so I guess we simply add EFI_MEMORY_PAGETYPE_MASK to the > > capabilities of all memory space map entries that have a type different > > from non-existent. We discussed it before and (apparently) it is > > considered safe. > > > > Yes. I've validated different OSs boot. It's safe to stay this way. > > > > while (MemorySpaceLength > 0) { > > > if (PageLength == 0) { > > > PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, > > &PageAttribute); > > > @@ -846,7 +857,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 +864,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)); > > > > > > > I'll let you decide about the EFI_PAGE_MASK conditions near the top. > > > > Acked-by: Laszlo Ersek <ler...@redhat.com> > > > > Thanks. > > > Thanks > > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel