[PATCH 0/3] x86/build: Clean up 32bit boot objects

[PATCH 0/3] x86/build: Clean up 32bit boot objects

[Andrew Cooper]

Removes several unnecessary steps from the build.

Andrew Cooper (3):
  x86/build: Rework binary conversion for boot/{cmdline,reloc}.c
  x86/build: Don't convert boot/{cmdline,head}.bin back to .S
  x86/build: Clean up boot/Makefile

 xen/arch/x86/boot/Makefile    | 39 +++++++++++------------------
 xen/arch/x86/boot/build32.lds | 58 +++++++++++++++++++------------------------
 xen/arch/x86/boot/head.S      | 10 ++++++--
 3 files changed, 47 insertions(+), 60 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/build: Rework binary conversion for boot/{cmdline,reloc}.c

[Andrew Cooper]

There is no need to opencode .got.plt size check; it can be done with linker
asserts instead.  Extend the checking to all dynamic linkage sections, and
drop the $(OBJDUMP) pass.

Furthermore, instead of removing .got.plt specifically, take only .text when
converting to a flat binary.  This makes the process invariant of .text's
position relative to the start of the binary, which avoids needing to discard
all sections, and removes the need to work around sections that certain
linkers are unhappy discarding.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/x86/boot/Makefile    | 13 +---------
 xen/arch/x86/boot/build32.lds | 58 +++++++++++++++++++------------------------
 2 files changed, 26 insertions(+), 45 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index ca8001c72b23..09d1f9f75394 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -22,19 +22,8 @@ $(head-srcs): %.S: %.bin
 	(od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \
 	sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@
 
-# Drop .got.plt during conversion to plain binary format.
-# Please check build32.lds for more details.
 %.bin: %.lnk
-	$(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | \
-		while read idx name sz rest; do \
-			case "$$name" in \
-			.got.plt) \
-				test $$sz != 0c || continue; \
-				echo "Error: non-empty $$name: 0x$$sz" >&2; \
-				exit $$(expr $$idx + 1);; \
-			esac; \
-		done
-	$(OBJCOPY) -O binary -R .got.plt $< $@
+	$(OBJCOPY) -j .text -O binary $< $@
 
 %.lnk: %.o $(src)/build32.lds
 	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
index 1ab941879312..d8fb9170ca40 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds
@@ -31,44 +31,36 @@ SECTIONS
         *(.bss.*)
   }
 
+  /* Dynamic linkage sections.  Collected simply so we can check they're empty. */
+  .got : {
+        *(.got)
+  }
   .got.plt : {
-        /*
-         * PIC/PIE executable contains .got.plt section even if it is not linked
-         * with dynamic libraries. In such case it is just placeholder for
-         * _GLOBAL_OFFSET_TABLE_ symbol and .PLT0. .PLT0 is filled by dynamic
-         * linker and our code is not supposed to be loaded by dynamic linker.
-         * So, from our point of view .PLT0 is unused. This means that there is
-         * pretty good chance that we can safely drop .got.plt as a whole here.
-         * Sadly this is not true. _GLOBAL_OFFSET_TABLE_ is used as a reference
-         * for relative addressing (and only for that thing) and ld complains if
-         * we remove .got.plt section here because it cannot find required symbol.
-         * However, _GLOBAL_OFFSET_TABLE_ is no longer needed in final output.
-         * So, drop .got.plt section during conversion to plain binary format.
-         *
-         * Please check build32.mk for more details.
-         */
         *(.got.plt)
   }
-
-  /*
-   * Discarding .shstrtab is not supported by LLD (LLVM LD) and will trigger an
-   * error. Also keep the rest of the control sections to match GNU LD behavior.
-   */
-  .shstrtab : {
-        *(.shstrtab)
+  .igot.plt : {
+        *(.igot.plt)
   }
-  .strtab : {
-        *(.strtab)
+  .iplt : {
+        *(.iplt)
   }
-  .symtab : {
-        *(.symtab)
+  .plt : {
+        *(.plt)
   }
-
-  /DISCARD/ : {
-        /*
-         * Discard everything else, to prevent linkers from putting
-         * orphaned sections ahead of .text, which needs to be first.
-         */
-        *(*)
+  .rela : {
+        *(.rela.*)
   }
 }
+
+ASSERT(SIZEOF(.got) == 0,         ".got non-empty")
+/*
+ * At least GNU ld 2.30 and earlier fail to discard the generic part of
+ * .got.plt when no actual entries were allocated. Permit this case alongside
+ * the section being empty.
+ */
+ASSERT(SIZEOF(.got.plt) == 0 ||
+       SIZEOF(.got.plt) == 3 * 4, "unexpected .got.plt size")
+ASSERT(SIZEOF(.igot.plt) == 0,    ".igot.plt non-empty")
+ASSERT(SIZEOF(.iplt) == 0,        ".iplt non-empty")
+ASSERT(SIZEOF(.plt) == 0,         ".plt non-empty")
+ASSERT(SIZEOF(.rela) == 0,        "leftover relocations")
-- 
2.11.0

Re: [PATCH 1/3] x86/build: Rework binary conversion for boot/{cmdline,reloc}.c

[Jan Beulich]

On 14.04.2022 13:47, Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds
> @@ -31,44 +31,36 @@ SECTIONS
>          *(.bss.*)
>    }
>  
> +  /* Dynamic linkage sections.  Collected simply so we can check they're empty. */
> +  .got : {
> +        *(.got)
> +  }
>    .got.plt : {
> -        /*
> -         * PIC/PIE executable contains .got.plt section even if it is not linked
> -         * with dynamic libraries. In such case it is just placeholder for
> -         * _GLOBAL_OFFSET_TABLE_ symbol and .PLT0. .PLT0 is filled by dynamic
> -         * linker and our code is not supposed to be loaded by dynamic linker.
> -         * So, from our point of view .PLT0 is unused. This means that there is
> -         * pretty good chance that we can safely drop .got.plt as a whole here.
> -         * Sadly this is not true. _GLOBAL_OFFSET_TABLE_ is used as a reference
> -         * for relative addressing (and only for that thing) and ld complains if
> -         * we remove .got.plt section here because it cannot find required symbol.
> -         * However, _GLOBAL_OFFSET_TABLE_ is no longer needed in final output.
> -         * So, drop .got.plt section during conversion to plain binary format.
> -         *
> -         * Please check build32.mk for more details.
> -         */
>          *(.got.plt)
>    }
> -
> -  /*
> -   * Discarding .shstrtab is not supported by LLD (LLVM LD) and will trigger an
> -   * error. Also keep the rest of the control sections to match GNU LD behavior.
> -   */
> -  .shstrtab : {
> -        *(.shstrtab)
> +  .igot.plt : {
> +        *(.igot.plt)
>    }
> -  .strtab : {
> -        *(.strtab)
> +  .iplt : {
> +        *(.iplt)
>    }
> -  .symtab : {
> -        *(.symtab)
> +  .plt : {
> +        *(.plt)
>    }
> -
> -  /DISCARD/ : {
> -        /*
> -         * Discard everything else, to prevent linkers from putting
> -         * orphaned sections ahead of .text, which needs to be first.
> -         */
> -        *(*)
> +  .rela : {
> +        *(.rela.*)
>    }
>  }
> +
> +ASSERT(SIZEOF(.got) == 0,         ".got non-empty")
> +/*
> + * At least GNU ld 2.30 and earlier fail to discard the generic part of
> + * .got.plt when no actual entries were allocated. Permit this case alongside
> + * the section being empty.
> + */
> +ASSERT(SIZEOF(.got.plt) == 0 ||
> +       SIZEOF(.got.plt) == 3 * 4, "unexpected .got.plt size")

While here you've adjusted for this being 32-bit code, ...

> +ASSERT(SIZEOF(.igot.plt) == 0,    ".igot.plt non-empty")
> +ASSERT(SIZEOF(.iplt) == 0,        ".iplt non-empty")
> +ASSERT(SIZEOF(.plt) == 0,         ".plt non-empty")
> +ASSERT(SIZEOF(.rela) == 0,        "leftover relocations")

... this (and the construct making the section) would need
converting (or amending) too, as 32-bit uses .rel.*. Then

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

Jan

[PATCH 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Andrew Cooper]

There's no point wasting time converting binaries back to asm source.  Just
use .incbin directly.  Explain in head.S what these binaries are.

Also, align the blobs.  While there's very little static data in the blobs,
they should have at least 4 byte alignment.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

Cleanup to $(head-srcs) deferred to the subsequent patch to make the change
legible.
---
 xen/arch/x86/boot/Makefile |  9 ++++-----
 xen/arch/x86/boot/head.S   | 10 ++++++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 09d1f9f75394..294ac2418583 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -7,7 +7,10 @@ targets += $(head-srcs:.S=.o)
 
 head-srcs := $(addprefix $(obj)/, $(head-srcs))
 
-$(obj)/head.o: $(head-srcs)
+# For .incbin - add $(obj) to the include path and add the dependencies
+# manually as they're not included in .d
+$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
+$(obj)/head.o: $(head-srcs:.S=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -18,10 +21,6 @@ CFLAGS_x86_32 += -I$(srctree)/include
 $(head-srcs:.S=.o): CFLAGS_stack_boundary :=
 $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 
-$(head-srcs): %.S: %.bin
-	(od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \
-	sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@
-
 %.bin: %.lnk
 	$(OBJCOPY) -j .text -O binary $< $@
 
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 3db47197b841..0fb7dd3029f2 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -777,11 +777,17 @@ trampoline_setup:
         /* Jump into the relocated trampoline. */
         lret
 
+        /*
+         * cmdline and reloc are written in C, and linked to be 32bit PIC with
+         * entrypoints at 0 and using the stdcall convention.
+         */
+        ALIGN
 cmdline_parse_early:
-#include "cmdline.S"
+        .incbin "cmdline.bin"
 
+        ALIGN
 reloc:
-#include "reloc.S"
+        .incbin "reloc.bin"
 
 ENTRY(trampoline_start)
 #include "trampoline.S"
-- 
2.11.0

[PATCH v1.1 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Andrew Cooper]

There's no point wasting time converting binaries back to asm source.  Just
use .incbin directly.  Explain in head.S what these binaries are.

Also, align the blobs.  While there's very little static data in the blobs,
they should have at least 4 byte alignment.  There was previously no guarantee
that cmdline_parse_early was aligned, and there is no longer an implicit
4-byte alignment between cmdline_parse_early and reloc caused by the use of
.long.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

v1.1:
 * Rebase over the out-of-tree build work

Cleanup to $(head-srcs) deferred to the subsequent patch to make the change
legible.
---
 xen/arch/x86/boot/Makefile |  9 ++++-----
 xen/arch/x86/boot/head.S   | 10 ++++++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index a5dd094836f6..0670e03b72e0 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -10,7 +10,10 @@ head-srcs := $(addprefix $(obj)/, $(head-srcs))
 ifdef building_out_of_srctree
 $(obj)/head.o: CFLAGS-y += -iquote $(obj)
 endif
-$(obj)/head.o: $(head-srcs)
+# For .incbin - add $(obj) to the include path and add the dependencies
+# manually as they're not included in .d
+$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
+$(obj)/head.o: $(head-srcs:.S=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -24,10 +27,6 @@ CFLAGS_x86_32 += -I$(srctree)/include
 $(head-srcs:.S=.o): CFLAGS_stack_boundary :=
 $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 
-$(head-srcs): %.S: %.bin
-	(od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \
-	sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@
-
 %.bin: %.lnk
 	$(OBJCOPY) -j .text -O binary $< $@
 
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 3db47197b841..0fb7dd3029f2 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -777,11 +777,17 @@ trampoline_setup:
         /* Jump into the relocated trampoline. */
         lret
 
+        /*
+         * cmdline and reloc are written in C, and linked to be 32bit PIC with
+         * entrypoints at 0 and using the stdcall convention.
+         */
+        ALIGN
 cmdline_parse_early:
-#include "cmdline.S"
+        .incbin "cmdline.bin"
 
+        ALIGN
 reloc:
-#include "reloc.S"
+        .incbin "reloc.bin"
 
 ENTRY(trampoline_start)
 #include "trampoline.S"
-- 
2.11.0

Re: [PATCH v1.1 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Anthony PERARD]

On Thu, Apr 14, 2022 at 05:27:39PM +0100, Andrew Cooper wrote:
> There's no point wasting time converting binaries back to asm source.  Just
> use .incbin directly.  Explain in head.S what these binaries are.
> 
> Also, align the blobs.  While there's very little static data in the blobs,
> they should have at least 4 byte alignment.  There was previously no guarantee
> that cmdline_parse_early was aligned, and there is no longer an implicit
> 4-byte alignment between cmdline_parse_early and reloc caused by the use of
> .long.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index a5dd094836f6..0670e03b72e0 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -10,7 +10,10 @@ head-srcs := $(addprefix $(obj)/, $(head-srcs))
>  ifdef building_out_of_srctree
>  $(obj)/head.o: CFLAGS-y += -iquote $(obj)

With this patch, we don't the "-iquote" option above, it was only useful
for both "#include" been removed.

>  endif
> -$(obj)/head.o: $(head-srcs)
> +# For .incbin - add $(obj) to the include path and add the dependencies
> +# manually as they're not included in .d
> +$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> +$(obj)/head.o: $(head-srcs:.S=.bin)

The manual dependencies are needed because `make` needs to know what
other target are needed before building "head.o". The .d files wouldn't
exist on a first build. I don't think a comment about that isn't really
necessary, but if there's one it should be about telling `make` to build
cmdline.bin and head.bin first.

Otherwise, the patch looks good.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v1.1 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Jan Beulich]

On 14.04.2022 18:27, Andrew Cooper wrote:
> There's no point wasting time converting binaries back to asm source.  Just
> use .incbin directly.  Explain in head.S what these binaries are.

Hmm, yes. Why in the world did we do this all these years?

> Also, align the blobs.  While there's very little static data in the blobs,
> they should have at least 4 byte alignment.  There was previously no guarantee
> that cmdline_parse_early was aligned, and there is no longer an implicit
> 4-byte alignment between cmdline_parse_early and reloc caused by the use of
> .long.

There's no alignment associated with using .long, so I think you
want to re-word this.

Jan

Re: [PATCH v1.1 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Andrew Cooper]

On 20/04/2022 13:11, Jan Beulich wrote:
> On 14.04.2022 18:27, Andrew Cooper wrote:
>> There's no point wasting time converting binaries back to asm source.  Just
>> use .incbin directly.  Explain in head.S what these binaries are.
> Hmm, yes. Why in the world did we do this all these years?

One of many good questions.  Others include "why the gratuitous
subsell?" and "who's teaching that 'od | tr | awk | sed | sed | sed' is
a clever idea?"  Apparently someone was under the impression that forks
and userspace pipes were free...

>> Also, align the blobs.  While there's very little static data in the blobs,
>> they should have at least 4 byte alignment.  There was previously no guarantee
>> that cmdline_parse_early was aligned, and there is no longer an implicit
>> 4-byte alignment between cmdline_parse_early and reloc caused by the use of
>> .long.
> There's no alignment associated with using .long, so I think you
> want to re-word this.

What I mean is that .long will guarantee to have a 4 byte size, so reloc
used to end up with the same alignment that cmdline_parse_early did (as
far as internal static data is concerned), whereas now it doesn't.

~Andrew

[PATCH v2 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Andrew Cooper]

There's no point wasting time converting binaries back to asm source.  Just
use .incbin directly.  Explain in head.S what these binaries are.

Also, explicitly align the blobs.  They contain 4-byte objects, and happen to
be 4-byte aligned currently because of the position of `lret` and the size of
cmdline.S but this is incredibly fragile.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

v2:
 * Drop CFLAGS -iquote
---
 xen/arch/x86/boot/Makefile | 10 ++--------
 xen/arch/x86/boot/head.S   | 10 ++++++++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 1fb0ca02e87f..96beb420a260 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -7,10 +7,8 @@ targets += $(head-srcs:.S=.o)
 
 head-srcs := $(addprefix $(obj)/, $(head-srcs))
 
-ifdef building_out_of_srctree
-$(obj)/head.o: CFLAGS-y += -iquote $(obj)
-endif
-$(obj)/head.o: $(head-srcs)
+$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
+$(obj)/head.o: $(head-srcs:.S=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -27,10 +25,6 @@ $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
 LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
 
-$(head-srcs): %.S: %.bin
-	(od -v -t x $< | tr -s ' ' | awk 'NR > 1 {print s} {s=$$0}' | \
-	sed 's/ /,0x/g' | sed 's/,0x$$//' | sed 's/^[0-9]*,/ .long /') >$@
-
 %.bin: %.lnk
 	$(OBJCOPY) -j .text -O binary $< $@
 
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 3db47197b841..0fb7dd3029f2 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -777,11 +777,17 @@ trampoline_setup:
         /* Jump into the relocated trampoline. */
         lret
 
+        /*
+         * cmdline and reloc are written in C, and linked to be 32bit PIC with
+         * entrypoints at 0 and using the stdcall convention.
+         */
+        ALIGN
 cmdline_parse_early:
-#include "cmdline.S"
+        .incbin "cmdline.bin"
 
+        ALIGN
 reloc:
-#include "reloc.S"
+        .incbin "reloc.bin"
 
 ENTRY(trampoline_start)
 #include "trampoline.S"
-- 
2.11.0

Re: [PATCH v2 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

[Jan Beulich]

On 12.08.2022 14:55, Andrew Cooper wrote:
> There's no point wasting time converting binaries back to asm source.  Just
> use .incbin directly.  Explain in head.S what these binaries are.
> 
> Also, explicitly align the blobs.  They contain 4-byte objects, and happen to
> be 4-byte aligned currently because of the position of `lret` and the size of
> cmdline.S but this is incredibly fragile.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[PATCH 3/3] x86/build: Clean up boot/Makefile

[Andrew Cooper]

There are no .S intermediate files, so rework in terms of head-bin-objs.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>

I'm slightly -1 on this, because

  head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))

is substantial obfuscation which I'd prefer to bin.

Anthony: Why does dropping the targets += line interfere with incremental
builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
what would cause that, nor why the normal dependencies on head.o don't work.
---
 xen/arch/x86/boot/Makefile | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 294ac2418583..527f3e393037 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,16 +1,17 @@
 obj-bin-y += head.o
-head-srcs := cmdline.S reloc.S
 
-nocov-y += $(head-srcs:.S=.o)
-noubsan-y += $(head-srcs:.S=.o)
-targets += $(head-srcs:.S=.o)
+head-bin-objs := cmdline.o reloc.o
 
-head-srcs := $(addprefix $(obj)/, $(head-srcs))
+nocov-y += $(head-bin-objs)
+noubsan-y += $(head-bin-objs)
+targets += $(head-bin-objs)
+
+head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
 
 # For .incbin - add $(obj) to the include path and add the dependencies
 # manually as they're not included in .d
 $(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(head-srcs:.S=.bin)
+$(obj)/head.o: $(head-bin-objs:.o=.bin)
 
 CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
 $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
@@ -18,8 +19,8 @@ CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
 CFLAGS_x86_32 += -I$(srctree)/include
 
 # override for 32bit binaries
-$(head-srcs:.S=.o): CFLAGS_stack_boundary :=
-$(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+$(head-bin-objs): CFLAGS_stack_boundary :=
+$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 
 %.bin: %.lnk
 	$(OBJCOPY) -j .text -O binary $< $@
@@ -27,4 +28,4 @@ $(head-srcs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
 %.lnk: %.o $(src)/build32.lds
 	$(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -N -T $(filter %.lds,$^) -o $@ $<
 
-clean-files := cmdline.S reloc.S *.lnk *.bin
+clean-files := *.lnk *.bin
-- 
2.11.0

Re: [PATCH 3/3] x86/build: Clean up boot/Makefile

[Anthony PERARD]

On Thu, Apr 14, 2022 at 12:47:08PM +0100, Andrew Cooper wrote:
> There are no .S intermediate files, so rework in terms of head-bin-objs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

The patch looks fine.

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

> ---
> I'm slightly -1 on this, because
> 
>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
> 
> is substantial obfuscation which I'd prefer to bin.

It might be possible to do something that Kbuild does, which would be to
teach the build system to look for "$(head-objs)" or maybe
"$(head-bin-objs)" when it want to build "head.o". That something that's
done in Kbuild I think to build a module from several source files.

> Anthony: Why does dropping the targets += line interfere with incremental
> builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
> what would cause that, nor why the normal dependencies on head.o don't work.

Try to build with "make V=2", make will display why a target is been
rebuild (when this target is built with $(if_changed, )

$(targets) is used by Rules.mk to findout which dependencies files (the
.cmd) to load and only load them if the target exist. Then the
$(if_changed, ) macro rerun the command if prereq are newer than the
target or if the command as changed. Without the .cmd file loaded, the
macro would compare the new command to an empty value and so rebuild the
target.

Now, the *.bin files are regenerated because cmdline.o is been rebuild
mostly because make didn't load the record of the previous command run.

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/3] x86/build: Clean up boot/Makefile

[Andrew Cooper]

On 14/04/2022 18:45, Anthony PERARD wrote:
> On Thu, Apr 14, 2022 at 12:47:08PM +0100, Andrew Cooper wrote:
>> There are no .S intermediate files, so rework in terms of head-bin-objs.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> The patch looks fine.
>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
>
>> ---
>> I'm slightly -1 on this, because
>>
>>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
>>
>> is substantial obfuscation which I'd prefer to bin.
> It might be possible to do something that Kbuild does, which would be to
> teach the build system to look for "$(head-objs)" or maybe
> "$(head-bin-objs)" when it want to build "head.o". That something that's
> done in Kbuild I think to build a module from several source files.
>
>> Anthony: Why does dropping the targets += line interfere with incremental
>> builds?  With it gone, *.bin are regenerated unconditionally, but I can't see
>> what would cause that, nor why the normal dependencies on head.o don't work.
> Try to build with "make V=2", make will display why a target is been
> rebuild (when this target is built with $(if_changed, )
>
> $(targets) is used by Rules.mk to findout which dependencies files (the
> .cmd) to load and only load them if the target exist. Then the
> $(if_changed, ) macro rerun the command if prereq are newer than the
> target or if the command as changed. Without the .cmd file loaded, the
> macro would compare the new command to an empty value and so rebuild the
> target.
>
> Now, the *.bin files are regenerated because cmdline.o is been rebuild
> mostly because make didn't load the record of the previous command run.

I'm not certain if this case is a match with Linux's module logic.  The
module logic is "compile each .c file, then link all the .o's together
into one .ko".

In this case, we're saying "to assemble head.S to head.o, you first need
to build {cmdline,reloc}.bin so the incbin doesn't explode".  I guess it
depends how generic the "$X depends on arbitrary $Y's" expression can be
made.

Between this patch an the previous one, I've clearly got mixed up with
what exactly the target+= and regular dependencies.

The comment specifically refers to the fact that the old #include
"cmdline.S" used to show up as a dep in .head.o.cmd, whereas .incbin
doesn't.  (Not surprising, because -M and friends are from the
preprocessor, not assembler, but it would be helpful if this limitation
didn't exist.)  As a consequence, the dependency needs adding back in
somehow.

From your description above, I assume that simply being listed as a dep
isn't good enough to trigger a recursive load of the .bin's .cmd file
(not that there is one), which is why they need adding specially to targets?

As I have simplified this to (almost) normal build runes, should we be
expressing it differently now to fit in with the new way of doing things?

~Andrew

Re: [PATCH 3/3] x86/build: Clean up boot/Makefile

[Jan Beulich]

On 14.04.2022 13:47, Andrew Cooper wrote:
> There are no .S intermediate files, so rework in terms of head-bin-objs.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> 
> I'm slightly -1 on this, because
> 
>   head-bin-objs := $(addprefix $(obj)/,$(head-bin-objs))
> 
> is substantial obfuscation which I'd prefer to bin.

Would ...

> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,16 +1,17 @@
>  obj-bin-y += head.o
> -head-srcs := cmdline.S reloc.S
>  
> -nocov-y += $(head-srcs:.S=.o)
> -noubsan-y += $(head-srcs:.S=.o)
> -targets += $(head-srcs:.S=.o)
> +head-bin-objs := cmdline.o reloc.o

head-bin-objs := $(obj)/cmdline.o $(obj)/reloc.o

> -head-srcs := $(addprefix $(obj)/, $(head-srcs))
> +nocov-y += $(head-bin-objs)
> +noubsan-y += $(head-bin-objs)
> +targets += $(head-bin-objs)

nocov-y += $(notdir $(head-bin-objs))

etc perhaps be a little less obfuscation?

Jan