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 >> <liming....@intel.com> >> Cc: Mike Turner <mike...@microsoft.com>; Wang, Jian J >> <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>; >> Bi, Dandan <dandan...@intel.com> >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: >> Fix for missing MAT update >> >> On 08/10/19 16:10, Liming Gao wrote: >>> From: Mike Turner <mike...@microsoft.com> >>> >>> 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 <jian.j.w...@intel.com> >>> Cc: Hao A Wu <hao.a...@intel.com> >>> Cc: Dandan Bi <dandan...@intel.com> >>> Signed-off-by: Liming Gao <liming....@intel.com> >>> --- >>> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45516): https://edk2.groups.io/g/devel/message/45516 Mute This Topic: https://groups.io/mt/32821535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
