* [PATCH 0/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM @ 2017-07-11 3:22 Laszlo Ersek 2017-07-11 3:22 ` [PATCH 1/1] " Laszlo Ersek 2017-07-25 21:39 ` [PATCH 0/1] " Laszlo Ersek 0 siblings, 2 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-07-11 3:22 UTC (permalink / raw) To: edk2-devel-01 Cc: Dr. David Alan Gilbert, Gerd Hoffmann, Igor Mammedov, Jordan Justen The commit message says it all. BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1468526 Repo: https://github.com/lersek/edk2.git Branch: highram1tb Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Thanks Laszlo Laszlo Ersek (1): OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM OvmfPkg/PlatformPei/MemDetect.c | 161 +++++++++++++++++++- 1 file changed, 159 insertions(+), 2 deletions(-) -- 2.13.1.3.g8be5a757fa67 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-11 3:22 [PATCH 0/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM Laszlo Ersek @ 2017-07-11 3:22 ` Laszlo Ersek 2017-07-11 8:38 ` Igor Mammedov 2017-07-26 0:13 ` Jordan Justen 2017-07-25 21:39 ` [PATCH 0/1] " Laszlo Ersek 1 sibling, 2 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-07-11 3:22 UTC (permalink / raw) To: edk2-devel-01 Cc: Dr. David Alan Gilbert, Gerd Hoffmann, Igor Mammedov, Jordan Justen In OVMF we currently get the upper (>=4GB) memory size with the GetSystemMemorySizeAbove4gb() function. The GetSystemMemorySizeAbove4gb() function is used in two places: (1) It is the starting point of the calculations in GetFirstNonAddress(). GetFirstNonAddress() in turn - determines the placement of the 64-bit PCI MMIO aperture, - provides input for the GCD memory space map's sizing (see AddressWidthInitialization(), and the CPU HOB in MiscInitialization()), - influences the permanent PEI RAM cap (the DXE core's page tables, built in permanent PEI RAM, grow as the RAM to map grows). (2) In QemuInitializeRam(), GetSystemMemorySizeAbove4gb() determines the single memory descriptor HOB that we produce for the upper memory. Respectively, there are two problems with GetSystemMemorySizeAbove4gb(): (1) It reads a 24-bit count of 64KB RAM chunks from the CMOS, and therefore cannot return a larger value than one terabyte. (2) It cannot express discontiguous high RAM. Starting with version 1.7.0, QEMU has provided the fw_cfg file called "etc/e820". Refer to the following QEMU commits: - 0624c7f916b4 ("e820: pass high memory too.", 2013-10-10), - 7d67110f2d9a ("pc: add etc/e820 fw_cfg file", 2013-10-18) - 7db16f2480db ("pc: register e820 entries for ram", 2013-10-10) Ever since these commits in v1.7.0 -- with the last QEMU release being v2.9.0, and v2.10.0 under development --, the only two RAM entries added to this E820 map correspond to the below-4GB RAM range, and the above-4GB RAM range. And, the above-4GB range exactly matches the CMOS registers in question; see the use of "pcms->above_4g_mem_size": pc_q35_init() | pc_init1() pc_memory_init() e820_add_entry(0x100000000ULL, pcms->above_4g_mem_size, E820_RAM); pc_cmos_init() val = pcms->above_4g_mem_size / 65536; rtc_set_memory(s, 0x5b, val); rtc_set_memory(s, 0x5c, val >> 8); rtc_set_memory(s, 0x5d, val >> 16); Therefore, remedy the above OVMF limitations as follows: (1) Start off GetFirstNonAddress() by scanning the E820 map for the highest exclusive >=4GB RAM address. Fall back to the CMOS if the E820 map is unavailable. Base all further calculations (such as 64-bit PCI MMIO aperture placement, GCD sizing etc) on this value. At the moment, the only difference this change makes is that we can have more than 1TB above 4GB -- given that the sole "high RAM" entry in the E820 map matches the CMOS exactly, modulo the most significant bits (see above). However, Igor plans to add discontiguous (cold-plugged) high RAM to the fw_cfg E820 RAM map later on, and then this scanning will adapt automatically. (2) In QemuInitializeRam(), describe the high RAM regions from the E820 map one by one with memory HOBs. Fall back to the CMOS only if the E820 map is missing. Again, right now this change only makes a difference if there is at least 1TB high RAM. Later on it will adapt to discontiguous high RAM (regardless of its size) automatically. -*- Implementation details: introduce the E820HighRamIterate() function, which reads the E820 entries from fw_cfg, and calls the requested callback function on each high RAM entry found. The RAM map is not read in a single go, because its size can vary, and in PlatformPei we should stay away from dynamic memory allocation, for the following reasons: - "Pool" allocations are limited to ~64KB, are served from HOBs, and cannot be released ever. - "Page" allocations are seriously limited before PlatformPei installs the permanent PEI RAM. Furthermore, page allocations can only be released in DXE, with dedicated code (so the address would have to be passed on with a HOB or PCD). - Raw memory allocation HOBs would require the same freeing in DXE. Therefore we process each E820 entry as soon as it is read from fw_cfg. -*- Considering the impact of high RAM on the DXE core: A few years ago, installing high RAM as *tested* would cause the DXE core to inhabit such ranges rather than carving out its home from the permanent PEI RAM. Fortunately, this was fixed in the following edk2 commit: 3a05b13106d1, "MdeModulePkg DxeCore: Take the range in resource HOB for PHIT as higher priority", 2015-09-18 which I regression-tested at the time: http://mid.mail-archive.com/55FC27B0.4070807@redhat.com Later on, OVMF was changed to install its high RAM as tested (effectively "arming" the earlier DXE core change for OVMF), in the following edk2 commit: 035ce3b37c90, "OvmfPkg/PlatformPei: Add memory above 4GB as tested", 2016-04-21 which I also regression-tested at the time: http://mid.mail-archive.com/571E8B90.1020102@redhat.com Therefore adding more "tested memory" HOBs is safe. Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1468526 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/PlatformPei/MemDetect.c | 161 +++++++++++++++++++- 1 file changed, 159 insertions(+), 2 deletions(-) diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c index 97f3fa5afcf5..67e136252e1f 100644 --- a/OvmfPkg/PlatformPei/MemDetect.c +++ b/OvmfPkg/PlatformPei/MemDetect.c @@ -19,6 +19,7 @@ Module Name: // // The package level header files this module uses // +#include <IndustryStandard/E820.h> #include <IndustryStandard/Q35MchIch9.h> #include <PiPei.h> @@ -103,6 +104,142 @@ Q35TsegMbytesInitialization ( } +/** + Callback function for the high RAM entries in QEMU's fw_cfg E820 RAM map. + + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. + + @param[in,out] Context Opaque context object used while looping over the + RAM map. +**/ +typedef +VOID +(*E820_HIGH_RAM_ENTRY_CALLBACK) ( + IN CONST EFI_E820_ENTRY64 *HighRamEntry, + IN OUT VOID *Context + ); + + +/** + Iterate over the high RAM entries in QEMU's fw_cfg E820 RAM map. + + @param[in] Callback The callback function to pass each high RAM entry to. + + @param[in,out] Context Context to pass to Callback invariably on each + invocation. + + @retval EFI_SUCCESS The fw_cfg E820 RAM map was found and processed. + + @retval EFI_PROTOCOL_ERROR The RAM map was found, but its size wasn't a + whole multiple of sizeof(EFI_E820_ENTRY64). + Callback() was not invoked. + + @return Error codes from QemuFwCfgFindFile(). Callback() + was not invoked. +**/ +STATIC +EFI_STATUS +E820HighRamIterate ( + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, + IN OUT VOID *Context + ) +{ + EFI_STATUS Status; + FIRMWARE_CONFIG_ITEM FwCfgItem; + UINTN FwCfgSize; + EFI_E820_ENTRY64 E820Entry; + UINTN Processed; + + Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize); + if (EFI_ERROR (Status)) { + return Status; + } + if (FwCfgSize % sizeof E820Entry != 0) { + return EFI_PROTOCOL_ERROR; + } + + QemuFwCfgSelectItem (FwCfgItem); + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); + DEBUG (( + DEBUG_VERBOSE, + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", + __FUNCTION__, + E820Entry.BaseAddr, + E820Entry.Length, + E820Entry.Type + )); + if (E820Entry.Type == EfiAcpiAddressRangeMemory && + E820Entry.BaseAddr >= BASE_4GB) { + Callback (&E820Entry, Context); + } + } + return EFI_SUCCESS; +} + + +/** + Callback function for E820HighRamIterate() that finds the highest exclusive + >=4GB RAM address. + + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. + + @param[in,out] MaxAddress The highest exclusive >=4GB RAM address, + represented as a UINT64, that has been found thus + far in the search. Before calling + E820HighRamIterate(), the caller shall set + MaxAddress to BASE_4GB. When E820HighRamIterate() + returns with success, MaxAddress holds the highest + exclusive >=4GB RAM address. +**/ +VOID +E820HighRamFindHighestExclusiveAddress ( + IN CONST EFI_E820_ENTRY64 *HighRamEntry, + IN OUT VOID *MaxAddress + ) +{ + UINT64 *Current; + UINT64 Candidate; + + Current = MaxAddress; + Candidate = HighRamEntry->BaseAddr + HighRamEntry->Length; + if (Candidate > *Current) { + *Current = Candidate; + DEBUG ((DEBUG_VERBOSE, "%a: MaxAddress=0x%Lx\n", __FUNCTION__, *Current)); + } +} + + +/** + Callback function for E820HighRamIterate() that produces memory resource + descriptor HOBs. + + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. + + @param[in,out] Context Ignored. +**/ +VOID +E820HighRamAddMemoryHob ( + IN CONST EFI_E820_ENTRY64 *HighRamEntry, + IN OUT VOID *Context + ) +{ + UINT64 Base; + UINT64 End; + + // + // Round up the start address, and round down the end address. + // + Base = ALIGN_VALUE (HighRamEntry->BaseAddr, (UINT64)EFI_PAGE_SIZE); + End = (HighRamEntry->BaseAddr + HighRamEntry->Length) & + ~(UINT64)EFI_PAGE_MASK; + if (Base < End) { + AddMemoryRangeHob (Base, End); + DEBUG ((DEBUG_VERBOSE, "%a: [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End)); + } +} + + UINT32 GetSystemMemorySizeBelow4gb ( VOID @@ -170,7 +307,21 @@ GetFirstNonAddress ( UINT64 HotPlugMemoryEnd; RETURN_STATUS PcdStatus; - FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb (); + // + // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM + // address from it. This can express an address >= 4GB+1TB. + // + // Otherwise, get the flat size of the memory above 4GB from the CMOS (which + // can only express a size smaller than 1TB), and add it to 4GB. + // + FirstNonAddress = BASE_4GB; + Status = E820HighRamIterate ( + E820HighRamFindHighestExclusiveAddress, + &FirstNonAddress + ); + if (EFI_ERROR (Status)) { + FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb (); + } // // If DXE is 32-bit, then we're done; PciBusDxe will degrade 64-bit MMIO @@ -525,7 +676,13 @@ QemuInitializeRam ( AddMemoryRangeHob (BASE_1MB, LowerMemorySize); } - if (UpperMemorySize != 0) { + // + // If QEMU presents an E820 map, then create memory HOBs for the >=4GB RAM + // entries. Otherwise, create a single memory HOB with the flat >=4GB + // memory size read from the CMOS. + // + Status = E820HighRamIterate (E820HighRamAddMemoryHob, NULL); + if (EFI_ERROR (Status) && UpperMemorySize != 0) { AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize); } } -- 2.13.1.3.g8be5a757fa67 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-11 3:22 ` [PATCH 1/1] " Laszlo Ersek @ 2017-07-11 8:38 ` Igor Mammedov 2017-07-11 13:10 ` Laszlo Ersek 2017-07-26 0:13 ` Jordan Justen 1 sibling, 1 reply; 9+ messages in thread From: Igor Mammedov @ 2017-07-11 8:38 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel-01, Dr. David Alan Gilbert, Gerd Hoffmann, Jordan Justen On Tue, 11 Jul 2017 05:22:31 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > In OVMF we currently get the upper (>=4GB) memory size with the > GetSystemMemorySizeAbove4gb() function. > > The GetSystemMemorySizeAbove4gb() function is used in two places: > > (1) It is the starting point of the calculations in GetFirstNonAddress(). > GetFirstNonAddress() in turn > - determines the placement of the 64-bit PCI MMIO aperture, that's probably broken in memory hotplug case as the first usable PCI MMIO address is at fwcfg("etc/reserved-memory-end") though the file exists only when memory hotplug is enabled ( to make life of FW writers harder :/ ) > - provides input for the GCD memory space map's sizing (see > AddressWidthInitialization(), and the CPU HOB in > MiscInitialization()), > - influences the permanent PEI RAM cap (the DXE core's page tables, > built in permanent PEI RAM, grow as the RAM to map grows). > > (2) In QemuInitializeRam(), GetSystemMemorySizeAbove4gb() determines the > single memory descriptor HOB that we produce for the upper memory. > > Respectively, there are two problems with GetSystemMemorySizeAbove4gb(): > > (1) It reads a 24-bit count of 64KB RAM chunks from the CMOS, and > therefore cannot return a larger value than one terabyte. > > (2) It cannot express discontiguous high RAM. > > Starting with version 1.7.0, QEMU has provided the fw_cfg file called > "etc/e820". Refer to the following QEMU commits: > > - 0624c7f916b4 ("e820: pass high memory too.", 2013-10-10), > - 7d67110f2d9a ("pc: add etc/e820 fw_cfg file", 2013-10-18) > - 7db16f2480db ("pc: register e820 entries for ram", 2013-10-10) > > Ever since these commits in v1.7.0 -- with the last QEMU release being > v2.9.0, and v2.10.0 under development --, the only two RAM entries added > to this E820 map correspond to the below-4GB RAM range, and the above-4GB > RAM range. And, the above-4GB range exactly matches the CMOS registers in > question; see the use of "pcms->above_4g_mem_size": > > pc_q35_init() | pc_init1() > pc_memory_init() > e820_add_entry(0x100000000ULL, pcms->above_4g_mem_size, E820_RAM); > pc_cmos_init() > val = pcms->above_4g_mem_size / 65536; > rtc_set_memory(s, 0x5b, val); > rtc_set_memory(s, 0x5c, val >> 8); > rtc_set_memory(s, 0x5d, val >> 16); > > Therefore, remedy the above OVMF limitations as follows: > > (1) Start off GetFirstNonAddress() by scanning the E820 map for the > highest exclusive >=4GB RAM address. Fall back to the CMOS if the E820 > map is unavailable. Base all further calculations (such as 64-bit PCI > MMIO aperture placement, GCD sizing etc) on this value. > > At the moment, the only difference this change makes is that we can > have more than 1TB above 4GB -- given that the sole "high RAM" entry > in the E820 map matches the CMOS exactly, modulo the most significant > bits (see above). > > However, Igor plans to add discontiguous (cold-plugged) high RAM to > the fw_cfg E820 RAM map later on, and then this scanning will adapt > automatically. > > (2) In QemuInitializeRam(), describe the high RAM regions from the E820 > map one by one with memory HOBs. Fall back to the CMOS only if the > E820 map is missing. > > Again, right now this change only makes a difference if there is at > least 1TB high RAM. Later on it will adapt to discontiguous high RAM > (regardless of its size) automatically. > > -*- > > Implementation details: introduce the E820HighRamIterate() function, which > reads the E820 entries from fw_cfg, and calls the requested callback > function on each high RAM entry found. The RAM map is not read in a single > go, because its size can vary, and in PlatformPei we should stay away from > dynamic memory allocation, for the following reasons: > > - "Pool" allocations are limited to ~64KB, are served from HOBs, and > cannot be released ever. > > - "Page" allocations are seriously limited before PlatformPei installs the > permanent PEI RAM. Furthermore, page allocations can only be released in > DXE, with dedicated code (so the address would have to be passed on with > a HOB or PCD). > > - Raw memory allocation HOBs would require the same freeing in DXE. > > Therefore we process each E820 entry as soon as it is read from fw_cfg. > > -*- > > Considering the impact of high RAM on the DXE core: > > A few years ago, installing high RAM as *tested* would cause the DXE core > to inhabit such ranges rather than carving out its home from the permanent > PEI RAM. Fortunately, this was fixed in the following edk2 commit: > > 3a05b13106d1, "MdeModulePkg DxeCore: Take the range in resource HOB for > PHIT as higher priority", 2015-09-18 > > which I regression-tested at the time: > > http://mid.mail-archive.com/55FC27B0.4070807@redhat.com > > Later on, OVMF was changed to install its high RAM as tested (effectively > "arming" the earlier DXE core change for OVMF), in the following edk2 > commit: > > 035ce3b37c90, "OvmfPkg/PlatformPei: Add memory above 4GB as tested", > 2016-04-21 > > which I also regression-tested at the time: > > http://mid.mail-archive.com/571E8B90.1020102@redhat.com > > Therefore adding more "tested memory" HOBs is safe. > > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1468526 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > OvmfPkg/PlatformPei/MemDetect.c | 161 +++++++++++++++++++- > 1 file changed, 159 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index 97f3fa5afcf5..67e136252e1f 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -19,6 +19,7 @@ Module Name: > // > // The package level header files this module uses > // > +#include <IndustryStandard/E820.h> > #include <IndustryStandard/Q35MchIch9.h> > #include <PiPei.h> > > @@ -103,6 +104,142 @@ Q35TsegMbytesInitialization ( > } > > > +/** > + Callback function for the high RAM entries in QEMU's fw_cfg E820 RAM map. > + > + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. > + > + @param[in,out] Context Opaque context object used while looping over the > + RAM map. > +**/ > +typedef > +VOID > +(*E820_HIGH_RAM_ENTRY_CALLBACK) ( > + IN CONST EFI_E820_ENTRY64 *HighRamEntry, > + IN OUT VOID *Context > + ); > + > + > +/** > + Iterate over the high RAM entries in QEMU's fw_cfg E820 RAM map. > + > + @param[in] Callback The callback function to pass each high RAM entry to. > + > + @param[in,out] Context Context to pass to Callback invariably on each > + invocation. > + > + @retval EFI_SUCCESS The fw_cfg E820 RAM map was found and processed. > + > + @retval EFI_PROTOCOL_ERROR The RAM map was found, but its size wasn't a > + whole multiple of sizeof(EFI_E820_ENTRY64). > + Callback() was not invoked. > + > + @return Error codes from QemuFwCfgFindFile(). Callback() > + was not invoked. > +**/ > +STATIC > +EFI_STATUS > +E820HighRamIterate ( > + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > + IN OUT VOID *Context > + ) > +{ > + EFI_STATUS Status; > + FIRMWARE_CONFIG_ITEM FwCfgItem; > + UINTN FwCfgSize; > + EFI_E820_ENTRY64 E820Entry; > + UINTN Processed; > + > + Status = QemuFwCfgFindFile ("etc/e820", &FwCfgItem, &FwCfgSize); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + if (FwCfgSize % sizeof E820Entry != 0) { > + return EFI_PROTOCOL_ERROR; > + } > + > + QemuFwCfgSelectItem (FwCfgItem); > + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { > + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); > + DEBUG (( > + DEBUG_VERBOSE, > + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", > + __FUNCTION__, > + E820Entry.BaseAddr, > + E820Entry.Length, > + E820Entry.Type > + )); > + if (E820Entry.Type == EfiAcpiAddressRangeMemory && > + E820Entry.BaseAddr >= BASE_4GB) { > + Callback (&E820Entry, Context); > + } > + } > + return EFI_SUCCESS; > +} > + > + > +/** > + Callback function for E820HighRamIterate() that finds the highest exclusive > + >=4GB RAM address. > + > + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. > + > + @param[in,out] MaxAddress The highest exclusive >=4GB RAM address, > + represented as a UINT64, that has been found thus > + far in the search. Before calling > + E820HighRamIterate(), the caller shall set > + MaxAddress to BASE_4GB. When E820HighRamIterate() > + returns with success, MaxAddress holds the highest > + exclusive >=4GB RAM address. > +**/ > +VOID > +E820HighRamFindHighestExclusiveAddress ( > + IN CONST EFI_E820_ENTRY64 *HighRamEntry, > + IN OUT VOID *MaxAddress > + ) > +{ > + UINT64 *Current; > + UINT64 Candidate; > + > + Current = MaxAddress; > + Candidate = HighRamEntry->BaseAddr + HighRamEntry->Length; > + if (Candidate > *Current) { > + *Current = Candidate; > + DEBUG ((DEBUG_VERBOSE, "%a: MaxAddress=0x%Lx\n", __FUNCTION__, *Current)); > + } > +} > + > + > +/** > + Callback function for E820HighRamIterate() that produces memory resource > + descriptor HOBs. > + > + @param[in] HighRamEntry The EFI_E820_ENTRY64 structure to process. > + > + @param[in,out] Context Ignored. > +**/ > +VOID > +E820HighRamAddMemoryHob ( > + IN CONST EFI_E820_ENTRY64 *HighRamEntry, > + IN OUT VOID *Context > + ) > +{ > + UINT64 Base; > + UINT64 End; > + > + // > + // Round up the start address, and round down the end address. > + // > + Base = ALIGN_VALUE (HighRamEntry->BaseAddr, (UINT64)EFI_PAGE_SIZE); > + End = (HighRamEntry->BaseAddr + HighRamEntry->Length) & > + ~(UINT64)EFI_PAGE_MASK; > + if (Base < End) { > + AddMemoryRangeHob (Base, End); > + DEBUG ((DEBUG_VERBOSE, "%a: [0x%Lx, 0x%Lx)\n", __FUNCTION__, Base, End)); > + } > +} > + > + > UINT32 > GetSystemMemorySizeBelow4gb ( > VOID > @@ -170,7 +307,21 @@ GetFirstNonAddress ( > UINT64 HotPlugMemoryEnd; > RETURN_STATUS PcdStatus; > > - FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb (); > + // > + // If QEMU presents an E820 map, then get the highest exclusive >=4GB RAM > + // address from it. This can express an address >= 4GB+1TB. > + // > + // Otherwise, get the flat size of the memory above 4GB from the CMOS (which > + // can only express a size smaller than 1TB), and add it to 4GB. > + // > + FirstNonAddress = BASE_4GB; > + Status = E820HighRamIterate ( > + E820HighRamFindHighestExclusiveAddress, > + &FirstNonAddress > + ); > + if (EFI_ERROR (Status)) { > + FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb (); > + } > > // > // If DXE is 32-bit, then we're done; PciBusDxe will degrade 64-bit MMIO > @@ -525,7 +676,13 @@ QemuInitializeRam ( > AddMemoryRangeHob (BASE_1MB, LowerMemorySize); > } > > - if (UpperMemorySize != 0) { > + // > + // If QEMU presents an E820 map, then create memory HOBs for the >=4GB RAM > + // entries. Otherwise, create a single memory HOB with the flat >=4GB > + // memory size read from the CMOS. > + // > + Status = E820HighRamIterate (E820HighRamAddMemoryHob, NULL); > + if (EFI_ERROR (Status) && UpperMemorySize != 0) { > AddMemoryBaseSizeHob (BASE_4GB, UpperMemorySize); > } > } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-11 8:38 ` Igor Mammedov @ 2017-07-11 13:10 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-07-11 13:10 UTC (permalink / raw) To: Igor Mammedov Cc: edk2-devel-01, Dr. David Alan Gilbert, Gerd Hoffmann, Jordan Justen On 07/11/17 10:38, Igor Mammedov wrote: > On Tue, 11 Jul 2017 05:22:31 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: > >> In OVMF we currently get the upper (>=4GB) memory size with the >> GetSystemMemorySizeAbove4gb() function. >> >> The GetSystemMemorySizeAbove4gb() function is used in two places: >> >> (1) It is the starting point of the calculations in GetFirstNonAddress(). >> GetFirstNonAddress() in turn >> - determines the placement of the 64-bit PCI MMIO aperture, > that's probably broken in memory hotplug case as > the first usable PCI MMIO address is at fwcfg("etc/reserved-memory-end") No, it's not broken :) I wrote above, "starting point of the calculations". The fw_cfg file "etc/reserved-memory-end" is specifically considered already. (You explained that file to me when I previously worked on this code, in 2016Q1 or so.) It's just a later point in the calculation. > though the file exists only when memory hotplug is enabled > ( to make life of FW writers harder :/ ) No problem, OVMF deals with "etc/reserved-memory-end" being either present or absent. Basically the GetFirstNonAddress() function performs a "cascade of increases" upwards, for determining the highest address plus one (i.e., the first non-address). If you'd like to see the full function (with this patch applied), it starts at: https://github.com/lersek/edk2/blob/highram1tb/OvmfPkg/PlatformPei/MemDetect.c#L292 It is heavily commented. Thanks! Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-11 3:22 ` [PATCH 1/1] " Laszlo Ersek 2017-07-11 8:38 ` Igor Mammedov @ 2017-07-26 0:13 ` Jordan Justen 2017-07-26 16:23 ` Laszlo Ersek 1 sibling, 1 reply; 9+ messages in thread From: Jordan Justen @ 2017-07-26 0:13 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Dr. David Alan Gilbert On 2017-07-10 20:22:31, Laszlo Ersek wrote: > +STATIC > +EFI_STATUS > +E820HighRamIterate ( > + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > + IN OUT VOID *Context > + ) I think a simpler option would be: STATIC EFI_STATUS ScanOrAddE820HighRam ( IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, OUT UINT64 *MaxAddress OPTIONAL ) If MaxAddress != NULL, then scan for it, otherwise add HOBs. Do you anticipate future needs where the iterate callback could be helpful? You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that the 'HighRam' is addresses that don't fit in 32 bits. > + QemuFwCfgSelectItem (FwCfgItem); > + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { > + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); > + DEBUG (( > + DEBUG_VERBOSE, > + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", > + __FUNCTION__, > + E820Entry.BaseAddr, > + E820Entry.Length, > + E820Entry.Type > + )); > + if (E820Entry.Type == EfiAcpiAddressRangeMemory && > + E820Entry.BaseAddr >= BASE_4GB) { I guess at least for IA32/X64, today, and for the foreseeable future the firmware device will cause a break at 4GB. It seems like we could just check for the end of the range to be above 4GB to easily remove this assumption, right? -Jordan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-26 0:13 ` Jordan Justen @ 2017-07-26 16:23 ` Laszlo Ersek 2017-08-04 8:50 ` Jordan Justen 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-07-26 16:23 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Dr. David Alan Gilbert On 07/26/17 02:13, Jordan Justen wrote: > On 2017-07-10 20:22:31, Laszlo Ersek wrote: >> +STATIC >> +EFI_STATUS >> +E820HighRamIterate ( >> + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, >> + IN OUT VOID *Context >> + ) > > I think a simpler option would be: > > STATIC > EFI_STATUS > ScanOrAddE820HighRam ( > IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > OUT UINT64 *MaxAddress OPTIONAL > ) > > If MaxAddress != NULL, then scan for it, otherwise add HOBs. > > Do you anticipate future needs where the iterate callback could be > helpful? Not at the moment. Originally I started with two open-coded loops, but the code duplication in this case was really ugly. Using callbacks simplified the call sites very nicely. And, I was a bit concerned that you wouldn't like a solution that wasn't generic enough :) I can rework the function like suggested if you prefer that. > > You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that > the 'HighRam' is addresses that don't fit in 32 bits. OK. > >> + QemuFwCfgSelectItem (FwCfgItem); >> + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { >> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); >> + DEBUG (( >> + DEBUG_VERBOSE, >> + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", >> + __FUNCTION__, >> + E820Entry.BaseAddr, >> + E820Entry.Length, >> + E820Entry.Type >> + )); >> + if (E820Entry.Type == EfiAcpiAddressRangeMemory && >> + E820Entry.BaseAddr >= BASE_4GB) { > > I guess at least for IA32/X64, today, and for the foreseeable future > the firmware device will cause a break at 4GB. Sorry, I don't understand. What do you mean by "firmware device" and "break at 4GB"? > It seems like we could just check for the end of the range to be above > 4GB to easily remove this assumption, right? I wanted to ensure that no RAM range would be included that (for any unexpected reason) straddled the 4GB mark. Do you mean that the pflash address range is guaranteed to end at 4GB (exclusive), so no RAM range can straddle the mark? Even so, what expression do you have in mind exactly? As far as I can imagine, checking the end vs. the base of the E820 entry would only complicate the above expression, not simplify it. Please elaborate :) Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-26 16:23 ` Laszlo Ersek @ 2017-08-04 8:50 ` Jordan Justen 2017-08-04 20:04 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Jordan Justen @ 2017-08-04 8:50 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel-01; +Cc: Dr. David Alan Gilbert On 2017-07-26 09:23:26, Laszlo Ersek wrote: > On 07/26/17 02:13, Jordan Justen wrote: > > On 2017-07-10 20:22:31, Laszlo Ersek wrote: > >> +STATIC > >> +EFI_STATUS > >> +E820HighRamIterate ( > >> + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > >> + IN OUT VOID *Context > >> + ) > > > > I think a simpler option would be: > > > > STATIC > > EFI_STATUS > > ScanOrAddE820HighRam ( > > IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, > > OUT UINT64 *MaxAddress OPTIONAL > > ) > > > > If MaxAddress != NULL, then scan for it, otherwise add HOBs. > > > > Do you anticipate future needs where the iterate callback could be > > helpful? > > Not at the moment. > > Originally I started with two open-coded loops, but the code duplication > in this case was really ugly. Using callbacks simplified the call sites > very nicely. And, I was a bit concerned that you wouldn't like a > solution that wasn't generic enough :) > > I can rework the function like suggested if you prefer that. I moved the code of the 2 callbacks into the the function and dropped the callbacks to see how it looked. It seemed a bit clearer and was less code. The callback almost seems like something to consider for fw-cfg lib, except I don't think we really have the need today. > > > > You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that > > the 'HighRam' is addresses that don't fit in 32 bits. > > OK. > > > > >> + QemuFwCfgSelectItem (FwCfgItem); > >> + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { > >> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); > >> + DEBUG (( > >> + DEBUG_VERBOSE, > >> + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", > >> + __FUNCTION__, > >> + E820Entry.BaseAddr, > >> + E820Entry.Length, > >> + E820Entry.Type > >> + )); > >> + if (E820Entry.Type == EfiAcpiAddressRangeMemory && > >> + E820Entry.BaseAddr >= BASE_4GB) { > > > > I guess at least for IA32/X64, today, and for the foreseeable future > > the firmware device will cause a break at 4GB. > > Sorry, I don't understand. What do you mean by "firmware device" and > "break at 4GB"? > > > It seems like we could just check for the end of the range to be above > > 4GB to easily remove this assumption, right? > > I wanted to ensure that no RAM range would be included that (for any > unexpected reason) straddled the 4GB mark. > > Do you mean that the pflash address range is guaranteed to end at 4GB > (exclusive), so no RAM range can straddle the mark? > > Even so, what expression do you have in mind exactly? As far as I can > imagine, checking the end vs. the base of the E820 entry would only > complicate the above expression, not simplify it. > > Please elaborate :) Yeah, I meant is there any easy way to handle a situation (unlike qemu piix4/q35 systems) where a RAM region went from below 4GB to above 4GB. If you don't think there is a simple was to handle it, then don't worry about it. -Jordan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-08-04 8:50 ` Jordan Justen @ 2017-08-04 20:04 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-08-04 20:04 UTC (permalink / raw) To: Jordan Justen, edk2-devel-01; +Cc: Dr. David Alan Gilbert On 08/04/17 10:50, Jordan Justen wrote: > On 2017-07-26 09:23:26, Laszlo Ersek wrote: >> On 07/26/17 02:13, Jordan Justen wrote: >>> On 2017-07-10 20:22:31, Laszlo Ersek wrote: >>>> +STATIC >>>> +EFI_STATUS >>>> +E820HighRamIterate ( >>>> + IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, >>>> + IN OUT VOID *Context >>>> + ) >>> >>> I think a simpler option would be: >>> >>> STATIC >>> EFI_STATUS >>> ScanOrAddE820HighRam ( >>> IN E820_HIGH_RAM_ENTRY_CALLBACK Callback, >>> OUT UINT64 *MaxAddress OPTIONAL >>> ) >>> >>> If MaxAddress != NULL, then scan for it, otherwise add HOBs. >>> >>> Do you anticipate future needs where the iterate callback could be >>> helpful? >> >> Not at the moment. >> >> Originally I started with two open-coded loops, but the code duplication >> in this case was really ugly. Using callbacks simplified the call sites >> very nicely. And, I was a bit concerned that you wouldn't like a >> solution that wasn't generic enough :) >> >> I can rework the function like suggested if you prefer that. > > I moved the code of the 2 callbacks into the the function and dropped > the callbacks to see how it looked. It seemed a bit clearer and was > less code. OK, I'll do that. > > The callback almost seems like something to consider for fw-cfg lib, > except I don't think we really have the need today. I agree that we don't need it now. > >>> >>> You might also consider ScanOrAdd64BitE820Ram to somewhat clarify that >>> the 'HighRam' is addresses that don't fit in 32 bits. >> >> OK. >> >>> >>>> + QemuFwCfgSelectItem (FwCfgItem); >>>> + for (Processed = 0; Processed < FwCfgSize; Processed += sizeof E820Entry) { >>>> + QemuFwCfgReadBytes (sizeof E820Entry, &E820Entry); >>>> + DEBUG (( >>>> + DEBUG_VERBOSE, >>>> + "%a: Base=0x%Lx Length=0x%Lx Type=%u\n", >>>> + __FUNCTION__, >>>> + E820Entry.BaseAddr, >>>> + E820Entry.Length, >>>> + E820Entry.Type >>>> + )); >>>> + if (E820Entry.Type == EfiAcpiAddressRangeMemory && >>>> + E820Entry.BaseAddr >= BASE_4GB) { >>> >>> I guess at least for IA32/X64, today, and for the foreseeable future >>> the firmware device will cause a break at 4GB. >> >> Sorry, I don't understand. What do you mean by "firmware device" and >> "break at 4GB"? >> >>> It seems like we could just check for the end of the range to be above >>> 4GB to easily remove this assumption, right? >> >> I wanted to ensure that no RAM range would be included that (for any >> unexpected reason) straddled the 4GB mark. >> >> Do you mean that the pflash address range is guaranteed to end at 4GB >> (exclusive), so no RAM range can straddle the mark? >> >> Even so, what expression do you have in mind exactly? As far as I can >> imagine, checking the end vs. the base of the E820 entry would only >> complicate the above expression, not simplify it. >> >> Please elaborate :) > > Yeah, I meant is there any easy way to handle a situation (unlike qemu > piix4/q35 systems) where a RAM region went from below 4GB to above > 4GB. If you don't think there is a simple was to handle it, then don't > worry about it. OK, thanks. I think it's out of scope for now. Thanks Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM 2017-07-11 3:22 [PATCH 0/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM Laszlo Ersek 2017-07-11 3:22 ` [PATCH 1/1] " Laszlo Ersek @ 2017-07-25 21:39 ` Laszlo Ersek 1 sibling, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-07-25 21:39 UTC (permalink / raw) To: Jordan Justen; +Cc: edk2-devel-01, Dr. David Alan Gilbert Jordan, can you please ack this? Thanks Laszlo On 07/11/17 05:22, Laszlo Ersek wrote: > The commit message says it all. > > BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1468526 > Repo: https://github.com/lersek/edk2.git > Branch: highram1tb > > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Thanks > Laszlo > > Laszlo Ersek (1): > OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high > RAM > > OvmfPkg/PlatformPei/MemDetect.c | 161 +++++++++++++++++++- > 1 file changed, 159 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-04 20:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-11 3:22 [PATCH 0/1] OvmfPkg/PlatformPei: support >=1TB high RAM, and discontiguous high RAM Laszlo Ersek 2017-07-11 3:22 ` [PATCH 1/1] " Laszlo Ersek 2017-07-11 8:38 ` Igor Mammedov 2017-07-11 13:10 ` Laszlo Ersek 2017-07-26 0:13 ` Jordan Justen 2017-07-26 16:23 ` Laszlo Ersek 2017-08-04 8:50 ` Jordan Justen 2017-08-04 20:04 ` Laszlo Ersek 2017-07-25 21:39 ` [PATCH 0/1] " Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox