Re: Multiboot ELF segment handling patch

[Konrad Rzeszutek Wilk]

On April 17, 2018 3:40:11 PM EDT, Daniel Kiper <address@hidden> 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
><address@hidden>

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 <address@hidden>
>> 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
>address@hidden
>https://lists.gnu.org/mailman/listinfo/grub-devel