On April 17, 2018 3:40:11 PM EDT, Daniel Kiper <dki...@net-space.pl> wrote: >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>
There is more to it then that. Doing an SoB means you abide by the developer certificate. See https://developercertificate.org/ > >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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
