Laszlo:
> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, August 13, 2019 5:48 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; 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/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
> >> //
I cherry pick this patch from Mu project with the minimal change.
I can update the comment style.
> >>
> >>
> >>> +
> >>> + 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?
>
gEfiMemoryTypeInformationGuid HOB is installed by platform PEI.
If the platform PEI doesn't install this HOB, it means this feature is
disabled.
Thanks
Liming
> 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 (#45607): https://edk2.groups.io/g/devel/message/45607
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]
-=-=-=-=-=-=-=-=-=-=-=-
