[U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()

[Bin Meng]

Hi Heinrich,

On Thu, Aug 29, 2019 at 1:25 PM Heinrich Schuchardt <xypron.glpk at 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.park at intel.com> wrote:
> >>
> >> Hi Bin,
> >>
> >>> -----Original Message-----
> >>> From: Bin Meng [mailto:bmeng.cn at gmail.com]
> >>> Sent: Wednesday, August 28, 2019 8:37 PM
> >>> To: Park, Aiden <aiden.park at intel.com>; Heinrich Schuchardt
> >>> <xypron.glpk at gmx.de>
> >>> Cc: Simon Glass <sjg at chromium.org>; u-boot at 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.park at 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.park at 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