From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id DCB5321A10979 for ; Mon, 27 Nov 2017 10:16:19 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BA9B878EC1; Mon, 27 Nov 2017 18:20:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-232.rdu2.redhat.com [10.10.120.232]) by smtp.corp.redhat.com (Postfix) with ESMTP id 71CF15C1A3; Mon, 27 Nov 2017 18:20:40 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Jiewen Yao , Star Zeng References: <20171127053456.14312-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <506bc507-cc7f-4773-bb9f-3455fe8ba592@redhat.com> Date: Mon, 27 Nov 2017 19:20:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171127053456.14312-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 27 Nov 2017 18:20:41 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/Core: Merge memory map after filtering paging capability X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2017 18:16:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Star Zeng > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > 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 Can you please add a note to TianoCore BZ#753 when you push this? Thanks! Laszlo