xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/8] x86/EFI: build adjustments
@ 2021-04-01  9:43 Jan Beulich
  2021-04-01  9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
                   ` (8 more replies)
  0 siblings, 9 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:43 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

So far we've taken care of just the immediate breakage caused by
binutils 2.36. But we can also take advantage, in particular to
avoid "manually" creating base relocations for xen.efi. Since it
was requested and is possible with up-to-date binutils, inclusion
of debug info in xen.efi is another 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: drop stale section special casing when generating base relocs
2: EFI: sections may not live at VA 0 in PE binaries
3: EFI: program headers are an ELF concept
4: EFI: redo .reloc section bounds determination
5: drop use of prelink-efi.o
6: EFI: avoid use of GNU ld's --disable-reloc-section when possible
7: EFI: keep debug info in xen.efi
8: EFI: don't have an overly large image size

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
@ 2021-04-01  9:44 ` Jan Beulich
  2021-04-01 11:51   ` Andrew Cooper
  2021-04-01  9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:44 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

As of commit a6066af5b142 ("xen/init: Annotate all command line
parameter infrastructure as const") .init.setup has been part of .init.
As of commit 544ad7f5caf5 ("xen/init: Move initcall infrastructure into
.init.data") .initcall* have been part of .init. Hence neither can be
encountered as a stand-alone section in the final binaries anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -346,9 +346,7 @@ int main(int argc, char *argv[])
          * Don't generate relocations for sections that definitely
          * aren't used by the boot loader code.
          */
-        if ( memcmp(sec1[i].name, ".initcal", sizeof(sec1[i].name)) == 0 ||
-             memcmp(sec1[i].name, ".init.se", sizeof(sec1[i].name)) == 0 ||
-             memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 ||
+        if ( memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 ||
              memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 )
             continue;
 



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
  2021-04-01  9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
@ 2021-04-01  9:44 ` Jan Beulich
  2021-04-01 12:01   ` Andrew Cooper
  2021-04-21  8:52   ` Roger Pau Monné
  2021-04-01  9:45 ` [PATCH 3/8] x86/EFI: program headers are an ELF concept Jan Beulich
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:44 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

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
@@ -347,6 +347,7 @@ SECTIONS
 #endif
   }
 
+#ifndef EFI
   /* Stabs debugging sections.  */
   .stab 0 : { *(.stab) }
   .stabstr 0 : { *(.stabstr) }
@@ -355,6 +356,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 -



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 3/8] x86/EFI: program headers are an ELF concept
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
  2021-04-01  9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
  2021-04-01  9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
@ 2021-04-01  9:45 ` Jan Beulich
  2021-04-21  9:11   ` Roger Pau Monné
  2021-04-01  9:45 ` [PATCH 4/8] x86/EFI: redo .reloc section bounds determination Jan Beulich
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:45 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

While they apparently do no harm when building xen.efi, their use is
potentially misleading. Conditionalize their use to be for just the ELF
binary we produce.

No change to the resulting binaries.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -34,13 +34,19 @@ OUTPUT_FORMAT(FORMAT, FORMAT, FORMAT)
 
 OUTPUT_ARCH(i386:x86-64)
 
+#ifndef EFI
 PHDRS
 {
   text PT_LOAD ;
-#if (defined(BUILD_ID) || defined (CONFIG_PVH_GUEST)) && !defined(EFI)
+#if defined(BUILD_ID) || defined(CONFIG_PVH_GUEST)
   note PT_NOTE ;
 #endif
 }
+#define PHDR(x) :x
+#else
+#define PHDR(x)
+#endif
+
 SECTIONS
 {
 #if !defined(EFI)
@@ -83,7 +89,7 @@ SECTIONS
        *(.text.kexec)
        *(.gnu.warning)
        _etext = .;             /* End of text section */
-  } :text = 0x9090
+  } PHDR(text) = 0x9090
 
   . = ALIGN(SECTION_ALIGN);
   __2M_text_end = .;
@@ -134,7 +140,7 @@ SECTIONS
        *(SORT(.data.vpci.*))
        __end_vpci_array = .;
 #endif
-  } :text
+  } PHDR(text)
 
 #if defined(CONFIG_PVH_GUEST) && !defined(EFI)
   DECL_SECTION(.note.Xen) {
@@ -160,7 +166,7 @@ SECTIONS
        __note_gnu_build_id_start = .;
        *(.buildid)
        __note_gnu_build_id_end = .;
-  } :text
+  }
 #endif
 #endif
 
@@ -260,7 +266,7 @@ SECTIONS
        *(SORT(.data.vpci.*))
        __end_vpci_array = .;
 #endif
-  } :text
+  } PHDR(text)
 
   . = ALIGN(SECTION_ALIGN);
   __init_end = .;
@@ -281,7 +287,7 @@ SECTIONS
        *(.data.paramhypfs)
        __paramhypfs_end = .;
 #endif
-  } :text
+  } PHDR(text)
 
   DECL_SECTION(.data) {
        *(.data.page_aligned)
@@ -289,7 +295,7 @@ SECTIONS
        *(.data.rel)
        *(.data.rel.*)
        CONSTRUCTORS
-  } :text
+  } PHDR(text)
 
   DECL_SECTION(.bss) {
        __bss_start = .;
@@ -306,7 +312,7 @@ SECTIONS
        *(.bss)
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
-  } :text
+  } PHDR(text)
   _end = . ;
 
   . = ALIGN(SECTION_ALIGN);
@@ -316,12 +322,12 @@ SECTIONS
   . = ALIGN(4);
   DECL_SECTION(.reloc) {
     *(.reloc)
-  } :text
+  }
   /* Trick the linker into setting the image size to exactly 16Mb. */
   . = ALIGN(__section_alignment__);
   DECL_SECTION(.pad) {
     . = ALIGN(MB(16));
-  } :text
+  }
 #endif
 
 #ifndef XEN_BUILD_EFI



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 4/8] x86/EFI: redo .reloc section bounds determination
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (2 preceding siblings ...)
  2021-04-01  9:45 ` [PATCH 3/8] x86/EFI: program headers are an ELF concept Jan Beulich
@ 2021-04-01  9:45 ` Jan Beulich
  2021-04-21  9:46   ` Roger Pau Monné
  2021-04-01  9:46 ` [PATCH 5/8] x86: drop use of prelink-efi.o Jan Beulich
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:45 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

There's no need to link relocs-dummy.o into the ELF binary. The two
symbols needed can as well be provided by the linker script. Then our
mkreloc tool also doesn't need to put them in the generated assembler
source.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -133,7 +133,6 @@ XEN_NO_PE_FIXUPS := $(if $(XEN_BUILD_EFI
 endif
 
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
-EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
 
 ifeq ($(CONFIG_LTO),y)
 # Gather all LTO objects together
@@ -141,13 +140,13 @@ prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
 	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
 
 # Link it with all the binary objects
-prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
+prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
 	$(call if_changed,ld)
 
 prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
 	$(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE
+prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
 	$(call if_changed,ld)
 
 prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -320,9 +320,7 @@ int main(int argc, char *argv[])
     }
 
     puts("\t.section .reloc, \"a\", @progbits\n"
-         "\t.balign 4\n"
-         "\t.globl __base_relocs_start, __base_relocs_end\n"
-         "__base_relocs_start:");
+         "\t.balign 4");
 
     for ( i = 0; i < nsec; ++i )
     {
@@ -373,8 +371,6 @@ int main(int argc, char *argv[])
 
     diff_sections(NULL, NULL, NULL, 0, 0, 0, 0);
 
-    puts("__base_relocs_end:");
-
     close(in1);
     close(in2);
 
--- a/xen/arch/x86/efi/relocs-dummy.S
+++ b/xen/arch/x86/efi/relocs-dummy.S
@@ -1,10 +1,8 @@
 
 	.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
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -170,18 +170,6 @@ SECTIONS
 #endif
 #endif
 
-/*
- * ELF builds are linked to a fixed virtual address, and in principle
- * shouldn't have a .reloc section.  However, due to the way EFI support is
- * currently implemented, retaining the .reloc section is necessary.
- */
-#if defined(XEN_BUILD_EFI) && !defined(EFI)
-  . = ALIGN(4);
-  DECL_SECTION(.reloc) {
-    *(.reloc)
-  } :text
-#endif
-
   _erodata = .;
 
   . = ALIGN(SECTION_ALIGN);
@@ -319,18 +307,27 @@ SECTIONS
   __2M_rwdata_end = .;
 
 #ifdef EFI
-  . = ALIGN(4);
-  DECL_SECTION(.reloc) {
+  .reloc ALIGN(4) : {
+    __base_relocs_start = .;
     *(.reloc)
+    __base_relocs_end = .;
   }
   /* Trick the linker into setting the image size to exactly 16Mb. */
   . = ALIGN(__section_alignment__);
   DECL_SECTION(.pad) {
     . = ALIGN(MB(16));
   }
-#endif
-
-#ifndef XEN_BUILD_EFI
+#elif defined(XEN_BUILD_EFI)
+  /*
+   * Due to the way EFI support is currently implemented, these two symbols
+   * need to be defined.  Their precise values shouldn't matter (the consuming
+   * function doesn't get called), but to be on the safe side both values would
+   * better match.  Of course the need to be reachable by the relocations
+   * referencing them.
+   */
+  PROVIDE(__base_relocs_start = .);
+  PROVIDE(__base_relocs_end = .);
+#else
   efi = .;
 #endif
 



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 5/8] x86: drop use of prelink-efi.o
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (3 preceding siblings ...)
  2021-04-01  9:45 ` [PATCH 4/8] x86/EFI: redo .reloc section bounds determination Jan Beulich
@ 2021-04-01  9:46 ` Jan Beulich
  2021-04-21  9:51   ` Roger Pau Monné
  2021-04-01  9:46 ` [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible Jan Beulich
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

Now that its contents matches prelink.o, use that one uniformly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -142,18 +142,12 @@ prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
 	$(call if_changed,ld)
-
-prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
-	$(call if_changed,ld)
 else
 prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
 	$(call if_changed,ld)
-
-prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
-	$(call if_changed,ld)
 endif
 
-targets += prelink.o prelink-efi.o
+targets += prelink.o
 
 $(TARGET)-syms: prelink.o xen.lds
 	$(LD) $(XEN_LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
@@ -214,7 +208,7 @@ endif
 note_file_option ?= $(note_file)
 
 ifeq ($(XEN_BUILD_PE),y)
-$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
+$(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
 	                $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) :



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (4 preceding siblings ...)
  2021-04-01  9:46 ` [PATCH 5/8] x86: drop use of prelink-efi.o Jan Beulich
@ 2021-04-01  9:46 ` Jan Beulich
  2021-04-21 10:21   ` Roger Pau Monné
  2021-04-01  9:47 ` [PATCH 7/8] x86/EFI: keep debug info in xen.efi Jan Beulich
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:46 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

As of commit 6fa7408d72b3 ("ld: don't generate base relocations in PE
output for absolute symbols") I'm feeling sufficiently confident in GNU
ld to use its logic for generating base relocations, which was enabled
for executables at some point last year (prior to that this would have
got done only for DLLs).

GNU ld, seeing the original relocations coming from the ELF object files,
generates different relocation types for our page tables (64-bit ones,
while mkreloc produces 32-bit ones). This requires also permitting and
handling that type in efi_arch_relocate_image().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -120,18 +120,37 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/
 	mv $(TMP) $(TARGET)
 
 ifneq ($(efi-y),)
+
 # Check if the compiler supports the MS ABI.
 export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y)
+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))
-CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
-# Check if the linker produces fixups in PE by default (we need to disable it doing so for now).
-XEN_NO_PE_FIXUPS := $(if $(XEN_BUILD_EFI), \
-                         $(shell $(LD) $(EFI_LDFLAGS) --disable-reloc-section -o efi/check.efi efi/check.o 2>/dev/null && \
-                                 echo --disable-reloc-section))
+
+ifeq ($(XEN_BUILD_PE),y)
+
+# Check if the linker produces fixups in PE by default
+nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l)
+ifeq ($(nr-fixups),2)
+MKRELOC := :
+relocs-dummy :=
+else
+MKRELOC := efi/mkreloc
+relocs-dummy := efi/relocs-dummy.o
+# If the linker produced fixups but not precisely two of them, we need to
+# disable it doing so.  But if it didn't produce any fixups, it also wouldn't
+# recognize the option.
+ifneq ($(nr-fixups),0)
+EFI_LDFLAGS += --disable-reloc-section
+endif
 endif
 
+endif # $(XEN_BUILD_PE)
+
+endif # $(efi-y)
+
 ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
 
 ifeq ($(CONFIG_LTO),y)
@@ -175,7 +194,7 @@ note.o: $(TARGET)-syms
 		--rename-section=.data=.note.gnu.build-id -S $@.bin $@
 	rm -f $@.bin
 
-EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0 $(XEN_NO_PE_FIXUPS)
+EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0
 EFI_LDFLAGS += --section-alignment=0x200000 --file-alignment=0x20
 EFI_LDFLAGS += --major-image-version=$(XEN_VERSION)
 EFI_LDFLAGS += --minor-image-version=$(XEN_SUBVERSION)
@@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
 endif
 
 $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
+ifeq ($(MKRELOC),:)
+$(TARGET).efi: ALT_BASE :=
+else
 $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
+endif
 
 ifneq ($(build_id_linker),)
 ifeq ($(call ld-ver-build-id,$(LD) $(filter -m%,$(EFI_LDFLAGS))),y)
@@ -210,16 +233,16 @@ note_file_option ?= $(note_file)
 ifeq ($(XEN_BUILD_PE),y)
 $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
-	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
+	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< $(relocs-dummy) \
 	                $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) :
-	efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
+	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
 	$(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
 		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file_option) -o $(@D)/.$(@F).$(base).1 &&) :
-	efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
+	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
 	$(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
 		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
--- a/xen/arch/x86/efi/check.c
+++ b/xen/arch/x86/efi/check.c
@@ -2,3 +2,17 @@ int __attribute__((__ms_abi__)) test(int
 {
     return i;
 }
+
+/*
+ * Populate an array with "addresses" of relocatable and absolute values.
+ * This is to probe ld for (a) emitting base relocations at all and (b) not
+ * emitting base relocations for absolute symbols.
+ */
+extern const unsigned char __image_base__[], __file_alignment__[],
+                           __section_alignment__[];
+const void *const data[] = {
+    __image_base__,
+    __file_alignment__,
+    __section_alignment__,
+    data,
+};
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -86,10 +86,12 @@ static void __init efi_arch_relocate_ima
                 }
                 break;
             case PE_BASE_RELOC_DIR64:
-                if ( in_page_tables(addr) )
-                    blexit(L"Unexpected relocation type");
                 if ( delta )
+                {
                     *(u64 *)addr += delta;
+                    if ( in_page_tables(addr) )
+                        *(u64 *)addr += xen_phys_start;
+                }
                 break;
             default:
                 blexit(L"Unsupported relocation type");



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (5 preceding siblings ...)
  2021-04-01  9:46 ` [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible Jan Beulich
@ 2021-04-01  9:47 ` Jan Beulich
  2021-04-21 11:15   ` Roger Pau Monné
  2021-04-01  9:47 ` [PATCH 8/8] x86/EFI: don't have an overly large image size Jan Beulich
  2021-04-15  9:53 ` Ping: [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:47 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

... 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>
---
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
@@ -126,8 +126,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)
 
@@ -232,6 +238,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,60 @@ 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));
+  .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)
   /*



^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 8/8] x86/EFI: don't have an overly large image size
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (6 preceding siblings ...)
  2021-04-01  9:47 ` [PATCH 7/8] x86/EFI: keep debug info in xen.efi Jan Beulich
@ 2021-04-01  9:47 ` Jan Beulich
  2021-04-21 11:18   ` Roger Pau Monné
  2021-04-15  9:53 ` Ping: [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
  8 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-01  9:47 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

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>
---
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) : {



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs
  2021-04-01  9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
@ 2021-04-01 11:51   ` Andrew Cooper
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Cooper @ 2021-04-01 11:51 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné

On 01/04/2021 10:44, Jan Beulich wrote:
> As of commit a6066af5b142 ("xen/init: Annotate all command line
> parameter infrastructure as const") .init.setup has been part of .init.
> As of commit 544ad7f5caf5 ("xen/init: Move initcall infrastructure into
> .init.data") .initcall* have been part of .init. Hence neither can be
> encountered as a stand-alone section in the final binaries anymore.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-01  9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
@ 2021-04-01 12:01   ` Andrew Cooper
  2021-04-01 13:51     ` Jan Beulich
  2021-04-21  8:52   ` Roger Pau Monné
  1 sibling, 1 reply; 48+ messages in thread
From: Andrew Cooper @ 2021-04-01 12:01 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné

On 01/04/2021 10:44, 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>
> ---
> It's certainly odd that we have stabs section entries in the script, but
> no Dwarf ones.

Its not odd in the slightest, given the heritage and lack of anyone
touching the linker file unless something is broken.

We've got dwarf symbols in xen-syms, have we not?

~Andrew



^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-01 12:01   ` Andrew Cooper
@ 2021-04-01 13:51     ` Jan Beulich
  0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-01 13:51 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Roger Pau Monné, xen-devel

On 01.04.2021 14:01, Andrew Cooper wrote:
> On 01/04/2021 10:44, 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>
>> ---
>> It's certainly odd that we have stabs section entries in the script, but
>> no Dwarf ones.
> 
> Its not odd in the slightest, given the heritage and lack of anyone
> touching the linker file unless something is broken.

Heritage? Was stabs debug info ever used in any build of Xen?

> We've got dwarf symbols in xen-syms, have we not?

Yes, and that's why I mention the oddity: We have Dwarf debug info (and
hence .debug_* sections) in xen-syms without mentioning them in the
script, and we don't have stabs debug info in xen-syms yet we mention
the sections.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Ping: [PATCH 0/8] x86/EFI: build adjustments
  2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
                   ` (7 preceding siblings ...)
  2021-04-01  9:47 ` [PATCH 8/8] x86/EFI: don't have an overly large image size Jan Beulich
@ 2021-04-15  9:53 ` Jan Beulich
  8 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-15  9:53 UTC (permalink / raw)
  To: Andrew Cooper, Wei Liu, Roger Pau Monné; +Cc: xen-devel

On 01.04.2021 11:43, Jan Beulich wrote:
> So far we've taken care of just the immediate breakage caused by
> binutils 2.36. But we can also take advantage, in particular to
> avoid "manually" creating base relocations for xen.efi. Since it
> was requested and is possible with up-to-date binutils, inclusion
> of debug info in xen.efi is another 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: drop stale section special casing when generating base relocs
> 2: EFI: sections may not live at VA 0 in PE binaries
> 3: EFI: program headers are an ELF concept
> 4: EFI: redo .reloc section bounds determination
> 5: drop use of prelink-efi.o
> 6: EFI: avoid use of GNU ld's --disable-reloc-section when possible
> 7: EFI: keep debug info in xen.efi
> 8: EFI: don't have an overly large image size

Only the first patch here has been acked (and has gone in). Any chance
of getting acks (or otherwise) for the rest?

Thanks, Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-01  9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
  2021-04-01 12:01   ` Andrew Cooper
@ 2021-04-21  8:52   ` Roger Pau Monné
  2021-04-21 10:32     ` Jan Beulich
  1 sibling, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21  8:52 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:44:45AM +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.

It's my understadng thonse sections are only there for debug purposes,
and never part of the final xen binary as they are stripped?

Could we maybe remove the section load address of 0 and instead just
use the (NOLOAD) directive?

Does it really matter to place them at address 0?

I also wonder, is this change fixing some existing bug, or it's just a
cleanup change?

I also only see the .comment section in my binary output, so maybe
it's fine to just remove them from the script?

Does the Arm linker script need a similar treatment?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/8] x86/EFI: program headers are an ELF concept
  2021-04-01  9:45 ` [PATCH 3/8] x86/EFI: program headers are an ELF concept Jan Beulich
@ 2021-04-21  9:11   ` Roger Pau Monné
  2021-04-21 10:36     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21  9:11 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:45:09AM +0200, Jan Beulich wrote:
> While they apparently do no harm when building xen.efi, their use is
> potentially misleading. Conditionalize their use to be for just the ELF
> binary we produce.
> 
> No change to the resulting binaries.

The GNU Linker manual notes that program headers would be ignored when
not generating an ELF file, so I'm not sure it's worth us adding more
churn to the linker script to hide something that's already ignored by
ld already.

Maybe adding a comment noting program headers are ignored when not
generating an ELF output would be enough?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 4/8] x86/EFI: redo .reloc section bounds determination
  2021-04-01  9:45 ` [PATCH 4/8] x86/EFI: redo .reloc section bounds determination Jan Beulich
@ 2021-04-21  9:46   ` Roger Pau Monné
  2021-04-21 10:44     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21  9:46 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote:
> There's no need to link relocs-dummy.o into the ELF binary. The two
> symbols needed can as well be provided by the linker script. Then our
> mkreloc tool also doesn't need to put them in the generated assembler
> source.

Maybe I'm just dense today, but I think the message needs to be
expanded a bit to mention that while the __base_relocs_{start,end} are
not used when loaded as an EFI application, they are used by the EFI
code in Xen when booted using the multiboot2 protocol for example, as
they are used by efi_arch_relocate_image.

I think relocation is not needed when natively loaded as an EFI
application, as then the load address matches the one expected by
Xen?

I also wonder, at some point there where plans for providing a single
binary that would work as multiboot{1,2} and also be capable of being
loaded as an EFI application (ie: have a PE/COFF header also I assume
together with the ELF one), won't the changes here make it more
difficult to reach that goal or require reverting later on, as I feel
they are adding more differences between the PE binary and the ELF
one.

The code LGTM, but I think at least I would like the commit message to
be expanded.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 5/8] x86: drop use of prelink-efi.o
  2021-04-01  9:46 ` [PATCH 5/8] x86: drop use of prelink-efi.o Jan Beulich
@ 2021-04-21  9:51   ` Roger Pau Monné
  0 siblings, 0 replies; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21  9:51 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:46:00AM +0200, Jan Beulich wrote:
> Now that its contents matches prelink.o, use that one uniformly.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-01  9:46 ` [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible Jan Beulich
@ 2021-04-21 10:21   ` Roger Pau Monné
  2021-04-21 12:03     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 10:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
> As of commit 6fa7408d72b3 ("ld: don't generate base relocations in PE
> output for absolute symbols") I'm feeling sufficiently confident in GNU
> ld to use its logic for generating base relocations, which was enabled
> for executables at some point last year (prior to that this would have
> got done only for DLLs).
> 
> GNU ld, seeing the original relocations coming from the ELF object files,
> generates different relocation types for our page tables (64-bit ones,
> while mkreloc produces 32-bit ones). This requires also permitting and
> handling that type in efi_arch_relocate_image().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -120,18 +120,37 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/
>  	mv $(TMP) $(TARGET)
>  
>  ifneq ($(efi-y),)
> +
>  # Check if the compiler supports the MS ABI.
>  export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check.o 2>/dev/null && echo y)
> +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))
> -CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI
> -# Check if the linker produces fixups in PE by default (we need to disable it doing so for now).
> -XEN_NO_PE_FIXUPS := $(if $(XEN_BUILD_EFI), \
> -                         $(shell $(LD) $(EFI_LDFLAGS) --disable-reloc-section -o efi/check.efi efi/check.o 2>/dev/null && \
> -                                 echo --disable-reloc-section))
> +
> +ifeq ($(XEN_BUILD_PE),y)
> +
> +# Check if the linker produces fixups in PE by default
> +nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l)
> +ifeq ($(nr-fixups),2)
> +MKRELOC := :
> +relocs-dummy :=
> +else
> +MKRELOC := efi/mkreloc
> +relocs-dummy := efi/relocs-dummy.o
> +# If the linker produced fixups but not precisely two of them, we need to
> +# disable it doing so.  But if it didn't produce any fixups, it also wouldn't
> +# recognize the option.
> +ifneq ($(nr-fixups),0)
> +EFI_LDFLAGS += --disable-reloc-section
> +endif
>  endif
>  
> +endif # $(XEN_BUILD_PE)
> +
> +endif # $(efi-y)
> +
>  ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
>  
>  ifeq ($(CONFIG_LTO),y)
> @@ -175,7 +194,7 @@ note.o: $(TARGET)-syms
>  		--rename-section=.data=.note.gnu.build-id -S $@.bin $@
>  	rm -f $@.bin
>  
> -EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0 $(XEN_NO_PE_FIXUPS)
> +EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0
>  EFI_LDFLAGS += --section-alignment=0x200000 --file-alignment=0x20
>  EFI_LDFLAGS += --major-image-version=$(XEN_VERSION)
>  EFI_LDFLAGS += --minor-image-version=$(XEN_SUBVERSION)
> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
>  endif
>  
>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
> +ifeq ($(MKRELOC),:)
> +$(TARGET).efi: ALT_BASE :=
> +else
>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')

Could you maybe check whether $(relocs-dummy) is set as the condition
here and use it here instead of efi/relocs-dummy.o?

> +endif
>  
>  ifneq ($(build_id_linker),)
>  ifeq ($(call ld-ver-build-id,$(LD) $(filter -m%,$(EFI_LDFLAGS))),y)
> @@ -210,16 +233,16 @@ note_file_option ?= $(note_file)
>  ifeq ($(XEN_BUILD_PE),y)
>  $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc

Do you need to also replace the target prerequisite to use $(relocs-dummy)?

>  	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
> -	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
> +	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< $(relocs-dummy) \
>  	                $(BASEDIR)/common/symbols-dummy.o $(note_file_option) -o $(@D)/.$(@F).$(base).0 &&) :
> -	efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
> +	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
>  	$(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
>  		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
>  	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
>  	          $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
>  	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file_option) -o $(@D)/.$(@F).$(base).1 &&) :
> -	efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
> +	$(MKRELOC) $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
>  	$(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
>  		| $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
> --- a/xen/arch/x86/efi/check.c
> +++ b/xen/arch/x86/efi/check.c
> @@ -2,3 +2,17 @@ int __attribute__((__ms_abi__)) test(int
>  {
>      return i;
>  }
> +
> +/*
> + * Populate an array with "addresses" of relocatable and absolute values.
> + * This is to probe ld for (a) emitting base relocations at all and (b) not
> + * emitting base relocations for absolute symbols.
> + */
> +extern const unsigned char __image_base__[], __file_alignment__[],
> +                           __section_alignment__[];
> +const void *const data[] = {
> +    __image_base__,
> +    __file_alignment__,
> +    __section_alignment__,
> +    data,
> +};
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -86,10 +86,12 @@ static void __init efi_arch_relocate_ima
>                  }
>                  break;
>              case PE_BASE_RELOC_DIR64:
> -                if ( in_page_tables(addr) )
> -                    blexit(L"Unexpected relocation type");
>                  if ( delta )
> +                {
>                      *(u64 *)addr += delta;
> +                    if ( in_page_tables(addr) )
> +                        *(u64 *)addr += xen_phys_start;

Doesn't the in_page_tables check and modification also apply when
delta == 0?

Maybe you could just break on !delta to reduce indentation if none of
this applies then?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-21  8:52   ` Roger Pau Monné
@ 2021-04-21 10:32     ` Jan Beulich
  2021-04-21 12:57       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 10:32 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 10:52, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:44:45AM +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.
> 
> It's my understadng thonse sections are only there for debug purposes,
> and never part of the final xen binary as they are stripped?
> 
> Could we maybe remove the section load address of 0 and instead just
> use the (NOLOAD) directive?

(NOLOAD) is meaningless for PE.

> Does it really matter to place them at address 0?

That's the convention for ELF, and also what ld defaults to for debugging
sections.

> I also wonder, is this change fixing some existing bug, or it's just a
> cleanup change?

If there were sections at 0, the resulting PE binary would end up broken.
Prior to binutils 2.37 this brokenness is silent, i.e. the linker doesn't
issue any form of diagnostic. The change therefore is addressing a latent
issue - if any such section started being non-empty, we'd be in trouble.

> I also only see the .comment section in my binary output, so maybe
> it's fine to just remove them from the script?

Which binary are you referring to - ELF or PE? In the former case, yes,
that's what the statement is for. In the latter case I can't see how this
would be, with .comment being explicitly part of /DISCARD/ in that case.

> Does the Arm linker script need a similar treatment?

No idea - they don't use ld to produce a PE binary. In fact during my
work on the binutils side for all of this, I was given a hint that on Arm
linking ELF objects into PE output may currently not be possible at all.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/8] x86/EFI: program headers are an ELF concept
  2021-04-21  9:11   ` Roger Pau Monné
@ 2021-04-21 10:36     ` Jan Beulich
  2021-04-21 14:21       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 10:36 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 11:11, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:45:09AM +0200, Jan Beulich wrote:
>> While they apparently do no harm when building xen.efi, their use is
>> potentially misleading. Conditionalize their use to be for just the ELF
>> binary we produce.
>>
>> No change to the resulting binaries.
> 
> The GNU Linker manual notes that program headers would be ignored when
> not generating an ELF file, so I'm not sure it's worth us adding more
> churn to the linker script to hide something that's already ignored by
> ld already.
> 
> Maybe adding a comment noting program headers are ignored when not
> generating an ELF output would be enough?

Maybe, but I'd prefer this to be explicit, and I'd prefer for efi.lds
to not have any PE-unrelated baggage. The churn by this patch isn't
all this significant, is it? In fact in two cases it actually deletes
meaningless stuff.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 4/8] x86/EFI: redo .reloc section bounds determination
  2021-04-21  9:46   ` Roger Pau Monné
@ 2021-04-21 10:44     ` Jan Beulich
  2021-04-21 14:54       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 10:44 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 11:46, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote:
>> There's no need to link relocs-dummy.o into the ELF binary. The two
>> symbols needed can as well be provided by the linker script. Then our
>> mkreloc tool also doesn't need to put them in the generated assembler
>> source.
> 
> Maybe I'm just dense today, but I think the message needs to be
> expanded a bit to mention that while the __base_relocs_{start,end} are
> not used when loaded as an EFI application, they are used by the EFI
> code in Xen when booted using the multiboot2 protocol for example, as
> they are used by efi_arch_relocate_image.
> 
> I think relocation is not needed when natively loaded as an EFI
> application, as then the load address matches the one expected by
> Xen?

It's quite the other way around: The EFI loader applies relocations
to put the binary at its loaded _physical_ address (the image gets
linked for the final virtual address). Hence we need to apply the
same relocations a 2nd time (undoing what the EFI loader did)
before we can branch from the physical (identity mapped) address
range where xen.efi was loaded to the intended virtual address
range where we mean to run Xen from.

For the ELF binary the symbols are needed solely to make ld happy.

> I also wonder, at some point there where plans for providing a single
> binary that would work as multiboot{1,2} and also be capable of being
> loaded as an EFI application (ie: have a PE/COFF header also I assume
> together with the ELF one), won't the changes here make it more
> difficult to reach that goal or require reverting later on, as I feel
> they are adding more differences between the PE binary and the ELF
> one.

There were such plans, yes, but from the last round of that series
I seem to recall that there was at least one issue breaking this
idea. So no, at this point I'm not intending to take precautions to
make that work easier (or not further complicate it). This said, I
don't think the change here complicates anything there.

> The code LGTM, but I think at least I would like the commit message to
> be expanded.

Well, once I know what exactly you're missing there, I can certainly
try to expand it.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-01  9:47 ` [PATCH 7/8] x86/EFI: keep debug info in xen.efi Jan Beulich
@ 2021-04-21 11:15   ` Roger Pau Monné
  2021-04-21 13:06     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 11:15 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:47:03AM +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.)

Isn't just using (NOLOAD) to signal those sections shouldn't be
loaded enough, and thus don't care about it's RVA?

> 
> 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>
> ---
> 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
> @@ -126,8 +126,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)
>  
> @@ -232,6 +238,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,60 @@ 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));
> +  .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)) : .;

I think this is inside an ifdef EFI region, since this is DWARF info
couldn't it also be present when building with EFI disabled?

Maybe I'm missing part of the point here, sorry.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 8/8] x86/EFI: don't have an overly large image size
  2021-04-01  9:47 ` [PATCH 8/8] x86/EFI: don't have an overly large image size Jan Beulich
@ 2021-04-21 11:18   ` Roger Pau Monné
  2021-04-21 13:15     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 11:18 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 01, 2021 at 11:47:35AM +0200, Jan Beulich wrote:
> 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.

So you just expand the load size.

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

We should likely define _end after __2M_rwdata_end to account for this
padding?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-21 10:21   ` Roger Pau Monné
@ 2021-04-21 12:03     ` Jan Beulich
  2021-04-21 15:20       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 12:03 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 12:21, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
>> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
>>  endif
>>  
>>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
>> +ifeq ($(MKRELOC),:)
>> +$(TARGET).efi: ALT_BASE :=
>> +else
>>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
> 
> Could you maybe check whether $(relocs-dummy) is set as the condition
> here and use it here instead of efi/relocs-dummy.o?

I can use it in the ifeq() if you think that's neater (the current way
is minimally shorter), but using it in the ALT_BASE assignment would
make this differ more from the VIRT_BASE one, which I'd like to avoid.

>> @@ -210,16 +233,16 @@ note_file_option ?= $(note_file)
>>  ifeq ($(XEN_BUILD_PE),y)
>>  $(TARGET).efi: prelink.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
> 
> Do you need to also replace the target prerequisite to use $(relocs-dummy)?

No - without the dependency the file might not be generated (if this ends
up being the only real dependency on $(BASEDIR)/arch/x86/efi/built_in.o).
We can't rely on $(note_file) resolving to efi/buildid.o, and hence
recursing into $(BASEDIR)/arch/x86/efi/ may not otherwise get triggered.
Yet to calculate VIRT_BASE we need efi/relocs-dummy.o.

>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -86,10 +86,12 @@ static void __init efi_arch_relocate_ima
>>                  }
>>                  break;
>>              case PE_BASE_RELOC_DIR64:
>> -                if ( in_page_tables(addr) )
>> -                    blexit(L"Unexpected relocation type");
>>                  if ( delta )
>> +                {
>>                      *(u64 *)addr += delta;
>> +                    if ( in_page_tables(addr) )
>> +                        *(u64 *)addr += xen_phys_start;
> 
> Doesn't the in_page_tables check and modification also apply when
> delta == 0?

No, it would be wrong to do so: efi_arch_load_addr_check() sets
xen_phys_start, and subsequently (to still be able to produce human
visible output) we invoke efi_arch_relocate_image() with an argument
of 0. Later we'll invoke efi_arch_relocate_image() a 2nd time (when
having exited boot services already, and hence when we can't produce
output via EFI anymore, and we can't produce output yet via Xen's
normal mechanisms), with a non-zero argument. Thus we'd add in
xen_phys_start twice.

> Maybe you could just break on !delta to reduce indentation if none of
> this applies then?

Could be done, sure, and if you think this makes sufficiently much
of a difference I can add a patch. The purpose here though it to
have this and the preceding case block look as similar as possible,
yet also not re-format that earlier one (which would be an unrelated
change).

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-21 10:32     ` Jan Beulich
@ 2021-04-21 12:57       ` Roger Pau Monné
  2021-04-21 13:28         ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 12:57 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 12:32:42PM +0200, Jan Beulich wrote:
> On 21.04.2021 10:52, Roger Pau Monné wrote:
> > On Thu, Apr 01, 2021 at 11:44:45AM +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.
> > 
> > It's my understadng thonse sections are only there for debug purposes,
> > and never part of the final xen binary as they are stripped?
> > 
> > Could we maybe remove the section load address of 0 and instead just
> > use the (NOLOAD) directive?
> 
> (NOLOAD) is meaningless for PE.
> 
> > Does it really matter to place them at address 0?
> 
> That's the convention for ELF, and also what ld defaults to for debugging
> sections.
> 
> > I also wonder, is this change fixing some existing bug, or it's just a
> > cleanup change?
> 
> If there were sections at 0, the resulting PE binary would end up broken.
> Prior to binutils 2.37 this brokenness is silent, i.e. the linker doesn't
> issue any form of diagnostic. The change therefore is addressing a latent
> issue - if any such section started being non-empty, we'd be in trouble.
> 
> > I also only see the .comment section in my binary output, so maybe
> > it's fine to just remove them from the script?
> 
> Which binary are you referring to - ELF or PE? In the former case, yes,
> that's what the statement is for. In the latter case I can't see how this
> would be, with .comment being explicitly part of /DISCARD/ in that case.

So from a bit of searching I just did it seems like stab sections
where used during the 90s with ELF, but that this has long been
superseded by DWARF 2 becoming the default in the late 90s, hence I
think it would be fine to just remove those sections even in the ELF
case?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-21 11:15   ` Roger Pau Monné
@ 2021-04-21 13:06     ` Jan Beulich
  2021-04-21 15:30       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 13:06 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 13:15, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:47:03AM +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.)
> 
> Isn't just using (NOLOAD) to signal those sections shouldn't be
> loaded enough, and thus don't care about it's RVA?

As said in a reply earlier on another sub-thread, (NOLOAD) is meaningless
for PE. The fact that I add them nevertheless is just for docs purposes
(or if, in the future, the item gains significance).

The main problem though isn't "load" vs "no-load" but, as I thought I
have expressed in the description, that there's no "don't care about it's
RVA" in PE. All sections have to have a non-zero VA above the image base.
This is the only way via which sane RVA values can result. If sections
get placed at VA 0 (which is perfectly fine for ELF), the RVA would be a
huge negative number truncated to 32 bits. (Again, prior to binutils 2.37
this will go all silently.) Plus the respective sections would come first
(rather than last) in the binary (which by itself may or may not be a
problem for the EFI loader, but I wouldn't want to chance it).

>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -312,10 +312,60 @@ 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));
>> +  .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)) : .;
> 
> I think this is inside an ifdef EFI region, since this is DWARF info
> couldn't it also be present when building with EFI disabled?

Of course (and it's not just "could" but "will"), yet the linker will
do fine (and perhaps even better) without when building ELF. Also
note that we'll be responsible for keeping the list of sections up-to-
date. The linker will recognize Dwarf sections by looking for a
.debug_ prefix. We can't use such here (or at least I'm not aware of
a suitable mechanism); .debug_* would mean munging together all the
different kinds of Dwarf sections. Hence by limiting the explicit
enumeration to PE, I'm trying to avoid anomalies in ELF down the road.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 8/8] x86/EFI: don't have an overly large image size
  2021-04-21 11:18   ` Roger Pau Monné
@ 2021-04-21 13:15     ` Jan Beulich
  0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 13:15 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 13:18, Roger Pau Monné wrote:
> On Thu, Apr 01, 2021 at 11:47:35AM +0200, Jan Beulich wrote:
>> 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.
> 
> So you just expand the load size.

Shrink. Or maybe I'm misunderstanding you.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> 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?
> 
> We should likely define _end after __2M_rwdata_end to account for this
> padding?

I don't think this would help - we'd need to arrange for the image size
to cover that extra padding. Like advancing . doesn't grow the image
size, I also don't think placing _end later would do.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries
  2021-04-21 12:57       ` Roger Pau Monné
@ 2021-04-21 13:28         ` Jan Beulich
  0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 13:28 UTC (permalink / raw)
  To: Roger Pau Monné, Andrew Cooper; +Cc: xen-devel, Wei Liu

On 21.04.2021 14:57, Roger Pau Monné wrote:
> So from a bit of searching I just did it seems like stab sections
> where used during the 90s with ELF, but that this has long been
> superseded by DWARF 2 becoming the default in the late 90s, hence I
> think it would be fine to just remove those sections even in the ELF
> case?

Well, maybe. Even up-to-date gcc still supports -gstabs. Plus I seem
to have a vague recollection that Andrew objected to their removal
at some (not overly distant) point. I can't find any reference there
though, so Andrew: Do you have any specific thoughts here?

Of course with the stabs sections gone, .comment would remain. I'm
very firm in not wanting to leave any statements there putting
sections at VA zero. Irrespective of .comment (to take this example)
being listed under /DISCARD/ for PE. This would only be acceptable
(to me) if ld would always have at least warned about such sections.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/8] x86/EFI: program headers are an ELF concept
  2021-04-21 10:36     ` Jan Beulich
@ 2021-04-21 14:21       ` Roger Pau Monné
  2021-04-21 14:30         ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 14:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 12:36:16PM +0200, Jan Beulich wrote:
> On 21.04.2021 11:11, Roger Pau Monné wrote:
> > On Thu, Apr 01, 2021 at 11:45:09AM +0200, Jan Beulich wrote:
> >> While they apparently do no harm when building xen.efi, their use is
> >> potentially misleading. Conditionalize their use to be for just the ELF
> >> binary we produce.
> >>
> >> No change to the resulting binaries.
> > 
> > The GNU Linker manual notes that program headers would be ignored when
> > not generating an ELF file, so I'm not sure it's worth us adding more
> > churn to the linker script to hide something that's already ignored by
> > ld already.
> > 
> > Maybe adding a comment noting program headers are ignored when not
> > generating an ELF output would be enough?
> 
> Maybe, but I'd prefer this to be explicit, and I'd prefer for efi.lds
> to not have any PE-unrelated baggage. The churn by this patch isn't
> all this significant, is it? In fact in two cases it actually deletes
> meaningless stuff.

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

I would prefer if the new PHDR macro was used for all program headers
directives for consistency though.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/8] x86/EFI: program headers are an ELF concept
  2021-04-21 14:21       ` Roger Pau Monné
@ 2021-04-21 14:30         ` Jan Beulich
  0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 14:30 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 16:21, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 12:36:16PM +0200, Jan Beulich wrote:
>> On 21.04.2021 11:11, Roger Pau Monné wrote:
>>> On Thu, Apr 01, 2021 at 11:45:09AM +0200, Jan Beulich wrote:
>>>> While they apparently do no harm when building xen.efi, their use is
>>>> potentially misleading. Conditionalize their use to be for just the ELF
>>>> binary we produce.
>>>>
>>>> No change to the resulting binaries.
>>>
>>> The GNU Linker manual notes that program headers would be ignored when
>>> not generating an ELF file, so I'm not sure it's worth us adding more
>>> churn to the linker script to hide something that's already ignored by
>>> ld already.
>>>
>>> Maybe adding a comment noting program headers are ignored when not
>>> generating an ELF output would be enough?
>>
>> Maybe, but I'd prefer this to be explicit, and I'd prefer for efi.lds
>> to not have any PE-unrelated baggage. The churn by this patch isn't
>> all this significant, is it? In fact in two cases it actually deletes
>> meaningless stuff.
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> I would prefer if the new PHDR macro was used for all program headers
> directives for consistency though.

Well, yes, I can certainly do so.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 4/8] x86/EFI: redo .reloc section bounds determination
  2021-04-21 10:44     ` Jan Beulich
@ 2021-04-21 14:54       ` Roger Pau Monné
  0 siblings, 0 replies; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 14:54 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 12:44:13PM +0200, Jan Beulich wrote:
> On 21.04.2021 11:46, Roger Pau Monné wrote:
> > On Thu, Apr 01, 2021 at 11:45:38AM +0200, Jan Beulich wrote:
> >> There's no need to link relocs-dummy.o into the ELF binary. The two
> >> symbols needed can as well be provided by the linker script. Then our
> >> mkreloc tool also doesn't need to put them in the generated assembler
> >> source.
> > 
> > Maybe I'm just dense today, but I think the message needs to be
> > expanded a bit to mention that while the __base_relocs_{start,end} are
> > not used when loaded as an EFI application, they are used by the EFI
> > code in Xen when booted using the multiboot2 protocol for example, as
> > they are used by efi_arch_relocate_image.
> > 
> > I think relocation is not needed when natively loaded as an EFI
> > application, as then the load address matches the one expected by
> > Xen?
> 
> It's quite the other way around: The EFI loader applies relocations
> to put the binary at its loaded _physical_ address (the image gets
> linked for the final virtual address). Hence we need to apply the
> same relocations a 2nd time (undoing what the EFI loader did)
> before we can branch from the physical (identity mapped) address
> range where xen.efi was loaded to the intended virtual address
> range where we mean to run Xen from.
> 
> For the ELF binary the symbols are needed solely to make ld happy.
> 
> > I also wonder, at some point there where plans for providing a single
> > binary that would work as multiboot{1,2} and also be capable of being
> > loaded as an EFI application (ie: have a PE/COFF header also I assume
> > together with the ELF one), won't the changes here make it more
> > difficult to reach that goal or require reverting later on, as I feel
> > they are adding more differences between the PE binary and the ELF
> > one.
> 
> There were such plans, yes, but from the last round of that series
> I seem to recall that there was at least one issue breaking this
> idea. So no, at this point I'm not intending to take precautions to
> make that work easier (or not further complicate it). This said, I
> don't think the change here complicates anything there.
> 
> > The code LGTM, but I think at least I would like the commit message to
> > be expanded.
> 
> Well, once I know what exactly you're missing there, I can certainly
> try to expand it.

OK, I think I now have a clearer view, the commit message is likely
fine as it already mentions the ELF binary only needs the dummy
__base_relocs_{start,end}, hence it's the EFI binary the one that
requires the relocation symbols.

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

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-21 12:03     ` Jan Beulich
@ 2021-04-21 15:20       ` Roger Pau Monné
  2021-04-21 15:34         ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 15:20 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 02:03:49PM +0200, Jan Beulich wrote:
> On 21.04.2021 12:21, Roger Pau Monné wrote:
> > On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
> >> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
> >>  endif
> >>  
> >>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
> >> +ifeq ($(MKRELOC),:)
> >> +$(TARGET).efi: ALT_BASE :=
> >> +else
> >>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
> > 
> > Could you maybe check whether $(relocs-dummy) is set as the condition
> > here and use it here instead of efi/relocs-dummy.o?
> 
> I can use it in the ifeq() if you think that's neater (the current way
> is minimally shorter), but using it in the ALT_BASE assignment would
> make this differ more from the VIRT_BASE one, which I'd like to avoid.

Sorry, I think I'm slightly confused because when the linker can
produce the required relocations relocs-dummy variable is left empty,
so it won't be added to $(TARGET).efi.

But we still need to generate the efi/relocs-dummy.o file for the ELF
build I assume?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-21 13:06     ` Jan Beulich
@ 2021-04-21 15:30       ` Roger Pau Monné
  2021-04-21 15:38         ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-21 15:30 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
> On 21.04.2021 13:15, Roger Pau Monné wrote:
> > On Thu, Apr 01, 2021 at 11:47:03AM +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.)
> > 
> > Isn't just using (NOLOAD) to signal those sections shouldn't be
> > loaded enough, and thus don't care about it's RVA?
> 
> As said in a reply earlier on another sub-thread, (NOLOAD) is meaningless
> for PE. The fact that I add them nevertheless is just for docs purposes
> (or if, in the future, the item gains significance).
> 
> The main problem though isn't "load" vs "no-load" but, as I thought I
> have expressed in the description, that there's no "don't care about it's
> RVA" in PE. All sections have to have a non-zero VA above the image base.
> This is the only way via which sane RVA values can result. If sections
> get placed at VA 0 (which is perfectly fine for ELF), the RVA would be a
> huge negative number truncated to 32 bits. (Again, prior to binutils 2.37
> this will go all silently.) Plus the respective sections would come first
> (rather than last) in the binary (which by itself may or may not be a
> problem for the EFI loader, but I wouldn't want to chance it).

Thanks for the explanation.

> >> --- a/xen/arch/x86/xen.lds.S
> >> +++ b/xen/arch/x86/xen.lds.S
> >> @@ -312,10 +312,60 @@ 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));
> >> +  .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)) : .;
> > 
> > I think this is inside an ifdef EFI region, since this is DWARF info
> > couldn't it also be present when building with EFI disabled?
> 
> Of course (and it's not just "could" but "will"), yet the linker will
> do fine (and perhaps even better) without when building ELF. Also
> note that we'll be responsible for keeping the list of sections up-to-
> date. The linker will recognize Dwarf sections by looking for a
> .debug_ prefix. We can't use such here (or at least I'm not aware of
> a suitable mechanism); .debug_* would mean munging together all the
> different kinds of Dwarf sections. Hence by limiting the explicit
> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.

Right, so we will have to keep this list of debug_ sections updated
manually if/when more of those appear as part of DWARF updates?

Do we have a way to get some kind of warning or error when a new
section not explicitly handled here appears?

Instead of adding DWARF debug info to the xen.efi binary, is there a
way to translate the DWARF from the ELF build into the native debug
format for PE/COFF?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-21 15:20       ` Roger Pau Monné
@ 2021-04-21 15:34         ` Jan Beulich
  2021-04-22  7:22           ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 15:34 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 17:20, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 02:03:49PM +0200, Jan Beulich wrote:
>> On 21.04.2021 12:21, Roger Pau Monné wrote:
>>> On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
>>>> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
>>>>  endif
>>>>  
>>>>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
>>>> +ifeq ($(MKRELOC),:)
>>>> +$(TARGET).efi: ALT_BASE :=
>>>> +else
>>>>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
>>>
>>> Could you maybe check whether $(relocs-dummy) is set as the condition
>>> here and use it here instead of efi/relocs-dummy.o?
>>
>> I can use it in the ifeq() if you think that's neater (the current way
>> is minimally shorter), but using it in the ALT_BASE assignment would
>> make this differ more from the VIRT_BASE one, which I'd like to avoid.
> 
> Sorry, I think I'm slightly confused because when the linker can
> produce the required relocations relocs-dummy variable is left empty,
> so it won't be added to $(TARGET).efi.
> 
> But we still need to generate the efi/relocs-dummy.o file for the ELF
> build I assume?

Not anymore (after "x86/EFI: redo .reloc section bounds determination").
We need it, as said, to fish VIRT_BASE out of it. It indeed wouldn't get
linked into any of the final binaries anymore.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-21 15:30       ` Roger Pau Monné
@ 2021-04-21 15:38         ` Jan Beulich
  2021-04-22  8:14           ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-21 15:38 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 21.04.2021 17:30, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/xen.lds.S
>>>> +++ b/xen/arch/x86/xen.lds.S
>>>> @@ -312,10 +312,60 @@ 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));
>>>> +  .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)) : .;
>>>
>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>> couldn't it also be present when building with EFI disabled?
>>
>> Of course (and it's not just "could" but "will"), yet the linker will
>> do fine (and perhaps even better) without when building ELF. Also
>> note that we'll be responsible for keeping the list of sections up-to-
>> date. The linker will recognize Dwarf sections by looking for a
>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>> a suitable mechanism); .debug_* would mean munging together all the
>> different kinds of Dwarf sections. Hence by limiting the explicit
>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
> 
> Right, so we will have to keep this list of debug_ sections updated
> manually if/when more of those appear as part of DWARF updates?

Yes.

> Do we have a way to get some kind of warning or error when a new
> section not explicitly handled here appears?

ld 2.37 will start warning about such sections, as they'd land at
VA 0 and hence below image base.

> Instead of adding DWARF debug info to the xen.efi binary, is there a
> way to translate the DWARF from the ELF build into the native debug
> format for PE/COFF?

I'm unaware of a single, "native" debug format (Dwarf also isn't
specifically tied to ELF). I further don't think all of the Dwarf
bits could reasonably be expressed in another debug format.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-21 15:34         ` Jan Beulich
@ 2021-04-22  7:22           ` Roger Pau Monné
  2021-04-22 10:42             ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-22  7:22 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 05:34:29PM +0200, Jan Beulich wrote:
> On 21.04.2021 17:20, Roger Pau Monné wrote:
> > On Wed, Apr 21, 2021 at 02:03:49PM +0200, Jan Beulich wrote:
> >> On 21.04.2021 12:21, Roger Pau Monné wrote:
> >>> On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
> >>>> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
> >>>>  endif
> >>>>  
> >>>>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
> >>>> +ifeq ($(MKRELOC),:)
> >>>> +$(TARGET).efi: ALT_BASE :=
> >>>> +else
> >>>>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
> >>>
> >>> Could you maybe check whether $(relocs-dummy) is set as the condition
> >>> here and use it here instead of efi/relocs-dummy.o?
> >>
> >> I can use it in the ifeq() if you think that's neater (the current way
> >> is minimally shorter), but using it in the ALT_BASE assignment would
> >> make this differ more from the VIRT_BASE one, which I'd like to avoid.
> > 
> > Sorry, I think I'm slightly confused because when the linker can
> > produce the required relocations relocs-dummy variable is left empty,
> > so it won't be added to $(TARGET).efi.
> > 
> > But we still need to generate the efi/relocs-dummy.o file for the ELF
> > build I assume?
> 
> Not anymore (after "x86/EFI: redo .reloc section bounds determination").
> We need it, as said, to fish VIRT_BASE out of it. It indeed wouldn't get
> linked into any of the final binaries anymore.

Could you please add some text to the commit message to note that
while relocs-dummy is not linked into the final binary it's still
needed to get VIRT_BASE?

With that:

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

FWIW, it would also be nice to add some comments to the $(TARGET).efi
target commands, as it's quite impenetrable right now.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-21 15:38         ` Jan Beulich
@ 2021-04-22  8:14           ` Roger Pau Monné
  2021-04-22 11:03             ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-22  8:14 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
> On 21.04.2021 17:30, Roger Pau Monné wrote:
> > On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
> >> On 21.04.2021 13:15, Roger Pau Monné wrote:
> >>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/xen.lds.S
> >>>> +++ b/xen/arch/x86/xen.lds.S
> >>>> @@ -312,10 +312,60 @@ 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));
> >>>> +  .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)) : .;
> >>>
> >>> I think this is inside an ifdef EFI region, since this is DWARF info
> >>> couldn't it also be present when building with EFI disabled?
> >>
> >> Of course (and it's not just "could" but "will"), yet the linker will
> >> do fine (and perhaps even better) without when building ELF. Also
> >> note that we'll be responsible for keeping the list of sections up-to-
> >> date. The linker will recognize Dwarf sections by looking for a
> >> .debug_ prefix. We can't use such here (or at least I'm not aware of
> >> a suitable mechanism); .debug_* would mean munging together all the
> >> different kinds of Dwarf sections. Hence by limiting the explicit
> >> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
> > 
> > Right, so we will have to keep this list of debug_ sections updated
> > manually if/when more of those appear as part of DWARF updates?
> 
> Yes.
> 
> > Do we have a way to get some kind of warning or error when a new
> > section not explicitly handled here appears?
> 
> ld 2.37 will start warning about such sections, as they'd land at
> VA 0 and hence below image base.

That seems like a bug in ld?

The '--image-base' option description mentions: "This is the lowest
memory location that will be used when your program or dll is
loaded.", so I would expect that if the option is used the default VA
should be >= image-base, or else the description of the option is not
consistent, as ld will still place sections at addresses below
image-base.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible
  2021-04-22  7:22           ` Roger Pau Monné
@ 2021-04-22 10:42             ` Jan Beulich
  0 siblings, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2021-04-22 10:42 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 22.04.2021 09:22, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 05:34:29PM +0200, Jan Beulich wrote:
>> On 21.04.2021 17:20, Roger Pau Monné wrote:
>>> On Wed, Apr 21, 2021 at 02:03:49PM +0200, Jan Beulich wrote:
>>>> On 21.04.2021 12:21, Roger Pau Monné wrote:
>>>>> On Thu, Apr 01, 2021 at 11:46:44AM +0200, Jan Beulich wrote:
>>>>>> @@ -189,7 +208,11 @@ EFI_LDFLAGS += --no-insert-timestamp
>>>>>>  endif
>>>>>>  
>>>>>>  $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
>>>>>> +ifeq ($(MKRELOC),:)
>>>>>> +$(TARGET).efi: ALT_BASE :=
>>>>>> +else
>>>>>>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
>>>>>
>>>>> Could you maybe check whether $(relocs-dummy) is set as the condition
>>>>> here and use it here instead of efi/relocs-dummy.o?
>>>>
>>>> I can use it in the ifeq() if you think that's neater (the current way
>>>> is minimally shorter), but using it in the ALT_BASE assignment would
>>>> make this differ more from the VIRT_BASE one, which I'd like to avoid.
>>>
>>> Sorry, I think I'm slightly confused because when the linker can
>>> produce the required relocations relocs-dummy variable is left empty,
>>> so it won't be added to $(TARGET).efi.
>>>
>>> But we still need to generate the efi/relocs-dummy.o file for the ELF
>>> build I assume?
>>
>> Not anymore (after "x86/EFI: redo .reloc section bounds determination").
>> We need it, as said, to fish VIRT_BASE out of it. It indeed wouldn't get
>> linked into any of the final binaries anymore.
> 
> Could you please add some text to the commit message to note that
> while relocs-dummy is not linked into the final binary it's still
> needed to get VIRT_BASE?

I've added

"Note that in the case that we leave base relocation generation to ld,
 while efi/relocs-dummy.o then won't be linked into any executable
 anymore, it still needs generating (and hence dependencies need to be
 kept as they are) in order to have VIRT_BASE pulled out of it."

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

Thanks.

> FWIW, it would also be nice to add some comments to the $(TARGET).efi
> target commands, as it's quite impenetrable right now.

Well, my preferred plan was to pull it (and $(TARGET)-sym's) apart
into separate steps, suitably linked together via dependencies. This
may not eliminate altogether the need for some comments, but we then
may get away with brief ones.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22  8:14           ` Roger Pau Monné
@ 2021-04-22 11:03             ` Jan Beulich
  2021-04-22 14:56               ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-22 11:03 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 22.04.2021 10:14, Roger Pau Monné wrote:
> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
>> On 21.04.2021 17:30, Roger Pau Monné wrote:
>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>> @@ -312,10 +312,60 @@ 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));
>>>>>> +  .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)) : .;
>>>>>
>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>>>> couldn't it also be present when building with EFI disabled?
>>>>
>>>> Of course (and it's not just "could" but "will"), yet the linker will
>>>> do fine (and perhaps even better) without when building ELF. Also
>>>> note that we'll be responsible for keeping the list of sections up-to-
>>>> date. The linker will recognize Dwarf sections by looking for a
>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>>>> a suitable mechanism); .debug_* would mean munging together all the
>>>> different kinds of Dwarf sections. Hence by limiting the explicit
>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
>>>
>>> Right, so we will have to keep this list of debug_ sections updated
>>> manually if/when more of those appear as part of DWARF updates?
>>
>> Yes.
>>
>>> Do we have a way to get some kind of warning or error when a new
>>> section not explicitly handled here appears?
>>
>> ld 2.37 will start warning about such sections, as they'd land at
>> VA 0 and hence below image base.
> 
> That seems like a bug in ld?
> 
> The '--image-base' option description mentions: "This is the lowest
> memory location that will be used when your program or dll is
> loaded.", so I would expect that if the option is used the default VA
> should be >= image-base, or else the description of the option is not
> consistent, as ld will still place sections at addresses below
> image-base.

ld's "general" logic is pretty ELF-centric. Hence debugging sections
get placed at VA 0 by default, not matter what the (PE-specific)
--image-base says. Whether that's a bug though I'm not sure: There
are no really good alternatives that could be used by default. Doing
what we do here (and what e.g. Cygwin does) via linker script may not
be appropriate in the common case. In particular it is not generally
correct for debug info to be part of what gets loaded into memory.
Microsoft's CodeView / PDB debug info gets represented differently,
for example, such that it can be part of the file, but wouldn't get
loaded. But that's a vendor's choice, again not something that's
necessarily generalizable.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22 11:03             ` Jan Beulich
@ 2021-04-22 14:56               ` Roger Pau Monné
  2021-04-22 15:46                 ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-22 14:56 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
> On 22.04.2021 10:14, Roger Pau Monné wrote:
> > On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
> >> On 21.04.2021 17:30, Roger Pau Monné wrote:
> >>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
> >>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
> >>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
> >>>>>> --- a/xen/arch/x86/xen.lds.S
> >>>>>> +++ b/xen/arch/x86/xen.lds.S
> >>>>>> @@ -312,10 +312,60 @@ 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));
> >>>>>> +  .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)) : .;
> >>>>>
> >>>>> I think this is inside an ifdef EFI region, since this is DWARF info
> >>>>> couldn't it also be present when building with EFI disabled?
> >>>>
> >>>> Of course (and it's not just "could" but "will"), yet the linker will
> >>>> do fine (and perhaps even better) without when building ELF. Also
> >>>> note that we'll be responsible for keeping the list of sections up-to-
> >>>> date. The linker will recognize Dwarf sections by looking for a
> >>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
> >>>> a suitable mechanism); .debug_* would mean munging together all the
> >>>> different kinds of Dwarf sections. Hence by limiting the explicit
> >>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
> >>>
> >>> Right, so we will have to keep this list of debug_ sections updated
> >>> manually if/when more of those appear as part of DWARF updates?
> >>
> >> Yes.
> >>
> >>> Do we have a way to get some kind of warning or error when a new
> >>> section not explicitly handled here appears?
> >>
> >> ld 2.37 will start warning about such sections, as they'd land at
> >> VA 0 and hence below image base.
> > 
> > That seems like a bug in ld?
> > 
> > The '--image-base' option description mentions: "This is the lowest
> > memory location that will be used when your program or dll is
> > loaded.", so I would expect that if the option is used the default VA
> > should be >= image-base, or else the description of the option is not
> > consistent, as ld will still place sections at addresses below
> > image-base.
> 
> ld's "general" logic is pretty ELF-centric. Hence debugging sections
> get placed at VA 0 by default, not matter what the (PE-specific)
> --image-base says. Whether that's a bug though I'm not sure: There
> are no really good alternatives that could be used by default. Doing
> what we do here (and what e.g. Cygwin does) via linker script may not
> be appropriate in the common case. In particular it is not generally
> correct for debug info to be part of what gets loaded into memory.

So with this change here you placate the warnings from newer ld about
having a VA < image base, but the end result is that now the debug
sections will also get loaded when booted from the EFI loader?
(because the NOLOAD doesn't have any effect with PE)

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22 14:56               ` Roger Pau Monné
@ 2021-04-22 15:46                 ` Jan Beulich
  2021-04-22 15:53                   ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-22 15:46 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 22.04.2021 16:56, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
>> On 22.04.2021 10:14, Roger Pau Monné wrote:
>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
>>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>> @@ -312,10 +312,60 @@ 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));
>>>>>>>> +  .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)) : .;
>>>>>>>
>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>>>>>> couldn't it also be present when building with EFI disabled?
>>>>>>
>>>>>> Of course (and it's not just "could" but "will"), yet the linker will
>>>>>> do fine (and perhaps even better) without when building ELF. Also
>>>>>> note that we'll be responsible for keeping the list of sections up-to-
>>>>>> date. The linker will recognize Dwarf sections by looking for a
>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>>>>>> a suitable mechanism); .debug_* would mean munging together all the
>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
>>>>>
>>>>> Right, so we will have to keep this list of debug_ sections updated
>>>>> manually if/when more of those appear as part of DWARF updates?
>>>>
>>>> Yes.
>>>>
>>>>> Do we have a way to get some kind of warning or error when a new
>>>>> section not explicitly handled here appears?
>>>>
>>>> ld 2.37 will start warning about such sections, as they'd land at
>>>> VA 0 and hence below image base.
>>>
>>> That seems like a bug in ld?
>>>
>>> The '--image-base' option description mentions: "This is the lowest
>>> memory location that will be used when your program or dll is
>>> loaded.", so I would expect that if the option is used the default VA
>>> should be >= image-base, or else the description of the option is not
>>> consistent, as ld will still place sections at addresses below
>>> image-base.
>>
>> ld's "general" logic is pretty ELF-centric. Hence debugging sections
>> get placed at VA 0 by default, not matter what the (PE-specific)
>> --image-base says. Whether that's a bug though I'm not sure: There
>> are no really good alternatives that could be used by default. Doing
>> what we do here (and what e.g. Cygwin does) via linker script may not
>> be appropriate in the common case. In particular it is not generally
>> correct for debug info to be part of what gets loaded into memory.
> 
> So with this change here you placate the warnings from newer ld about
> having a VA < image base,

It's not just about silencing the warnings. The resulting image is
unusable when the sections don't get placed at a suitable VA.

> but the end result is that now the debug
> sections will also get loaded when booted from the EFI loader?
> (because the NOLOAD doesn't have any effect with PE)

Yes. I currently see no other way to retain debug info in xen.efi.
But to be clear, the memory debug info occupies isn't lost - we
still free space from _end (or alike) onwards. .reloc, for example,
also lives there. And I was wondering whether we shouldn't keep
.comment this way as well.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22 15:46                 ` Jan Beulich
@ 2021-04-22 15:53                   ` Roger Pau Monné
  2021-04-22 16:01                     ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-22 15:53 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote:
> On 22.04.2021 16:56, Roger Pau Monné wrote:
> > On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
> >> On 22.04.2021 10:14, Roger Pau Monné wrote:
> >>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
> >>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
> >>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
> >>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
> >>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
> >>>>>>>> --- a/xen/arch/x86/xen.lds.S
> >>>>>>>> +++ b/xen/arch/x86/xen.lds.S
> >>>>>>>> @@ -312,10 +312,60 @@ 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));
> >>>>>>>> +  .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)) : .;
> >>>>>>>
> >>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
> >>>>>>> couldn't it also be present when building with EFI disabled?
> >>>>>>
> >>>>>> Of course (and it's not just "could" but "will"), yet the linker will
> >>>>>> do fine (and perhaps even better) without when building ELF. Also
> >>>>>> note that we'll be responsible for keeping the list of sections up-to-
> >>>>>> date. The linker will recognize Dwarf sections by looking for a
> >>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
> >>>>>> a suitable mechanism); .debug_* would mean munging together all the
> >>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
> >>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
> >>>>>
> >>>>> Right, so we will have to keep this list of debug_ sections updated
> >>>>> manually if/when more of those appear as part of DWARF updates?
> >>>>
> >>>> Yes.
> >>>>
> >>>>> Do we have a way to get some kind of warning or error when a new
> >>>>> section not explicitly handled here appears?
> >>>>
> >>>> ld 2.37 will start warning about such sections, as they'd land at
> >>>> VA 0 and hence below image base.
> >>>
> >>> That seems like a bug in ld?
> >>>
> >>> The '--image-base' option description mentions: "This is the lowest
> >>> memory location that will be used when your program or dll is
> >>> loaded.", so I would expect that if the option is used the default VA
> >>> should be >= image-base, or else the description of the option is not
> >>> consistent, as ld will still place sections at addresses below
> >>> image-base.
> >>
> >> ld's "general" logic is pretty ELF-centric. Hence debugging sections
> >> get placed at VA 0 by default, not matter what the (PE-specific)
> >> --image-base says. Whether that's a bug though I'm not sure: There
> >> are no really good alternatives that could be used by default. Doing
> >> what we do here (and what e.g. Cygwin does) via linker script may not
> >> be appropriate in the common case. In particular it is not generally
> >> correct for debug info to be part of what gets loaded into memory.
> > 
> > So with this change here you placate the warnings from newer ld about
> > having a VA < image base,
> 
> It's not just about silencing the warnings. The resulting image is
> unusable when the sections don't get placed at a suitable VA.

And this wasn't an issue before because the linker won't even attempt
to place DWARF sections into a PE output.

> > but the end result is that now the debug
> > sections will also get loaded when booted from the EFI loader?
> > (because the NOLOAD doesn't have any effect with PE)
> 
> Yes. I currently see no other way to retain debug info in xen.efi.
> But to be clear, the memory debug info occupies isn't lost - we
> still free space from _end (or alike) onwards. .reloc, for example,
> also lives there. And I was wondering whether we shouldn't keep
> .comment this way as well.

Yes, I already realized all this is past _end.

I wonder however if the use of (NOLOAD) makes all this more confusing,
such sections should only be present on the linker script used for the
PE output, and then the (NOLOAD) doesn't make sense there?

If so, I think the (NOLOAD) directive should be dropped, and a comment
noting that the debug sections need to be manually added to the PE
output in order to avoid them being placed at VA 0 would be helpful
IMO, likely also mentioning that they would be loaded but discarded
afterwards by Xen because they are all past _end.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22 15:53                   ` Roger Pau Monné
@ 2021-04-22 16:01                     ` Jan Beulich
  2021-04-23  7:30                       ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-22 16:01 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 22.04.2021 17:53, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote:
>> On 22.04.2021 16:56, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 10:14, Roger Pau Monné wrote:
>>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
>>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
>>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>>>> @@ -312,10 +312,60 @@ 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));
>>>>>>>>>> +  .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)) : .;
>>>>>>>>>
>>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>>>>>>>> couldn't it also be present when building with EFI disabled?
>>>>>>>>
>>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will
>>>>>>>> do fine (and perhaps even better) without when building ELF. Also
>>>>>>>> note that we'll be responsible for keeping the list of sections up-to-
>>>>>>>> date. The linker will recognize Dwarf sections by looking for a
>>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>>>>>>>> a suitable mechanism); .debug_* would mean munging together all the
>>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
>>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
>>>>>>>
>>>>>>> Right, so we will have to keep this list of debug_ sections updated
>>>>>>> manually if/when more of those appear as part of DWARF updates?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> Do we have a way to get some kind of warning or error when a new
>>>>>>> section not explicitly handled here appears?
>>>>>>
>>>>>> ld 2.37 will start warning about such sections, as they'd land at
>>>>>> VA 0 and hence below image base.
>>>>>
>>>>> That seems like a bug in ld?
>>>>>
>>>>> The '--image-base' option description mentions: "This is the lowest
>>>>> memory location that will be used when your program or dll is
>>>>> loaded.", so I would expect that if the option is used the default VA
>>>>> should be >= image-base, or else the description of the option is not
>>>>> consistent, as ld will still place sections at addresses below
>>>>> image-base.
>>>>
>>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections
>>>> get placed at VA 0 by default, not matter what the (PE-specific)
>>>> --image-base says. Whether that's a bug though I'm not sure: There
>>>> are no really good alternatives that could be used by default. Doing
>>>> what we do here (and what e.g. Cygwin does) via linker script may not
>>>> be appropriate in the common case. In particular it is not generally
>>>> correct for debug info to be part of what gets loaded into memory.
>>>
>>> So with this change here you placate the warnings from newer ld about
>>> having a VA < image base,
>>
>> It's not just about silencing the warnings. The resulting image is
>> unusable when the sections don't get placed at a suitable VA.
> 
> And this wasn't an issue before because the linker won't even attempt
> to place DWARF sections into a PE output.

No, this wasn't an issue before since, for things to work, we
simply had to uniformly strip debug info when linking xen.efi. And
this is what Andrew said should change. I was initially opposed,
until I saw that Cygwin does just this as well.

>>> but the end result is that now the debug
>>> sections will also get loaded when booted from the EFI loader?
>>> (because the NOLOAD doesn't have any effect with PE)
>>
>> Yes. I currently see no other way to retain debug info in xen.efi.
>> But to be clear, the memory debug info occupies isn't lost - we
>> still free space from _end (or alike) onwards. .reloc, for example,
>> also lives there. And I was wondering whether we shouldn't keep
>> .comment this way as well.
> 
> Yes, I already realized all this is past _end.
> 
> I wonder however if the use of (NOLOAD) makes all this more confusing,
> such sections should only be present on the linker script used for the
> PE output, and then the (NOLOAD) doesn't make sense there?
> 
> If so, I think the (NOLOAD) directive should be dropped, and a comment
> noting that the debug sections need to be manually added to the PE
> output in order to avoid them being placed at VA 0 would be helpful
> IMO, likely also mentioning that they would be loaded but discarded
> afterwards by Xen because they are all past _end.

Earlier on (another sub-thread, maybe) I think I've already said that
I'd like to keep (NOLOAD) both for documentation purposes and just in
case the linker develops some smarts to actually translate it into
anything sensible when linking PE. This is quite different from
.reloc, after all - that section has to be loaded for our re-
relocation to work correctly. Hence that section not having (NOLOAD)
and the debugging sections having it points out the difference.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-22 16:01                     ` Jan Beulich
@ 2021-04-23  7:30                       ` Roger Pau Monné
  2021-04-23  8:51                         ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-23  7:30 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Thu, Apr 22, 2021 at 06:01:06PM +0200, Jan Beulich wrote:
> On 22.04.2021 17:53, Roger Pau Monné wrote:
> > On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote:
> >> On 22.04.2021 16:56, Roger Pau Monné wrote:
> >>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
> >>>> On 22.04.2021 10:14, Roger Pau Monné wrote:
> >>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
> >>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
> >>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
> >>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
> >>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
> >>>>>>>>>> --- a/xen/arch/x86/xen.lds.S
> >>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
> >>>>>>>>>> @@ -312,10 +312,60 @@ 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));
> >>>>>>>>>> +  .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)) : .;
> >>>>>>>>>
> >>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
> >>>>>>>>> couldn't it also be present when building with EFI disabled?
> >>>>>>>>
> >>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will
> >>>>>>>> do fine (and perhaps even better) without when building ELF. Also
> >>>>>>>> note that we'll be responsible for keeping the list of sections up-to-
> >>>>>>>> date. The linker will recognize Dwarf sections by looking for a
> >>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
> >>>>>>>> a suitable mechanism); .debug_* would mean munging together all the
> >>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
> >>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
> >>>>>>>
> >>>>>>> Right, so we will have to keep this list of debug_ sections updated
> >>>>>>> manually if/when more of those appear as part of DWARF updates?
> >>>>>>
> >>>>>> Yes.
> >>>>>>
> >>>>>>> Do we have a way to get some kind of warning or error when a new
> >>>>>>> section not explicitly handled here appears?
> >>>>>>
> >>>>>> ld 2.37 will start warning about such sections, as they'd land at
> >>>>>> VA 0 and hence below image base.
> >>>>>
> >>>>> That seems like a bug in ld?
> >>>>>
> >>>>> The '--image-base' option description mentions: "This is the lowest
> >>>>> memory location that will be used when your program or dll is
> >>>>> loaded.", so I would expect that if the option is used the default VA
> >>>>> should be >= image-base, or else the description of the option is not
> >>>>> consistent, as ld will still place sections at addresses below
> >>>>> image-base.
> >>>>
> >>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections
> >>>> get placed at VA 0 by default, not matter what the (PE-specific)
> >>>> --image-base says. Whether that's a bug though I'm not sure: There
> >>>> are no really good alternatives that could be used by default. Doing
> >>>> what we do here (and what e.g. Cygwin does) via linker script may not
> >>>> be appropriate in the common case. In particular it is not generally
> >>>> correct for debug info to be part of what gets loaded into memory.
> >>>
> >>> So with this change here you placate the warnings from newer ld about
> >>> having a VA < image base,
> >>
> >> It's not just about silencing the warnings. The resulting image is
> >> unusable when the sections don't get placed at a suitable VA.
> > 
> > And this wasn't an issue before because the linker won't even attempt
> > to place DWARF sections into a PE output.
> 
> No, this wasn't an issue before since, for things to work, we
> simply had to uniformly strip debug info when linking xen.efi. And
> this is what Andrew said should change. I was initially opposed,
> until I saw that Cygwin does just this as well.

Just for my own education, do you have a reference about this way of
packaging debug data by Cygwin?

I've found:

https://cygwin.com/pipermail/cygwin/2003-January/090110.html

Which mentions not setting the ALLOC flag on the debug sections in
order to prevent them from being loaded. I'm however not able to
figure out which flag is that on the PE spec, or whether it can be set
from the linker script.

> >>> but the end result is that now the debug
> >>> sections will also get loaded when booted from the EFI loader?
> >>> (because the NOLOAD doesn't have any effect with PE)
> >>
> >> Yes. I currently see no other way to retain debug info in xen.efi.
> >> But to be clear, the memory debug info occupies isn't lost - we
> >> still free space from _end (or alike) onwards. .reloc, for example,
> >> also lives there. And I was wondering whether we shouldn't keep
> >> .comment this way as well.
> > 
> > Yes, I already realized all this is past _end.
> > 
> > I wonder however if the use of (NOLOAD) makes all this more confusing,
> > such sections should only be present on the linker script used for the
> > PE output, and then the (NOLOAD) doesn't make sense there?
> > 
> > If so, I think the (NOLOAD) directive should be dropped, and a comment
> > noting that the debug sections need to be manually added to the PE
> > output in order to avoid them being placed at VA 0 would be helpful
> > IMO, likely also mentioning that they would be loaded but discarded
> > afterwards by Xen because they are all past _end.
> 
> Earlier on (another sub-thread, maybe) I think I've already said that
> I'd like to keep (NOLOAD) both for documentation purposes and just in
> case the linker develops some smarts to actually translate it into
> anything sensible when linking PE. This is quite different from
> .reloc, after all - that section has to be loaded for our re-
> relocation to work correctly. Hence that section not having (NOLOAD)
> and the debugging sections having it points out the difference.

Sure, that's all fine. I think a comment could be appropriate here, to
note both that NOLOAD is likely useless and just used for
documentation purposes, and to also mention the sections needs to be
explicitly placed in the PE linker script so they are not set at VA 0.

/*
 * 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, even when currently ignored by PE output, in
 * order to note those sections shouldn't be loaded into memory.
 *
 * Note such sections are past _end, so if loaded will be discarded by
 * Xen anyway.
 */

Feel free to reword or expand the comment. Not sure there's some
reference we could add here about how debug sections are placed in PE
files usually.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-23  7:30                       ` Roger Pau Monné
@ 2021-04-23  8:51                         ` Jan Beulich
  2021-04-23 10:07                           ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-23  8:51 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 23.04.2021 09:30, Roger Pau Monné wrote:
> On Thu, Apr 22, 2021 at 06:01:06PM +0200, Jan Beulich wrote:
>> On 22.04.2021 17:53, Roger Pau Monné wrote:
>>> On Thu, Apr 22, 2021 at 05:46:28PM +0200, Jan Beulich wrote:
>>>> On 22.04.2021 16:56, Roger Pau Monné wrote:
>>>>> On Thu, Apr 22, 2021 at 01:03:13PM +0200, Jan Beulich wrote:
>>>>>> On 22.04.2021 10:14, Roger Pau Monné wrote:
>>>>>>> On Wed, Apr 21, 2021 at 05:38:42PM +0200, Jan Beulich wrote:
>>>>>>>> On 21.04.2021 17:30, Roger Pau Monné wrote:
>>>>>>>>> On Wed, Apr 21, 2021 at 03:06:36PM +0200, Jan Beulich wrote:
>>>>>>>>>> On 21.04.2021 13:15, Roger Pau Monné wrote:
>>>>>>>>>>> On Thu, Apr 01, 2021 at 11:47:03AM +0200, Jan Beulich wrote:
>>>>>>>>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>>>>>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>>>>>>>>> @@ -312,10 +312,60 @@ 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));
>>>>>>>>>>>> +  .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)) : .;
>>>>>>>>>>>
>>>>>>>>>>> I think this is inside an ifdef EFI region, since this is DWARF info
>>>>>>>>>>> couldn't it also be present when building with EFI disabled?
>>>>>>>>>>
>>>>>>>>>> Of course (and it's not just "could" but "will"), yet the linker will
>>>>>>>>>> do fine (and perhaps even better) without when building ELF. Also
>>>>>>>>>> note that we'll be responsible for keeping the list of sections up-to-
>>>>>>>>>> date. The linker will recognize Dwarf sections by looking for a
>>>>>>>>>> .debug_ prefix. We can't use such here (or at least I'm not aware of
>>>>>>>>>> a suitable mechanism); .debug_* would mean munging together all the
>>>>>>>>>> different kinds of Dwarf sections. Hence by limiting the explicit
>>>>>>>>>> enumeration to PE, I'm trying to avoid anomalies in ELF down the road.
>>>>>>>>>
>>>>>>>>> Right, so we will have to keep this list of debug_ sections updated
>>>>>>>>> manually if/when more of those appear as part of DWARF updates?
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>> Do we have a way to get some kind of warning or error when a new
>>>>>>>>> section not explicitly handled here appears?
>>>>>>>>
>>>>>>>> ld 2.37 will start warning about such sections, as they'd land at
>>>>>>>> VA 0 and hence below image base.
>>>>>>>
>>>>>>> That seems like a bug in ld?
>>>>>>>
>>>>>>> The '--image-base' option description mentions: "This is the lowest
>>>>>>> memory location that will be used when your program or dll is
>>>>>>> loaded.", so I would expect that if the option is used the default VA
>>>>>>> should be >= image-base, or else the description of the option is not
>>>>>>> consistent, as ld will still place sections at addresses below
>>>>>>> image-base.
>>>>>>
>>>>>> ld's "general" logic is pretty ELF-centric. Hence debugging sections
>>>>>> get placed at VA 0 by default, not matter what the (PE-specific)
>>>>>> --image-base says. Whether that's a bug though I'm not sure: There
>>>>>> are no really good alternatives that could be used by default. Doing
>>>>>> what we do here (and what e.g. Cygwin does) via linker script may not
>>>>>> be appropriate in the common case. In particular it is not generally
>>>>>> correct for debug info to be part of what gets loaded into memory.
>>>>>
>>>>> So with this change here you placate the warnings from newer ld about
>>>>> having a VA < image base,
>>>>
>>>> It's not just about silencing the warnings. The resulting image is
>>>> unusable when the sections don't get placed at a suitable VA.
>>>
>>> And this wasn't an issue before because the linker won't even attempt
>>> to place DWARF sections into a PE output.
>>
>> No, this wasn't an issue before since, for things to work, we
>> simply had to uniformly strip debug info when linking xen.efi. And
>> this is what Andrew said should change. I was initially opposed,
>> until I saw that Cygwin does just this as well.
> 
> Just for my own education, do you have a reference about this way of
> packaging debug data by Cygwin?

I've simply built a test program and looked at the binary. The best
reference I could think of is their default linker script in binutils
(ld/scripttempl/pep.sc).

> I've found:
> 
> https://cygwin.com/pipermail/cygwin/2003-January/090110.html
> 
> Which mentions not setting the ALLOC flag on the debug sections in
> order to prevent them from being loaded. I'm however not able to
> figure out which flag is that on the PE spec, or whether it can be set
> from the linker script.

There's no truly corresponding flag in COFF.

>>>>> but the end result is that now the debug
>>>>> sections will also get loaded when booted from the EFI loader?
>>>>> (because the NOLOAD doesn't have any effect with PE)
>>>>
>>>> Yes. I currently see no other way to retain debug info in xen.efi.
>>>> But to be clear, the memory debug info occupies isn't lost - we
>>>> still free space from _end (or alike) onwards. .reloc, for example,
>>>> also lives there. And I was wondering whether we shouldn't keep
>>>> .comment this way as well.
>>>
>>> Yes, I already realized all this is past _end.
>>>
>>> I wonder however if the use of (NOLOAD) makes all this more confusing,
>>> such sections should only be present on the linker script used for the
>>> PE output, and then the (NOLOAD) doesn't make sense there?
>>>
>>> If so, I think the (NOLOAD) directive should be dropped, and a comment
>>> noting that the debug sections need to be manually added to the PE
>>> output in order to avoid them being placed at VA 0 would be helpful
>>> IMO, likely also mentioning that they would be loaded but discarded
>>> afterwards by Xen because they are all past _end.
>>
>> Earlier on (another sub-thread, maybe) I think I've already said that
>> I'd like to keep (NOLOAD) both for documentation purposes and just in
>> case the linker develops some smarts to actually translate it into
>> anything sensible when linking PE. This is quite different from
>> .reloc, after all - that section has to be loaded for our re-
>> relocation to work correctly. Hence that section not having (NOLOAD)
>> and the debugging sections having it points out the difference.
> 
> Sure, that's all fine. I think a comment could be appropriate here, to
> note both that NOLOAD is likely useless and just used for
> documentation purposes, and to also mention the sections needs to be
> explicitly placed in the PE linker script so they are not set at VA 0.
> 
> /*
>  * 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, even when currently ignored by PE output, in
>  * order to note those sections shouldn't be loaded into memory.
>  *
>  * Note such sections are past _end, so if loaded will be discarded by
>  * Xen anyway.
>  */
> 
> Feel free to reword or expand the comment.

Yes, I've edited it some while inserting. Will see to get to
submitting v2 then.

> Not sure there's some
> reference we could add here about how debug sections are placed in PE
> files usually.

As said before - I don't think there's any "usually" here, which is
why different environments have invented different ways. The debug
info native to COFF is more like ELF's symbol table (with a little
bit of extra information) plus Dwarf's .debug_line, but not really
fully covering what you'd expect from debug info.

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-23  8:51                         ` Jan Beulich
@ 2021-04-23 10:07                           ` Roger Pau Monné
  2021-04-23 10:45                             ` Jan Beulich
  0 siblings, 1 reply; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-23 10:07 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote:
> On 23.04.2021 09:30, Roger Pau Monné wrote:
> > /*
> >  * 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, even when currently ignored by PE output, in
> >  * order to note those sections shouldn't be loaded into memory.
> >  *
> >  * Note such sections are past _end, so if loaded will be discarded by
> >  * Xen anyway.
> >  */
> > 
> > Feel free to reword or expand the comment.
> 
> Yes, I've edited it some while inserting. Will see to get to
> submitting v2 then.
> 
> > Not sure there's some
> > reference we could add here about how debug sections are placed in PE
> > files usually.
> 
> As said before - I don't think there's any "usually" here, which is
> why different environments have invented different ways. The debug
> info native to COFF is more like ELF's symbol table (with a little
> bit of extra information) plus Dwarf's .debug_line, but not really
> fully covering what you'd expect from debug info.

One last thing, do you know if the newly added debug_* sections get
the IMAGE_SCN_MEM_DISCARDABLE section flag set?

Not sure there's a way we can force it from the linker script TBH, but
would be nice and seems to be a recommended flag for debug sections
according to Microsoft [0].

Thanks, Roger.

[0] https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#section-flags


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-23 10:07                           ` Roger Pau Monné
@ 2021-04-23 10:45                             ` Jan Beulich
  2021-04-23 10:58                               ` Roger Pau Monné
  0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2021-04-23 10:45 UTC (permalink / raw)
  To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu

On 23.04.2021 12:07, Roger Pau Monné wrote:
> On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote:
>> On 23.04.2021 09:30, Roger Pau Monné wrote:
>>> /*
>>>  * 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, even when currently ignored by PE output, in
>>>  * order to note those sections shouldn't be loaded into memory.
>>>  *
>>>  * Note such sections are past _end, so if loaded will be discarded by
>>>  * Xen anyway.
>>>  */
>>>
>>> Feel free to reword or expand the comment.
>>
>> Yes, I've edited it some while inserting. Will see to get to
>> submitting v2 then.
>>
>>> Not sure there's some
>>> reference we could add here about how debug sections are placed in PE
>>> files usually.
>>
>> As said before - I don't think there's any "usually" here, which is
>> why different environments have invented different ways. The debug
>> info native to COFF is more like ELF's symbol table (with a little
>> bit of extra information) plus Dwarf's .debug_line, but not really
>> fully covering what you'd expect from debug info.
> 
> One last thing, do you know if the newly added debug_* sections get
> the IMAGE_SCN_MEM_DISCARDABLE section flag set?

At least with an up-to-date ld (i.e. one meeting the requirements so
we wouldn't force debug info to be stripped) they do.

> Not sure there's a way we can force it from the linker script TBH, but
> would be nice and seems to be a recommended flag for debug sections
> according to Microsoft [0].

The linker does this for debugging sections irrespective of what the
linker script says:

   if ((sec_flags & SEC_DEBUGGING) != 0)
     styp_flags |= IMAGE_SCN_MEM_DISCARDABLE;

Jan


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 7/8] x86/EFI: keep debug info in xen.efi
  2021-04-23 10:45                             ` Jan Beulich
@ 2021-04-23 10:58                               ` Roger Pau Monné
  0 siblings, 0 replies; 48+ messages in thread
From: Roger Pau Monné @ 2021-04-23 10:58 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu

On Fri, Apr 23, 2021 at 12:45:14PM +0200, Jan Beulich wrote:
> On 23.04.2021 12:07, Roger Pau Monné wrote:
> > On Fri, Apr 23, 2021 at 10:51:40AM +0200, Jan Beulich wrote:
> >> On 23.04.2021 09:30, Roger Pau Monné wrote:
> >>> /*
> >>>  * 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, even when currently ignored by PE output, in
> >>>  * order to note those sections shouldn't be loaded into memory.
> >>>  *
> >>>  * Note such sections are past _end, so if loaded will be discarded by
> >>>  * Xen anyway.
> >>>  */
> >>>
> >>> Feel free to reword or expand the comment.
> >>
> >> Yes, I've edited it some while inserting. Will see to get to
> >> submitting v2 then.
> >>
> >>> Not sure there's some
> >>> reference we could add here about how debug sections are placed in PE
> >>> files usually.
> >>
> >> As said before - I don't think there's any "usually" here, which is
> >> why different environments have invented different ways. The debug
> >> info native to COFF is more like ELF's symbol table (with a little
> >> bit of extra information) plus Dwarf's .debug_line, but not really
> >> fully covering what you'd expect from debug info.
> > 
> > One last thing, do you know if the newly added debug_* sections get
> > the IMAGE_SCN_MEM_DISCARDABLE section flag set?
> 
> At least with an up-to-date ld (i.e. one meeting the requirements so
> we wouldn't force debug info to be stripped) they do.
> 
> > Not sure there's a way we can force it from the linker script TBH, but
> > would be nice and seems to be a recommended flag for debug sections
> > according to Microsoft [0].
> 
> The linker does this for debugging sections irrespective of what the
> linker script says:
> 
>    if ((sec_flags & SEC_DEBUGGING) != 0)
>      styp_flags |= IMAGE_SCN_MEM_DISCARDABLE;

Great, that's good to know. Note sure it's worth adding to the commit
message, maybe I've just missed part of the documentation where LD
notes that IMAGE_SCN_MEM_DISCARDABLE will be unconditionally added to
debug sections.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2021-04-23 10:58 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01  9:43 [PATCH 0/8] x86/EFI: build adjustments Jan Beulich
2021-04-01  9:44 ` [PATCH 1/8] x86/EFI: drop stale section special casing when generating base relocs Jan Beulich
2021-04-01 11:51   ` Andrew Cooper
2021-04-01  9:44 ` [PATCH 2/8] x86/EFI: sections may not live at VA 0 in PE binaries Jan Beulich
2021-04-01 12:01   ` Andrew Cooper
2021-04-01 13:51     ` Jan Beulich
2021-04-21  8:52   ` Roger Pau Monné
2021-04-21 10:32     ` Jan Beulich
2021-04-21 12:57       ` Roger Pau Monné
2021-04-21 13:28         ` Jan Beulich
2021-04-01  9:45 ` [PATCH 3/8] x86/EFI: program headers are an ELF concept Jan Beulich
2021-04-21  9:11   ` Roger Pau Monné
2021-04-21 10:36     ` Jan Beulich
2021-04-21 14:21       ` Roger Pau Monné
2021-04-21 14:30         ` Jan Beulich
2021-04-01  9:45 ` [PATCH 4/8] x86/EFI: redo .reloc section bounds determination Jan Beulich
2021-04-21  9:46   ` Roger Pau Monné
2021-04-21 10:44     ` Jan Beulich
2021-04-21 14:54       ` Roger Pau Monné
2021-04-01  9:46 ` [PATCH 5/8] x86: drop use of prelink-efi.o Jan Beulich
2021-04-21  9:51   ` Roger Pau Monné
2021-04-01  9:46 ` [PATCH 6/8] x86/EFI: avoid use of GNU ld's --disable-reloc-section when possible Jan Beulich
2021-04-21 10:21   ` Roger Pau Monné
2021-04-21 12:03     ` Jan Beulich
2021-04-21 15:20       ` Roger Pau Monné
2021-04-21 15:34         ` Jan Beulich
2021-04-22  7:22           ` Roger Pau Monné
2021-04-22 10:42             ` Jan Beulich
2021-04-01  9:47 ` [PATCH 7/8] x86/EFI: keep debug info in xen.efi Jan Beulich
2021-04-21 11:15   ` Roger Pau Monné
2021-04-21 13:06     ` Jan Beulich
2021-04-21 15:30       ` Roger Pau Monné
2021-04-21 15:38         ` Jan Beulich
2021-04-22  8:14           ` Roger Pau Monné
2021-04-22 11:03             ` Jan Beulich
2021-04-22 14:56               ` Roger Pau Monné
2021-04-22 15:46                 ` Jan Beulich
2021-04-22 15:53                   ` Roger Pau Monné
2021-04-22 16:01                     ` Jan Beulich
2021-04-23  7:30                       ` Roger Pau Monné
2021-04-23  8:51                         ` Jan Beulich
2021-04-23 10:07                           ` Roger Pau Monné
2021-04-23 10:45                             ` Jan Beulich
2021-04-23 10:58                               ` Roger Pau Monné
2021-04-01  9:47 ` [PATCH 8/8] x86/EFI: don't have an overly large image size Jan Beulich
2021-04-21 11:18   ` Roger Pau Monné
2021-04-21 13:15     ` Jan Beulich
2021-04-15  9:53 ` Ping: [PATCH 0/8] x86/EFI: build adjustments Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).