Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

["Michael D Kinney" <michael.d.kinney@intel.com>]

> -----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.gao@intel.com>
> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@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 <miketur@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.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Signed-off-by: Liming Gao <liming.gao@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.

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.  

The only time these types of check can be disabled is if the
Memory Type Information feature is disabled.

Thanks,

Mike

> 
> Thanks,
> Laszlo
> 
> 
> >
> >    if (Type == AllocateMaxAddress) {
> >
> 
> 
>