Hi Alexander,
On Sat, Apr 14, 2018 at 12:07:29AM +0200, Alexander Boettcher wrote:
> Hello,
>
> On 06.04.2018 14:28, Daniel Kiper wrote:
> > On Thu, Mar 29, 2018 at 11:20:37AM +0200, Alexander Boettcher wrote:
> >
> >> Can you please have a look and check regarding what should/could be
> >> changed to get it upstream? We did not test the dynamic relocation part,
> >
> > Sure thing, please take a look below...
> >
> >> since we have no such (kernel) setup. Thanks in advance.
> >
> > You can use Xen (git://xenbits.xen.org/xen.git) for tests.
> > Just compile hypervisor without any tools and use xen.gz.
> > It produces nice output and you can see it is relocated or not.
> > Of course you have to use Multiboot2 protocol.
>
> Thanks, I managed to setup it. Based on your comments and on the Xen
> test case I had to re-work the patch, so that it now works for
> relocation and non-relocation kernels.
>
> Please review again.
> > However, this does not mean that I will not take this patch. Though
> > it requires some tweaking.
> >
> > First of all, lack of SOB.
>
> What is a SOB ?
Signed-off-by: Alexander Boettcher <alexander.boettc...@genode-labs.com>
Next time please use git format-patch/send-email to send the patches.
> From c37727840be8aeb81ea4bbc95ac09a1b1e3d3658 Mon Sep 17 00:00:00 2001
> From: Alexander Boettcher <alexander.boettc...@genode-labs.com>
> Date: Fri, 13 Apr 2018 23:37:01 +0200
> Subject: [PATCH] mbi: use per segment a separate relocator chunk
>
> if elf is non relocatable.
>
> If the segments are not dense packed, the original code set up a huge
> relocator chunk comprising all segments.
>
> During the final relocation in grub_relocator_prepare_relocs, the chunk
> is moved to its desired place and overrides memory which are actually not
> covered/touched by the specified segments.
>
> The overriden memory may contain device memory (vga text mode e.g.), which
> leads to strange boot behaviour.
Have you been able to take a look at memory allocator/relocator code why
this happened?
> ---
> grub-core/loader/multiboot_elfxx.c | 56
> ++++++++++++++++++++++++--------------
> 1 file changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/grub-core/loader/multiboot_elfxx.c
> b/grub-core/loader/multiboot_elfxx.c
> index 67daf59..d936223 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;
>
> /* Calculate lowest and highest load address. */
> for (i = 0; i < ehdr->e_phnum; i++)
> @@ -108,27 +109,19 @@ 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);
>
> - 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);
I would not drop it completely from ~here. I think that you can display
link_base_addr and relocatable just before current line 102. And you can
put "load_size = highest_load - mld->link_base_addr;" just before current
line 104. Additionally, load_size does not have a real meaning for non
relocatable images right now. Hence, I think that it should not be displayed
for them. Especially if there are more than one PT_LOAD segment.
> - 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);
> + 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);
> + }
>
> /* Load every loadable segment in memory. */
> for (i = 0; i < ehdr->e_phnum; i++)
> @@ -139,7 +132,24 @@ 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;
> + 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;
> + }
> +
> + if (mld->load_base_addr > get_physical_target_address (ch))
> + mld->load_base_addr = get_physical_target_address (ch);
mld->load_base_addr = grub_min (mld->load_base_addr,
get_physical_target_address (ch));
> +
> + source = get_virtual_current_address (ch);
> + }
>
> if (phdr(i)->p_filesz != 0)
> {
> @@ -163,6 +173,10 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t
> *mld)
> }
> }
>
> + 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,
Well load_size is not meaningful for non relocatable images. Hmmm...
You can make it more meaningful by summing up all phdr(i)->p_memsz.
Anyway, patch looks much better right now.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
