[edk2-devel] [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
Patrick Rudolph
patrick.rudolph at 9elements.com
Thu Jun 17 13:20:56 UTC 2021
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 at 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 at 9elements.com>
> > Sent: Tuesday, June 15, 2021 6:23
> > To: devel at edk2.groups.io
> > Cc: Ma, Maurice <maurice.ma at intel.com>; Dong, Guo
> > <guo.dong at intel.com>; You, Benjamin <benjamin.you at 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 at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list