| Message ID | A1484485FD99714DB2AB2C5EF81E7AC2AA7AE283@ORSMSX116.amr.corp.intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,1/1] x86: efi_loader: Fix invalid address return from efi_alloc() | expand |
+Heinrich, On Wed, Aug 28, 2019 at 2:35 AM Park, Aiden <aiden.park@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@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? > + 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
Hi Bin, > -----Original Message----- > From: Bin Meng [mailto:bmeng.cn@gmail.com] > Sent: Wednesday, August 28, 2019 8:37 PM > To: Park, Aiden <aiden.park@intel.com>; Heinrich Schuchardt > <xypron.glpk@gmx.de> > Cc: Simon Glass <sjg@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.park@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@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 Best Regards, Aiden
Hi Aiden, On Thu, Aug 29, 2019 at 12:02 PM Park, Aiden <aiden.park@intel.com> wrote: > > Hi Bin, > > > -----Original Message----- > > From: Bin Meng [mailto:bmeng.cn@gmail.com] > > Sent: Wednesday, August 28, 2019 8:37 PM > > To: Park, Aiden <aiden.park@intel.com>; Heinrich Schuchardt > > <xypron.glpk@gmx.de> > > Cc: Simon Glass <sjg@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.park@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@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
On 8/29/19 5:36 AM, Bin Meng wrote: > +Heinrich, > > On Wed, Aug 28, 2019 at 2:35 AM Park, Aiden <aiden.park@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@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? Best regards Heinrich > >> + } 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 >
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@intel.com> wrote: >> >> Hi Bin, >> >>> -----Original Message----- >>> From: Bin Meng [mailto:bmeng.cn@gmail.com] >>> Sent: Wednesday, August 28, 2019 8:37 PM >>> To: Park, Aiden <aiden.park@intel.com>; Heinrich Schuchardt >>> <xypron.glpk@gmx.de> >>> Cc: Simon Glass <sjg@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.park@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@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. Is this a U-Boot or a QEMU issue? Best regards Heinrich > > 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 >
Hi Heinrich, On Thu, Aug 29, 2019 at 1:25 PM Heinrich Schuchardt <xypron.glpk@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@intel.com> wrote: > >> > >> Hi Bin, > >> > >>> -----Original Message----- > >>> From: Bin Meng [mailto:bmeng.cn@gmail.com] > >>> Sent: Wednesday, August 28, 2019 8:37 PM > >>> To: Park, Aiden <aiden.park@intel.com>; Heinrich Schuchardt > >>> <xypron.glpk@gmx.de> > >>> Cc: Simon Glass <sjg@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.park@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@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
Hi Heinrich and Bin, > -----Original Message----- > From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de] > Sent: Wednesday, August 28, 2019 10:16 PM > To: Bin Meng <bmeng.cn@gmail.com>; Park, Aiden <aiden.park@intel.com> > Cc: Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de; Alexander Graf > <agraf@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.park@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@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. > Best regards > > Heinrich > > > > >> + } 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 > > Best Regards, Aiden
Hi Aiden, On Fri, Aug 30, 2019 at 5:31 AM Park, Aiden <aiden.park@intel.com> wrote: > > Hi Heinrich and Bin, > > > -----Original Message----- > > From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de] > > Sent: Wednesday, August 28, 2019 10:16 PM > > To: Bin Meng <bmeng.cn@gmail.com>; Park, Aiden <aiden.park@intel.com> > > Cc: Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de; Alexander Graf > > <agraf@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.park@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@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. But as Heinrich mentioned, there might be some room to refactor the codes a little bit to share as much common parts as possible. Regards, Bin
Hi Bin, > -----Original Message----- > From: Bin Meng [mailto:bmeng.cn@gmail.com] > Sent: Friday, August 30, 2019 12:22 AM > To: Park, Aiden <aiden.park@intel.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>; Simon Glass > <sjg@chromium.org>; u-boot@lists.denx.de; Alexander Graf > <agraf@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.park@intel.com> wrote: > > > > Hi Heinrich and Bin, > > > > > -----Original Message----- > > > From: Heinrich Schuchardt [mailto:xypron.glpk@gmx.de] > > > Sent: Wednesday, August 28, 2019 10:16 PM > > > To: Bin Meng <bmeng.cn@gmail.com>; Park, Aiden > > > <aiden.park@intel.com> > > > Cc: Simon Glass <sjg@chromium.org>; u-boot@lists.denx.de; Alexander > > > Graf <agraf@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.park@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@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
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) + 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); + } 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) */
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@intel.com> --- arch/x86/lib/e820.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)