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

["Ma, Maurice" <maurice.ma@intel.com>]

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