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

From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
To: Daniel Kiper <daniel.kiper@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>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@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>,
	"stefano.stabellini@eu.citrix.com"
	<stefano.stabellini@eu.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>
Subject: Re: [GRUB2 PATCH v4 4/4] multiboot2: Add support for relocatable images
Date: Tue, 15 Mar 2016 17:30:20 +0100	[thread overview]
Message-ID: <CAEaD8JPS9cbmmS+a0pjCMboGTd-jkwCZp59KYpnEhOgpGva6zw__25465.1733710945$1458059506$gmane$org@mail.gmail.com> (raw)
In-Reply-To: <CAEaD8JOin-GSP8+kqC3bnS-_boKzvFgV-WoByLrkDOaeNThMGg@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2752 bytes --]

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

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

-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #1.2: Type: text/html, Size: 3637 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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