On Tue, Jun 12, 2018 at 08:19:35PM +0200, Alexander Boettcher wrote:
> 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/
> Signed-off-by: Alexander Boettcher <alexander.boettc...@genode-labs.com>
> ---
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
