Discussion:
[PATCH v3] mbi: use per segment a separate relocator chunk
Alexander Boettcher
2018-06-12 18:19:35 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-13 14:31:43 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
s/If/Currently, if/
(between the segments) which gets overridden by the relst() invocation of the
movers code in grub_relocator16/32/64_boot().
Please drop this empty line and start next sentence immediately behind previous one.
The overridden memory may contain reserved ranges like VGA memory or ACPI
tables, which leads to strange boot behaviour.
s/leads/may lead to crashes or at least/
---
grub-core/loader/multiboot_elfxx.c | 58 +++++++++++++++++++++++---------------
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 67daf59..f493b0a 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
@@ -97,10 +97,13 @@ 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;
-
if (mld->relocatable)
{
+ load_size = highest_load - mld->link_base_addr;
+
+ grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, load_size=0x%x avoid_efi_boot_services=%d\n",
+ (long) mld->align, mld->preference, load_size, mld->avoid_efi_boot_services);
Too long line and lack of ",". I would like to see this:

grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, "
"load_size=0x%x, avoid_efi_boot_services=%d\n",
(long) mld->align, mld->preference, load_size,
mld->avoid_efi_boot_services);
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 +111,21 @@ 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
- 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;
- }
+ 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);
+ mld->load_base_addr = get_physical_target_address (ch);
+ source = get_virtual_current_address (ch);
+ }
+ else
+ mld->load_base_addr = mld->link_base_addr;
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);
-
- 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);
+ "relocatable=%d\n", mld->link_base_addr, mld->load_base_addr, mld->relocatable);
I think that this would be much better:
grub_dprintf ("multiboot_loader", "relocatable=%d, link_base_addr=0x%x, "
"load_base_addr=0x%x\n", mld->relocatable,
mld->link_base_addr, mld->load_base_addr);

As I you can see mostly nitpicks. So, I think that after next
iteration everything will be OK.

Thank you for doing the work.

Daniel

Continue reading on narkive:
Loading...