Re: [PATCH] MdeModulePkg/Core: Merge memory map after filtering paging capability

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

Make sense. Thanks for the comment.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 28, 2017 2:21 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/Core: Merge memory map after
> filtering paging capability
> 
> Hello Jian,
> 
> On 11/27/17 06:34, Jian J Wang wrote:
> > Once the paging capabilities were filtered out, there might be some adjacent
> entries
> > sharing the same capabilities. It's recommended to merge those entries for the
> OS
> > compatibility purpose.
> >
> > This patch makes use of existing method MergeMemoryMap() to do it. This is
> done by
> > simply turning this method from static to extern, and call it after filter code.
> >
> > This patch is related to an issue described at
> >    https://bugzilla.tianocore.org/show_bug.cgi?id=753
> >
> > This patch is also passed test of booting follow OSs:
> >     Windows 10
> >     Windows Server 2016
> >     Fedora 26
> >     Fedora 25
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Star Zeng <star.zeng@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>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h              | 18 ++++++++++++++++++
> >  MdeModulePkg/Core/Dxe/Mem/Page.c             |  1 +
> >  MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c |  1 -
> >  3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> > index 1a0babba71..07b86ba696 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -2948,4 +2948,22 @@ ApplyMemoryProtectionPolicy (
> >    IN  UINT64                Length
> >    );
> >
> > +/**
> > +  Merge continous memory map entries whose have same attributes.
> > +
> > +  @param  MemoryMap       A pointer to the buffer in which firmware places
> > +                          the current memory map.
> > +  @param  MemoryMapSize   A pointer to the size, in bytes, of the
> > +                          MemoryMap buffer. On input, this is the size of
> > +                          the current memory map.  On output,
> > +                          it is the size of new memory map after merge.
> > +  @param  DescriptorSize  Size, in bytes, of an individual
> EFI_MEMORY_DESCRIPTOR.
> > +**/
> > +VOID
> > +MergeMemoryMap (
> > +  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
> > +  IN OUT UINTN                  *MemoryMapSize,
> > +  IN UINTN                      DescriptorSize
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index 962ae90d3d..ca4ce69a3f 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1915,6 +1915,7 @@ CoreGetMemoryMap (
> >                                        EFI_MEMORY_XP);
> >      MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
> >    }
> > +  MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
> >
> >    Status = EFI_SUCCESS;
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> > index 6cf5edcbe5..75d9b14c1f 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> > @@ -182,7 +182,6 @@ SortMemoryMap (
> >                                   it is the size of new memory map after merge.
> >    @param  DescriptorSize         Size, in bytes, of an individual
> EFI_MEMORY_DESCRIPTOR.
> >  **/
> > -STATIC
> >  VOID
> >  MergeMemoryMap (
> >    IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
> >
> 
> This patch looks good to me -- I expect merging the memmap can only
> collapse more entries into fewer entries, so the repro / test
> instructions that I added to the BZ earlier, and the expected OS
> behavior should remain unchanged.
> 
> I have one small suggestion: like before, I suggest keeping the local
> variables up-to-date after adding the new code, so that further code
> need not hunt down invariants as a starting step. Therefore, after the
> MergeMemoryMap() call, how about updating MemoryMapEnd, like this:
> 
>   MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *)(
>                    (UINT8 *)MemoryMapStart + BufferSize
>                    );
> 
> I think (if Jiewen and Star agree with this suggestion) that you don't
> need to post a v2 just for this.
> 
> I'm also fine with the patch if the MemoryMapEnd update is rejected.
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Can you please add a note to TianoCore BZ#753 when you push this?
> 
> Thanks!
> Laszlo