Hi Heinrich, On Thu, Aug 29, 2019 at 1:25 PM Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 8/29/19 7:04 AM, Bin Meng wrote: > > Hi Aiden, > > > > On Thu, Aug 29, 2019 at 12:02 PM Park, Aiden <aiden.p...@intel.com> wrote: > >> > >> Hi Bin, > >> > >>> -----Original Message----- > >>> From: Bin Meng [mailto:bmeng...@gmail.com] > >>> Sent: Wednesday, August 28, 2019 8:37 PM > >>> To: Park, Aiden <aiden.p...@intel.com>; Heinrich Schuchardt > >>> <xypron.g...@gmx.de> > >>> Cc: Simon Glass <s...@chromium.org>; u-boot@lists.denx.de > >>> Subject: Re: [PATCH 1/1] x86: efi_loader: Fix invalid address return from > >>> efi_alloc() > >>> > >>> +Heinrich, > >>> > >>> On Wed, Aug 28, 2019 at 2:35 AM Park, Aiden <aiden.p...@intel.com> wrote: > >>>> > >>>> This issue can be seen on 32bit operation when one of E820_RAM type > >>>> entries is greater than 4GB memory space. > >>>> > >>>> The efi_alloc() finds a free memory in the conventional memory which > >>>> is greater than 4GB. But, it does type cast to 32bit address space and > >>>> eventually returns invalid address. > >>>> > >>>> Signed-off-by: Aiden Park <aiden.p...@intel.com> > >>>> --- > >>>> arch/x86/lib/e820.c | 22 +++++++++++++++++++++- > >>>> 1 file changed, 21 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/x86/lib/e820.c b/arch/x86/lib/e820.c index > >>>> d6ae2c4e9d..3e93931231 100644 > >>>> --- a/arch/x86/lib/e820.c > >>>> +++ b/arch/x86/lib/e820.c > >>>> @@ -41,11 +41,15 @@ void efi_add_known_memory(void) { > >>>> struct e820_entry e820[E820MAX]; > >>>> unsigned int i, num; > >>>> - u64 start, pages; > >>>> + u64 start, pages, ram_top; > >>>> int type; > >>>> > >>>> num = install_e820_map(ARRAY_SIZE(e820), e820); > >>>> > >>>> + ram_top = (u64)gd->ram_top & ~EFI_PAGE_MASK; > >>>> + if (!ram_top) > >>> > >>> So for the logic here to work, gd->ram_top is already zero in 32-bit, > >>> right? I was > >>> wondering how U-Boot could boot on such target? > >>> > >> The efi_add_known_memory() in lib/efi_loader/efi_memory.c covers this case. > >> > >>>> + ram_top = 0x100000000ULL; > >>>> + > >>>> for (i = 0; i < num; ++i) { > >>>> start = e820[i].addr; > >>>> pages = ALIGN(e820[i].size, EFI_PAGE_SIZE) >> > >>>> EFI_PAGE_SHIFT; @@ -70,6 +74,22 @@ void efi_add_known_memory(void) > >>>> } > >>>> > >>>> efi_add_memory_map(start, pages, type, false); > >>>> + > >>>> + if (type == EFI_CONVENTIONAL_MEMORY) { > >>>> + u64 end = start + (pages << EFI_PAGE_SHIFT); > >>>> + > >>>> + /* reserve the memory region greater than > >>>> ram_top */ > >>>> + if (ram_top < start) { > >>>> + efi_add_memory_map(start, pages, > >>>> + > >>>> EFI_BOOT_SERVICES_DATA, > >>>> + true); > >>> > >>> Heinrich, could you please review the changes here? > >>> > >>>> + } else if (start < ram_top && ram_top < end) { > >>>> + pages = (end - ram_top) >> > >>>> EFI_PAGE_SHIFT; > >>>> + efi_add_memory_map(ram_top, pages, > >>>> + > >>>> EFI_BOOT_SERVICES_DATA, > >>>> + true); > >>>> + } > >>>> + } > >>>> } > >>>> } > >>>> #endif /* CONFIG_IS_ENABLED(EFI_LOADER) */ > >>>> -- > >>> > >>> Regards, > >>> Bin > >> > >> I have replicated this issue with qemu-x86_defconfig as below. > >> > >> diff --git a/arch/x86/cpu/qemu/e820.c b/arch/x86/cpu/qemu/e820.c > >> index e682486547..7e5ae38c07 100644 > >> --- a/arch/x86/cpu/qemu/e820.c > >> +++ b/arch/x86/cpu/qemu/e820.c > >> @@ -42,5 +42,9 @@ unsigned int install_e820_map(unsigned int max_entries, > >> entries[5].size = CONFIG_PCIE_ECAM_SIZE; > >> entries[5].type = E820_RESERVED; > >> > >> - return 6; > >> + entries[6].addr = 0x100000000ULL; > >> + entries[6].size = 0x100000000ULL; > >> + entries[6].type = E820_RAM; > >> + > >> + return 7; > >> } > >> diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig > >> index e71b8a0ee1..2998d18bdd 100644 > >> --- a/configs/qemu-x86_defconfig > >> +++ b/configs/qemu-x86_defconfig > >> @@ -41,3 +41,4 @@ CONFIG_FRAMEBUFFER_SET_VESA_MODE=y > >> CONFIG_FRAMEBUFFER_VESA_MODE_USER=y > >> CONFIG_FRAMEBUFFER_VESA_MODE=0x144 > >> CONFIG_CONSOLE_SCROLL_LINES=5 > >> +CONFIG_CMD_BOOTEFI_HELLO=y > >> > >> $ qemu-system-i386 -nographic -bios u-boot.rom -m 8192 > >> => bootefi hello > > > > OK, thanks for the test case. However I believe this never broke QEMU x86. > > > > As in arch/x86/cpu/qemu/dram.c::dram_init(): > > > > gd->ram_size will be always set to 3GiB when "-m 4G" or more memory is > > specified for QEMU target, hence gd->ram_top is always set to 3GiB. > > Why would we have such an artificial limit? An LPAE kernel should work > fine with 8GB. >
It's what QEMU tells us. > Is this a U-Boot or a QEMU issue? Currently U-Boot only asks for low memory from QEMU. I will prepare a patch for high memory. Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot