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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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

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