[v3,1/3] x86/linker: add a reloc section to ELF linker script
diff mbox series

Message ID 20190627093335.56355-1-roger.pau@citrix.com
State New, archived
Headers show
Series
  • [v3,1/3] x86/linker: add a reloc section to ELF linker script
Related show

Commit Message

Roger Pau Monne June 27, 2019, 9:33 a.m. UTC
if the hypervisor has been built with EFI support (ie: multiboot2).
This allows to position the .reloc section correctly in the output
binary.

Note that for the ELF output format the .reloc section is moved before
.bss because the data it contains is read-only, so it belongs with the
other sections containing read-only data.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
---
Changes since v2:
 - Fix subject to mention the section is added to the linker script.
 - Fix commit message to note .reloc is added together with the rest
   of the read-only sections.

Changes since v1:
 - Move the .reloc section position in the output binary only for the
   ELF output format.
---
 xen/arch/x86/xen.lds.S | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Andrew Cooper June 27, 2019, 10:59 a.m. UTC | #1
On 27/06/2019 10:33, Roger Pau Monne wrote:
> if the hypervisor has been built with EFI support (ie: multiboot2).
> This allows to position the .reloc section correctly in the output
> binary.
>
> Note that for the ELF output format the .reloc section is moved before
> .bss because the data it contains is read-only, so it belongs with the
> other sections containing read-only data.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I have to admit that I'm still confused as to why we need this in the
first place.

The ELF build is linked to a fixed virtual address, irrespective of
whether grub loads it via MB1 or MB2 and/or with EFI details.

i.e. the non-EFI build shouldn't have any remaining relocations by the
time it is fully linked.

Or am I missing something?

~Andrew
Roger Pau Monne June 27, 2019, 11:07 a.m. UTC | #2
On Thu, Jun 27, 2019 at 11:59:46AM +0100, Andrew Cooper wrote:
> On 27/06/2019 10:33, Roger Pau Monne wrote:
> > if the hypervisor has been built with EFI support (ie: multiboot2).
> > This allows to position the .reloc section correctly in the output
> > binary.
> >
> > Note that for the ELF output format the .reloc section is moved before
> > .bss because the data it contains is read-only, so it belongs with the
> > other sections containing read-only data.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I have to admit that I'm still confused as to why we need this in the
> first place.
> 
> The ELF build is linked to a fixed virtual address, irrespective of
> whether grub loads it via MB1 or MB2 and/or with EFI details.
> 
> i.e. the non-EFI build shouldn't have any remaining relocations by the
> time it is fully linked.
> 
> Or am I missing something?

Right, but there's code that depends on the symbols defined in .reloc
(__base_relocs_start/__base_relocs_end), so unless those symbols are
defined the linker will throw a missing symbols error on the final
link step.

I could add .reloc to the discarded sections list and create the
__base_relocs_start and __base_relocs_end symbols on the linker script
maybe, but I'm not sure that's any better than just having the dummy
.reloc section.

Or another option would be to compile the units that use those symbols
twice, one for the ELF build and one for the PE build, but again that
doesn't seem much better IMO.

Thanks, Roger.
Jan Beulich June 27, 2019, 11:23 a.m. UTC | #3
>>> On 27.06.19 at 11:33, <roger.pau@citrix.com> wrote:
> if the hypervisor has been built with EFI support (ie: multiboot2).
> This allows to position the .reloc section correctly in the output
> binary.
> 
> Note that for the ELF output format the .reloc section is moved before
> .bss because the data it contains is read-only, so it belongs with the
> other sections containing read-only data.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Assuming you've addressed Andrew's concerns in your reply,
Acked-by: Jan Beulich <jbeulich@suse.com>
(But Andrew, please confirm.)

Jan
Andrew Cooper June 27, 2019, 11:27 a.m. UTC | #4
On 27/06/2019 12:07, Roger Pau Monné wrote:
> On Thu, Jun 27, 2019 at 11:59:46AM +0100, Andrew Cooper wrote:
>> On 27/06/2019 10:33, Roger Pau Monne wrote:
>>> if the hypervisor has been built with EFI support (ie: multiboot2).
>>> This allows to position the .reloc section correctly in the output
>>> binary.
>>>
>>> Note that for the ELF output format the .reloc section is moved before
>>> .bss because the data it contains is read-only, so it belongs with the
>>> other sections containing read-only data.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> I have to admit that I'm still confused as to why we need this in the
>> first place.
>>
>> The ELF build is linked to a fixed virtual address, irrespective of
>> whether grub loads it via MB1 or MB2 and/or with EFI details.
>>
>> i.e. the non-EFI build shouldn't have any remaining relocations by the
>> time it is fully linked.
>>
>> Or am I missing something?
> Right, but there's code that depends on the symbols defined in .reloc
> (__base_relocs_start/__base_relocs_end), so unless those symbols are
> defined the linker will throw a missing symbols error on the final
> link step.

Ok.  I can certainly accept that this is how the code currently functions.

> I could add .reloc to the discarded sections list and create the
> __base_relocs_start and __base_relocs_end symbols on the linker script
> maybe, but I'm not sure that's any better than just having the dummy
> .reloc section.
>
> Or another option would be to compile the units that use those symbols
> twice, one for the ELF build and one for the PE build, but again that
> doesn't seem much better IMO.

So the bug here is that efi_arch_relocate_image() isn't inside an #ifdef
EFI (or is it XEN_BUILD_EFI?)

And the reason #ifdef-ing it won't work is because we have a single pass
of extra objects to link into the main Xen to add EFI support.

I think the proper longterm fix is to have CONFIG_EFI (suitably guarded
on toolchain support), seeing as it is common across our two binaries,
and the extra bits for the real EFI build then become rather smaller.

However, it is definitely not fair to lump this fix on you for this
series, so given a comment explaining that this isn't used in the ELF
build, but needs to be present for compatibility with the EFI build,
I'll be ok with the patch in this form.

Again, a comment can be fixed up on commit if everyone is happy with
this approach.

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 98a99444c2..19aa4332cf 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -175,6 +175,14 @@  SECTIONS
   } :text
 #endif
 #endif
+
+#if defined(XEN_BUILD_EFI) && !defined(EFI)
+  . = ALIGN(4);
+  DECL_SECTION(.reloc) {
+    *(.reloc)
+  } :text
+#endif
+
   _erodata = .;
 
   . = ALIGN(SECTION_ALIGN);