Re: [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Ard,

I think MAX_ALLOC_ADDRESS will affect other archs besides ARM. Please do enough
test for them (IA32/X64 for my concern).

In addition, do you think it's safer to replace MAX_ADDRESS with MAX_ALLOC_ADDRESS
in MemoryAllocationLib like following situation?

(MdeModulePkg\Library\DxeCoreMemoryAllocationLib\MemoryAllocationLib.c)
VOID *
InternalAllocateCopyPool (
  IN EFI_MEMORY_TYPE  PoolType,
  IN UINTN            AllocationSize,
  IN CONST VOID       *Buffer
  )
{
  VOID  *Memory;

  ASSERT (Buffer != NULL);
  ASSERT (AllocationSize <= (MAX_ADDRESS - (UINTN) Buffer + 1));
  ...
}

Regards,
Jian


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, December 07, 2018 7:23 PM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang,
> Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Leif
> Lindholm <leif.lindholm@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Eric
> Auger <eric.auger@redhat.com>; Andrew Jones <drjones@redhat.com>;
> Philippe Mathieu-Daude <philmd@redhat.com>
> Subject: [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take
> MAX_ALLOC_ADDRESS into account
> 
> Take MAX_ALLOC_ADDRESS into account in the implementation of the
> page allocation routines, so that they will only return memory
> that is addressable by the CPU at boot time, even if more memory
> is available in the GCD memory map.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 52 ++++++++++----------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 961c5b833546..5ad8e1171ef7 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -52,26 +52,26 @@ LIST_ENTRY   mFreeMemoryMapEntryList =
> INITIALIZE_LIST_HEAD_VARIABLE (mFreeMemor
>  BOOLEAN      mMemoryTypeInformationInitialized = FALSE;
> 
>  EFI_MEMORY_TYPE_STATISTICS mMemoryTypeStatistics[EfiMaxMemoryType +
> 1] = {
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiReservedMemoryType
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiLoaderCode
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiLoaderData
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiBootServicesCode
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiBootServicesData
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  //
> EfiRuntimeServicesCode
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  //
> EfiRuntimeServicesData
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiConventionalMemory
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiUnusableMemory
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiACPIReclaimMemory
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiACPIMemoryNVS
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiMemoryMappedIO
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiMemoryMappedIOPortSpace
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  // EfiPalCode
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiPersistentMemory
> -  { 0, MAX_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE }   //
> EfiMaxMemoryType
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiReservedMemoryType
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiLoaderCode
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiLoaderData
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiBootServicesCode
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiBootServicesData
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  //
> EfiRuntimeServicesCode
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  //
> EfiRuntimeServicesData
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiConventionalMemory
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiUnusableMemory
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiACPIReclaimMemory
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  FALSE },  //
> EfiACPIMemoryNVS
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiMemoryMappedIO
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiMemoryMappedIOPortSpace
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, TRUE,  TRUE  },  //
> EfiPalCode
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE },  //
> EfiPersistentMemory
> +  { 0, MAX_ALLOC_ADDRESS, 0, 0, EfiMaxMemoryType, FALSE, FALSE }   //
> EfiMaxMemoryType
>  };
> 
> -EFI_PHYSICAL_ADDRESS mDefaultMaximumAddress = MAX_ADDRESS;
> -EFI_PHYSICAL_ADDRESS mDefaultBaseAddress = MAX_ADDRESS;
> +EFI_PHYSICAL_ADDRESS mDefaultMaximumAddress = MAX_ALLOC_ADDRESS;
> +EFI_PHYSICAL_ADDRESS mDefaultBaseAddress = MAX_ALLOC_ADDRESS;
> 
>  EFI_MEMORY_TYPE_INFORMATION
> gMemoryTypeInformation[EfiMaxMemoryType + 1] = {
>    { EfiReservedMemoryType,      0 },
> @@ -419,7 +419,7 @@ PromoteMemoryResource (
>      Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
> 
>      if (Entry->GcdMemoryType == EfiGcdMemoryTypeReserved &&
> -        Entry->EndAddress < MAX_ADDRESS &&
> +        Entry->EndAddress < MAX_ALLOC_ADDRESS &&
>          (Entry->Capabilities & (EFI_MEMORY_PRESENT |
> EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
>            (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
>        //
> @@ -640,7 +640,7 @@ CoreAddMemoryDescriptor (
>                gMemoryTypeInformation[FreeIndex].NumberOfPages
>                );
>              mMemoryTypeStatistics[Type].BaseAddress    = 0;
> -            mMemoryTypeStatistics[Type].MaximumAddress = MAX_ADDRESS;
> +            mMemoryTypeStatistics[Type].MaximumAddress =
> MAX_ALLOC_ADDRESS;
>            }
>          }
>          return;
> @@ -697,7 +697,7 @@ CoreAddMemoryDescriptor (
>        }
>      }
>      mMemoryTypeStatistics[Type].CurrentNumberOfPages = 0;
> -    if (mMemoryTypeStatistics[Type].MaximumAddress == MAX_ADDRESS) {
> +    if (mMemoryTypeStatistics[Type].MaximumAddress ==
> MAX_ALLOC_ADDRESS) {
>        mMemoryTypeStatistics[Type].MaximumAddress =
> mDefaultMaximumAddress;
>      }
>    }
> @@ -1318,15 +1318,15 @@ CoreInternalAllocatePages (
>    //
>    // The max address is the max natively addressable address for the processor
>    //
> -  MaxAddress = MAX_ADDRESS;
> +  MaxAddress = MAX_ALLOC_ADDRESS;
> 
>    //
>    // Check for Type AllocateAddress,
>    // if NumberOfPages is 0 or
> -  // if (NumberOfPages << EFI_PAGE_SHIFT) is above MAX_ADDRESS or
> +  // if (NumberOfPages << EFI_PAGE_SHIFT) is above MAX_ALLOC_ADDRESS or
>    // if (Start + NumberOfBytes) rolls over 0 or
> -  // if Start is above MAX_ADDRESS or
> -  // if End is above MAX_ADDRESS,
> +  // if Start is above MAX_ALLOC_ADDRESS or
> +  // if End is above MAX_ALLOC_ADDRESS,
>    // return EFI_NOT_FOUND.
>    //
>    if (Type == AllocateAddress) {
> @@ -1968,7 +1968,7 @@ CoreAllocatePoolPages (
>    //
>    // Find the pages to convert
>    //
> -  Start = FindFreePages (MAX_ADDRESS, NumberOfPages, PoolType, Alignment,
> +  Start = FindFreePages (MAX_ALLOC_ADDRESS, NumberOfPages, PoolType,
> Alignment,
>                           NeedGuard);
> 
>    //
> --
> 2.19.2