Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
Hi Bin,
> -Original Message-
> From: Bin Meng [mailto:bmeng...@gmail.com]
> Sent: Friday, August 30, 2019 12:22 AM
> To: Park, Aiden
> Cc: Heinrich Schuchardt ; Simon Glass
> ; u-boot@lists.denx.de; Alexander Graf
>
> 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 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 ; Park, Aiden
> > >
> > > Cc: Simon Glass ; u-boot@lists.denx.de; Alexander
> > > Graf
> > > 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
> 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
> > > >> ---
> > > >> 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 = 0x1ULL;
> > >
> > > 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
Re: [U-Boot] [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 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 ; Park, Aiden
> > Cc: Simon Glass ; u-boot@lists.denx.de; Alexander Graf
> >
> > 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 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
> > >> ---
> > >> 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 = 0x1ULL;
> >
> > 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
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 ; Park, Aiden
> Cc: Simon Glass ; u-boot@lists.denx.de; Alexander Graf
>
> 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 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
> >> ---
> >> 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 = 0x1ULL;
>
> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
Hi Heinrich,
On Thu, Aug 29, 2019 at 1:25 PM Heinrich Schuchardt wrote:
>
> On 8/29/19 7:04 AM, Bin Meng wrote:
> > Hi Aiden,
> >
> > On Thu, Aug 29, 2019 at 12:02 PM Park, Aiden wrote:
> >>
> >> Hi Bin,
> >>
> >>> -Original Message-
> >>> From: Bin Meng [mailto:bmeng...@gmail.com]
> >>> Sent: Wednesday, August 28, 2019 8:37 PM
> >>> To: Park, Aiden ; Heinrich Schuchardt
> >>>
> >>> Cc: Simon Glass ; 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 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
> ---
> 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 = 0x1ULL;
> +
> 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 = 0x1ULL;
> >> + entries[6].size = 0x1ULL;
> >> + 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
Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
On 8/29/19 7:04 AM, Bin Meng wrote:
Hi Aiden,
On Thu, Aug 29, 2019 at 12:02 PM Park, Aiden wrote:
Hi Bin,
-Original Message-
From: Bin Meng [mailto:bmeng...@gmail.com]
Sent: Wednesday, August 28, 2019 8:37 PM
To: Park, Aiden ; Heinrich Schuchardt
Cc: Simon Glass ; 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 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
---
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 = 0x1ULL;
+
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 = 0x1ULL;
+ entries[6].size = 0x1ULL;
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [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 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
---
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 = 0x1ULL;
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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
Hi Aiden,
On Thu, Aug 29, 2019 at 12:02 PM Park, Aiden wrote:
>
> Hi Bin,
>
> > -Original Message-
> > From: Bin Meng [mailto:bmeng...@gmail.com]
> > Sent: Wednesday, August 28, 2019 8:37 PM
> > To: Park, Aiden ; Heinrich Schuchardt
> >
> > Cc: Simon Glass ; 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 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
> > > ---
> > > 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 = 0x1ULL;
> > > +
> > > 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 = 0x1ULL;
> + entries[6].size = 0x1ULL;
> + 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
Re: [U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
Hi Bin,
> -Original Message-
> From: Bin Meng [mailto:bmeng...@gmail.com]
> Sent: Wednesday, August 28, 2019 8:37 PM
> To: Park, Aiden ; Heinrich Schuchardt
>
> Cc: Simon Glass ; 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 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
> > ---
> > 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 = 0x1ULL;
> > +
> > 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 = 0x1ULL;
+ entries[6].size = 0x1ULL;
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [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 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
> ---
> 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 = 0x1ULL;
> +
> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
