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


Ma, Maurice
 

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

Join devel@edk2.groups.io to automatically receive all group messages.