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; Mon, 12 Aug 2019 09:24:20 -0700 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B75CFC008601; Mon, 12 Aug 2019 16:24:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-247.ams2.redhat.com [10.36.116.247]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2D6C7601AC; Mon, 12 Aug 2019 16:24:17 +0000 (UTC) Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update To: devel@edk2.groups.io, liming.gao@intel.com Cc: Mike Turner , Jian J Wang , Hao A Wu , Dandan Bi References: <20190810141022.18228-1-liming.gao@intel.com> From: "Laszlo Ersek" Message-ID: <66665886-fff1-248a-77e5-6b8fb6966f86@redhat.com> Date: Mon, 12 Aug 2019 18:24:17 +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: <20190810141022.18228-1-liming.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 12 Aug 2019 16:24:19 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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"? > 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/a9be767d9be96af94016ebd391ea6f340920735a > 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? Thanks, Laszlo > > if (Type == AllocateMaxAddress) { >