* Re: [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
2021-06-21 8:09 [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing Patrick Rudolph
@ 2021-06-21 14:06 ` Ma, Maurice
2021-06-22 17:54 ` Guo Dong
2021-06-22 18:11 ` Guo Dong
2 siblings, 0 replies; 4+ messages in thread
From: Ma, Maurice @ 2021-06-21 14:06 UTC (permalink / raw)
To: Patrick Rudolph, devel@edk2.groups.io; +Cc: Dong, Guo, You, Benjamin
Looks good to me.
Reviewed-by: Maurice Ma <maurice.ma@intel.com>
Regards
-Maurice
> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Monday, June 21, 2021 1:10
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
2021-06-21 8:09 [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing Patrick Rudolph
2021-06-21 14:06 ` Ma, Maurice
@ 2021-06-22 17:54 ` Guo Dong
2021-06-22 18:11 ` Guo Dong
2 siblings, 0 replies; 4+ messages in thread
From: Guo Dong @ 2021-06-22 17:54 UTC (permalink / raw)
To: Patrick Rudolph, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing
2021-06-21 8:09 [PATCH v3] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing Patrick Rudolph
2021-06-21 14:06 ` Ma, Maurice
2021-06-22 17:54 ` Guo Dong
@ 2021-06-22 18:11 ` Guo Dong
2 siblings, 0 replies; 4+ messages in thread
From: Guo Dong @ 2021-06-22 18:11 UTC (permalink / raw)
To: Patrick Rudolph, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin
Patch pushed @ https://github.com/tianocore/edk2/commit/1e5e58d39bb18a127e978d6e46a7454430799e57
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread