Re: [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing

["Guo Dong" <guo.dong@intel.com>]

Reviewed-by: Guo Dong <guo.dong@intel.com>

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Monday, June 21, 2021 1:10 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: [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader
> memrange parsing
> 
> Currently several DXE crash due to invalid memory resource settings.
> The PciHostBridgeDxe which expects the MMCONF and PCI Aperature
> to be EfiMemoryMappedIO, but currently those regions are (partly)
> mapped as EfiReservedMemoryType.
> 
> 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 the bootloader
> provided
> memory ranges. Memory types of RAM, ACPI and ACPI NVS below 4 GiB are
> used
> to increment TOLUD and reserved memory ranges touching TOLUD at the
> base
> are also assumed to be reserved DRAM, which increment TOLUD.
> 
> Then mark everything reserved below TOLUD as EfiReservedMemoryType
> and
> everything reserved above TOLUD as EfiMemoryMappedIO.
> 
> This fixes assertions seen in PciHostBridgeDxe.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  .../UefiPayloadEntry/UefiPayloadEntry.c       | 190 +++++++++++++++++-
>  .../UefiPayloadEntry/UefiPayloadEntry.h       |  10 +
>  2 files changed, 197 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> index 805f5448d9..04c58f776c 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> @@ -7,10 +7,159 @@
> 
> 
>  #include "UefiPayloadEntry.h"
> 
> 
> 
> +STATIC UINT32 mTopOfLowerUsableDram = 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 EFI_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 EFI_SUCCESS;
> 
> +  }
> 
> +
> 
> +  if (MemoryMapEntry->Base == AcpiBoardInfo->PcieBaseAddress) {
> 
> +    //
> 
> +    // MMCONF is always MMIO
> 
> +    //
> 
> +    Type = EFI_RESOURCE_MEMORY_MAPPED_IO;
> 
> +  } else if (MemoryMapEntry->Base < mTopOfLowerUsableDram) {
> 
> +    //
> 
> +    // It's in DRAM and thus must be reserved
> 
> +    //
> 
> +    Type = EFI_RESOURCE_MEMORY_RESERVED;
> 
> +  } else if ((MemoryMapEntry->Base < 0x100000000ULL) &&
> (MemoryMapEntry->Base >= mTopOfLowerUsableDram)) {
> 
> +    //
> 
> +    // 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_UNUSABLE ||
> 
> +    MemoryMapEntry->Type == E820_DISABLED) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiUnusableMemory);
> 
> +  } else if (MemoryMapEntry->Type == E820_PMEM) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiPersistentMemory);
> 
> +  }
> 
> +
> 
> +  return EFI_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 EFI_SUCCESS            Successfully updated
> mTopOfLowerUsableDram.
> 
> +**/
> 
> +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_UNUSABLE) || (MemoryMapEntry-
> >Type == E820_DISABLED) ||
> 
> +    (MemoryMapEntry->Type == E820_PMEM)) {
> 
> +    return EFI_SUCCESS;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // Skip resources above 4GiB
> 
> +  //
> 
> +  if ((MemoryMapEntry->Base + MemoryMapEntry->Size) >
> 0x100000000ULL) {
> 
> +    return EFI_SUCCESS;
> 
> +  }
> 
> +
> 
> +  if ((MemoryMapEntry->Type == E820_RAM) || (MemoryMapEntry->Type
> == E820_ACPI) ||
> 
> +    (MemoryMapEntry->Type == E820_NVS)) {
> 
> +    //
> 
> +    // It's usable DRAM. Update TOLUD.
> 
> +    //
> 
> +    if (mTopOfLowerUsableDram < (MemoryMapEntry->Base +
> MemoryMapEntry->Size)) {
> 
> +      mTopOfLowerUsableDram = (UINT32)(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 (mTopOfLowerUsableDram == MemoryMapEntry->Base) {
> 
> +      mTopOfLowerUsableDram = (UINT32)(MemoryMapEntry->Base +
> MemoryMapEntry->Size);
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return EFI_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 +177,16 @@ 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) &&
> 
> +    (MemoryMapEntry->Type != E820_NVS)) {
> 
> +    return RETURN_SUCCESS;
> 
> +  }
> 
> +
> 
> +  Type    = EFI_RESOURCE_SYSTEM_MEMORY;
> 
>    Base    = MemoryMapEntry->Base;
> 
>    Size    = MemoryMapEntry->Size;
> 
> 
> 
> @@ -40,7 +198,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 +206,12 @@ 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);
> 
> +  } else if (MemoryMapEntry->Type == E820_NVS) {
> 
> +    BuildMemoryAllocationHob (Base, Size, EfiACPIMemoryNVS);
> 
> +  }
> 
> +
> 
>    return RETURN_SUCCESS;
> 
>  }
> 
> 
> 
> @@ -236,8 +400,19 @@ BuildHobFromBl (
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
> 
> 
> 
>    //
> 
> -  // Parse memory info and build memory HOBs
> 
> +  // First find TOLUD
> 
> +  //
> 
> +  DEBUG ((DEBUG_INFO , "Guessing Top of Lower Usable DRAM:\n"));
> 
> +  Status = ParseMemoryInfo (FindToludCallback, NULL);
> 
> +  if (EFI_ERROR(Status)) {
> 
> +    return Status;
> 
> +  }
> 
> +  DEBUG ((DEBUG_INFO , "Assuming TOLUD = 0x%x\n",
> mTopOfLowerUsableDram));
> 
> +
> 
> +  //
> 
> +  // Parse memory info and build memory HOBs for Usable RAM
> 
>    //
> 
> +  DEBUG ((DEBUG_INFO , "Building ResourceDescriptorHobs for usable
> memory:\n"));
> 
>    Status = ParseMemoryInfo (MemInfoCallback, NULL);
> 
>    if (EFI_ERROR(Status)) {
> 
>      return Status;
> 
> @@ -289,6 +464,15 @@ BuildHobFromBl (
>      DEBUG ((DEBUG_INFO, "Create acpi board info guid hob\n"));
> 
>    }
> 
> 
> 
> +  //
> 
> +  // Parse memory info and build memory HOBs for reserved DRAM and
> MMIO
> 
> +  //
> 
> +  DEBUG ((DEBUG_INFO , "Building ResourceDescriptorHobs for reserved
> memory:\n"));
> 
> +  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..4fd50e47cd 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