Re: [edk2-devel] [PATCH V2 14/14] MdeModulePkg: Pool and page functions accept memory when OOM occurs

["Ni, Ray" <ray.ni@intel.com>]

Can you please explain more about this patch?
I am a little nervous when seeing patches that change the fundamental memory services.

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas via groups.io
> Sent: Tuesday, August 30, 2022 4:47 AM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Bi,
> Dandan <dandan.bi@intel.com>; Aktas, Erdem <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH V2 14/14] MdeModulePkg: Pool and page functions accept memory when OOM occurs
> 
> On 8/27/22 01:21, Min Xu wrote:
> > From: Jiaqi Gao <jiaqi.gao@intel.com>
> >
> > RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3937
> >
> > When CoreAllocatePages() / CoreAllocatePool() meets error of
> > EFI_OUT_OF_RESOURCES, locate the EdkiiMemoryAcceptProtocol and accept extra
> > memory dynamically.
> >
> > Firstly, find the unaccpeted memory region with enough size in GCD
> 
> s/unaccpeted/unaccepted/
> 
> > entries. Then locate the EdkiiMemoryAcceptProtocol and accept the memory.
> > Finally, update the GCD memory and gMemoryMap entries.
> >
> > After updating the memory infomation, CoreInternalAllocatePages() /
> > CoreInternalAllocatePool() will be recalled to allocate pages / pool.
> 
> What path does allocation take when called through boot services? If I set
> a 256MB accepted memory size, I can get to the bootloader and select my
> kernel. But then the kernel dies in efi_relocate_kernel() with:
> 
> EFI stub: ERROR: Failed to allocate usable memory for kernel.
> EFI stub: ERROR: efi_relocate_kernel() failed!
> EFI stub: ERROR: efi_main() failed!
> 
> because both efi_bs_call(allocate_pages, ...) and efi_low_alloc_above() fail.
> 
> Similar to DXE, should OVMF accept more memory through this path to let
> the kernel boot?
> 
> Thanks,
> Tom
> 
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
> > Signed-off-by: Min Xu <min.m.xu@intel.com>
> > ---
> >   MdeModulePkg/Core/Dxe/DxeMain.inf |   1 +
> >   MdeModulePkg/Core/Dxe/Mem/Imem.h  |  16 +++
> >   MdeModulePkg/Core/Dxe/Mem/Page.c  | 190 ++++++++++++++++++++++++++++++
> >   MdeModulePkg/Core/Dxe/Mem/Pool.c  |  14 +++
> >   4 files changed, 221 insertions(+)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index e4bca895773d..371ba45357be 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -169,6 +169,7 @@
> >     gEfiVariableArchProtocolGuid                  ## CONSUMES
> >     gEfiCapsuleArchProtocolGuid                   ## CONSUMES
> >     gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
> > +  gEdkiiMemoryAcceptProtocolGuid                ## CONSUMES
> >
> >   [Pcd]
> >     gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber    ## SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Imem.h b/MdeModulePkg/Core/Dxe/Mem/Imem.h
> > index 2f0bf2bf631f..22e0d0e44030 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Imem.h
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Imem.h
> > @@ -47,6 +47,22 @@ typedef struct {
> >   // Internal prototypes
> >   //
> >
> > +/**
> > +  Internal function.  Used by the pool and page functions to accept memory
> > +  when OOM occurs.
> > +
> > +  @param  Type                   The type of allocation to perform.
> > +  @param  AcceptSize             Size of memory to be accepted.
> > +  @param  Memory                 Accept memory address
> > +
> > +**/
> > +EFI_STATUS
> > +AcceptMemoryResource (
> > +  IN EFI_ALLOCATE_TYPE         Type,
> > +  IN UINTN                     AcceptSize,
> > +  IN OUT EFI_PHYSICAL_ADDRESS  *Memory
> > +  );
> > +
> >   /**
> >     Internal function.  Used by the pool functions to allocate pages
> >     to back pool allocation requests.
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index 160289c1f9ec..513792a7fe04 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -10,6 +10,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >   #include "Imem.h"
> >   #include "HeapGuard.h"
> >   #include <Pi/PrePiDxeCis.h>
> > +#include <Protocol/MemoryAccept.h>
> >
> >   //
> >   // Entry for tracking the memory regions for each memory type to coalesce similar memory types
> > @@ -379,6 +380,176 @@ CoreFreeMemoryMapStack (
> >     mFreeMapStack -= 1;
> >   }
> >
> > +/**
> > +  Used to accept memory when OOM occurs.
> > +
> > +  @param  Type                   The type of allocation to perform.
> > +  @param  AcceptSize             Size of memory to be accepted.
> > +  @param  Memory                 Accept memory address
> > +
> > +**/
> > +EFI_STATUS
> > +AcceptMemoryResource (
> > +  IN EFI_ALLOCATE_TYPE         Type,
> > +  IN UINTN                     AcceptSize,
> > +  IN OUT EFI_PHYSICAL_ADDRESS  *Memory
> > +  )
> > +{
> > + #ifdef MDE_CPU_X64
> > +
> > +  LIST_ENTRY                    *Link;
> > +  EFI_GCD_MAP_ENTRY             *GcdEntry;
> > +  EFI_GCD_MAP_ENTRY             UnacceptedEntry;
> > +  EDKII_MEMORY_ACCEPT_PROTOCOL  *MemoryAcceptProtocol;
> > +  UINTN                         Start;
> > +  UINTN                         End;
> > +  EFI_STATUS                    Status;
> > +
> > +  //
> > +  // We accept n*32MB at one time to improve the efficiency.
> > +  //
> > +  AcceptSize = (AcceptSize + SIZE_32MB - 1) & ~(SIZE_32MB - 1);
> > +
> > +  if (AcceptSize == 0) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  Status = gBS->LocateProtocol (&gEdkiiMemoryAcceptProtocolGuid, NULL, (VOID **)&MemoryAcceptProtocol);
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_UNSUPPORTED;
> > +  }
> > +
> > +  if (Type == AllocateAddress) {
> > +    Start = *Memory;
> > +    End   = *Memory + AcceptSize;
> > +  }
> > +
> > +  if (Type == AllocateMaxAddress) {
> > +    if (*Memory < EFI_PAGE_MASK) {
> > +      return EFI_INVALID_PARAMETER;
> > +    }
> > +
> > +    if ((*Memory & EFI_PAGE_MASK) != EFI_PAGE_MASK) {
> > +      //
> > +      // Change MaxAddress to be 1 page lower
> > +      //
> > +      *Memory -= EFI_PAGE_SIZE;
> > +
> > +      //
> > +      // Set MaxAddress to a page boundary
> > +      //
> > +      *Memory &= ~(UINT64)EFI_PAGE_MASK;
> > +
> > +      //
> > +      // Set MaxAddress to end of the page
> > +      //
> > +      *Memory |= EFI_PAGE_MASK;
> > +    }
> > +  }
> > +
> > +  //
> > +  // Traverse the mGcdMemorySpaceMap to find out the unaccepted
> > +  // memory entry with big enough size.
> > +  //
> > +  Link = mGcdMemorySpaceMap.ForwardLink;
> > +  while (Link != &mGcdMemorySpaceMap) {
> > +    GcdEntry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> > +    if (GcdEntry->GcdMemoryType == EFI_GCD_MEMORY_TYPE_UNACCEPTED) {
> > +      if (Type == AllocateMaxAddress) {
> > +        if (GcdEntry->BaseAddress + AcceptSize - 1 > *Memory) {
> > +          Link = Link->ForwardLink;
> > +          continue;
> > +        }
> > +      } else if (Type == AllocateAddress) {
> > +        if ((GcdEntry->BaseAddress > *Memory) || (GcdEntry->EndAddress < *Memory + AcceptSize - 1)) {
> > +          Link = Link->ForwardLink;
> > +          continue;
> > +        }
> > +      }
> > +
> > +      //
> > +      // Is the entry big enough?
> > +      //
> > +      if (AcceptSize <= GcdEntry->EndAddress - GcdEntry->BaseAddress + 1) {
> > +        UnacceptedEntry = *GcdEntry;
> > +        if (Type != AllocateAddress) {
> > +          Start = UnacceptedEntry.BaseAddress;
> > +          End   = UnacceptedEntry.BaseAddress + AcceptSize - 1;
> > +        }
> > +
> > +        break;
> > +      }
> > +    }
> > +
> > +    Link = Link->ForwardLink;
> > +  }
> > +
> > +  if (Link == &mGcdMemorySpaceMap) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  //
> > +  // Accept memory using the interface provide by the protocol.
> > +  //
> > +  Status = MemoryAcceptProtocol->AcceptMemory (MemoryAcceptProtocol, Start, AcceptSize);
> > +  if (EFI_ERROR (Status)) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  //
> > +  // If memory is accepted successfully, remove the target memory space from GCD.
> > +  //
> > +  CoreRemoveMemorySpace (UnacceptedEntry.BaseAddress, UnacceptedEntry.EndAddress -
> UnacceptedEntry.BaseAddress + 1);
> > +
> > +  //
> > +  // Add the remain lower part of unaccepted memory to the
> > +  // Gcd memory space and memory map.
> > +  //
> > +  if (Start > UnacceptedEntry.BaseAddress) {
> > +    CoreAddMemorySpace (
> > +      EFI_GCD_MEMORY_TYPE_UNACCEPTED,
> > +      UnacceptedEntry.BaseAddress,
> > +      Start - UnacceptedEntry.BaseAddress,
> > +      UnacceptedEntry.Capabilities
> > +      );
> > +  }
> > +
> > +  //
> > +  // Update accepted part of the memory entry to type of EfiGcdMemoryTypeSystemMemory
> > +  // and add the range to the memory map.
> > +  //
> > +  CoreAddMemorySpace (
> > +    EfiGcdMemoryTypeSystemMemory,
> > +    Start,
> > +    AcceptSize,
> > +    //
> > +    // Hardcode memory space attributes.
> > +    //
> > +    EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
> > +    );
> > +
> > +  //
> > +  // Add the remain higher part of unaccepted memory to the
> > +  // Gcd memory space and memory map.
> > +  //
> > +  if (UnacceptedEntry.EndAddress > End) {
> > +    CoreAddMemorySpace (
> > +      EFI_GCD_MEMORY_TYPE_UNACCEPTED,
> > +      End + 1,
> > +      UnacceptedEntry.EndAddress - End,
> > +      UnacceptedEntry.Capabilities
> > +      );
> > +  }
> > +
> > +  return EFI_SUCCESS;
> > +
> > + #else
> > +
> > +  return EFI_UNSUPPORTED;
> > +
> > + #endif
> > +}
> > +
> >   /**
> >     Find untested but initialized memory regions in GCD map and convert them to be DXE allocatable.
> >
> > @@ -1486,6 +1657,25 @@ CoreAllocatePages (
> >                   Memory,
> >                   NeedGuard
> >                   );
> > + #ifdef MDE_CPU_X64
> > +
> > +  if (Status == EFI_OUT_OF_RESOURCES) {
> > +    Status = AcceptMemoryResource (Type, NumberOfPages << EFI_PAGE_SHIFT, Memory);
> > +    if (!EFI_ERROR (Status)) {
> > +      Status = CoreInternalAllocatePages (
> > +                 Type,
> > +                 MemoryType,
> > +                 NumberOfPages,
> > +                 Memory,
> > +                 NeedGuard
> > +                 );
> > +    } else {
> > +      Status = EFI_OUT_OF_RESOURCES;
> > +    }
> > +  }
> > +
> > + #endif
> > +
> >     if (!EFI_ERROR (Status)) {
> >       CoreUpdateProfile (
> >         (EFI_PHYSICAL_ADDRESS)(UINTN)RETURN_ADDRESS (0),
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> > index 7aaf501600cf..9e8c8611c1ef 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> > @@ -273,6 +273,20 @@ CoreAllocatePool (
> >     EFI_STATUS  Status;
> >
> >     Status = CoreInternalAllocatePool (PoolType, Size, Buffer);
> > +
> > + #ifdef MDE_CPU_X64
> > +
> > +  if (Status == EFI_OUT_OF_RESOURCES) {
> > +    Status = AcceptMemoryResource (AllocateAnyPages, Size, NULL);
> > +    if (!EFI_ERROR (Status)) {
> > +      Status = CoreInternalAllocatePool (PoolType, Size, Buffer);
> > +    } else {
> > +      Status = EFI_OUT_OF_RESOURCES;
> > +    }
> > +  }
> > +
> > + #endif
> > +
> >     if (!EFI_ERROR (Status)) {
> >       CoreUpdateProfile (
> >         (EFI_PHYSICAL_ADDRESS)(UINTN)RETURN_ADDRESS (0),
> 
> 
> 
>