Hi Bin, > -----Original Message----- > From: Bin Meng [mailto:bmeng...@gmail.com] > Sent: Friday, August 30, 2019 12:22 AM > To: Park, Aiden <aiden.p...@intel.com> > Cc: Heinrich Schuchardt <xypron.g...@gmx.de>; Simon Glass > <s...@chromium.org>; u-boot@lists.denx.de; Alexander Graf > <ag...@csgraf.de> > Subject: Re: [PATCH 1/1] x86: efi_loader: Fix invalid address return from > efi_alloc() > > Hi Aiden, > > On Fri, Aug 30, 2019 at 5:31 AM Park, Aiden <aiden.p...@intel.com> wrote: > > > > Hi Heinrich and Bin, > > > > > -----Original Message----- > > > From: Heinrich Schuchardt [mailto:xypron.g...@gmx.de] > > > Sent: Wednesday, August 28, 2019 10:16 PM > > > To: Bin Meng <bmeng...@gmail.com>; Park, Aiden > > > <aiden.p...@intel.com> > > > Cc: Simon Glass <s...@chromium.org>; u-boot@lists.denx.de; Alexander > > > Graf <ag...@csgraf.de> > > > Subject: Re: [PATCH 1/1] x86: efi_loader: Fix invalid address return > > > from > > > efi_alloc() > > > > > > On 8/29/19 5:36 AM, Bin Meng wrote: > > > > +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? > > > > > > If in 32bit mode RAM addresses up to 2^32-1 are available, > > > gd->ram_top is 0 due to overflow. > > > > > > > > > > >> + ram_top = 0x100000000ULL; > > > > > > In this special case we correct the value to 4GB. > > > > > > >> + > > > >> 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? > > > > > > These lines are verbatim copies of what we have in > lib/efi_loader/efi_memory.c. > > > Function wise this is ok. > > > > > > But this creates code duplication. Most of what is in this loop and > > > in the loop in lib/efi_loader/efi_memory.c (starting at /* Remove > > > partial pages */) could be put into a separate common function. > > > > > > Aiden, do you want to give a try? > > > > > I have a quick question here. Do we really need to override > > efi_add_known_memory() in arch/x86/lib/e820.c? > > I think the original weak function would be okay for x86 arch as well. > > > > Yep, I see the generic one provided by the EFI loader is using > gd->bd->bi_dram[i] to populate the EFI memory. The only handles > EFI_CONVENTIONAL_MEMORY, but for x86, it may have > EFI_ACPI_RECLAIM_MEMORY and EFI_ACPI_MEMORY_NVS which > gd->bd->bi_dram[i] does not distinguish normal memory usage and these > special ones. Hence I think we still need the x86 specific one. > Yeah Right. The regions can be corrupted as those can be reported as conventional memory even if efi_loader does not use those memory types. Thanks for clarification.
> But as Heinrich mentioned, there might be some room to refactor the codes > a little bit to share as much common parts as possible. > Let me refactor the common part. Thanks. > Regards, > Bin Best Regards, Aiden _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
