From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 13 Aug 2019 02:47:59 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8A202155DB; Tue, 13 Aug 2019 09:47:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-193.ams2.redhat.com [10.36.117.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8604C804F8; Tue, 13 Aug 2019 09:47:56 +0000 (UTC) Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update To: "Kinney, Michael D" , "devel@edk2.groups.io" , "Gao, Liming" Cc: Mike Turner , "Wang, Jian J" , "Wu, Hao A" , "Bi, Dandan" References: <20190810141022.18228-1-liming.gao@intel.com> <66665886-fff1-248a-77e5-6b8fb6966f86@redhat.com> From: "Laszlo Ersek" Message-ID: <96f6e8a3-7049-478a-a8f2-4389c06f0e12@redhat.com> Date: Tue, 13 Aug 2019 11:47:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 13 Aug 2019 09:47:58 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/13/19 01:22, Kinney, Michael D wrote: > > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] >> On Behalf Of Laszlo Ersek >> Sent: Monday, August 12, 2019 9:24 AM >> To: devel@edk2.groups.io; Gao, Liming >> >> Cc: Mike Turner ; Wang, Jian J >> ; Wu, Hao A ; >> Bi, Dandan >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: >> Fix for missing MAT update >> >> On 08/10/19 16:10, Liming Gao wrote: >>> From: Mike Turner >>> >>> The Fpdt driver (FirmwarePerformanceDxe) saves a memory >> address across >>> reboots, and then does an AllocatePage for that memory >> address. >>> If, on this boot, that memory comes from a Runtime >> memory bucket, the >>> MAT table is not updated. This causes Windows to boot >> into Recovery. >> >> (1) What is "MAT"? > > Memory Attributes Table (EFI_MEMORY_ATTRIBUTES_TABLE) > >> >>> This patch blocks the memory manager from changing the >> page from a >>> special bucket to a different memory type. Once the >> buckets are >>> allocated, we freeze the memory ranges for the OS, and >> fragmenting the >>> special buckets will cause errors resuming from >> hibernate. >> >> (2) My understanding is that CoreConvertPages() will only >> hand out the requested pages if those pages are currently >> free. I suggest clarifying the commit message that the >> intent is to prevent the allocation of otherwise *free* >> pages, if the allocation would fragment special buckets. >> >> (3) I don't understand the conjunction "and". I would >> understand if the statement were: >> >> Once the buckets are allocated, we freeze the memory >> ranges for the >> OS, *because* fragmenting the special buckets *would* >> cause errors >> resuming from hibernate. >> >> Is this the intent? >> >>> >>> This patch is cherry pick from Project Mu: >>> >> https://github.com/microsoft/mu_basecore/commit/a9be767d9 >> be96af94016eb >>> d391ea6f340920735a >>> With the minor change, >>> 1. Update commit message format to keep the message in >> 80 characters one line. >>> 2. Remove // MU_CHANGE comments in source code. >>> >>> Cc: Jian J Wang >>> Cc: Hao A Wu >>> Cc: Dandan Bi >>> Signed-off-by: Liming Gao >>> --- >>> MdeModulePkg/Core/Dxe/Mem/Page.c | 43 >>> ++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 37 insertions(+), 6 deletions(-) >>> >>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> index bd9e116aa5..637518889d 100644 >>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c >>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c >>> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages ( >>> IN BOOLEAN NeedGuard >>> ) >>> { >>> - EFI_STATUS Status; >>> - UINT64 Start; >>> - UINT64 NumberOfBytes; >>> - UINT64 End; >>> - UINT64 MaxAddress; >>> - UINTN Alignment; >>> + EFI_STATUS Status; >>> + UINT64 Start; >>> + UINT64 NumberOfBytes; >>> + UINT64 End; >>> + UINT64 MaxAddress; >>> + UINTN Alignment; >>> + EFI_MEMORY_TYPE CheckType; >>> >>> if ((UINT32)Type >= MaxAllocateType) { >>> return EFI_INVALID_PARAMETER; >>> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages ( >>> // if (Start + NumberOfBytes) rolls over 0 or >>> // if Start is above MAX_ALLOC_ADDRESS or >>> // if End is above MAX_ALLOC_ADDRESS, >>> + // if Start..End overlaps any tracked >> MemoryTypeStatistics range >>> // return EFI_NOT_FOUND. >>> // >>> if (Type == AllocateAddress) { >>> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages ( >>> (End > MaxAddress)) { >>> return EFI_NOT_FOUND; >>> } >>> + >>> + // Problem summary >>> + >>> + /* >>> + A driver is allowed to call AllocatePages using an >> AllocateAddress type. This type of >>> + AllocatePage request the exact physical address if >> it is not used. The existing code >>> + will allow this request even in 'special' pages. >> The problem with this is that the >>> + reason to have 'special' pages for OS >> hibernate/resume is defeated as memory is >>> + fragmented. >>> + */ >> >> (4) This comment style is not native to edk2. >> >> I think the "problem summary" line should be removed, and >> the actual problem statement should use the following >> comment style: >> >> // >> // blah >> // >> >> >>> + >>> + for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType < >> EfiMaxMemoryType; CheckType++) { >>> + if (MemoryType != CheckType && >>> + mMemoryTypeStatistics[CheckType].Special && >>> + >> mMemoryTypeStatistics[CheckType].NumberOfPages > 0) { >>> + if (Start >= >> mMemoryTypeStatistics[CheckType].BaseAddress && >>> + Start <= >> mMemoryTypeStatistics[CheckType].MaximumAddress) { >>> + return EFI_NOT_FOUND; >>> + } >>> + if (End >= >> mMemoryTypeStatistics[CheckType].BaseAddress && >>> + End <= >> mMemoryTypeStatistics[CheckType].MaximumAddress) { >>> + return EFI_NOT_FOUND; >>> + } >>> + if (Start < >> mMemoryTypeStatistics[CheckType].BaseAddress && >>> + End > >> mMemoryTypeStatistics[CheckType].MaximumAddress) { >>> + return EFI_NOT_FOUND; >>> + } >>> + } >>> + } >>> } >> >> (5) Checking for overlap (i.e., whether the intersection >> is non-empty) can be done more simply (i.e., with fewer >> comparisons in the worst case, and with less code): >> >> if (MAX (Start, >> mMemoryTypeStatistics[CheckType].BaseAddress) <= >> MIN (End, >> mMemoryTypeStatistics[CheckType].MaximumAddress)) { >> return EFI_NOT_FOUND; >> } >> >> but the proposed intersection check is technically right >> already, IMO, so there's no strong need to update it. >> >> (Somewhat unusually for this kind of comparison, all four >> boundaries are inclusive here.) >> >> (6) Both the commit message and the code comment state >> that this problem is specific to S4. Therefore, we can >> distinguish three cases: >> >> (6a) Platform doesn't support (or doesn't enable) S4 at >> all. >> >> (6b) Platform supports & enables S4, and this is a normal >> boot. >> >> (6c) Platform supports & enables S4, and this is actually >> an S4 resume. >> >> The code being proposed applies to all three cases. Is >> that the intent? >> Shouldn't the new check be made conditional on (6c) -- >> from the boot mode HOB --, or at least on (6b)||(6c) -- >> i.e. the check should be disabled if S4 is absent >> entirely? > > Hi Laszlo, > > I think this check should be added for all cases. Without > this change, memory allocations using type AllocateAddress > Is allowed to allocate in the unused portion of a bin. This > means the memory allocations are not consist with the memory > map returned by GetMemoryMap() that shows the entire bin as > allocated. The only exception that is allowed is if an > AllocateAddress request is made to the unused portion of a > bin where the request and the bin have the same MemoryType. Thanks for the explanation. It helps! I understand now. > The references to S4 here are the use case that fails. This > failure is root caused to an inconsistent behavior of the > core memory services themselves when type AllocateAddress is > used. Can the commit message be framed accordingly, please? The main issue is apparently with the UEFI memory map -- the UEFI memory map reflects the pre-allocated bins, but the actual allocations at fixed addresses may go out of sync with that. Everything else, such as: - EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync, - S4 failing are just symptoms / consequences. > The only time these types of check can be disabled is if the > Memory Type Information feature is disabled. The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it is built at all -- no later than in the DXE IPL PEIM (if VariablePei is included in the platform, and the underlying UEFI variable exists). Is that correct? Becase if it is correct, then I think the check could be based (in the DXE core) on the presence of this HOB. Thank you! Laszlo