Looks good to me. Reviewed-by: Maurice Ma <maurice...@intel.com> Regards -Maurice
> -----Original Message----- > From: Patrick Rudolph <patrick.rudo...@9elements.com> > Sent: Monday, June 21, 2021 1:10 > To: devel@edk2.groups.io > Cc: Ma, Maurice <maurice...@intel.com>; Dong, Guo > <guo.d...@intel.com>; You, Benjamin <benjamin....@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.rudo...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76808): https://edk2.groups.io/g/devel/message/76808 Mute This Topic: https://groups.io/mt/83683813/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-