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 D25532034C5F8 for ; Mon, 20 Nov 2017 12:19:02 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1BE358B13E; Mon, 20 Nov 2017 20:23:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-222.rdu2.redhat.com [10.10.120.222]) by smtp.corp.redhat.com (Postfix) with ESMTP id 90B1460274; Mon, 20 Nov 2017 20:23:14 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Jiewen Yao , Star Zeng , Ard Biesheuvel References: <20171116072700.11456-1-jian.j.wang@intel.com> <20171116072700.11456-2-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <86061b68-6828-691b-6ab7-5c5f8fe85054@redhat.com> Date: Mon, 20 Nov 2017 21:23:13 +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: <20171116072700.11456-2-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 20 Nov 2017 20:23:16 +0000 (UTC) Subject: Re: [PATCH v6 1/2] MdeModulePkg/DxeCore: Filter out all paging capabilities 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, 20 Nov 2017 20:19:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/16/17 08:26, Jian J Wang wrote: > Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really > set attributes and change memory paging attribute accordingly. > But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by > value from Capabilities in GCD memory map. This might cause > boot problems. Clearing all paging related capabilities can > workaround it. The code added in this patch is supposed to > be removed once the usage of EFI_MEMORY_DESCRIPTOR.Attribute > is clarified in UEFI spec and adopted by both EDK-II Core and > all supported OSs. > > Cc: Jiewen Yao > Cc: Star Zeng > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c > index c9219cc068..783b576e35 100644 > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c > @@ -1829,6 +1829,23 @@ CoreGetMemoryMap ( > // > BufferSize = ((UINT8 *)MemoryMap - (UINT8 *)MemoryMapStart); > > + // > + // WORKAROUND: Some OSs will treat EFI_MEMORY_DESCRIPTOR.Attribute as really > + // set attributes and change memory paging attribute accordingly. > + // But current EFI_MEMORY_DESCRIPTOR.Attribute is assigned by > + // value from Capabilities in GCD memory map. This might cause > + // boot problems. Clearing all paging related capabilities can > + // workaround it. Following code is supposed to be removed once > + // the usage of EFI_MEMORY_DESCRIPTOR.Attribute is clarified in > + // UEFI spec and adopted by both EDK-II Core and all supported > + // OSs. > + // > + while (MemoryMapStart < MemoryMap) { > + MemoryMapStart->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO | > + EFI_MEMORY_XP); > + MemoryMapStart = NEXT_MEMORY_DESCRIPTOR(MemoryMapStart, Size); > + } > + > Status = EFI_SUCCESS; > > Done: > OK, let me check if I understand the discussion thus far, for this patch: - Ard asked about EFI_MEMORY_WP, but clearing that bit is not necessary because CpuDxe does not add it anyway, in the GCD memory space map. - The code comment might be updated (according to Jiewen's suggestion) before pushing the patch. I don't have any particular opinion about the comment. - If I understand correctly, Jiewen agrees with applying both patches in this series. I have one superficial comment on this patch: in the CoreGetMemoryMap() function, we keep "MemoryMapStart" unchanged (after the initial assignment), and keep incrementing "MemoryMap". At the location where the new code is being added, "MemoryMap" points one past the last descriptor, and "MemoryMapStart" still points to the first one. In order to keep this property for possible future "scans" of the full map, I would prefer keeping "MemoryMapStart" unchanged in this location, and using an independent loop variable. ... On the other hand, we can easily restore "MemoryMapStart", should it be necessary, with the help of "BufferSize". So, I guess the function does not become more difficult to work with after this patch. Reviewed-by: Laszlo Ersek (I hope that Star and/or Jiewen will also R-b this patch, possibly with the updated code comment.) Thanks, Laszlo