Re: [Xen-devel] [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images

["Vladimir 'phcoder' Serbinenko" <phcoder@xxxxxxxxx>]

> +           if (mld->relocatable)
> +             err = grub_relocator_alloc_chunk_align (grub_multiboot_relocator, &ch,
> +                                                     mld->min_addr, mld->max_addr - phdr(i)->p_memsz,
> +                                                     phdr(i)->p_memsz, mld->align ? mld->align : 1,
> +                                                     mld->preference, mld->avoid_efi_boot_services);
> +           else
> +             err = grub_relocator_alloc_chunk_addr (grub_multiboot_relocator,
> +                                                    &ch, phdr(i)->p_paddr,
> +                                                    phdr(i)->p_memsz);

I believe this is faulty if you have more than one PHDR. You load every PHDR individually to essentially random address. Pieces have no reasonable way to find each other. Moreover entry point calculation is also faulty. Imagine sth like this:

PHDR 1M-2M

PHDR 2M-5M

Entry point 2.5M (in second PHDR)

then if first PHDR is loaded to 1M and second to 10M then base and link addr are both 1M, so entry point will be calculated as 2.5M, which points to no segment. I see 2 solutions:

1) Look where entry falls in original layout, then adjust it in accordance with where this phdr will be loaded. This requires least efforts. Finding different PHDRs is still impossible but it will be possible in the future with relocations.

2) Allocate a buffer of size highest - lowest and load everything into this buffer keeping relative offsets. If we do this, then we need to document if it's required for boorloader to behave this way or not. If it is, we can in future provide a tag to say that image is fine with rearrangement of PHDR, if it ever becomes relevant (I heavily doubt it).

I guess that xen is a single phdr image and so essentially any code will work with it. 

This problem appears in couple of other places, I'll skip commenting on them explicitly.

> +  if (mld.relocatable)
> +    {
> +      if (mld.load_base_addr >= mld.link_base_addr)
> +       grub_multiboot_payload_eip += mld.load_base_addr - mld.link_base_addr;
> +      else
> +       grub_multiboot_payload_eip -= mld.link_base_addr - mld.load_base_addr;
> +    }

Both branches are mathematically equivalent. Any reason to have if at all? 

-- 
Regards
Vladimir 'phcoder' Serbinenko

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel