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. So it never happens in QEMU. I think you encountered an issue on real hardware. Shouldn't we fix gd->ram_top instead? Regards, Bn _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
