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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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),