[PATCH v2 0/3] x86/EFI: build adjustments

[PATCH v2 0/3] x86/EFI: build adjustments

[Jan Beulich]

Since it was requested and is possible with up-to-date binutils, inclusion
of debug info in xen.efi is the remaining part here. The other changes here
are cleanup related to both the work here as well as the one to make
binutils fit for our purposes.

1: EFI: sections may not live at VA 0 in PE binaries
2: EFI: keep debug info in xen.efi
3: EFI: don't have an overly large image size

Jan

[PATCH v2 1/3] x86/EFI: sections may not live at VA 0 in PE binaries

[Jan Beulich]

PE binaries specify section addresses by (32-bit) RVA. GNU ld up to at
least 2.36 would silently truncate the (negative) difference when a
section is placed below the image base. Such sections would also be
wrongly placed ahead of all "normal" ones. Since, for the time being,
we build xen.efi with --strip-debug anyway, .stab* can't appear. And
.comment has an entry in /DISCARD/ already anyway in the EFI case.

Because of their unclear origin, keep the directives for the ELF case
though.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
It's certainly odd that we have stabs section entries in the script, but
no Dwarf ones.

--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -350,6 +350,7 @@ SECTIONS
 #endif
   }
 
+#ifndef EFI
   /* Stabs debugging sections.  */
   .stab 0 : { *(.stab) }
   .stabstr 0 : { *(.stabstr) }
@@ -358,6 +359,7 @@ SECTIONS
   .stab.index 0 : { *(.stab.index) }
   .stab.indexstr 0 : { *(.stab.indexstr) }
   .comment 0 : { *(.comment) }
+#endif
 }
 
 ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -

Re: [PATCH v2 1/3] x86/EFI: sections may not live at VA 0 in PE binaries

[Roger Pau Monné]

On Fri, Apr 23, 2021 at 01:03:34PM +0200, Jan Beulich wrote:
> PE binaries specify section addresses by (32-bit) RVA. GNU ld up to at
> least 2.36 would silently truncate the (negative) difference when a
> section is placed below the image base. Such sections would also be
> wrongly placed ahead of all "normal" ones. Since, for the time being,
> we build xen.efi with --strip-debug anyway, .stab* can't appear. And
> .comment has an entry in /DISCARD/ already anyway in the EFI case.
> 
> Because of their unclear origin, keep the directives for the ELF case
> though.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Albeit I would remove those - even if gcc can still generate stabs
debug info I don't think it's used at all, and in any case a user
would have to also modify the build system in order to force gcc to
produce stabs debug info.

Thanks, Roger.

Re: [PATCH v2 1/3] x86/EFI: sections may not live at VA 0 in PE binaries

[Jan Beulich]

On 23.04.2021 16:25, Roger Pau Monné wrote:
> On Fri, Apr 23, 2021 at 01:03:34PM +0200, Jan Beulich wrote:
>> PE binaries specify section addresses by (32-bit) RVA. GNU ld up to at
>> least 2.36 would silently truncate the (negative) difference when a
>> section is placed below the image base. Such sections would also be
>> wrongly placed ahead of all "normal" ones. Since, for the time being,
>> we build xen.efi with --strip-debug anyway, .stab* can't appear. And
>> .comment has an entry in /DISCARD/ already anyway in the EFI case.
>>
>> Because of their unclear origin, keep the directives for the ELF case
>> though.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Albeit I would remove those - even if gcc can still generate stabs
> debug info I don't think it's used at all, and in any case a user
> would have to also modify the build system in order to force gcc to
> produce stabs debug info.

I'd be fine dropping them, but I'd prefer doing so in a separate
change then. As to modifying the build system - with all the different
Dwarf versions and with different debug info levels I was wondering
whether we shouldn't allow selecting the precise details via Kconfig.

Jan

[PATCH v2 2/3] x86/EFI: keep debug info in xen.efi

[Jan Beulich]

... provided the linker supports it (which it does as of commit
2dfa8341e079 ["ELF DWARF in PE output"]).

Without mentioning debugging sections, the linker would put them at
VA 0, thus making them unreachable by 32-bit (relative or absolute)
relocations. If relocations were resolvable (or absent) the resulting
binary would have invalid section RVAs (0 - __image_base__, truncated to
32 bits). Mentioning debugging sections without specifying an address
will result in the linker putting them all on the same RVA. A loader is,
afaict, free to reject loading such an image, as sections shouldn't
overlap. (The above describes GNU ld 2.36 behavior, which - if deemed
buggy - could change.)

Make sure our up-to-16Mb padding doesn't unnecessarily further extend
the image.

Take the opportunity and also switch to using $(call ld-option,...).

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Add comment to linker script.
---
This way we could also avoid discarding .comment for xen.efi.

I'd like to point out that the linking of the debug info takes far
longer than the linking of the "normal" parts of the image. The change
therefore has the downside of slowing down debug builds.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -129,8 +129,14 @@ export XEN_BUILD_EFI := $(shell $(CC) $(
 CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
 
 # Check if the linker supports PE.
-EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 --strip-debug
-XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) $(EFI_LDFLAGS) -o efi/check.efi efi/check.o 2>/dev/null && echo y))
+EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10
+XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o))
+# If the above failed, it may be merely because of the linker not dealing well
+# with debug info. Try again with stripping it.
+ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n)
+EFI_LDFLAGS += --strip-debug
+XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x100000000 -o efi/check.efi efi/check.o)
+endif
 
 ifeq ($(XEN_BUILD_PE),y)
 
@@ -235,6 +241,9 @@ note_file_option ?= $(note_file)
 
 ifeq ($(XEN_BUILD_PE),y)
 $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
+ifeq ($(CONFIG_DEBUG_INFO),y)
+	$(if $(filter --strip-debug,$(EFI_LDFLAGS)),echo,:) "Will strip debug info from $(@F)"
+endif
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< $(relocs-dummy) \
 	                $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) :
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -312,10 +312,70 @@ SECTIONS
     *(.reloc)
     __base_relocs_end = .;
   }
-  /* Trick the linker into setting the image size to exactly 16Mb. */
-  . = ALIGN(__section_alignment__);
-  DECL_SECTION(.pad) {
-    . = ALIGN(MB(16));
+  /*
+   * Explicitly list debug section for the PE output so that they don't end
+   * up at VA 0 which is below image base and thus invalid. Also use the
+   * NOLOAD directive, despite currently ignored by ld for PE output, in
+   * order to record that we'd prefer these sections to not be loaded into
+   * memory.
+   *
+   * Note that we're past _end here, so if these sections get loaded they'll
+   * be discarded at runtime anyway.
+   */
+  .debug_abbrev ALIGN(1) (NOLOAD) : {
+     *(.debug_abbrev)
+  }
+  .debug_info ALIGN(1) (NOLOAD) : {
+    *(.debug_info)
+    *(.gnu.linkonce.wi.*)
+  }
+  .debug_types ALIGN(1) (NOLOAD) : {
+    *(.debug_types)
+  }
+  .debug_str ALIGN(1) (NOLOAD) : {
+    *(.debug_str)
+  }
+  .debug_line ALIGN(1) (NOLOAD) : {
+    *(.debug_line)
+    *(.debug_line.*)
+  }
+  .debug_line_str ALIGN(1) (NOLOAD) : {
+    *(.debug_line_str)
+  }
+  .debug_names ALIGN(4) (NOLOAD) : {
+    *(.debug_names)
+  }
+  .debug_frame ALIGN(4) (NOLOAD) : {
+    *(.debug_frame)
+  }
+  .debug_loc ALIGN(1) (NOLOAD) : {
+    *(.debug_loc)
+  }
+  .debug_loclists ALIGN(4) (NOLOAD) : {
+    *(.debug_loclists)
+  }
+  .debug_ranges ALIGN(8) (NOLOAD) : {
+    *(.debug_ranges)
+  }
+  .debug_rnglists ALIGN(4) (NOLOAD) : {
+    *(.debug_rnglists)
+  }
+  .debug_addr ALIGN(8) (NOLOAD) : {
+    *(.debug_addr)
+  }
+  .debug_aranges ALIGN(1) (NOLOAD) : {
+    *(.debug_aranges)
+  }
+  .debug_pubnames ALIGN(1) (NOLOAD) : {
+    *(.debug_pubnames)
+  }
+  .debug_pubtypes ALIGN(1) (NOLOAD) : {
+    *(.debug_pubtypes)
+  }
+  /* Trick the linker into setting the image size to no less than 16Mb. */
+  __image_end__ = .;
+  .pad ALIGN(__section_alignment__) : {
+    . = __image_end__ < __image_base__ + MB(16) ? ALIGN(MB(16)) : .;
   }
 #elif defined(XEN_BUILD_EFI)
   /*

Re: [PATCH v2 2/3] x86/EFI: keep debug info in xen.efi

[Roger Pau Monné]

On Fri, Apr 23, 2021 at 01:04:10PM +0200, Jan Beulich wrote:
> ... provided the linker supports it (which it does as of commit
> 2dfa8341e079 ["ELF DWARF in PE output"]).
> 
> Without mentioning debugging sections, the linker would put them at
> VA 0, thus making them unreachable by 32-bit (relative or absolute)
> relocations. If relocations were resolvable (or absent) the resulting
> binary would have invalid section RVAs (0 - __image_base__, truncated to
> 32 bits). Mentioning debugging sections without specifying an address
> will result in the linker putting them all on the same RVA. A loader is,
> afaict, free to reject loading such an image, as sections shouldn't
> overlap. (The above describes GNU ld 2.36 behavior, which - if deemed
> buggy - could change.)
> 
> Make sure our up-to-16Mb padding doesn't unnecessarily further extend
> the image.
> 
> Take the opportunity and also switch to using $(call ld-option,...).
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH v2 3/3] x86/EFI: don't have an overly large image size

[Jan Beulich]

While without debug info the difference is benign (so far), since we pad
the image to 16Mb anyway, forcing the .reloc section to a 2Mb boundary
causes subsequent .debug_* sections to go farther beyond 16Mb than
needed. There's no reason to advance . for establishing __2M_rwdata_end,
as all data past _end is of no interest at runtime anymore anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
This makes more explicit a possible latent problem with the ELF image:
It ends at _end, not __2M_rwdata_end (advancing . as was done here does
not have the effect of increasing the image size). Interestingly the
conversion xen-syms => xen rounds up the program header specified size
suitably, as per the comment "Do not use p_memsz: it does not include
BSS alignment padding" in mkelf32.c. I do think this would instead want
taking care of in the linker script. Commit 7a95e0a2c572 ("x86: properly
calculate xen ELF end of image address") clearly only hacked an existing
hack rather than addressing the root cause. Thoughts?

--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -303,8 +303,7 @@ SECTIONS
   } PHDR(text)
   _end = . ;
 
-  . = ALIGN(SECTION_ALIGN);
-  __2M_rwdata_end = .;
+  __2M_rwdata_end = ALIGN(SECTION_ALIGN);
 
 #ifdef EFI
   .reloc ALIGN(4) : {