Re: [PATCH v2] mbi: use per segment a separate relocator chunk

[Alexander Boettcher]

Agree, resend it as v3 patch.

On 11.06.2018 22:09, Daniel Kiper wrote:
> On Tue, Jun 05, 2018 at 10:14:49PM +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
>> (between the segments) which gets overridden by the relst() invocation of the
>> movers code in grub_relocator16/32/64_boot().
>>
>> The overridden memory may contain reserved ranges like VGA memory or ACPI
>> tables, which leads to strange boot behaviour.
>>
>> Signed-off-by: Alexander Boettcher <address@hidden>
>> ---
>>  grub-core/loader/multiboot_elfxx.c | 60 
>> +++++++++++++++++++++++---------------
>>  1 file changed, 37 insertions(+), 23 deletions(-)
>>
>> diff --git a/grub-core/loader/multiboot_elfxx.c 
>> b/grub-core/loader/multiboot_elfxx.c
>> index 67daf59..6565b50 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;
> 
> We do not need it. Please look below.
> 
>>    /* Calculate lowest and highest load address.  */
>>    for (i = 0; i < ehdr->e_phnum; i++)
>> @@ -97,10 +98,15 @@ 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;
>> +  grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, relocatable=%d, "
>> +            "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n",
>> +            mld->link_base_addr, mld->relocatable, (long) mld->align,
>> +            mld->preference, mld->avoid_efi_boot_services);
> 
> I have just realized that it does not make sense to print align,
> preference and avoid_efi_boot_services if mld->relocatable is not
> set. Please take a look at grub-core/loader/multiboot_mbi2.c for
> more details. Sorry about that. In this case you can probably drop
> this grub_dprintf() and...
> 
>>    if (mld->relocatable)
>>      {
>> +      load_size = highest_load - mld->link_base_addr;
>> +
>>        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 +114,16 @@ 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
> 
> Please do not drop this else. You can put this here:
>   mld->load_base_addr = mld->link_base_addr;
> 
> load_base_addr == link_base_addr in non relocatable case.
> Am I right?
> 
>> -    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;
>> -    }
>>
>> -  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);
> 
> ...you can leave this excluding load_size and...
> 
>> +      if (err)
>> +        {
>> +          grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS 
>> image\n");
>> +          return err;
>> +        }
>>
>> -  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);
> 
> ...you can leave this grub_dprintf() and move load_size from above.
> You can put it after preference.
> 
>> +      mld->load_base_addr = get_physical_target_address (ch);
>> +      source = get_virtual_current_address (ch);
>> +    }
>>
>>    /* Load every loadable segment in memory.  */
>>    for (i = 0; i < ehdr->e_phnum; i++)
>> @@ -139,7 +134,26 @@ 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;
> 
> Please add this here:
>   grub_dprintf ("multiboot_loader", "segment %d: load_offset=0x%x\n", i, 
> load_offset);
> 
>> +      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;
>> +            }
>> +
>> +          mld->load_base_addr = grub_min (mld->load_base_addr, 
>> get_physical_target_address (ch));
> 
> You can drop this. Please look above.
> 
>> +
>> +          source = get_virtual_current_address (ch);
>> +        }
>> +
>> +      grub_dprintf ("multiboot_loader", "segment %d: load_base_addr=0x%x, "
>> +                    "load_offset=0x%x\n", i, mld->load_base_addr, 
>> load_offset);
> 
> Please drop this grub_dprintf().
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

-- 
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