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:27:53 +0100	[thread overview]
Message-ID: <CAEaD8JOin-GSP8+kqC3bnS-_boKzvFgV-WoByLrkDOaeNThMGg__22296.4799301282$1458059348$gmane$org@mail.gmail.com> (raw)
In-Reply-To: <1458055562-24950-6-git-send-email-daniel.kiper@oracle.com>


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

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

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

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

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