x86/build: Clean up 32bit boot objects

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

Posted by Andrew Cooper 3 years, 10 months ago

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 v2 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

Posted by Andrew Cooper 3 years, 6 months ago

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

Posted by Jan Beulich 3 years, 6 months ago

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 v1.1 2/3] x86/build: Don't convert boot/{cmdline,head}.bin back to .S

Posted by Andrew Cooper 3 years, 10 months ago

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

Posted by Jan Beulich 3 years, 9 months ago

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

Posted by Andrew Cooper 3 years, 9 months ago

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

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

Posted by Anthony PERARD 3 years, 10 months ago

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