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

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

Great. Glad to know my memory is still good enough.

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 09, 2017 9:49 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of
> RT_CODE in memory map
> 
> FED00000 is HPET region. It is MMIO.
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, November 9, 2017 8:42 AM
> > To: Laszlo Ersek <lersek@redhat.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
> >
> > 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