Discussion:
[PATCH v2] mbi: use per segment a separate relocator chunk
Alexander Boettcher
2018-06-05 20:14:49 UTC
Permalink
--
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
GeschÀftsfÌhrer: Dr.-Ing. Norman Feske, Christian Helmuth
Daniel Kiper
2018-06-11 20:09:06 UTC
Permalink
Instead of setting up a all comprising relocator chunk for all segments,
use per segment a separate relocator chunk.
If the ELF is non-relocatable, a single relocator chunk will comprise memory
(between the segments) which gets overridden by the relst() invocation of the
movers code in grub_relocator16/32/64_boot().
The overridden memory may contain reserved ranges like VGA memory or ACPI
tables, which leads to strange boot behaviour.
---
grub-core/loader/multiboot_elfxx.c | 60 +++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 67daf59..6565b50 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
char *phdr_base;
grub_err_t err;
grub_relocator_chunk_t ch;
- grub_uint32_t load_offset, load_size;
+ grub_uint32_t load_offset = 0, load_size;
int i;
- void *source;
+ void *source = NULL;
if (ehdr->e_ident[EI_MAG0] != ELFMAG0
|| ehdr->e_ident[EI_MAG1] != ELFMAG1
@@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
#define phdr(i) ((Elf_Phdr *) (phdr_base + (i) * ehdr->e_phentsize))
mld->link_base_addr = ~0;
+ mld->load_base_addr = ~0;
We do not need it. Please look below.
/* Calculate lowest and highest load address. */
for (i = 0; i < ehdr->e_phnum; i++)
@@ -97,10 +98,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
#endif
- load_size = highest_load - mld->link_base_addr;
+ grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, relocatable=%d, "
+ "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
+ mld->link_base_addr, mld->relocatable, (long) mld->align,
+ mld->preference, mld->avoid_efi_boot_services);
I have just realized that it does not make sense to print align,
preference and avoid_efi_boot_services if mld->relocatable is not
set. Please take a look at grub-core/loader/multiboot_mbi2.c for
more details. Sorry about that. In this case you can probably drop
this grub_dprintf() and...
if (mld->relocatable)
{
+ load_size = highest_load - mld->link_base_addr;
+
if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - load_size)
return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or load size");
@@ -108,27 +114,16 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
mld->min_addr, mld->max_addr - load_size,
load_size, mld->align ? mld->align : 1,
mld->preference, mld->avoid_efi_boot_services);
- }
- else
Please do not drop this else. You can put this here:
mld->load_base_addr = mld->link_base_addr;

load_base_addr == link_base_addr in non relocatable case.
Am I right?
- err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
- mld->link_base_addr, load_size);
-
- if (err)
- {
- grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
- return err;
- }
- mld->load_base_addr = get_physical_target_address (ch);
- source = get_virtual_current_address (ch);
-
- grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, "
- "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
- mld->load_base_addr, load_size, mld->relocatable);
...you can leave this excluding load_size and...
+ if (err)
+ {
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+ }
- if (mld->relocatable)
- grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
- (long) mld->align, mld->preference, mld->avoid_efi_boot_services);
...you can leave this grub_dprintf() and move load_size from above.
You can put it after preference.
+ mld->load_base_addr = get_physical_target_address (ch);
+ source = get_virtual_current_address (ch);
+ }
/* Load every loadable segment in memory. */
for (i = 0; i < ehdr->e_phnum; i++)
@@ -139,7 +134,26 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
- load_offset = phdr(i)->p_paddr - mld->link_base_addr;
+ if (mld->relocatable)
+ load_offset = phdr(i)->p_paddr - mld->link_base_addr;
Please add this here:
grub_dprintf ("multiboot_loader", "segment %d: load_offset=0x%x\n", i, load_offset);
+ else
+ {
+ err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
+ phdr(i)->p_paddr, phdr(i)->p_memsz);
+
+ if (err)
+ {
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+ }
+
+ mld->load_base_addr = grub_min (mld->load_base_addr, get_physical_target_address (ch));
You can drop this. Please look above.
+
+ source = get_virtual_current_address (ch);
+ }
+
+ grub_dprintf ("multiboot_loader", "segment %d: load_base_addr=0x%x, "
+ "load_offset=0x%x\n", i, mld->load_base_addr, load_offset);
Please drop this grub_dprintf().

Daniel
Alexander Boettcher
2018-06-12 18:18:19 UTC
Permalink
Agree, resend it as v3 patch.
Post by Daniel Kiper
Instead of setting up a all comprising relocator chunk for all segments,
use per segment a separate relocator chunk.
If the ELF is non-relocatable, a single relocator chunk will comprise memory
(between the segments) which gets overridden by the relst() invocation of the
movers code in grub_relocator16/32/64_boot().
The overridden memory may contain reserved ranges like VGA memory or ACPI
tables, which leads to strange boot behaviour.
---
grub-core/loader/multiboot_elfxx.c | 60 +++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 67daf59..6565b50 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
char *phdr_base;
grub_err_t err;
grub_relocator_chunk_t ch;
- grub_uint32_t load_offset, load_size;
+ grub_uint32_t load_offset = 0, load_size;
int i;
- void *source;
+ void *source = NULL;
if (ehdr->e_ident[EI_MAG0] != ELFMAG0
|| ehdr->e_ident[EI_MAG1] != ELFMAG1
@@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
#define phdr(i) ((Elf_Phdr *) (phdr_base + (i) * ehdr->e_phentsize))
mld->link_base_addr = ~0;
+ mld->load_base_addr = ~0;
We do not need it. Please look below.
/* Calculate lowest and highest load address. */
for (i = 0; i < ehdr->e_phnum; i++)
@@ -97,10 +98,15 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
#endif
- load_size = highest_load - mld->link_base_addr;
+ grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, relocatable=%d, "
+ "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
+ mld->link_base_addr, mld->relocatable, (long) mld->align,
+ mld->preference, mld->avoid_efi_boot_services);
I have just realized that it does not make sense to print align,
preference and avoid_efi_boot_services if mld->relocatable is not
set. Please take a look at grub-core/loader/multiboot_mbi2.c for
more details. Sorry about that. In this case you can probably drop
this grub_dprintf() and...
if (mld->relocatable)
{
+ load_size = highest_load - mld->link_base_addr;
+
if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - load_size)
return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or load size");
@@ -108,27 +114,16 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
mld->min_addr, mld->max_addr - load_size,
load_size, mld->align ? mld->align : 1,
mld->preference, mld->avoid_efi_boot_services);
- }
- else
mld->load_base_addr = mld->link_base_addr;
load_base_addr == link_base_addr in non relocatable case.
Am I right?
- err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
- mld->link_base_addr, load_size);
-
- if (err)
- {
- grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
- return err;
- }
- mld->load_base_addr = get_physical_target_address (ch);
- source = get_virtual_current_address (ch);
-
- grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, "
- "load_size=0x%x, relocatable=%d\n", mld->link_base_addr,
- mld->load_base_addr, load_size, mld->relocatable);
...you can leave this excluding load_size and...
+ if (err)
+ {
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+ }
- if (mld->relocatable)
- grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
- (long) mld->align, mld->preference, mld->avoid_efi_boot_services);
...you can leave this grub_dprintf() and move load_size from above.
You can put it after preference.
+ mld->load_base_addr = get_physical_target_address (ch);
+ source = get_virtual_current_address (ch);
+ }
/* Load every loadable segment in memory. */
for (i = 0; i < ehdr->e_phnum; i++)
@@ -139,7 +134,26 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n",
i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr);
- load_offset = phdr(i)->p_paddr - mld->link_base_addr;
+ if (mld->relocatable)
+ load_offset = phdr(i)->p_paddr - mld->link_base_addr;
grub_dprintf ("multiboot_loader", "segment %d: load_offset=0x%x\n", i, load_offset);
+ else
+ {
+ err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch,
+ phdr(i)->p_paddr, phdr(i)->p_memsz);
+
+ if (err)
+ {
+ grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n");
+ return err;
+ }
+
+ mld->load_base_addr = grub_min (mld->load_base_addr, get_physical_target_address (ch));
You can drop this. Please look above.
+
+ source = get_virtual_current_address (ch);
+ }
+
+ grub_dprintf ("multiboot_loader", "segment %d: load_base_addr=0x%x, "
+ "load_offset=0x%x\n", i, mld->load_base_addr, load_offset);
Please drop this grub_dprintf().
Daniel
_______________________________________________
Grub-devel mailing list
https://lists.gnu.org/mailman/listinfo/grub-devel
--
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
Continue reading on narkive:
Loading...