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

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

Hi Laszlo,

The memory range is 00000000FED00000 - 00000000FED003FF. Actually I don't 
know if it's for MMIO or not. I might be mess it with other things.

Thanks,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 08, 2017 10:17 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 v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> On 11/08/17 01:10, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Wednesday, November 08, 2017 1:14 AM
> >> 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 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.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 | 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.
> 
> Can you describe the base address and size of this MMIO entry?
> 
> I don't see how it is possible to add such an entry in the first place
> -- if you try to add it in PEI, via a HOB, then IIRC HobLib will
> ASSERT(). If you try to add it with gDS->AddMemorySpace() in DXE, then
> the call should fail.
> 
> I believe it might be useful to investigate this entry more closely.
> Knowing the base address could help us.
> 
> Thanks!
> Laszlo