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

From: Daniel Kiper <daniel.kiper@oracle.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "jgross@suse.com" <jgross@suse.com>,
	"grub-devel@gnu.org" <grub-devel@gnu.org>,
	"eric.snowberg@oracle.com" <eric.snowberg@oracle.com>,
	"arvidjaar@gmail.com" <arvidjaar@gmail.com>,
	Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>,
	"stefano.stabellini@eu.citrix.com"
	<stefano.stabellini@eu.citrix.com>,
	"cardoe@cardoe.com" <cardoe@cardoe.com>,
	"pgnet.dev@gmail.com" <pgnet.dev@gmail.com>,
	"roy.franz@linaro.org" <roy.franz@linaro.org>,
	"ning.sun@intel.com" <ning.sun@intel.com>,
	"david.vrabel@citrix.com" <david.vrabel@citrix.com>,
	"jbeulich@suse.com" <jbeulich@suse.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"qiaowei.ren@intel.com" <qiaowei.ren@intel.com>,
	"richard.l.maliszewski@intel.com"
	<richard.l.maliszewski@intel.com>,
	"gang.wei@intel.com" <gang.wei@intel.com>,
	"fu.wei@linaro.org" <fu.wei@linaro.org>,
	"seth.goldberg@oracle.com" <seth.goldberg@oracle.com>
Subject: Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images
Date: Wed, 16 Mar 2016 11:34:19 +0100	[thread overview]
Message-ID: <20160316103419.GG31771@olila.local.net-space.pl> (raw)
In-Reply-To: <20160315235408.GF29495@char.us.oracle.com>

On Tue, Mar 15, 2016 at 07:54:08PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 15, 2016 at 10:42:21PM +0100, Daniel Kiper wrote:
> > On Tue, Mar 15, 2016 at 05:30:20PM +0100, Vladimir 'phcoder' Serbinenko wrote:
> > > On Tuesday, March 15, 2016, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
> > > 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.
>
> Won't be in Xen 4.7.
> > > > 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.
>
> Daniel, my xSplice patches make the Xen have two ELF PHDRS: 1)the PT_LOAD
> and 2) PT_NOTE (which points to smack in the .text section) so you can try
> that as an example payload.
>
> (If you want to put your patches on top of mine:
> git://xenbits.xen.org/people/konradwilk/xen.git #xsplice.v4)

Thanks, but multiboot2 loads just PT_LOAD segments.

Daniel