| Message ID | 20190627093335.56355-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] x86/linker: add a reloc section to ELF linker script | expand |
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
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.
>>> 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
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
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);
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(+)