Re: [Xen-devel] [PATCH 3/4] x86/linker: add a reloc section to ELF binary

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 25, 2019 at 03:18:14AM -0600, Jan Beulich wrote:
> >>> On 25.06.19 at 10:10, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Jun 24, 2019 at 01:24:02PM +0200, Daniel Kiper wrote:
> >> On Fri, Jun 21, 2019 at 12:34:13AM -0600, Jan Beulich wrote:
> >> > >>> On 19.06.19 at 17:06, <roger.pau@xxxxxxxxxx> wrote:
> >> > > On Wed, Jun 19, 2019 at 06:57:05AM -0600, Jan Beulich wrote:
> >> > >> >>> On 19.06.19 at 13:02, <roger.pau@xxxxxxxxxx> wrote:
> >> > >> > If the hypervisor has been built with EFI support (ie: multiboot2).
> >> > >> > This allows to position the .reloc section correctly in the output
> >> > >> > binary, or else the linker might place .reloc before the .text
> >> > >> > section.
> >> > >> >
> >> > >> > Note that the .reloc section is moved before .bss for two reasons: 
> >> > >> > in
> >> > >> > order for the resulting binary to not contain any section with data
> >> > >> > after .bss, so that the file size can be smaller than the loaded
> >> > >> > memory size, and because the data it contains is read-only, so it
> >> > >> > belongs with the other sections containing read-only data.
> >> > >>
> >> > >> While this may be fine for ELF, I'm afraid it would be calling for
> >> > >> subtle issues with xen.efi (i.e. the PE binary): There a .reloc
> >> > >> section is generally expected to come after "normal" data
> >> > >> sections.
> >> > >
> >> > > OK, would you like me to leave the .reloc section at the previous
> >> > > position for EFI builds then?
> >> >
> >> > Well, this part is a requirement, not a question of me liking you
> >> > to do so.
> >> >
> >> > > Or do we prefer to leave .reloc orphaned in the ELF build?
> >> >
> >> > Daniel might have an opinion here with his plans to actually
> >> > add relocations there in the non-linker-generated-PE build. I
> >> > don't have a strong opinion either way, as long as the
> >> > current method of building gets left as is (or even simplified).
> >> 
> >> I would not drop .reloc section from xen-syms because it can be useful
> >> for "manual" EFI image relocs generation. However, I am not strongly
> >> tied to it. If you wish to drop it go ahead. I can readd it latter if
> >> I get back to my new PE build work.
> > 
> > Do you mean that the dummy .reloc section added to non-PE builds can
> > be dropped? (ie: remove xen/arch/x86/efi/relocs-dummy.S from the build)
> 
> Given my earlier reply it's not clear to me what you mean by "remove"
> here. As a result ...
> 
> > I'm slightly lost, .reloc begin a section that's explicitly added to
> > non-PE builds by relocs-dummy.S I assumed it was needed for some
> > reason.
> 
> ... it's also not clear what exactly you mean here, and hence whether
> there's any reason needed beyond the reference to the two bounding
> symbols by efi_arch_relocate_image().

Sorry for not being clear. By remove I mean `git rm
xen/arch/x86/efi/relocs-dummy.S` and fix the build, like the diff
appended below.

Is there any reason we should keep the dummy .reloc in the ELF
output?

Thanks, Roger.
---8<---
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 4bc0a196e9..5849604766 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -11,6 +11,6 @@ $(call 
cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
 
 obj-y := stub.o
-obj-$(XEN_BUILD_EFI) := $(EFIOBJ) relocs-dummy.o
+obj-$(XEN_BUILD_EFI) := $(EFIOBJ)
 extra-$(XEN_BUILD_EFI) += buildid.o
 nocov-$(XEN_BUILD_EFI) += stub.o
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7a13a30bc0..2cf440e2ae 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -39,6 +39,7 @@ extern const intpte_t __page_tables_start[], 
__page_tables_end[];
 #define PE_BASE_RELOC_HIGHLOW  3
 #define PE_BASE_RELOC_DIR64   10
 
+#ifdef XEN_BUILD_PE
 extern const struct pe_base_relocs {
     u32 rva;
     u32 size;
@@ -97,6 +98,12 @@ static void __init efi_arch_relocate_image(unsigned long 
delta)
         base_relocs = (const void *)(base_relocs->entries + i + (i & 1));
     }
 }
+#else /* !XEN_BUILD_PE */
+static void __init efi_arch_relocate_image(unsigned long delta)
+{
+    ASSERT_UNREACHABLE();
+}
+#endif /* XEN_BUILD_PE */
 
 extern const s32 __trampoline_rel_start[], __trampoline_rel_stop[];
 extern const s32 __trampoline_seg_start[], __trampoline_seg_stop[];
diff --git a/xen/arch/x86/efi/relocs-dummy.S b/xen/arch/x86/efi/relocs-dummy.S
deleted file mode 100644
index d928a82d53..0000000000
--- a/xen/arch/x86/efi/relocs-dummy.S
+++ /dev/null
@@ -1,11 +0,0 @@
-
-       .section .reloc, "a", @progbits
-       .balign 4
-GLOBAL(__base_relocs_start)
-       .long 0
-       .long 8
-GLOBAL(__base_relocs_end)
-
-       .globl VIRT_START, ALT_START
-       .equ VIRT_START, XEN_VIRT_START
-       .equ ALT_START, XEN_VIRT_END


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel