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
