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

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

On Tuesday, March 15, 2016, Daniel Kiper <daniel.kiper@xxxxxxxxxx> wrote:
> On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@xxxxxxxxx>
> > wrote:
> >
> > >
> > >> +           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
>
> Argh... You are right!
>
> > > 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.
>
> It looks that we should store somewhere and export to image via relevant tags
> link addresses and load addresses. Hmmm... Maybe we should just provide load
> addresses to image. Image can have link addresses in its data. And this
> probably does not require huge changes.
>
> > > 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.
> > >
> > I take back the part "requires least effort" for solution 1. Solution 2 is
> > probably simpler and less error-prone as developper doesn't control if
> > binutils decode to put several phdrs.
>
> #2 looks promising but what if PHDR_1 is at 1 MiB - 2 MiB and PHDR_2 is at
> 808 MiB - 809 MiB? Then we will allocate more than 800 MiB just for an
> unusable hole. So, I think that we should go that way if solution #1
> is too complicated.

If 'RELOCATABLE' means 'image is fine with any of its phdr going anywhere' then why would it declare such a layout if it's fine with second phdr going right after the first one? Why not describe the same thing as 1-2M and 2-3M? 

> > > +  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?
>
> Yep, you are right. However, it looks that real life (C?) is more complicated.
> I am trying to avoid wrap around here if mld.load_base_addr < mld.link_base_addr.
> If you look at C operator precedence then everything should work. However,
> I am not 100% sure that a given compiler will not optimize/break my stuff.
> So, maybe we should use signed 64-bit int here.
>
> Daniel


-- 
Regards
Vladimir 'phcoder' Serbinenko

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