[RFC PATCH 0/7] introduce MAX_ALLOC_ADDRESS to limit boot time allocations

[RFC PATCH 0/7] introduce MAX_ALLOC_ADDRESS to limit boot time allocations

[Ard Biesheuvel]

Since modifying MAX_ADDRESS to limit the memory used at boot time has
turned out to be intractible, this series proposes another approach to do
the same, by introducing MAX_ALLOC_ADDRESS for firmware internal use.

I tested these patches with ArmVirtQemu in the following way:
- limit MAX_ALLOC_ADDRESS to 0xFFFFFFFF (4 GB)
- build QEMU_EFI.fd
- run it under mach-virt with 4 GB of DRAM and highmem=off

This runs as expected, and produces a memory map ending in the following
lines

BS_Data    00000000FFFFD000-00000000FFFFFFFF 0000000000000003 0000000000000008
Available  0000000100000000-000000013FFFFFFF 0000000000040000 0000000000000008

which proves that the memory above the limit is recorded and reported by
the OS, but left untouched by the firmware memory allocation routines.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>
Cc: Philippe Mathieu-Daude <philmd@redhat.com>

Ard Biesheuvel (7):
  MdePkg/Base: introduce MAX_ALLOC_ADDRESS
  MdeModulePkg/Dxe/Gcd: disregard memory above MAX_ALLOC_ADDRESS
  MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account
  ArmPkg/ArmMmuLib: take MAX_ALLOC_ADDRESS into account
  ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account
  ArmVirtPkg/MemoryInitPeiLib: split memory HOB based on
    MAX_ALLOC_ADDRESS
  MdePkg/ProcessorBind AARCH64: limit MAX_ALLOC_ADDRESS to 48 bits

 MdePkg/Include/AArch64/ProcessorBind.h        |  5 ++
 MdePkg/Include/Base.h                         |  4 ++
 .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  2 +-
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c |  4 +-
 .../ArmVirtMemoryInitPeiLib.c                 |  8 +--
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c               |  8 +--
 MdeModulePkg/Core/Dxe/Mem/Page.c              | 52 +++++++++----------
 7 files changed, 46 insertions(+), 37 deletions(-)

-- 
2.19.2

[RFC PATCH 1/7] MdePkg/Base: introduce MAX_ALLOC_ADDRESS

[Ard Biesheuvel]

On some architectures, the maximum representable address deviates from
the virtual address range that is accessible by the firmware at boot
time. For instance, on AArch64, UEFI mandates a 4 KB page size, which
limits the address space to 48 bits, while more than that may be
populated on a particular platform, for use by the OS.

So introduce a new macro MAX_ALLOC_ADDRESS, which represent the maximum
address the firmware should take into account when allocating memory
ranges that need to be accessible by the CPU at boot time. Initially,
it just defaults to MAX_ADDRESS, but later on, architectures may elect
to override it to a smaller number.

This means that all replacements of MAX_ADDRESS with MAX_ALLOC_ADDRESS
are functional no-ops unless an architecture sets a value for the latter.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Include/Base.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index bc877d8125a5..618f8ea85ce7 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -404,6 +404,10 @@ struct _LIST_ENTRY {
 #define MIN_INT32  (((INT32) -2147483647) - 1)
 #define MIN_INT64  (((INT64) -9223372036854775807LL) - 1)
 
+#ifndef MAX_ALLOC_ADDRESS
+#define MAX_ALLOC_ADDRESS   MAX_ADDRESS
+#endif
+
 #define  BIT0     0x00000001
 #define  BIT1     0x00000002
 #define  BIT2     0x00000004
-- 
2.19.2

Re: [RFC PATCH 1/7] MdePkg/Base: introduce MAX_ALLOC_ADDRESS

[Laszlo Ersek]

On 12/07/18 12:22, Ard Biesheuvel wrote:
> On some architectures, the maximum representable address deviates from
> the virtual address range that is accessible by the firmware at boot
> time. For instance, on AArch64, UEFI mandates a 4 KB page size, which
> limits the address space to 48 bits, while more than that may be
> populated on a particular platform, for use by the OS.
> 
> So introduce a new macro MAX_ALLOC_ADDRESS, which represent the maximum
> address the firmware should take into account when allocating memory
> ranges that need to be accessible by the CPU at boot time. Initially,
> it just defaults to MAX_ADDRESS, but later on, architectures may elect
> to override it to a smaller number.
> 
> This means that all replacements of MAX_ADDRESS with MAX_ALLOC_ADDRESS
> are functional no-ops unless an architecture sets a value for the latter.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Include/Base.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> index bc877d8125a5..618f8ea85ce7 100644
> --- a/MdePkg/Include/Base.h
> +++ b/MdePkg/Include/Base.h
> @@ -404,6 +404,10 @@ struct _LIST_ENTRY {
>  #define MIN_INT32  (((INT32) -2147483647) - 1)
>  #define MIN_INT64  (((INT64) -9223372036854775807LL) - 1)
>  
> +#ifndef MAX_ALLOC_ADDRESS
> +#define MAX_ALLOC_ADDRESS   MAX_ADDRESS
> +#endif
> +
>  #define  BIT0     0x00000001
>  #define  BIT1     0x00000002
>  #define  BIT2     0x00000004
> 

Given that the page size is dictated by the spec, I think adding
MAX_ALLOC_ADDRESS as a macro, and doing it here, is appropriate.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[RFC PATCH 2/7] MdeModulePkg/Dxe/Gcd: disregard memory above MAX_ALLOC_ADDRESS

[Ard Biesheuvel]

Update the GCD memory map initialization code so it disregards
memory that is not addressable by the CPU at boot time. This
only affects the first memory descriptor that is added, other
memory descriptors are permitted that describe memory ranges
that may be accessible to the CPU itself only when executing
under the OS.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index a76d2db73c46..504e14a74e1d 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2284,7 +2284,7 @@ CoreInitializeMemoryServices (
     // region that is big enough to initialize the DXE core.  Always skip the PHIT Resource HOB.
     // The max address must be within the physically addressible range for the processor.
     //
-    HighAddress = MAX_ADDRESS;
+    HighAddress = MAX_ALLOC_ADDRESS;
     for (Hob.Raw = *HobStart; !END_OF_HOB_LIST(Hob); Hob.Raw = GET_NEXT_HOB(Hob)) {
       //
       // Skip the Resource Descriptor HOB that contains the PHIT
@@ -2300,7 +2300,7 @@ CoreInitializeMemoryServices (
       }
 
       //
-      // Skip Resource Descriptor HOBs that do not describe tested system memory below MAX_ADDRESS
+      // Skip Resource Descriptor HOBs that do not describe tested system memory below MAX_ALLOC_ADDRESS
       //
       ResourceHob = Hob.ResourceDescriptor;
       if (ResourceHob->ResourceType != EFI_RESOURCE_SYSTEM_MEMORY) {
@@ -2309,14 +2309,14 @@ CoreInitializeMemoryServices (
       if ((ResourceHob->ResourceAttribute & MEMORY_ATTRIBUTE_MASK) != TESTED_MEMORY_ATTRIBUTES) {
         continue;
       }
-      if ((ResourceHob->PhysicalStart + ResourceHob->ResourceLength) > (EFI_PHYSICAL_ADDRESS)MAX_ADDRESS) {
+      if ((ResourceHob->PhysicalStart + ResourceHob->ResourceLength) > (EFI_PHYSICAL_ADDRESS)MAX_ALLOC_ADDRESS) {
         continue;
       }
 
       //
       // Skip Resource Descriptor HOBs that are below a previously found Resource Descriptor HOB
       //
-      if (HighAddress != (EFI_PHYSICAL_ADDRESS)MAX_ADDRESS && ResourceHob->PhysicalStart <= HighAddress) {
+      if (HighAddress != (EFI_PHYSICAL_ADDRESS)MAX_ALLOC_ADDRESS && ResourceHob->PhysicalStart <= HighAddress) {
         continue;
       }
 
-- 
2.19.2

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

[Ard Biesheuvel]

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

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

[Wang, Jian J]

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

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

[Ard Biesheuvel]

On Mon, 10 Dec 2018 at 03:04, Wang, Jian J <jian.j.wang@intel.com> wrote:
>
> Hi Ard,
>
> I think MAX_ALLOC_ADDRESS will affect other archs besides ARM. Please do enough
> test for them (IA32/X64 for my concern).
>

For all other architectures, MAX_ADDRESS == MAX_ALLOC_ADDRESS is
always true, so these changes only affect AARCH64.

> 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));
>   ...

This assert ensures that the copied buffer does not extend across the
end of the address space and wraps. This is a separate concern, and is
similar to numerous other occurrences of MAX_ADDRESS that maybe we
should update as well at some point. However, it does not affect page
allocation at all, it only puts an upper bound on the *size* of the
allocation. So the changes as they are will be sufficient to ensure
that AllocateCopyPool() does not allocate from a region that is not
addressable by the CPU.

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

[Gao, Liming]

Ard:
  I prefer to define MAX_ALLOC_ADDRESS together with MAX_ADDRESS in ProcessorBind.h. I don't want to leave the choice to override MAX_ALLOC_ADDRESS definition. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Monday, December 10, 2018 3:23 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: Andrew Jones <drjones@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account
> 
> On Mon, 10 Dec 2018 at 03:04, Wang, Jian J <jian.j.wang@intel.com> wrote:
> >
> > Hi Ard,
> >
> > I think MAX_ALLOC_ADDRESS will affect other archs besides ARM. Please do enough
> > test for them (IA32/X64 for my concern).
> >
> 
> For all other architectures, MAX_ADDRESS == MAX_ALLOC_ADDRESS is
> always true, so these changes only affect AARCH64.
> 
> > 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));
> >   ...
> 
> This assert ensures that the copied buffer does not extend across the
> end of the address space and wraps. This is a separate concern, and is
> similar to numerous other occurrences of MAX_ADDRESS that maybe we
> should update as well at some point. However, it does not affect page
> allocation at all, it only puts an upper bound on the *size* of the
> allocation. So the changes as they are will be sufficient to ensure
> that AllocateCopyPool() does not allocate from a region that is not
> addressable by the CPU.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On Mon, 10 Dec 2018 at 15:52, Gao, Liming <liming.gao@intel.com> wrote:
>
> Ard:
>   I prefer to define MAX_ALLOC_ADDRESS together with MAX_ADDRESS in ProcessorBind.h. I don't want to leave the choice to override MAX_ALLOC_ADDRESS definition.
>

Seems reasonable. What should be the value for X64?

> Thanks
> Liming
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> > Sent: Monday, December 10, 2018 3:23 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: Andrew Jones <drjones@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> > <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > Subject: Re: [edk2] [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account
> >
> > On Mon, 10 Dec 2018 at 03:04, Wang, Jian J <jian.j.wang@intel.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > I think MAX_ALLOC_ADDRESS will affect other archs besides ARM. Please do enough
> > > test for them (IA32/X64 for my concern).
> > >
> >
> > For all other architectures, MAX_ADDRESS == MAX_ALLOC_ADDRESS is
> > always true, so these changes only affect AARCH64.
> >
> > > 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));
> > >   ...
> >
> > This assert ensures that the copied buffer does not extend across the
> > end of the address space and wraps. This is a separate concern, and is
> > similar to numerous other occurrences of MAX_ADDRESS that maybe we
> > should update as well at some point. However, it does not affect page
> > allocation at all, it only puts an upper bound on the *size* of the
> > allocation. So the changes as they are will be sufficient to ensure
> > that AllocateCopyPool() does not allocate from a region that is not
> > addressable by the CPU.
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

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

[Gao, Liming]

Keep the same value of MAX_ADDRESS. There is no change now. 

Thanks
Liming
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, December 10, 2018 10:54 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Andrew Jones <drjones@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>;
> edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account
> 
> On Mon, 10 Dec 2018 at 15:52, Gao, Liming <liming.gao@intel.com> wrote:
> >
> > Ard:
> >   I prefer to define MAX_ALLOC_ADDRESS together with MAX_ADDRESS in ProcessorBind.h. I don't want to leave the choice to
> override MAX_ALLOC_ADDRESS definition.
> >
> 
> Seems reasonable. What should be the value for X64?
> 
> > Thanks
> > Liming
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> > > Sent: Monday, December 10, 2018 3:23 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: Andrew Jones <drjones@redhat.com>; Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org; Gao, Liming
> > > <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> > > Subject: Re: [edk2] [RFC PATCH 3/7] MdeModulePkg/Dxe/Page: take MAX_ALLOC_ADDRESS into account
> > >
> > > On Mon, 10 Dec 2018 at 03:04, Wang, Jian J <jian.j.wang@intel.com> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > I think MAX_ALLOC_ADDRESS will affect other archs besides ARM. Please do enough
> > > > test for them (IA32/X64 for my concern).
> > > >
> > >
> > > For all other architectures, MAX_ADDRESS == MAX_ALLOC_ADDRESS is
> > > always true, so these changes only affect AARCH64.
> > >
> > > > 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));
> > > >   ...
> > >
> > > This assert ensures that the copied buffer does not extend across the
> > > end of the address space and wraps. This is a separate concern, and is
> > > similar to numerous other occurrences of MAX_ADDRESS that maybe we
> > > should update as well at some point. However, it does not affect page
> > > allocation at all, it only puts an upper bound on the *size* of the
> > > allocation. So the changes as they are will be sufficient to ensure
> > > that AllocateCopyPool() does not allocate from a region that is not
> > > addressable by the CPU.
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel

[RFC PATCH 4/7] ArmPkg/ArmMmuLib: take MAX_ALLOC_ADDRESS into account

[Ard Biesheuvel]

When creating the page tables for the 1:1 mapping, ensure that we don't
attempt to map more than what is architecturally permitted when running
with 4 KB pages, which is 48 bits of VA. This will be reflected in the
value of MAX_ALLOC_ADDRESS once we override it for AArch64, so use that
macro instead of MAX_ADDRESS.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 5403b8d4070e..e41044142ef4 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -612,7 +612,7 @@ ArmConfigureMmu (
   // use of 4 KB pages.
   //
   MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
-                    MAX_ADDRESS);
+                    MAX_ALLOC_ADDRESS);
 
   // Lookup the Table Level to get the information
   LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
-- 
2.19.2

Re: [RFC PATCH 4/7] ArmPkg/ArmMmuLib: take MAX_ALLOC_ADDRESS into account

[Laszlo Ersek]

On 12/07/18 12:23, Ard Biesheuvel wrote:
> When creating the page tables for the 1:1 mapping, ensure that we don't
> attempt to map more than what is architecturally permitted when running
> with 4 KB pages, which is 48 bits of VA. This will be reflected in the
> value of MAX_ALLOC_ADDRESS once we override it for AArch64, so use that
> macro instead of MAX_ADDRESS.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 5403b8d4070e..e41044142ef4 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -612,7 +612,7 @@ ArmConfigureMmu (
>    // use of 4 KB pages.
>    //
>    MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
> -                    MAX_ADDRESS);
> +                    MAX_ALLOC_ADDRESS);
>  
>    // Lookup the Table Level to get the information
>    LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[RFC PATCH 5/7] ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account

[Ard Biesheuvel]

Limit the PEI memory region so it will not extend beyond what we can
address architecturally when running with 4 KB pages.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
index 389a2e6f1abd..25db87fb2374 100644
--- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
+++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
@@ -109,8 +109,8 @@ InitializeMemory (
 
   SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
   SystemMemoryTop = SystemMemoryBase + PcdGet64 (PcdSystemMemorySize);
-  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
-    SystemMemoryTop = (UINT64)MAX_ADDRESS + 1;
+  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
+    SystemMemoryTop = (UINT64)MAX_ALLOC_ADDRESS + 1;
   }
   FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
   FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
-- 
2.19.2

Re: [RFC PATCH 5/7] ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account

[Laszlo Ersek]

On 12/07/18 12:23, Ard Biesheuvel wrote:
> Limit the PEI memory region so it will not extend beyond what we can
> address architecturally when running with 4 KB pages.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> index 389a2e6f1abd..25db87fb2374 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> @@ -109,8 +109,8 @@ InitializeMemory (
>  
>    SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
>    SystemMemoryTop = SystemMemoryBase + PcdGet64 (PcdSystemMemorySize);
> -  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
> -    SystemMemoryTop = (UINT64)MAX_ADDRESS + 1;
> +  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
> +    SystemMemoryTop = (UINT64)MAX_ALLOC_ADDRESS + 1;
>    }
>    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
>    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> 

Shouldn't you also update

  ASSERT (PcdGet64 (PcdSystemMemoryBase) < (UINT64)MAX_ADDRESS);

just above the context visible here?

Thanks
Laszlo

Re: [RFC PATCH 5/7] ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account

[Ard Biesheuvel]

On Fri, 7 Dec 2018 at 13:46, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/07/18 12:23, Ard Biesheuvel wrote:
> > Limit the PEI memory region so it will not extend beyond what we can
> > address architecturally when running with 4 KB pages.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > index 389a2e6f1abd..25db87fb2374 100644
> > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > @@ -109,8 +109,8 @@ InitializeMemory (
> >
> >    SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
> >    SystemMemoryTop = SystemMemoryBase + PcdGet64 (PcdSystemMemorySize);
> > -  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
> > -    SystemMemoryTop = (UINT64)MAX_ADDRESS + 1;
> > +  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
> > +    SystemMemoryTop = (UINT64)MAX_ALLOC_ADDRESS + 1;
> >    }
> >    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
> >    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> >
>
> Shouldn't you also update
>
>   ASSERT (PcdGet64 (PcdSystemMemoryBase) < (UINT64)MAX_ADDRESS);
>
> just above the context visible here?
>

No, it's fine for PcdSystemMemoryBase/PcdSystemMemorySize to describe
a region we cannot access in its entirety, as long as we don't use the
top part (See the next patch as well)

Re: [RFC PATCH 5/7] ArmPlatformPkg/MemoryInitPeim: take MAX_ALLOC_ADDRESS into account

[Ard Biesheuvel]

On Fri, 7 Dec 2018 at 13:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 7 Dec 2018 at 13:46, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > On 12/07/18 12:23, Ard Biesheuvel wrote:
> > > Limit the PEI memory region so it will not extend beyond what we can
> > > address architecturally when running with 4 KB pages.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > index 389a2e6f1abd..25db87fb2374 100644
> > > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
> > > @@ -109,8 +109,8 @@ InitializeMemory (
> > >
> > >    SystemMemoryBase = (UINTN)PcdGet64 (PcdSystemMemoryBase);
> > >    SystemMemoryTop = SystemMemoryBase + PcdGet64 (PcdSystemMemorySize);
> > > -  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
> > > -    SystemMemoryTop = (UINT64)MAX_ADDRESS + 1;
> > > +  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
> > > +    SystemMemoryTop = (UINT64)MAX_ALLOC_ADDRESS + 1;
> > >    }
> > >    FdBase = (UINTN)PcdGet64 (PcdFdBaseAddress);
> > >    FdTop = FdBase + (UINTN)PcdGet32 (PcdFdSize);
> > >
> >
> > Shouldn't you also update
> >
> >   ASSERT (PcdGet64 (PcdSystemMemoryBase) < (UINT64)MAX_ADDRESS);
> >
> > just above the context visible here?
> >
>
> No, it's fine for PcdSystemMemoryBase/PcdSystemMemorySize to describe
> a region we cannot access in its entirety, as long as we don't use the
> top part (See the next patch as well)

Whoops, no you are right. That is Base not Base+Size, so indeed, Base
should be below MAX_ALLOC_ADDRESS

[RFC PATCH 6/7] ArmVirtPkg/MemoryInitPeiLib: split memory HOB based on MAX_ALLOC_ADDRESS

[Ard Biesheuvel]

The current ArmVirtMemoryInitPeiLib code splits the memory region passed
via PcdSystemMemoryBase/PcdSystemMemorySize in two if the region extends
beyond the MAX_ADDRESS limit. This was introduced for 32-bit ARM, which
may support more than 4 GB of physical address space, but cannot address
all of it via a 1:1 mapping, and a single region that is not mappable
in its entirety is unusable by the PEI core.

AArch64 is in a similar situation now: platforms may support more than
256 TB of physical address space, but only 256 TB is addressable by the
CPU, and so a memory region that extends from below this limit to above
it should be split.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
index 05afd1282422..66925fc05ebd 100644
--- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
+++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
@@ -75,18 +75,18 @@ MemoryPeim (
   SystemMemoryTop = PcdGet64 (PcdSystemMemoryBase) +
                     PcdGet64 (PcdSystemMemorySize);
 
-  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
+  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
     BuildResourceDescriptorHob (
         EFI_RESOURCE_SYSTEM_MEMORY,
         ResourceAttributes,
         PcdGet64 (PcdSystemMemoryBase),
-        (UINT64)MAX_ADDRESS - PcdGet64 (PcdSystemMemoryBase) + 1
+        (UINT64)MAX_ALLOC_ADDRESS - PcdGet64 (PcdSystemMemoryBase) + 1
         );
     BuildResourceDescriptorHob (
         EFI_RESOURCE_SYSTEM_MEMORY,
         ResourceAttributes,
-        (UINT64)MAX_ADDRESS + 1,
-        SystemMemoryTop - MAX_ADDRESS - 1
+        (UINT64)MAX_ALLOC_ADDRESS + 1,
+        SystemMemoryTop - MAX_ALLOC_ADDRESS - 1
         );
   } else {
     BuildResourceDescriptorHob (
-- 
2.19.2

Re: [RFC PATCH 6/7] ArmVirtPkg/MemoryInitPeiLib: split memory HOB based on MAX_ALLOC_ADDRESS

[Laszlo Ersek]

On 12/07/18 12:23, Ard Biesheuvel wrote:
> The current ArmVirtMemoryInitPeiLib code splits the memory region passed
> via PcdSystemMemoryBase/PcdSystemMemorySize in two if the region extends
> beyond the MAX_ADDRESS limit. This was introduced for 32-bit ARM, which
> may support more than 4 GB of physical address space, but cannot address
> all of it via a 1:1 mapping, and a single region that is not mappable
> in its entirety is unusable by the PEI core.
> 
> AArch64 is in a similar situation now: platforms may support more than
> 256 TB of physical address space, but only 256 TB is addressable by the
> CPU, and so a memory region that extends from below this limit to above
> it should be split.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> index 05afd1282422..66925fc05ebd 100644
> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
> @@ -75,18 +75,18 @@ MemoryPeim (
>    SystemMemoryTop = PcdGet64 (PcdSystemMemoryBase) +
>                      PcdGet64 (PcdSystemMemorySize);
>  
> -  if (SystemMemoryTop - 1 > MAX_ADDRESS) {
> +  if (SystemMemoryTop - 1 > MAX_ALLOC_ADDRESS) {
>      BuildResourceDescriptorHob (
>          EFI_RESOURCE_SYSTEM_MEMORY,
>          ResourceAttributes,
>          PcdGet64 (PcdSystemMemoryBase),
> -        (UINT64)MAX_ADDRESS - PcdGet64 (PcdSystemMemoryBase) + 1
> +        (UINT64)MAX_ALLOC_ADDRESS - PcdGet64 (PcdSystemMemoryBase) + 1
>          );
>      BuildResourceDescriptorHob (
>          EFI_RESOURCE_SYSTEM_MEMORY,
>          ResourceAttributes,
> -        (UINT64)MAX_ADDRESS + 1,
> -        SystemMemoryTop - MAX_ADDRESS - 1
> +        (UINT64)MAX_ALLOC_ADDRESS + 1,
> +        SystemMemoryTop - MAX_ALLOC_ADDRESS - 1
>          );
>    } else {
>      BuildResourceDescriptorHob (
> 

Looks correct to me.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[RFC PATCH 7/7] MdePkg/ProcessorBind AARCH64: limit MAX_ALLOC_ADDRESS to 48 bits

[Ard Biesheuvel]

Limit MAX_ALLOC_ADDRESS to 48 bits on AArch64 so that the memory
handling routines running at boot time take care not to allocate
memory that the CPU itself cannot access due to the fact that it
runs with 4 KB pages and thus an address space that is limited to
256 TB.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Include/AArch64/ProcessorBind.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
index 968c18f915ae..a8c698484a1d 100644
--- a/MdePkg/Include/AArch64/ProcessorBind.h
+++ b/MdePkg/Include/AArch64/ProcessorBind.h
@@ -142,6 +142,11 @@ typedef INT64   INTN;
 ///
 #define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
 
+///
+/// Maximum address usable at boot services time (48 bits for 4 KB pages)
+///
+#define MAX_ALLOC_ADDRESS   0xFFFFFFFFFFFFULL
+
 ///
 /// Maximum legal AArch64 INTN and UINTN values.
 ///
-- 
2.19.2

Re: [RFC PATCH 7/7] MdePkg/ProcessorBind AARCH64: limit MAX_ALLOC_ADDRESS to 48 bits

[Laszlo Ersek]

On 12/07/18 12:23, Ard Biesheuvel wrote:
> Limit MAX_ALLOC_ADDRESS to 48 bits on AArch64 so that the memory
> handling routines running at boot time take care not to allocate
> memory that the CPU itself cannot access due to the fact that it
> runs with 4 KB pages and thus an address space that is limited to
> 256 TB.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Include/AArch64/ProcessorBind.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/MdePkg/Include/AArch64/ProcessorBind.h b/MdePkg/Include/AArch64/ProcessorBind.h
> index 968c18f915ae..a8c698484a1d 100644
> --- a/MdePkg/Include/AArch64/ProcessorBind.h
> +++ b/MdePkg/Include/AArch64/ProcessorBind.h
> @@ -142,6 +142,11 @@ typedef INT64   INTN;
>  ///
>  #define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
>  
> +///
> +/// Maximum address usable at boot services time (48 bits for 4 KB pages)
> +///
> +#define MAX_ALLOC_ADDRESS   0xFFFFFFFFFFFFULL
> +
>  ///
>  /// Maximum legal AArch64 INTN and UINTN values.
>  ///
> 

I think this patch is safer than the previous variant. If this series
omits some replacements, from MAX_ADDRESS to MAX_ALLOC_ADDRESS, at
various spots in the edk2 tree, we should be able to fix those up
incrementally. Whereas changing MAX_ADDRESS was a heavy hammer that hit
everything simultaneously.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo