[edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing

Patrick Rudolph posted 1 patch 2 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20210615132254.3364910-1-patrick.rudolph@9elements.com
There is a newer version of this series
.../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
.../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
2 files changed, 194 insertions(+), 3 deletions(-)
[edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Patrick Rudolph 2 years, 9 months ago
Currently several DXE crash due to invalid memory resource settings.
coreboot and slimbootloader provide an e820 compatible memory map,
which doesn't work well with EDK2 as the e820 spec is missing MMIO regions.
In e820 'reserved' could either mean "DRAM used by boot firmware" or "MMIO
in use and not detectable by OS".

Guess Top of lower usable DRAM (TOLUD) by walking memory ranges and then
mark everything reserved below TOLUD as DRAM and everything reserved above
TOLUD as MMIO.

This fixes several assertions seen in PciHostBridgeDxe.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
 .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
 2 files changed, 194 insertions(+), 3 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 805f5448d9..d20e1a0862 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -7,10 +7,162 @@
 
 #include "UefiPayloadEntry.h"
 
+STATIC UINT32 TopOfLowerUsableDram = 0;
+
 /**
    Callback function to build resource descriptor HOB
 
    This function build a HOB based on the memory map entry info.
+   It creates only EFI_RESOURCE_MEMORY_MAPPED_IO and EFI_RESOURCE_MEMORY_RESERVED
+   resources.
+
+   @param MemoryMapEntry         Memory map entry info got from bootloader.
+   @param Params                 A pointer to ACPI_BOARD_INFO.
+
+  @retval RETURN_SUCCESS         Successfully build a HOB.
+  @retval EFI_INVALID_PARAMETER  Invalid parameter provided.
+**/
+EFI_STATUS
+MemInfoCallbackMMIO (
+  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
+  IN VOID                      *Params
+  )
+{
+  EFI_PHYSICAL_ADDRESS         Base;
+  EFI_RESOURCE_TYPE            Type;
+  UINT64                       Size;
+  EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
+  ACPI_BOARD_INFO              *AcpiBoardInfo;
+
+  AcpiBoardInfo = (ACPI_BOARD_INFO *)Params;
+  if (AcpiBoardInfo == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Skip types already handled in MemInfoCallback
+  //
+  if (MemoryMapEntry->Type == E820_RAM || MemoryMapEntry->Type == E820_ACPI) {
+    return RETURN_SUCCESS;
+  }
+
+  if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {
+    //
+    // MMCONF is always MMIO
+    //
+    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
+  } else if (MemoryMapEntry->Base < TopOfLowerUsableDram) {
+    //
+    // It's in DRAM and thus must be reserved
+    //
+    Type = EFI_RESOURCE_MEMORY_RESERVED;
+  } else if (MemoryMapEntry->Base < 0x100000000ULL &&
+    MemoryMapEntry->Base >= TopOfLowerUsableDram) {
+    //
+    // It's not in DRAM, must be MMIO
+    //
+    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
+  } else {
+    Type = EFI_RESOURCE_MEMORY_RESERVED;
+  }
+
+  Base    = MemoryMapEntry->Base;
+  Size    = MemoryMapEntry->Size;
+
+  Attribue = EFI_RESOURCE_ATTRIBUTE_PRESENT |
+             EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+             EFI_RESOURCE_ATTRIBUTE_TESTED |
+             EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
+             EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+             EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+             EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
+
+  BuildResourceDescriptorHob (Type, Attribue, (EFI_PHYSICAL_ADDRESS)Base, Size);
+  DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type));
+
+  if (MemoryMapEntry->Type == E820_NVS) {
+    BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);
+  } else if (MemoryMapEntry->Type == E820_UNUSABLE ||
+    MemoryMapEntry->Type == E820_DISABLED) {
+    BuildMemoryAllocationHob (Base, Size, EfiUnusableMemory);
+  } else if (MemoryMapEntry->Type == E820_PMEM) {
+    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);
+  }
+
+  return RETURN_SUCCESS;
+}
+
+
+/**
+   Callback function to find TOLUD (Top of Lower Usable DRAM)
+
+   Estimate where TOLUD (Top of Lower Usable DRAM) resides. The exact position
+   would require platform specific code.
+
+   @param MemoryMapEntry         Memory map entry info got from bootloader.
+   @param Params                 Not used for now.
+
+  @retval RETURN_SUCCESS         Successfully updated TopOfLowerUsableDram.
+**/
+EFI_STATUS
+FindToludCallback (
+  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
+  IN VOID                      *Params
+  )
+{
+  //
+  // This code assumes that the memory map on this x86 machine below 4GiB is continous
+  // until TOLUD. In addition it assumes that the bootloader provided memory tables have
+  // no "holes" and thus the first memory range not covered by e820 marks the end of
+  // usable DRAM. In addition it's assumed that every reserved memory region touching
+  // usable RAM is also covering DRAM, everything else that is marked reserved thus must be
+  // MMIO not detectable by bootloader/OS
+  //
+
+  //
+  // Skip memory types not RAM or reserved
+  //
+  if (MemoryMapEntry->Type == E820_NVS || MemoryMapEntry->Type == E820_UNUSABLE ||
+    MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry->Type == E820_PMEM) {
+    return RETURN_SUCCESS;
+  }
+
+  //
+  // Skip resources above 4GiB
+  //
+  if (MemoryMapEntry->Base >= 0x100000000ULL) {
+    return RETURN_SUCCESS;
+  }
+
+  if ((MemoryMapEntry->Type == E820_RAM) ||
+    (MemoryMapEntry->Type == E820_ACPI)) {
+    //
+    // It's usable DRAM. Update TOLUD.
+    //
+    if (TopOfLowerUsableDram < (MemoryMapEntry->Base + MemoryMapEntry->Size)) {
+      TopOfLowerUsableDram = MemoryMapEntry->Base + MemoryMapEntry->Size;
+    }
+  } else {
+    //
+    // It might be reserved DRAM or MMIO.
+    //
+    // If it touches usable DRAM at Base assume it's DRAM as well,
+    // as it could be bootloader installed tables, TSEG, GTT, ...
+    //
+    if (TopOfLowerUsableDram == MemoryMapEntry->Base) {
+      TopOfLowerUsableDram = MemoryMapEntry->Base + MemoryMapEntry->Size;
+    }
+  }
+
+  return RETURN_SUCCESS;
+}
+
+
+/**
+   Callback function to build resource descriptor HOB
+
+   This function build a HOB based on the memory map entry info.
+   Only add EFI_RESOURCE_SYSTEM_MEMORY.
 
    @param MemoryMapEntry         Memory map entry info got from bootloader.
    @param Params                 Not used for now.
@@ -28,7 +180,15 @@ MemInfoCallback (
   UINT64                       Size;
   EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
 
-  Type    = (MemoryMapEntry->Type == 1) ? EFI_RESOURCE_SYSTEM_MEMORY : EFI_RESOURCE_MEMORY_RESERVED;
+  //
+  // Skip everything not known to be usable DRAM.
+  // It will be added later.
+  //
+  if (MemoryMapEntry->Type != E820_RAM && MemoryMapEntry->Type != E820_ACPI) {
+    return RETURN_SUCCESS;
+  }
+
+  Type    = EFI_RESOURCE_SYSTEM_MEMORY;
   Base    = MemoryMapEntry->Base;
   Size    = MemoryMapEntry->Size;
 
@@ -40,7 +200,7 @@ MemInfoCallback (
              EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
              EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
 
-  if (Base >= BASE_4GB ) {
+  if (Base >= BASE_4GB) {
     // Remove tested attribute to avoid DXE core to dispatch driver to memory above 4GB
     Attribue &= ~EFI_RESOURCE_ATTRIBUTE_TESTED;
   }
@@ -48,6 +208,10 @@ MemInfoCallback (
   BuildResourceDescriptorHob (Type, Attribue, (EFI_PHYSICAL_ADDRESS)Base, Size);
   DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type));
 
+  if (MemoryMapEntry->Type == E820_ACPI) {
+    BuildMemoryAllocationHob (Base, Size, EfiACPIReclaimMemory);
+  }
+
   return RETURN_SUCCESS;
 }
 
@@ -236,7 +400,16 @@ BuildHobFromBl (
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
 
   //
-  // Parse memory info and build memory HOBs
+  // First find TOLUD
+  //
+  Status = ParseMemoryInfo (FindToludCallback, NULL);
+  if (EFI_ERROR(Status)) {
+    return Status;
+  }
+  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n", TopOfLowerUsableDram));
+
+  //
+  // Parse memory info and build memory HOBs for Usable RAM
   //
   Status = ParseMemoryInfo (MemInfoCallback, NULL);
   if (EFI_ERROR(Status)) {
@@ -289,6 +462,14 @@ BuildHobFromBl (
     DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));
   }
 
+  //
+  // Parse memory info and build memory HOBs for reserved DRAM and MMIO
+  //
+  Status = ParseMemoryInfo (MemInfoCallbackMMIO, &AcpiBoardInfo);
+  if (EFI_ERROR(Status)) {
+    return Status;
+  }
+
   //
   // Parse platform specific information.
   //
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
index 2c84d6ed53..35ea23d202 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
@@ -38,6 +38,16 @@
 #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \
   ((ActualSize) + (((Alignment) - ((ActualSize) & ((Alignment) - 1))) & ((Alignment) - 1)))
 
+
+#define E820_RAM        1
+#define E820_RESERVED   2
+#define E820_ACPI       3
+#define E820_NVS        4
+#define E820_UNUSABLE   5
+#define E820_DISABLED   6
+#define E820_PMEM       7
+#define E820_UNDEFINED  8
+
 /**
   Auto-generated function that calls the library constructors for all of the module's
   dependent libraries.
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76517): https://edk2.groups.io/g/devel/message/76517
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Guo Dong 2 years, 9 months ago
Could you provide more info on the issues in fixed by this patch?
There is a UEFI payload patch (under code review) to support the bootloader to provide a HOB
on the PCI resources, this way the UEFI payload doesn't need collect resources again by parsing
all the PCI devices. Not sure if that patch (UefiPayloadPkg: UefiPayload retrieve PCI root bridge
from Guid Hob) could avoid the issues.

In this patch, as it mentioned some information (system memory or MMIO) is missing in the
bootloader E820 style table. Guessing the TOLUD might work in most cases, but may fail since
the assumptions.

I saw this patch fixed the memory type for ACPI memory. I think we could have this fix if the
TOLUD guessing could be avoided.

Thanks,
Guo

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Patrick
> Rudolph
> Sent: Tuesday, June 15, 2021 6:23 AM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve
> bootloader memrange parsing
> 
> Currently several DXE crash due to invalid memory resource settings.
> coreboot and slimbootloader provide an e820 compatible memory map,
> which doesn't work well with EDK2 as the e820 spec is missing MMIO regions.
> In e820 'reserved' could either mean "DRAM used by boot firmware" or
> "MMIO
> in use and not detectable by OS".
> 
> Guess Top of lower usable DRAM (TOLUD) by walking memory ranges and
> then
> mark everything reserved below TOLUD as DRAM and everything reserved
> above
> TOLUD as MMIO.
> 
> This fixes several assertions seen in PciHostBridgeDxe.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
>  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
>  2 files changed, 194 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> index 805f5448d9..d20e1a0862 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> @@ -7,10 +7,162 @@
> 
> 
>  #include "UefiPayloadEntry.h"
> 
> 
> 
> +STATIC UINT32 TopOfLowerUsableDram = 0;
> 
> +
> 
>  /**
> 
>     Callback function to build resource descriptor HOB
> 
> 
> 
>     This function build a HOB based on the memory map entry info.
> 
> +   It creates only EFI_RESOURCE_MEMORY_MAPPED_IO and
> EFI_RESOURCE_MEMORY_RESERVED
> 
> +   resources.
> 
> +
> 
> +   @param MemoryMapEntry         Memory map entry info got from
> bootloader.
> 
> +   @param Params                 A pointer to ACPI_BOARD_INFO.
> 
> +
> 
> +  @retval RETURN_SUCCESS         Successfully build a HOB.
> 
> +  @retval EFI_INVALID_PARAMETER  Invalid parameter provided.
> 
> +**/
> 
> +EFI_STATUS
> 
> +MemInfoCallbackMMIO (
> 
> +  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
> 
> +  IN VOID                      *Params
> 
> +  )
> 
> +{
> 
> +  EFI_PHYSICAL_ADDRESS         Base;
> 
> +  EFI_RESOURCE_TYPE            Type;
> 
> +  UINT64                       Size;
> 
> +  EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
> 
> +  ACPI_BOARD_INFO              *AcpiBoardInfo;
> 
> +
> 
> +  AcpiBoardInfo = (ACPI_BOARD_INFO *)Params;
> 
> +  if (AcpiBoardInfo == NULL) {
> 
> +    return EFI_INVALID_PARAMETER;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Skip types already handled in MemInfoCallback
> 
> +  //
> 
> +  if (MemoryMapEntry->Type == E820_RAM || MemoryMapEntry->Type ==
> E820_ACPI) {
> 
> +    return RETURN_SUCCESS;
> 
> +  }
> 
> +
> 
> +  if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {
> 
> +    //
> 
> +    // MMCONF is always MMIO
> 
> +    //
> 
> +    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
> 
> +  } else if (MemoryMapEntry->Base < TopOfLowerUsableDram) {
> 
> +    //
> 
> +    // It's in DRAM and thus must be reserved
> 
> +    //
> 
> +    Type = EFI_RESOURCE_MEMORY_RESERVED;
> 
> +  } else if (MemoryMapEntry->Base < 0x100000000ULL &&
> 
> +    MemoryMapEntry->Base >= TopOfLowerUsableDram) {
> 
> +    //
> 
> +    // It's not in DRAM, must be MMIO
> 
> +    //
> 
> +    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
> 
> +  } else {
> 
> +    Type = EFI_RESOURCE_MEMORY_RESERVED;
> 
> +  }
> 
> +
> 
> +  Base    = MemoryMapEntry->Base;
> 
> +  Size    = MemoryMapEntry->Size;
> 
> +
> 
> +  Attribue = EFI_RESOURCE_ATTRIBUTE_PRESENT |
> 
> +             EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> 
> +             EFI_RESOURCE_ATTRIBUTE_TESTED |
> 
> +             EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> 
> +             EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> 
> +             EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> 
> +             EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
> 
> +
> 
> +  BuildResourceDescriptorHob (Type, Attribue,
> (EFI_PHYSICAL_ADDRESS)Base, Size);
> 
> +  DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> 0x%x\n", Base, Size, Type));
> 
> +
> 
> +  if (MemoryMapEntry->Type == E820_NVS) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);
> 
> +  } else if (MemoryMapEntry->Type == E820_UNUSABLE ||
> 
> +    MemoryMapEntry->Type == E820_DISABLED) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiUnusableMemory);
> 
> +  } else if (MemoryMapEntry->Type == E820_PMEM) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);
> 
> +  }
> 
> +
> 
> +  return RETURN_SUCCESS;
> 
> +}
> 
> +
> 
> +
> 
> +/**
> 
> +   Callback function to find TOLUD (Top of Lower Usable DRAM)
> 
> +
> 
> +   Estimate where TOLUD (Top of Lower Usable DRAM) resides. The exact
> position
> 
> +   would require platform specific code.
> 
> +
> 
> +   @param MemoryMapEntry         Memory map entry info got from
> bootloader.
> 
> +   @param Params                 Not used for now.
> 
> +
> 
> +  @retval RETURN_SUCCESS         Successfully updated
> TopOfLowerUsableDram.
> 
> +**/
> 
> +EFI_STATUS
> 
> +FindToludCallback (
> 
> +  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
> 
> +  IN VOID                      *Params
> 
> +  )
> 
> +{
> 
> +  //
> 
> +  // This code assumes that the memory map on this x86 machine below
> 4GiB is continous
> 
> +  // until TOLUD. In addition it assumes that the bootloader provided
> memory tables have
> 
> +  // no "holes" and thus the first memory range not covered by e820 marks
> the end of
> 
> +  // usable DRAM. In addition it's assumed that every reserved memory
> region touching
> 
> +  // usable RAM is also covering DRAM, everything else that is marked
> reserved thus must be
> 
> +  // MMIO not detectable by bootloader/OS
> 
> +  //
> 
> +
> 
> +  //
> 
> +  // Skip memory types not RAM or reserved
> 
> +  //
> 
> +  if (MemoryMapEntry->Type == E820_NVS || MemoryMapEntry->Type ==
> E820_UNUSABLE ||
> 
> +    MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry->Type
> == E820_PMEM) {
> 
> +    return RETURN_SUCCESS;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Skip resources above 4GiB
> 
> +  //
> 
> +  if (MemoryMapEntry->Base >= 0x100000000ULL) {
> 
> +    return RETURN_SUCCESS;
> 
> +  }
> 
> +
> 
> +  if ((MemoryMapEntry->Type == E820_RAM) ||
> 
> +    (MemoryMapEntry->Type == E820_ACPI)) {
> 
> +    //
> 
> +    // It's usable DRAM. Update TOLUD.
> 
> +    //
> 
> +    if (TopOfLowerUsableDram < (MemoryMapEntry->Base +
> MemoryMapEntry->Size)) {
> 
> +      TopOfLowerUsableDram = MemoryMapEntry->Base +
> MemoryMapEntry->Size;
> 
> +    }
> 
> +  } else {
> 
> +    //
> 
> +    // It might be reserved DRAM or MMIO.
> 
> +    //
> 
> +    // If it touches usable DRAM at Base assume it's DRAM as well,
> 
> +    // as it could be bootloader installed tables, TSEG, GTT, ...
> 
> +    //
> 
> +    if (TopOfLowerUsableDram == MemoryMapEntry->Base) {
> 
> +      TopOfLowerUsableDram = MemoryMapEntry->Base +
> MemoryMapEntry->Size;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return RETURN_SUCCESS;
> 
> +}
> 
> +
> 
> +
> 
> +/**
> 
> +   Callback function to build resource descriptor HOB
> 
> +
> 
> +   This function build a HOB based on the memory map entry info.
> 
> +   Only add EFI_RESOURCE_SYSTEM_MEMORY.
> 
> 
> 
>     @param MemoryMapEntry         Memory map entry info got from
> bootloader.
> 
>     @param Params                 Not used for now.
> 
> @@ -28,7 +180,15 @@ MemInfoCallback (
>    UINT64                       Size;
> 
>    EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
> 
> 
> 
> -  Type    = (MemoryMapEntry->Type == 1) ?
> EFI_RESOURCE_SYSTEM_MEMORY : EFI_RESOURCE_MEMORY_RESERVED;
> 
> +  //
> 
> +  // Skip everything not known to be usable DRAM.
> 
> +  // It will be added later.
> 
> +  //
> 
> +  if (MemoryMapEntry->Type != E820_RAM && MemoryMapEntry->Type !=
> E820_ACPI) {
> 
> +    return RETURN_SUCCESS;
> 
> +  }
> 
> +
> 
> +  Type    = EFI_RESOURCE_SYSTEM_MEMORY;
> 
>    Base    = MemoryMapEntry->Base;
> 
>    Size    = MemoryMapEntry->Size;
> 
> 
> 
> @@ -40,7 +200,7 @@ MemInfoCallback (
>               EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> 
>               EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
> 
> 
> 
> -  if (Base >= BASE_4GB ) {
> 
> +  if (Base >= BASE_4GB) {
> 
>      // Remove tested attribute to avoid DXE core to dispatch driver to memory
> above 4GB
> 
>      Attribue &= ~EFI_RESOURCE_ATTRIBUTE_TESTED;
> 
>    }
> 
> @@ -48,6 +208,10 @@ MemInfoCallback (
>    BuildResourceDescriptorHob (Type, Attribue,
> (EFI_PHYSICAL_ADDRESS)Base, Size);
> 
>    DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> 0x%x\n", Base, Size, Type));
> 
> 
> 
> +  if (MemoryMapEntry->Type == E820_ACPI) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiACPIReclaimMemory);
> 
> +  }
> 
> +
> 
>    return RETURN_SUCCESS;
> 
>  }
> 
> 
> 
> @@ -236,7 +400,16 @@ BuildHobFromBl (
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
> 
> 
> 
>    //
> 
> -  // Parse memory info and build memory HOBs
> 
> +  // First find TOLUD
> 
> +  //
> 
> +  Status = ParseMemoryInfo (FindToludCallback, NULL);
> 
> +  if (EFI_ERROR(Status)) {
> 
> +    return Status;
> 
> +  }
> 
> +  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n",
> TopOfLowerUsableDram));
> 
> +
> 
> +  //
> 
> +  // Parse memory info and build memory HOBs for Usable RAM
> 
>    //
> 
>    Status = ParseMemoryInfo (MemInfoCallback, NULL);
> 
>    if (EFI_ERROR(Status)) {
> 
> @@ -289,6 +462,14 @@ BuildHobFromBl (
>      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));
> 
>    }
> 
> 
> 
> +  //
> 
> +  // Parse memory info and build memory HOBs for reserved DRAM and
> MMIO
> 
> +  //
> 
> +  Status = ParseMemoryInfo (MemInfoCallbackMMIO, &AcpiBoardInfo);
> 
> +  if (EFI_ERROR(Status)) {
> 
> +    return Status;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Parse platform specific information.
> 
>    //
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> index 2c84d6ed53..35ea23d202 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> @@ -38,6 +38,16 @@
>  #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \
> 
>    ((ActualSize) + (((Alignment) - ((ActualSize) & ((Alignment) - 1))) &
> ((Alignment) - 1)))
> 
> 
> 
> +
> 
> +#define E820_RAM        1
> 
> +#define E820_RESERVED   2
> 
> +#define E820_ACPI       3
> 
> +#define E820_NVS        4
> 
> +#define E820_UNUSABLE   5
> 
> +#define E820_DISABLED   6
> 
> +#define E820_PMEM       7
> 
> +#define E820_UNDEFINED  8
> 
> +
> 
>  /**
> 
>    Auto-generated function that calls the library constructors for all of the
> module's
> 
>    dependent libraries.
> 
> --
> 2.30.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#76517): https://edk2.groups.io/g/devel/message/76517
> Mute This Topic: https://groups.io/mt/83555387/1781375
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.dong@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76563): https://edk2.groups.io/g/devel/message/76563
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Patrick Rudolph 2 years, 9 months ago
Hi Guo,
The PciHostBridgeDxe expects the PCI Aperature and MMCONF to be marked
as EfiMemoryMappedIO,
however as the bootloader provides an e820 compatible memory map, it's
actually marked as
EfiReservedMemoryType. The PciHostBridgeDxe asserts on this incorrect
memory type.
This is what the patch tries to fix by marking MMIO (between TOLUD and
4GiB) as EfiMemoryMappedIO.

I don't think that the mentioned patch (UefiPayloadPkg: UefiPayload
retrieve PCI root bridge
from Guid Hob) would change that, as the same memory ranges are still
being incorrectly marked
reserved by UefiPayloadEntry.

Even with TOLUD guessing not working the OS wouldn't see a difference
as EfiMemoryMappedIO and
EfiReservedMemoryType both appear as reserved to the OS, so it's still
the same memory from OS point of view.
In case TOLUD is estimated too high, MMIO would be marked as Reserved,
so it's the same situation as now and
DXE drivers might fail.
In case TOLUD is estimated too low, Reserved DRAM would be marked as
MMIO, however I can't think of a DXE
that would actually care, it would skip the region as it would be
marked reserved.

Thanks,
Patrick

On Wed, Jun 16, 2021 at 12:20 AM Dong, Guo <guo.dong@intel.com> wrote:
>
>
> Could you provide more info on the issues in fixed by this patch?
> There is a UEFI payload patch (under code review) to support the bootloader to provide a HOB
> on the PCI resources, this way the UEFI payload doesn't need collect resources again by parsing
> all the PCI devices. Not sure if that patch (UefiPayloadPkg: UefiPayload retrieve PCI root bridge
> from Guid Hob) could avoid the issues.
>
> In this patch, as it mentioned some information (system memory or MMIO) is missing in the
> bootloader E820 style table. Guessing the TOLUD might work in most cases, but may fail since
> the assumptions.
>
> I saw this patch fixed the memory type for ACPI memory. I think we could have this fix if the
> TOLUD guessing could be avoided.
>
> Thanks,
> Guo
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Patrick
> > Rudolph
> > Sent: Tuesday, June 15, 2021 6:23 AM
> > To: devel@edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > Subject: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve
> > bootloader memrange parsing
> >
> > Currently several DXE crash due to invalid memory resource settings.
> > coreboot and slimbootloader provide an e820 compatible memory map,
> > which doesn't work well with EDK2 as the e820 spec is missing MMIO regions.
> > In e820 'reserved' could either mean "DRAM used by boot firmware" or
> > "MMIO
> > in use and not detectable by OS".
> >
> > Guess Top of lower usable DRAM (TOLUD) by walking memory ranges and
> > then
> > mark everything reserved below TOLUD as DRAM and everything reserved
> > above
> > TOLUD as MMIO.
> >
> > This fixes several assertions seen in PciHostBridgeDxe.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > ---
> >  .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
> >  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
> >  2 files changed, 194 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > index 805f5448d9..d20e1a0862 100644
> > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > @@ -7,10 +7,162 @@
> >
> >
> >  #include "UefiPayloadEntry.h"
> >
> >
> >
> > +STATIC UINT32 TopOfLowerUsableDram = 0;
> >
> > +
> >
> >  /**
> >
> >     Callback function to build resource descriptor HOB
> >
> >
> >
> >     This function build a HOB based on the memory map entry info.
> >
> > +   It creates only EFI_RESOURCE_MEMORY_MAPPED_IO and
> > EFI_RESOURCE_MEMORY_RESERVED
> >
> > +   resources.
> >
> > +
> >
> > +   @param MemoryMapEntry         Memory map entry info got from
> > bootloader.
> >
> > +   @param Params                 A pointer to ACPI_BOARD_INFO.
> >
> > +
> >
> > +  @retval RETURN_SUCCESS         Successfully build a HOB.
> >
> > +  @retval EFI_INVALID_PARAMETER  Invalid parameter provided.
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +MemInfoCallbackMMIO (
> >
> > +  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
> >
> > +  IN VOID                      *Params
> >
> > +  )
> >
> > +{
> >
> > +  EFI_PHYSICAL_ADDRESS         Base;
> >
> > +  EFI_RESOURCE_TYPE            Type;
> >
> > +  UINT64                       Size;
> >
> > +  EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
> >
> > +  ACPI_BOARD_INFO              *AcpiBoardInfo;
> >
> > +
> >
> > +  AcpiBoardInfo = (ACPI_BOARD_INFO *)Params;
> >
> > +  if (AcpiBoardInfo == NULL) {
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Skip types already handled in MemInfoCallback
> >
> > +  //
> >
> > +  if (MemoryMapEntry->Type == E820_RAM || MemoryMapEntry->Type ==
> > E820_ACPI) {
> >
> > +    return RETURN_SUCCESS;
> >
> > +  }
> >
> > +
> >
> > +  if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {
> >
> > +    //
> >
> > +    // MMCONF is always MMIO
> >
> > +    //
> >
> > +    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
> >
> > +  } else if (MemoryMapEntry->Base < TopOfLowerUsableDram) {
> >
> > +    //
> >
> > +    // It's in DRAM and thus must be reserved
> >
> > +    //
> >
> > +    Type = EFI_RESOURCE_MEMORY_RESERVED;
> >
> > +  } else if (MemoryMapEntry->Base < 0x100000000ULL &&
> >
> > +    MemoryMapEntry->Base >= TopOfLowerUsableDram) {
> >
> > +    //
> >
> > +    // It's not in DRAM, must be MMIO
> >
> > +    //
> >
> > +    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
> >
> > +  } else {
> >
> > +    Type = EFI_RESOURCE_MEMORY_RESERVED;
> >
> > +  }
> >
> > +
> >
> > +  Base    = MemoryMapEntry->Base;
> >
> > +  Size    = MemoryMapEntry->Size;
> >
> > +
> >
> > +  Attribue = EFI_RESOURCE_ATTRIBUTE_PRESENT |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_TESTED |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> >
> > +             EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
> >
> > +
> >
> > +  BuildResourceDescriptorHob (Type, Attribue,
> > (EFI_PHYSICAL_ADDRESS)Base, Size);
> >
> > +  DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> > 0x%x\n", Base, Size, Type));
> >
> > +
> >
> > +  if (MemoryMapEntry->Type == E820_NVS) {
> >
> > +    BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);
> >
> > +  } else if (MemoryMapEntry->Type == E820_UNUSABLE ||
> >
> > +    MemoryMapEntry->Type == E820_DISABLED) {
> >
> > +    BuildMemoryAllocationHob (Base, Size, EfiUnusableMemory);
> >
> > +  } else if (MemoryMapEntry->Type == E820_PMEM) {
> >
> > +    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);
> >
> > +  }
> >
> > +
> >
> > +  return RETURN_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +
> >
> > +/**
> >
> > +   Callback function to find TOLUD (Top of Lower Usable DRAM)
> >
> > +
> >
> > +   Estimate where TOLUD (Top of Lower Usable DRAM) resides. The exact
> > position
> >
> > +   would require platform specific code.
> >
> > +
> >
> > +   @param MemoryMapEntry         Memory map entry info got from
> > bootloader.
> >
> > +   @param Params                 Not used for now.
> >
> > +
> >
> > +  @retval RETURN_SUCCESS         Successfully updated
> > TopOfLowerUsableDram.
> >
> > +**/
> >
> > +EFI_STATUS
> >
> > +FindToludCallback (
> >
> > +  IN MEMROY_MAP_ENTRY          *MemoryMapEntry,
> >
> > +  IN VOID                      *Params
> >
> > +  )
> >
> > +{
> >
> > +  //
> >
> > +  // This code assumes that the memory map on this x86 machine below
> > 4GiB is continous
> >
> > +  // until TOLUD. In addition it assumes that the bootloader provided
> > memory tables have
> >
> > +  // no "holes" and thus the first memory range not covered by e820 marks
> > the end of
> >
> > +  // usable DRAM. In addition it's assumed that every reserved memory
> > region touching
> >
> > +  // usable RAM is also covering DRAM, everything else that is marked
> > reserved thus must be
> >
> > +  // MMIO not detectable by bootloader/OS
> >
> > +  //
> >
> > +
> >
> > +  //
> >
> > +  // Skip memory types not RAM or reserved
> >
> > +  //
> >
> > +  if (MemoryMapEntry->Type == E820_NVS || MemoryMapEntry->Type ==
> > E820_UNUSABLE ||
> >
> > +    MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry->Type
> > == E820_PMEM) {
> >
> > +    return RETURN_SUCCESS;
> >
> > +  }
> >
> > +
> >
> > +  //
> >
> > +  // Skip resources above 4GiB
> >
> > +  //
> >
> > +  if (MemoryMapEntry->Base >= 0x100000000ULL) {
> >
> > +    return RETURN_SUCCESS;
> >
> > +  }
> >
> > +
> >
> > +  if ((MemoryMapEntry->Type == E820_RAM) ||
> >
> > +    (MemoryMapEntry->Type == E820_ACPI)) {
> >
> > +    //
> >
> > +    // It's usable DRAM. Update TOLUD.
> >
> > +    //
> >
> > +    if (TopOfLowerUsableDram < (MemoryMapEntry->Base +
> > MemoryMapEntry->Size)) {
> >
> > +      TopOfLowerUsableDram = MemoryMapEntry->Base +
> > MemoryMapEntry->Size;
> >
> > +    }
> >
> > +  } else {
> >
> > +    //
> >
> > +    // It might be reserved DRAM or MMIO.
> >
> > +    //
> >
> > +    // If it touches usable DRAM at Base assume it's DRAM as well,
> >
> > +    // as it could be bootloader installed tables, TSEG, GTT, ...
> >
> > +    //
> >
> > +    if (TopOfLowerUsableDram == MemoryMapEntry->Base) {
> >
> > +      TopOfLowerUsableDram = MemoryMapEntry->Base +
> > MemoryMapEntry->Size;
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +  return RETURN_SUCCESS;
> >
> > +}
> >
> > +
> >
> > +
> >
> > +/**
> >
> > +   Callback function to build resource descriptor HOB
> >
> > +
> >
> > +   This function build a HOB based on the memory map entry info.
> >
> > +   Only add EFI_RESOURCE_SYSTEM_MEMORY.
> >
> >
> >
> >     @param MemoryMapEntry         Memory map entry info got from
> > bootloader.
> >
> >     @param Params                 Not used for now.
> >
> > @@ -28,7 +180,15 @@ MemInfoCallback (
> >    UINT64                       Size;
> >
> >    EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue;
> >
> >
> >
> > -  Type    = (MemoryMapEntry->Type == 1) ?
> > EFI_RESOURCE_SYSTEM_MEMORY : EFI_RESOURCE_MEMORY_RESERVED;
> >
> > +  //
> >
> > +  // Skip everything not known to be usable DRAM.
> >
> > +  // It will be added later.
> >
> > +  //
> >
> > +  if (MemoryMapEntry->Type != E820_RAM && MemoryMapEntry->Type !=
> > E820_ACPI) {
> >
> > +    return RETURN_SUCCESS;
> >
> > +  }
> >
> > +
> >
> > +  Type    = EFI_RESOURCE_SYSTEM_MEMORY;
> >
> >    Base    = MemoryMapEntry->Base;
> >
> >    Size    = MemoryMapEntry->Size;
> >
> >
> >
> > @@ -40,7 +200,7 @@ MemInfoCallback (
> >               EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> >
> >               EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;
> >
> >
> >
> > -  if (Base >= BASE_4GB ) {
> >
> > +  if (Base >= BASE_4GB) {
> >
> >      // Remove tested attribute to avoid DXE core to dispatch driver to memory
> > above 4GB
> >
> >      Attribue &= ~EFI_RESOURCE_ATTRIBUTE_TESTED;
> >
> >    }
> >
> > @@ -48,6 +208,10 @@ MemInfoCallback (
> >    BuildResourceDescriptorHob (Type, Attribue,
> > (EFI_PHYSICAL_ADDRESS)Base, Size);
> >
> >    DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> > 0x%x\n", Base, Size, Type));
> >
> >
> >
> > +  if (MemoryMapEntry->Type == E820_ACPI) {
> >
> > +    BuildMemoryAllocationHob (Base, Size, EfiACPIReclaimMemory);
> >
> > +  }
> >
> > +
> >
> >    return RETURN_SUCCESS;
> >
> >  }
> >
> >
> >
> > @@ -236,7 +400,16 @@ BuildHobFromBl (
> >    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
> >
> >
> >
> >    //
> >
> > -  // Parse memory info and build memory HOBs
> >
> > +  // First find TOLUD
> >
> > +  //
> >
> > +  Status = ParseMemoryInfo (FindToludCallback, NULL);
> >
> > +  if (EFI_ERROR(Status)) {
> >
> > +    return Status;
> >
> > +  }
> >
> > +  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n",
> > TopOfLowerUsableDram));
> >
> > +
> >
> > +  //
> >
> > +  // Parse memory info and build memory HOBs for Usable RAM
> >
> >    //
> >
> >    Status = ParseMemoryInfo (MemInfoCallback, NULL);
> >
> >    if (EFI_ERROR(Status)) {
> >
> > @@ -289,6 +462,14 @@ BuildHobFromBl (
> >      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));
> >
> >    }
> >
> >
> >
> > +  //
> >
> > +  // Parse memory info and build memory HOBs for reserved DRAM and
> > MMIO
> >
> > +  //
> >
> > +  Status = ParseMemoryInfo (MemInfoCallbackMMIO, &AcpiBoardInfo);
> >
> > +  if (EFI_ERROR(Status)) {
> >
> > +    return Status;
> >
> > +  }
> >
> > +
> >
> >    //
> >
> >    // Parse platform specific information.
> >
> >    //
> >
> > diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > index 2c84d6ed53..35ea23d202 100644
> > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > @@ -38,6 +38,16 @@
> >  #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \
> >
> >    ((ActualSize) + (((Alignment) - ((ActualSize) & ((Alignment) - 1))) &
> > ((Alignment) - 1)))
> >
> >
> >
> > +
> >
> > +#define E820_RAM        1
> >
> > +#define E820_RESERVED   2
> >
> > +#define E820_ACPI       3
> >
> > +#define E820_NVS        4
> >
> > +#define E820_UNUSABLE   5
> >
> > +#define E820_DISABLED   6
> >
> > +#define E820_PMEM       7
> >
> > +#define E820_UNDEFINED  8
> >
> > +
> >
> >  /**
> >
> >    Auto-generated function that calls the library constructors for all of the
> > module's
> >
> >    dependent libraries.
> >
> > --
> > 2.30.2
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#76517): https://edk2.groups.io/g/devel/message/76517
> > Mute This Topic: https://groups.io/mt/83555387/1781375
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.dong@intel.com]
> > -=-=-=-=-=-=
> >
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76575): https://edk2.groups.io/g/devel/message/76575
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Ma, Maurice 2 years, 9 months ago
Hi, Rudolph,

Thank you for submitting the patch.   In general the approach looks good to me. 
Here I have several minor comments on your patch as listed below:

1.  For global variable in module, could we add "m" prefix ?   EX:  Change  "TopOfLowerUsableDram" to "mTopOfLowerUsableDram" ?

2.  Please rename "MemInfoCallbackMMIO" to "MemInfoCallbackMmio" to follow naming convention.

3.  Maybe we can add parentheses for some condition expressions to make it easier to read.
     EX:  Change: 
           (MemoryMapEntry->Base < 0x100000000ULL &&  MemoryMapEntry->Base >= TopOfLowerUsableDram)
             To:
           ((MemoryMapEntry->Base < 0x100000000ULL) &&  (MemoryMapEntry->Base >= TopOfLowerUsableDram))

4.  If we use "EFI_STATUS" for function status, then function return type should match that.  
     EX:  Use "EFI_SUCCESS" instead of "RETURN_SUCCESS ".

5.  I think the new FindToludCallback() function will find the correct TOLUD if ACPI NVS memory region is below ACPI RECLAIM memory.  
     However, if it is the other way around, the TOLUD calculation might be incorrect.  Should we consider both cases?
     For example, given the memory map below,  your patch will get TOLUM = 0x1EB6C000.  But the actual TOLUM should be 0x20000000.
                 Base                             Length                         E820 Type
     MEM: 0000000000000000 00000000000A0000  1
     MEM: 00000000000A0000 0000000000060000  2
     MEM: 0000000000100000 000000001EA00000  1
     MEM: 000000001EB00000 0000000000004000  2
     MEM: 000000001EB04000 0000000000068000  3 (ACPI Reclaim)
     MEM: 000000001EB6C000 0000000000008000  4 (ACPI NVS)
     MEM: 000000001EB74000 000000000038C000  2
     MEM: 000000001EF00000 0000000000100000  2
     MEM: 000000001F000000 0000000001000000  2

Thanks
Maurice

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Tuesday, June 15, 2021 6:23
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader
> memrange parsing
> 
> Currently several DXE crash due to invalid memory resource settings.
> coreboot and slimbootloader provide an e820 compatible memory map,
> which doesn't work well with EDK2 as the e820 spec is missing MMIO regions.
> In e820 'reserved' could either mean "DRAM used by boot firmware" or
> "MMIO in use and not detectable by OS".
> 
> Guess Top of lower usable DRAM (TOLUD) by walking memory ranges and
> then mark everything reserved below TOLUD as DRAM and everything
> reserved above TOLUD as MMIO.
> 
> This fixes several assertions seen in PciHostBridgeDxe.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
>  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
>  2 files changed, 194 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> index 805f5448d9..d20e1a0862 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> @@ -7,10 +7,162 @@
>   #include "UefiPayloadEntry.h" +STATIC UINT32 TopOfLowerUsableDram =
> 0;+ /**    Callback function to build resource descriptor HOB     This function
> build a HOB based on the memory map entry info.+   It creates only
> EFI_RESOURCE_MEMORY_MAPPED_IO and
> EFI_RESOURCE_MEMORY_RESERVED+   resources.++   @param
> MemoryMapEntry         Memory map entry info got from bootloader.+
> @param Params                 A pointer to ACPI_BOARD_INFO.++  @retval
> RETURN_SUCCESS         Successfully build a HOB.+  @retval
> EFI_INVALID_PARAMETER  Invalid parameter
> provided.+**/+EFI_STATUS+MemInfoCallbackMMIO (+  IN
> MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> *Params+  )+{+  EFI_PHYSICAL_ADDRESS         Base;+  EFI_RESOURCE_TYPE
> Type;+  UINT64                       Size;+  EFI_RESOURCE_ATTRIBUTE_TYPE
> Attribue;+  ACPI_BOARD_INFO              *AcpiBoardInfo;++  AcpiBoardInfo =
> (ACPI_BOARD_INFO *)Params;+  if (AcpiBoardInfo == NULL) {+    return
> EFI_INVALID_PARAMETER;+  }++  //+  // Skip types already handled in
> MemInfoCallback+  //+  if (MemoryMapEntry->Type == E820_RAM ||
> MemoryMapEntry->Type == E820_ACPI) {+    return RETURN_SUCCESS;+  }++
> if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {+    //+
> // MMCONF is always MMIO+    //+    Type =
> EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else if (MemoryMapEntry->Base
> < TopOfLowerUsableDram) {+    //+    // It's in DRAM and thus must be
> reserved+    //+    Type = EFI_RESOURCE_MEMORY_RESERVED;+  } else if
> (MemoryMapEntry->Base < 0x100000000ULL &&+    MemoryMapEntry-
> >Base >= TopOfLowerUsableDram) {+    //+    // It's not in DRAM, must be
> MMIO+    //+    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else {+
> Type = EFI_RESOURCE_MEMORY_RESERVED;+  }++  Base    =
> MemoryMapEntry->Base;+  Size    = MemoryMapEntry->Size;++  Attribue =
> EFI_RESOURCE_ATTRIBUTE_PRESENT |+
> EFI_RESOURCE_ATTRIBUTE_INITIALIZED |+
> EFI_RESOURCE_ATTRIBUTE_TESTED |+
> EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |+
> EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |+
> EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |+
> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;++
> BuildResourceDescriptorHob (Type, Attribue, (EFI_PHYSICAL_ADDRESS)Base,
> Size);+  DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> 0x%x\n", Base, Size, Type));++  if (MemoryMapEntry->Type == E820_NVS) {+
> BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);+  } else if
> (MemoryMapEntry->Type == E820_UNUSABLE ||+    MemoryMapEntry-
> >Type == E820_DISABLED) {+    BuildMemoryAllocationHob (Base, Size,
> EfiUnusableMemory);+  } else if (MemoryMapEntry->Type == E820_PMEM)
> {+    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);+  }++
> return RETURN_SUCCESS;+}+++/**+   Callback function to find TOLUD (Top
> of Lower Usable DRAM)++   Estimate where TOLUD (Top of Lower Usable
> DRAM) resides. The exact position+   would require platform specific code.++
> @param MemoryMapEntry         Memory map entry info got from
> bootloader.+   @param Params                 Not used for now.++  @retval
> RETURN_SUCCESS         Successfully updated
> TopOfLowerUsableDram.+**/+EFI_STATUS+FindToludCallback (+  IN
> MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> *Params+  )+{+  //+  // This code assumes that the memory map on this x86
> machine below 4GiB is continous+  // until TOLUD. In addition it assumes that
> the bootloader provided memory tables have+  // no "holes" and thus the
> first memory range not covered by e820 marks the end of+  // usable DRAM.
> In addition it's assumed that every reserved memory region touching+  //
> usable RAM is also covering DRAM, everything else that is marked reserved
> thus must be+  // MMIO not detectable by bootloader/OS+  //++  //+  // Skip
> memory types not RAM or reserved+  //+  if (MemoryMapEntry->Type ==
> E820_NVS || MemoryMapEntry->Type == E820_UNUSABLE ||+
> MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry->Type ==
> E820_PMEM) {+    return RETURN_SUCCESS;+  }++  //+  // Skip resources
> above 4GiB+  //+  if (MemoryMapEntry->Base >= 0x100000000ULL) {+
> return RETURN_SUCCESS;+  }++  if ((MemoryMapEntry->Type == E820_RAM)
> ||+    (MemoryMapEntry->Type == E820_ACPI)) {+    //+    // It's usable DRAM.
> Update TOLUD.+    //+    if (TopOfLowerUsableDram < (MemoryMapEntry-
> >Base + MemoryMapEntry->Size)) {+      TopOfLowerUsableDram =
> MemoryMapEntry->Base + MemoryMapEntry->Size;+    }+  } else {+    //+    //
> It might be reserved DRAM or MMIO.+    //+    // If it touches usable DRAM at
> Base assume it's DRAM as well,+    // as it could be bootloader installed tables,
> TSEG, GTT, ...+    //+    if (TopOfLowerUsableDram == MemoryMapEntry-
> >Base) {+      TopOfLowerUsableDram = MemoryMapEntry->Base +
> MemoryMapEntry->Size;+    }+  }++  return RETURN_SUCCESS;+}+++/**+
> Callback function to build resource descriptor HOB++   This function build a
> HOB based on the memory map entry info.+   Only add
> EFI_RESOURCE_SYSTEM_MEMORY.     @param MemoryMapEntry
> Memory map entry info got from bootloader.    @param Params                 Not
> used for now.@@ -28,7 +180,15 @@ MemInfoCallback (
>    UINT64                       Size;   EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue; -  Type
> = (MemoryMapEntry->Type == 1) ? EFI_RESOURCE_SYSTEM_MEMORY :
> EFI_RESOURCE_MEMORY_RESERVED;+  //+  // Skip everything not known to
> be usable DRAM.+  // It will be added later.+  //+  if (MemoryMapEntry-
> >Type != E820_RAM && MemoryMapEntry->Type != E820_ACPI) {+    return
> RETURN_SUCCESS;+  }++  Type    = EFI_RESOURCE_SYSTEM_MEMORY;   Base
> = MemoryMapEntry->Base;   Size    = MemoryMapEntry->Size; @@ -40,7
> +200,7 @@ MemInfoCallback (
>               EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE; -  if (Base >=
> BASE_4GB ) {+  if (Base >= BASE_4GB) {     // Remove tested attribute to
> avoid DXE core to dispatch driver to memory above 4GB     Attribue &=
> ~EFI_RESOURCE_ATTRIBUTE_TESTED;   }@@ -48,6 +208,10 @@
> MemInfoCallback (
>    BuildResourceDescriptorHob (Type, Attribue,
> (EFI_PHYSICAL_ADDRESS)Base, Size);   DEBUG ((DEBUG_INFO , "buildhob:
> base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type)); +  if
> (MemoryMapEntry->Type == E820_ACPI) {+    BuildMemoryAllocationHob
> (Base, Size, EfiACPIReclaimMemory);+  }+   return RETURN_SUCCESS; } @@ -
> 236,7 +400,16 @@ BuildHobFromBl (
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;    //-  // Parse
> memory info and build memory HOBs+  // First find TOLUD+  //+  Status =
> ParseMemoryInfo (FindToludCallback, NULL);+  if (EFI_ERROR(Status)) {+
> return Status;+  }+  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n",
> TopOfLowerUsableDram));++  //+  // Parse memory info and build memory
> HOBs for Usable RAM   //   Status = ParseMemoryInfo (MemInfoCallback,
> NULL);   if (EFI_ERROR(Status)) {@@ -289,6 +462,14 @@ BuildHobFromBl (
>      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));   } +  //+  //
> Parse memory info and build memory HOBs for reserved DRAM and MMIO+
> //+  Status = ParseMemoryInfo (MemInfoCallbackMMIO, &AcpiBoardInfo);+
> if (EFI_ERROR(Status)) {+    return Status;+  }+   //   // Parse platform specific
> information.   //diff --git
> a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> index 2c84d6ed53..35ea23d202 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> @@ -38,6 +38,16 @@
>  #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \   ((ActualSize) +
> (((Alignment) - ((ActualSize) & ((Alignment) - 1))) & ((Alignment) - 1)))
> ++#define E820_RAM        1+#define E820_RESERVED   2+#define E820_ACPI
> 3+#define E820_NVS        4+#define E820_UNUSABLE   5+#define
> E820_DISABLED   6+#define E820_PMEM       7+#define E820_UNDEFINED  8+
> /**   Auto-generated function that calls the library constructors for all of the
> module's   dependent libraries.--
> 2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76621): https://edk2.groups.io/g/devel/message/76621
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Patrick Rudolph 2 years, 9 months ago
Hi Maurice,
I've implemented the requested changes. It now also accepts ACPI_NVS
as usable DRAM.

Thanks,
Patrick

On Thu, Jun 17, 2021 at 2:33 AM Ma, Maurice <maurice.ma@intel.com> wrote:
>
> Hi, Rudolph,
>
> Thank you for submitting the patch.   In general the approach looks good to me.
> Here I have several minor comments on your patch as listed below:
>
> 1.  For global variable in module, could we add "m" prefix ?   EX:  Change  "TopOfLowerUsableDram" to "mTopOfLowerUsableDram" ?
>
> 2.  Please rename "MemInfoCallbackMMIO" to "MemInfoCallbackMmio" to follow naming convention.
>
> 3.  Maybe we can add parentheses for some condition expressions to make it easier to read.
>      EX:  Change:
>            (MemoryMapEntry->Base < 0x100000000ULL &&  MemoryMapEntry->Base >= TopOfLowerUsableDram)
>              To:
>            ((MemoryMapEntry->Base < 0x100000000ULL) &&  (MemoryMapEntry->Base >= TopOfLowerUsableDram))
>
> 4.  If we use "EFI_STATUS" for function status, then function return type should match that.
>      EX:  Use "EFI_SUCCESS" instead of "RETURN_SUCCESS ".
>
> 5.  I think the new FindToludCallback() function will find the correct TOLUD if ACPI NVS memory region is below ACPI RECLAIM memory.
>      However, if it is the other way around, the TOLUD calculation might be incorrect.  Should we consider both cases?
>      For example, given the memory map below,  your patch will get TOLUM = 0x1EB6C000.  But the actual TOLUM should be 0x20000000.
>                  Base                             Length                         E820 Type
>      MEM: 0000000000000000 00000000000A0000  1
>      MEM: 00000000000A0000 0000000000060000  2
>      MEM: 0000000000100000 000000001EA00000  1
>      MEM: 000000001EB00000 0000000000004000  2
>      MEM: 000000001EB04000 0000000000068000  3 (ACPI Reclaim)
>      MEM: 000000001EB6C000 0000000000008000  4 (ACPI NVS)
>      MEM: 000000001EB74000 000000000038C000  2
>      MEM: 000000001EF00000 0000000000100000  2
>      MEM: 000000001F000000 0000000001000000  2
>
> Thanks
> Maurice
>
> > -----Original Message-----
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Sent: Tuesday, June 15, 2021 6:23
> > To: devel@edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > Subject: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader
> > memrange parsing
> >
> > Currently several DXE crash due to invalid memory resource settings.
> > coreboot and slimbootloader provide an e820 compatible memory map,
> > which doesn't work well with EDK2 as the e820 spec is missing MMIO regions.
> > In e820 'reserved' could either mean "DRAM used by boot firmware" or
> > "MMIO in use and not detectable by OS".
> >
> > Guess Top of lower usable DRAM (TOLUD) by walking memory ranges and
> > then mark everything reserved below TOLUD as DRAM and everything
> > reserved above TOLUD as MMIO.
> >
> > This fixes several assertions seen in PciHostBridgeDxe.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > ---
> >  .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
> >  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
> >  2 files changed, 194 insertions(+), 3 deletions(-)
> >
> > diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > index 805f5448d9..d20e1a0862 100644
> > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > @@ -7,10 +7,162 @@
> >   #include "UefiPayloadEntry.h" +STATIC UINT32 TopOfLowerUsableDram =
> > 0;+ /**    Callback function to build resource descriptor HOB     This function
> > build a HOB based on the memory map entry info.+   It creates only
> > EFI_RESOURCE_MEMORY_MAPPED_IO and
> > EFI_RESOURCE_MEMORY_RESERVED+   resources.++   @param
> > MemoryMapEntry         Memory map entry info got from bootloader.+
> > @param Params                 A pointer to ACPI_BOARD_INFO.++  @retval
> > RETURN_SUCCESS         Successfully build a HOB.+  @retval
> > EFI_INVALID_PARAMETER  Invalid parameter
> > provided.+**/+EFI_STATUS+MemInfoCallbackMMIO (+  IN
> > MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> > *Params+  )+{+  EFI_PHYSICAL_ADDRESS         Base;+  EFI_RESOURCE_TYPE
> > Type;+  UINT64                       Size;+  EFI_RESOURCE_ATTRIBUTE_TYPE
> > Attribue;+  ACPI_BOARD_INFO              *AcpiBoardInfo;++  AcpiBoardInfo =
> > (ACPI_BOARD_INFO *)Params;+  if (AcpiBoardInfo == NULL) {+    return
> > EFI_INVALID_PARAMETER;+  }++  //+  // Skip types already handled in
> > MemInfoCallback+  //+  if (MemoryMapEntry->Type == E820_RAM ||
> > MemoryMapEntry->Type == E820_ACPI) {+    return RETURN_SUCCESS;+  }++
> > if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {+    //+
> > // MMCONF is always MMIO+    //+    Type =
> > EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else if (MemoryMapEntry->Base
> > < TopOfLowerUsableDram) {+    //+    // It's in DRAM and thus must be
> > reserved+    //+    Type = EFI_RESOURCE_MEMORY_RESERVED;+  } else if
> > (MemoryMapEntry->Base < 0x100000000ULL &&+    MemoryMapEntry-
> > >Base >= TopOfLowerUsableDram) {+    //+    // It's not in DRAM, must be
> > MMIO+    //+    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else {+
> > Type = EFI_RESOURCE_MEMORY_RESERVED;+  }++  Base    =
> > MemoryMapEntry->Base;+  Size    = MemoryMapEntry->Size;++  Attribue =
> > EFI_RESOURCE_ATTRIBUTE_PRESENT |+
> > EFI_RESOURCE_ATTRIBUTE_INITIALIZED |+
> > EFI_RESOURCE_ATTRIBUTE_TESTED |+
> > EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |+
> > EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |+
> > EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |+
> > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;++
> > BuildResourceDescriptorHob (Type, Attribue, (EFI_PHYSICAL_ADDRESS)Base,
> > Size);+  DEBUG ((DEBUG_INFO , "buildhob: base = 0x%lx, size = 0x%lx, type =
> > 0x%x\n", Base, Size, Type));++  if (MemoryMapEntry->Type == E820_NVS) {+
> > BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);+  } else if
> > (MemoryMapEntry->Type == E820_UNUSABLE ||+    MemoryMapEntry-
> > >Type == E820_DISABLED) {+    BuildMemoryAllocationHob (Base, Size,
> > EfiUnusableMemory);+  } else if (MemoryMapEntry->Type == E820_PMEM)
> > {+    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);+  }++
> > return RETURN_SUCCESS;+}+++/**+   Callback function to find TOLUD (Top
> > of Lower Usable DRAM)++   Estimate where TOLUD (Top of Lower Usable
> > DRAM) resides. The exact position+   would require platform specific code.++
> > @param MemoryMapEntry         Memory map entry info got from
> > bootloader.+   @param Params                 Not used for now.++  @retval
> > RETURN_SUCCESS         Successfully updated
> > TopOfLowerUsableDram.+**/+EFI_STATUS+FindToludCallback (+  IN
> > MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> > *Params+  )+{+  //+  // This code assumes that the memory map on this x86
> > machine below 4GiB is continous+  // until TOLUD. In addition it assumes that
> > the bootloader provided memory tables have+  // no "holes" and thus the
> > first memory range not covered by e820 marks the end of+  // usable DRAM.
> > In addition it's assumed that every reserved memory region touching+  //
> > usable RAM is also covering DRAM, everything else that is marked reserved
> > thus must be+  // MMIO not detectable by bootloader/OS+  //++  //+  // Skip
> > memory types not RAM or reserved+  //+  if (MemoryMapEntry->Type ==
> > E820_NVS || MemoryMapEntry->Type == E820_UNUSABLE ||+
> > MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry->Type ==
> > E820_PMEM) {+    return RETURN_SUCCESS;+  }++  //+  // Skip resources
> > above 4GiB+  //+  if (MemoryMapEntry->Base >= 0x100000000ULL) {+
> > return RETURN_SUCCESS;+  }++  if ((MemoryMapEntry->Type == E820_RAM)
> > ||+    (MemoryMapEntry->Type == E820_ACPI)) {+    //+    // It's usable DRAM.
> > Update TOLUD.+    //+    if (TopOfLowerUsableDram < (MemoryMapEntry-
> > >Base + MemoryMapEntry->Size)) {+      TopOfLowerUsableDram =
> > MemoryMapEntry->Base + MemoryMapEntry->Size;+    }+  } else {+    //+    //
> > It might be reserved DRAM or MMIO.+    //+    // If it touches usable DRAM at
> > Base assume it's DRAM as well,+    // as it could be bootloader installed tables,
> > TSEG, GTT, ...+    //+    if (TopOfLowerUsableDram == MemoryMapEntry-
> > >Base) {+      TopOfLowerUsableDram = MemoryMapEntry->Base +
> > MemoryMapEntry->Size;+    }+  }++  return RETURN_SUCCESS;+}+++/**+
> > Callback function to build resource descriptor HOB++   This function build a
> > HOB based on the memory map entry info.+   Only add
> > EFI_RESOURCE_SYSTEM_MEMORY.     @param MemoryMapEntry
> > Memory map entry info got from bootloader.    @param Params                 Not
> > used for now.@@ -28,7 +180,15 @@ MemInfoCallback (
> >    UINT64                       Size;   EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue; -  Type
> > = (MemoryMapEntry->Type == 1) ? EFI_RESOURCE_SYSTEM_MEMORY :
> > EFI_RESOURCE_MEMORY_RESERVED;+  //+  // Skip everything not known to
> > be usable DRAM.+  // It will be added later.+  //+  if (MemoryMapEntry-
> > >Type != E820_RAM && MemoryMapEntry->Type != E820_ACPI) {+    return
> > RETURN_SUCCESS;+  }++  Type    = EFI_RESOURCE_SYSTEM_MEMORY;   Base
> > = MemoryMapEntry->Base;   Size    = MemoryMapEntry->Size; @@ -40,7
> > +200,7 @@ MemInfoCallback (
> >               EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE; -  if (Base >=
> > BASE_4GB ) {+  if (Base >= BASE_4GB) {     // Remove tested attribute to
> > avoid DXE core to dispatch driver to memory above 4GB     Attribue &=
> > ~EFI_RESOURCE_ATTRIBUTE_TESTED;   }@@ -48,6 +208,10 @@
> > MemInfoCallback (
> >    BuildResourceDescriptorHob (Type, Attribue,
> > (EFI_PHYSICAL_ADDRESS)Base, Size);   DEBUG ((DEBUG_INFO , "buildhob:
> > base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type)); +  if
> > (MemoryMapEntry->Type == E820_ACPI) {+    BuildMemoryAllocationHob
> > (Base, Size, EfiACPIReclaimMemory);+  }+   return RETURN_SUCCESS; } @@ -
> > 236,7 +400,16 @@ BuildHobFromBl (
> >    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;    //-  // Parse
> > memory info and build memory HOBs+  // First find TOLUD+  //+  Status =
> > ParseMemoryInfo (FindToludCallback, NULL);+  if (EFI_ERROR(Status)) {+
> > return Status;+  }+  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n",
> > TopOfLowerUsableDram));++  //+  // Parse memory info and build memory
> > HOBs for Usable RAM   //   Status = ParseMemoryInfo (MemInfoCallback,
> > NULL);   if (EFI_ERROR(Status)) {@@ -289,6 +462,14 @@ BuildHobFromBl (
> >      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));   } +  //+  //
> > Parse memory info and build memory HOBs for reserved DRAM and MMIO+
> > //+  Status = ParseMemoryInfo (MemInfoCallbackMMIO, &AcpiBoardInfo);+
> > if (EFI_ERROR(Status)) {+    return Status;+  }+   //   // Parse platform specific
> > information.   //diff --git
> > a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > index 2c84d6ed53..35ea23d202 100644
> > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > @@ -38,6 +38,16 @@
> >  #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \   ((ActualSize) +
> > (((Alignment) - ((ActualSize) & ((Alignment) - 1))) & ((Alignment) - 1)))
> > ++#define E820_RAM        1+#define E820_RESERVED   2+#define E820_ACPI
> > 3+#define E820_NVS        4+#define E820_UNUSABLE   5+#define
> > E820_DISABLED   6+#define E820_PMEM       7+#define E820_UNDEFINED  8+
> > /**   Auto-generated function that calls the library constructors for all of the
> > module's   dependent libraries.--
> > 2.30.2
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76671): https://edk2.groups.io/g/devel/message/76671
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Posted by Ma, Maurice 2 years, 9 months ago
Hi, Patrick,

Thank you,  I will take a look on the new patch!

Regards
Maurice
> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Thursday, June 17, 2021 6:21
> To: Ma, Maurice <maurice.ma@intel.com>
> Cc: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; You,
> Benjamin <benjamin.you@intel.com>
> Subject: Re: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader
> memrange parsing
> 
> Hi Maurice,
> I've implemented the requested changes. It now also accepts ACPI_NVS as
> usable DRAM.
> 
> Thanks,
> Patrick
> 
> On Thu, Jun 17, 2021 at 2:33 AM Ma, Maurice <maurice.ma@intel.com>
> wrote:
> >
> > Hi, Rudolph,
> >
> > Thank you for submitting the patch.   In general the approach looks good to
> me.
> > Here I have several minor comments on your patch as listed below:
> >
> > 1.  For global variable in module, could we add "m" prefix ?   EX:  Change
> "TopOfLowerUsableDram" to "mTopOfLowerUsableDram" ?
> >
> > 2.  Please rename "MemInfoCallbackMMIO" to "MemInfoCallbackMmio"
> to follow naming convention.
> >
> > 3.  Maybe we can add parentheses for some condition expressions to make
> it easier to read.
> >      EX:  Change:
> >            (MemoryMapEntry->Base < 0x100000000ULL &&  MemoryMapEntry-
> >Base >= TopOfLowerUsableDram)
> >              To:
> >            ((MemoryMapEntry->Base < 0x100000000ULL) &&
> > (MemoryMapEntry->Base >= TopOfLowerUsableDram))
> >
> > 4.  If we use "EFI_STATUS" for function status, then function return type
> should match that.
> >      EX:  Use "EFI_SUCCESS" instead of "RETURN_SUCCESS ".
> >
> > 5.  I think the new FindToludCallback() function will find the correct TOLUD
> if ACPI NVS memory region is below ACPI RECLAIM memory.
> >      However, if it is the other way around, the TOLUD calculation might be
> incorrect.  Should we consider both cases?
> >      For example, given the memory map below,  your patch will get TOLUM
> = 0x1EB6C000.  But the actual TOLUM should be 0x20000000.
> >                  Base                             Length                         E820 Type
> >      MEM: 0000000000000000 00000000000A0000  1
> >      MEM: 00000000000A0000 0000000000060000  2
> >      MEM: 0000000000100000 000000001EA00000  1
> >      MEM: 000000001EB00000 0000000000004000  2
> >      MEM: 000000001EB04000 0000000000068000  3 (ACPI Reclaim)
> >      MEM: 000000001EB6C000 0000000000008000  4 (ACPI NVS)
> >      MEM: 000000001EB74000 000000000038C000  2
> >      MEM: 000000001EF00000 0000000000100000  2
> >      MEM: 000000001F000000 0000000001000000  2
> >
> > Thanks
> > Maurice
> >
> > > -----Original Message-----
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Sent: Tuesday, June 15, 2021 6:23
> > > To: devel@edk2.groups.io
> > > Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> > > <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > > Subject: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader
> > > memrange parsing
> > >
> > > Currently several DXE crash due to invalid memory resource settings.
> > > coreboot and slimbootloader provide an e820 compatible memory map,
> > > which doesn't work well with EDK2 as the e820 spec is missing MMIO
> regions.
> > > In e820 'reserved' could either mean "DRAM used by boot firmware" or
> > > "MMIO in use and not detectable by OS".
> > >
> > > Guess Top of lower usable DRAM (TOLUD) by walking memory ranges
> and
> > > then mark everything reserved below TOLUD as DRAM and everything
> > > reserved above TOLUD as MMIO.
> > >
> > > This fixes several assertions seen in PciHostBridgeDxe.
> > >
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > ---
> > >  .../UefiPayloadEntry/UefiPayloadEntry.c       | 187 +++++++++++++++++-
> > >  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
> > >  2 files changed, 194 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > index 805f5448d9..d20e1a0862 100644
> > > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> > > @@ -7,10 +7,162 @@
> > >   #include "UefiPayloadEntry.h" +STATIC UINT32 TopOfLowerUsableDram
> =
> > > 0;+ /**    Callback function to build resource descriptor HOB     This
> function
> > > build a HOB based on the memory map entry info.+   It creates only
> > > EFI_RESOURCE_MEMORY_MAPPED_IO and
> > > EFI_RESOURCE_MEMORY_RESERVED+   resources.++   @param
> > > MemoryMapEntry         Memory map entry info got from bootloader.+
> > > @param Params                 A pointer to ACPI_BOARD_INFO.++  @retval
> > > RETURN_SUCCESS         Successfully build a HOB.+  @retval
> > > EFI_INVALID_PARAMETER  Invalid parameter
> > > provided.+**/+EFI_STATUS+MemInfoCallbackMMIO (+  IN
> > > MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> > > *Params+  )+{+  EFI_PHYSICAL_ADDRESS         Base;+
> EFI_RESOURCE_TYPE
> > > Type;+  UINT64                       Size;+  EFI_RESOURCE_ATTRIBUTE_TYPE
> > > Attribue;+  ACPI_BOARD_INFO              *AcpiBoardInfo;++  AcpiBoardInfo
> =
> > > (ACPI_BOARD_INFO *)Params;+  if (AcpiBoardInfo == NULL) {+    return
> > > EFI_INVALID_PARAMETER;+  }++  //+  // Skip types already handled in
> > > MemInfoCallback+  //+  if (MemoryMapEntry->Type == E820_RAM ||
> > > MemoryMapEntry->Type == E820_ACPI) {+    return
> RETURN_SUCCESS;+  }++
> > > if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {+
> //+
> > > // MMCONF is always MMIO+    //+    Type =
> > > EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else if (MemoryMapEntry-
> >Base
> > > < TopOfLowerUsableDram) {+    //+    // It's in DRAM and thus must be
> > > reserved+    //+    Type = EFI_RESOURCE_MEMORY_RESERVED;+  } else if
> > > (MemoryMapEntry->Base < 0x100000000ULL &&+    MemoryMapEntry-
> > > >Base >= TopOfLowerUsableDram) {+    //+    // It's not in DRAM, must be
> > > MMIO+    //+    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;+  } else {+
> > > Type = EFI_RESOURCE_MEMORY_RESERVED;+  }++  Base    =
> > > MemoryMapEntry->Base;+  Size    = MemoryMapEntry->Size;++  Attribue
> =
> > > EFI_RESOURCE_ATTRIBUTE_PRESENT |+
> > > EFI_RESOURCE_ATTRIBUTE_INITIALIZED |+
> EFI_RESOURCE_ATTRIBUTE_TESTED
> > > |+ EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |+
> > > EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |+
> > > EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |+
> > > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE;++
> > > BuildResourceDescriptorHob (Type, Attribue,
> > > (EFI_PHYSICAL_ADDRESS)Base, Size);+  DEBUG ((DEBUG_INFO ,
> "buildhob:
> > > base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type));++
> > > if (MemoryMapEntry->Type == E820_NVS) {+ BuildMemoryAllocationHob
> (Base, Size, EfiACPIMemoryNVS);+  } else if
> > > (MemoryMapEntry->Type == E820_UNUSABLE ||+    MemoryMapEntry-
> > > >Type == E820_DISABLED) {+    BuildMemoryAllocationHob (Base, Size,
> > > EfiUnusableMemory);+  } else if (MemoryMapEntry->Type ==
> E820_PMEM)
> > > {+    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);+  }++
> > > return RETURN_SUCCESS;+}+++/**+   Callback function to find TOLUD
> (Top
> > > of Lower Usable DRAM)++   Estimate where TOLUD (Top of Lower Usable
> > > DRAM) resides. The exact position+   would require platform specific
> code.++
> > > @param MemoryMapEntry         Memory map entry info got from
> > > bootloader.+   @param Params                 Not used for now.++  @retval
> > > RETURN_SUCCESS         Successfully updated
> > > TopOfLowerUsableDram.+**/+EFI_STATUS+FindToludCallback (+  IN
> > > MEMROY_MAP_ENTRY          *MemoryMapEntry,+  IN VOID
> > > *Params+  )+{+  //+  // This code assumes that the memory map on
> > > this x86 machine below 4GiB is continous+  // until TOLUD. In
> > > addition it assumes that the bootloader provided memory tables have+
> > > // no "holes" and thus the first memory range not covered by e820 marks
> the end of+  // usable DRAM.
> > > In addition it's assumed that every reserved memory region touching+
> > > // usable RAM is also covering DRAM, everything else that is marked
> > > reserved thus must be+  // MMIO not detectable by bootloader/OS+
> > > //++  //+  // Skip memory types not RAM or reserved+  //+  if
> > > (MemoryMapEntry->Type == E820_NVS || MemoryMapEntry->Type ==
> > > E820_UNUSABLE ||+
> > > MemoryMapEntry->Type == E820_DISABLED || MemoryMapEntry-
> >Type ==
> > > E820_PMEM) {+    return RETURN_SUCCESS;+  }++  //+  // Skip resources
> > > above 4GiB+  //+  if (MemoryMapEntry->Base >= 0x100000000ULL) {+
> > > return RETURN_SUCCESS;+  }++  if ((MemoryMapEntry->Type ==
> E820_RAM)
> > > ||+    (MemoryMapEntry->Type == E820_ACPI)) {+    //+    // It's usable
> DRAM.
> > > Update TOLUD.+    //+    if (TopOfLowerUsableDram <
> (MemoryMapEntry-
> > > >Base + MemoryMapEntry->Size)) {+      TopOfLowerUsableDram =
> > > MemoryMapEntry->Base + MemoryMapEntry->Size;+    }+  } else {+    //+
> //
> > > It might be reserved DRAM or MMIO.+    //+    // If it touches usable
> DRAM at
> > > Base assume it's DRAM as well,+    // as it could be bootloader installed
> tables,
> > > TSEG, GTT, ...+    //+    if (TopOfLowerUsableDram == MemoryMapEntry-
> > > >Base) {+      TopOfLowerUsableDram = MemoryMapEntry->Base +
> > > MemoryMapEntry->Size;+    }+  }++  return RETURN_SUCCESS;+}+++/**+
> > > Callback function to build resource descriptor HOB++   This function build
> a
> > > HOB based on the memory map entry info.+   Only add
> > > EFI_RESOURCE_SYSTEM_MEMORY.     @param MemoryMapEntry
> > > Memory map entry info got from bootloader.    @param Params
> Not
> > > used for now.@@ -28,7 +180,15 @@ MemInfoCallback (
> > >    UINT64                       Size;   EFI_RESOURCE_ATTRIBUTE_TYPE  Attribue; -
> Type
> > > = (MemoryMapEntry->Type == 1) ? EFI_RESOURCE_SYSTEM_MEMORY :
> > > EFI_RESOURCE_MEMORY_RESERVED;+  //+  // Skip everything not
> known to
> > > be usable DRAM.+  // It will be added later.+  //+  if
> > > (MemoryMapEntry-
> > > >Type != E820_RAM && MemoryMapEntry->Type != E820_ACPI) {+
> return
> > > RETURN_SUCCESS;+  }++  Type    = EFI_RESOURCE_SYSTEM_MEMORY;
> Base
> > > = MemoryMapEntry->Base;   Size    = MemoryMapEntry->Size; @@ -40,7
> > > +200,7 @@ MemInfoCallback (
> > >               EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
> > > EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE; -  if (Base >=
> > > BASE_4GB ) {+  if (Base >= BASE_4GB) {     // Remove tested attribute to
> > > avoid DXE core to dispatch driver to memory above 4GB     Attribue &=
> > > ~EFI_RESOURCE_ATTRIBUTE_TESTED;   }@@ -48,6 +208,10 @@
> > > MemInfoCallback (
> > >    BuildResourceDescriptorHob (Type, Attribue,
> > > (EFI_PHYSICAL_ADDRESS)Base, Size);   DEBUG ((DEBUG_INFO , "buildhob:
> > > base = 0x%lx, size = 0x%lx, type = 0x%x\n", Base, Size, Type)); +  if
> > > (MemoryMapEntry->Type == E820_ACPI) {+
> BuildMemoryAllocationHob
> > > (Base, Size, EfiACPIReclaimMemory);+  }+   return RETURN_SUCCESS; }
> @@ -
> > > 236,7 +400,16 @@ BuildHobFromBl (
> > >    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;    //-  //
> Parse
> > > memory info and build memory HOBs+  // First find TOLUD+  //+
> > > Status = ParseMemoryInfo (FindToludCallback, NULL);+  if
> > > (EFI_ERROR(Status)) {+ return Status;+  }+  DEBUG ((DEBUG_INFO ,
> > > "Assuming TOLUD = 0x%x\n", TopOfLowerUsableDram));++  //+  // Parse
> memory info and build memory
> > > HOBs for Usable RAM   //   Status = ParseMemoryInfo (MemInfoCallback,
> > > NULL);   if (EFI_ERROR(Status)) {@@ -289,6 +462,14 @@ BuildHobFromBl (
> > >      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));   } +  //+
> //
> > > Parse memory info and build memory HOBs for reserved DRAM and
> MMIO+
> > > //+  Status = ParseMemoryInfo (MemInfoCallbackMMIO,
> &AcpiBoardInfo);+
> > > if (EFI_ERROR(Status)) {+    return Status;+  }+   //   // Parse platform
> specific
> > > information.   //diff --git
> > > a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > > b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > > index 2c84d6ed53..35ea23d202 100644
> > > --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > > +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> > > @@ -38,6 +38,16 @@
> > >  #define GET_OCCUPIED_SIZE(ActualSize, Alignment) \   ((ActualSize) +
> > > (((Alignment) - ((ActualSize) & ((Alignment) - 1))) & ((Alignment) -
> > > 1)))
> > > ++#define E820_RAM        1+#define E820_RESERVED   2+#define
> E820_ACPI
> > > 3+#define E820_NVS        4+#define E820_UNUSABLE   5+#define
> > > E820_DISABLED   6+#define E820_PMEM       7+#define E820_UNDEFINED
> 8+
> > > /**   Auto-generated function that calls the library constructors for all of
> the
> > > module's   dependent libraries.--
> > > 2.30.2
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76673): https://edk2.groups.io/g/devel/message/76673
Mute This Topic: https://groups.io/mt/83555387/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-