From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by mx.groups.io with SMTP id smtpd.web11.125.1623936068641206808 for ; Thu, 17 Jun 2021 06:21:08 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@9elements.com header.s=google header.b=hNcQU9ZE; spf=pass (domain: 9elements.com, ip: 209.85.210.177, mailfrom: patrick.rudolph@9elements.com) Received: by mail-pf1-f177.google.com with SMTP id q25so4986429pfh.7 for ; Thu, 17 Jun 2021 06:21:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rh15V+yte6Rxiqr6fw49zr2UYe598P6/xuIfukWNZxQ=; b=hNcQU9ZE/Lm/Ywq1FzsoA8JTnsh4Z+4Eo0qGwA7EFmPYPMzntrzpMy3GM5J1jhVHfO PSyTSjkgRelvNz/GGpXlRzKOla0hUz7oR79aXUgFlnkMsmIwjDSK+HgutEUE6hFKYP7q kftYT3L0n9FZYl3nE/tXdLf0ERQcEenCQfqWBdwgBAcOGpnkCZ5A4yZ1SQSiAB8M/5H4 tGPu7yi08U1UFQWY5tmzNLANe/xRN2WoUy63kMxvdbI7YkT8se0Z6RMv2VFuJIXyHvyI wFNgVraXFPe2riW5hO39mHhKyAlNpYAlc72E561EKIxTBRIgzRiOCpxc7pb81MqXWLyd PY/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rh15V+yte6Rxiqr6fw49zr2UYe598P6/xuIfukWNZxQ=; b=Thy0S0XDPFjs9kOJ0wR9N/yTzbK5Zto24uzeXlWSTNfHDbD3DsG6Jsy+NHul+3Kv3P v7W4HovWDTh1Yj/mI+T/KLTEeixuHPEcbhvB0iM7rmhS5+QeN/fZvPotYKC9h4oCbcsq 8/w9iT3rIZ9ThD7IhJCv4N3lZDKXPqKzcTVQagUjfwVvsxopt3/t7U3RWczYG1N8bjXg iWompcsbYCyC6+dOcxc4rppf5qzcIvgfuRIqk4/bGccO3Ang1yQrRfvLjeS7xbIxEyuQ 1OUY1j/g7MtjmJfUpT68HjSIE4ZFUwXwHLMxtkPKbO7ks5OyItxpBMfBtxZFH7iz9TjX CQuA== X-Gm-Message-State: AOAM532uxRHs+95GMuorjxZ4e4Rw3VM61sAjtECs6celA0oawa3PCdum yydm+Ye7ggQ25WAluu5fsZP7lluFGrmiz8w0y+F6vA== X-Google-Smtp-Source: ABdhPJwgd/hgqJ4nn+sU+PCoGAQ8LVil+JAYwyzRs1wf6uBHvyuPmzGowrpMjm4z96YyxBwF34O/Goh14K+EVQeqi5g= X-Received: by 2002:a05:6a00:15d4:b029:2ea:75b:60f5 with SMTP id o20-20020a056a0015d4b02902ea075b60f5mr5276590pfu.4.1623936068064; Thu, 17 Jun 2021 06:21:08 -0700 (PDT) MIME-Version: 1.0 References: <20210615132254.3364910-1-patrick.rudolph@9elements.com> In-Reply-To: From: "Patrick Rudolph" Date: Thu, 17 Jun 2021 15:20:56 +0200 Message-ID: Subject: Re: [PATCH] UefiPayloadPkg/UefiPayloadEntry: Improve bootloader memrange parsing To: "Ma, Maurice" Cc: "devel@edk2.groups.io" , "Dong, Guo" , "You, Benjamin" Content-Type: text/plain; charset="UTF-8" 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 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 > > Sent: Tuesday, June 15, 2021 6:23 > > To: devel@edk2.groups.io > > Cc: Ma, Maurice ; Dong, Guo > > ; You, Benjamin > > 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 > > --- > > .../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 >