[U-Boot] [PATCH 1/1] x86: efi_loader: Fix invalid address return from efi_alloc()
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) + 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) */
+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 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);}} }--
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);}} }--
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
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);}} }--
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);}} }--
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
participants (3)
-
Bin Meng -
Heinrich Schuchardt -
Park, Aiden