[XEN PATCH v7 41/51] build,x86: remove the need for build32.mk

Anthony PERARD posted 51 patches 4 years, 5 months ago
There is a newer version of this series
[XEN PATCH v7 41/51] build,x86: remove the need for build32.mk
Posted by Anthony PERARD 4 years, 5 months ago
Rework "arch/x86/boot/Makefile" to allow it to build both file
"cmdline.S" and "reloc.S" without "build32.mk".

These will now use the main rules for "%.o: %.c", and thus generate a
dependency file. (We will not need to track the dependency manually
anymore.)

But for that, we need to override the main CFLAGS to do a 32bit build.
Thus we copy all the necessary flags from "Config.mk", and apply them
only to "cmdline.o" and "reloc.o".

Specificaly apply the rule "%.S: %.bin" to both cmdline.S and reloc.S
to avoid make trying to regenerate other %.S files with it.

There is no change expected to the resulting "cmdline.S" and
"reloc.S", only the *.o file changes as their symbole for FILE goes
from "cmdline.c" to "arch/x86//cmdline.c". (No idea why "boot" is
missing from the string.) (I've only check with gcc, not clang.)

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/x86/boot/Makefile   | 60 ++++++++++++++++++++++++++----------
 xen/arch/x86/boot/build32.mk | 40 ------------------------
 2 files changed, 44 insertions(+), 56 deletions(-)
 delete mode 100644 xen/arch/x86/boot/build32.mk

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 73a3b1514965..3580a9b656e8 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,23 +1,51 @@
 obj-bin-y += head.o
+head-objs := cmdline.S reloc.S
 
-DEFS_H_DEPS = $(BASEDIR)/$(src)/defs.h $(BASEDIR)/include/xen/stdbool.h
+nocov-y += $(head-objs:.S=.o)
+noubsan-y += $(head-objs:.S=.o)
+targets += $(head-objs:.S=.o)
 
-CMDLINE_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/$(src)/video.h \
-	       $(BASEDIR)/include/xen/kconfig.h \
-	       $(BASEDIR)/include/generated/autoconf.h
+head-objs := $(addprefix $(obj)/, $(head-objs))
 
-RELOC_DEPS = $(DEFS_H_DEPS) \
-	     $(BASEDIR)/include/generated/autoconf.h \
-	     $(BASEDIR)/include/xen/kconfig.h \
-	     $(BASEDIR)/include/xen/multiboot.h \
-	     $(BASEDIR)/include/xen/multiboot2.h \
-	     $(BASEDIR)/include/xen/const.h \
-	     $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
+$(obj)/head.o: $(head-objs)
 
-$(obj)/head.o: $(obj)/cmdline.S $(obj)/reloc.S
+LDFLAGS_DIRECT_OpenBSD = _obsd
+LDFLAGS_DIRECT_FreeBSD = _fbsd
+$(head-objs:.S=.lnk): LDFLAGS_DIRECT := -melf_i386$(LDFLAGS_DIRECT_$(XEN_OS))
 
-$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds
-	$(MAKE) -f $(BASEDIR)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)"
+CFLAGS_x86_32 := -m32 -march=i686
+CFLAGS_x86_32 += -fno-strict-aliasing
+CFLAGS_x86_32 += -std=gnu99
+CFLAGS_x86_32 += -Wall -Wstrict-prototypes
+$(call cc-option-add,CFLAGS_x86_32,CC,-Wdeclaration-after-statement)
+$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-but-set-variable)
+$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-local-typedefs)
+$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
+CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
+CFLAGS_x86_32 += -I$(srctree)/include
 
-$(obj)/reloc.S: $(src)/reloc.c $(RELOC_DEPS) $(src)/build32.lds
-	$(MAKE) -f $(BASEDIR)/$(src)/build32.mk -C $(obj) $(@F) RELOC_DEPS="$(RELOC_DEPS)"
+# override for 32bit binaries
+$(head-objs:.S=.o): CFLAGS-stack-boundary :=
+$(head-objs:.S=.o): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
+
+$(head-objs): %.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 $< $@
+
+
+%.lnk: %.o $(src)/build32.lds
+	$(LD) $(LDFLAGS_DIRECT) -N -T $(filter %.lds,$^) -o $@ $<
diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
deleted file mode 100644
index e90680cd9f52..000000000000
--- a/xen/arch/x86/boot/build32.mk
+++ /dev/null
@@ -1,40 +0,0 @@
-override XEN_TARGET_ARCH=x86_32
-CFLAGS =
-include $(XEN_ROOT)/Config.mk
-
-$(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
-
-CFLAGS += -Werror -fno-builtin -g0 -msoft-float
-CFLAGS += -I$(BASEDIR)/include
-CFLAGS := $(filter-out -flto,$(CFLAGS)) 
-
-# NB. awk invocation is a portable alternative to 'head -n -1'
-%.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 $< $@
-
-%.lnk: %.o build32.lds
-	$(LD) $(LDFLAGS_DIRECT) -N -T build32.lds -o $@ $<
-
-%.o: %.c
-	$(CC) $(CFLAGS) -c -fpic $< -o $@
-
-cmdline.o: cmdline.c $(CMDLINE_DEPS)
-
-reloc.o: reloc.c $(RELOC_DEPS)
-
-.PRECIOUS: %.bin %.lnk
-- 
Anthony PERARD


Re: [XEN PATCH v7 41/51] build,x86: remove the need for build32.mk
Posted by Jan Beulich 4 years, 3 months ago
On 24.08.2021 12:50, Anthony PERARD wrote:
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,23 +1,51 @@
>  obj-bin-y += head.o
> +head-objs := cmdline.S reloc.S
>  
> -DEFS_H_DEPS = $(BASEDIR)/$(src)/defs.h $(BASEDIR)/include/xen/stdbool.h
> +nocov-y += $(head-objs:.S=.o)
> +noubsan-y += $(head-objs:.S=.o)
> +targets += $(head-objs:.S=.o)

This working right depends on targets initially getting set with := ,
because of ...

> -CMDLINE_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/$(src)/video.h \
> -	       $(BASEDIR)/include/xen/kconfig.h \
> -	       $(BASEDIR)/include/generated/autoconf.h
> +head-objs := $(addprefix $(obj)/, $(head-objs))

... this subsequent adjustment to the variable. Might it be more future
proof for start with

head-srcs := cmdline.S reloc.S

and then derive head-objs only here?

> -RELOC_DEPS = $(DEFS_H_DEPS) \
> -	     $(BASEDIR)/include/generated/autoconf.h \
> -	     $(BASEDIR)/include/xen/kconfig.h \
> -	     $(BASEDIR)/include/xen/multiboot.h \
> -	     $(BASEDIR)/include/xen/multiboot2.h \
> -	     $(BASEDIR)/include/xen/const.h \
> -	     $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
> +$(obj)/head.o: $(head-objs)
>  
> -$(obj)/head.o: $(obj)/cmdline.S $(obj)/reloc.S
> +LDFLAGS_DIRECT_OpenBSD = _obsd
> +LDFLAGS_DIRECT_FreeBSD = _fbsd

This is somewhat ugly - it means needing to change things in two places
when config/x86_32.mk would change (e.g. to add another build OS). How
about ...

> +$(head-objs:.S=.lnk): LDFLAGS_DIRECT := -melf_i386$(LDFLAGS_DIRECT_$(XEN_OS))

... instead:

$(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst x86_64,i386,$(LDFLAGS_DIRECT))

? Or if deemed still too broad

$(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst elf_x86_64,elf_i386,$(LDFLAGS_DIRECT))

?

> -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds
> -	$(MAKE) -f $(BASEDIR)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)"
> +CFLAGS_x86_32 := -m32 -march=i686
> +CFLAGS_x86_32 += -fno-strict-aliasing
> +CFLAGS_x86_32 += -std=gnu99
> +CFLAGS_x86_32 += -Wall -Wstrict-prototypes
> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wdeclaration-after-statement)
> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-but-set-variable)
> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-local-typedefs)
> +$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> +CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
> +CFLAGS_x86_32 += -I$(srctree)/include

I'm afraid I'm not convinced that having to keep this in sync with the
original is in fair balance with the removal of build32.mk.

Jan


Re: [XEN PATCH v7 41/51] build,x86: remove the need for build32.mk
Posted by Anthony PERARD 4 years, 3 months ago
On Thu, Oct 14, 2021 at 10:32:05AM +0200, Jan Beulich wrote:
> On 24.08.2021 12:50, Anthony PERARD wrote:
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -1,23 +1,51 @@
> >  obj-bin-y += head.o
> > +head-objs := cmdline.S reloc.S
> >  
> > -DEFS_H_DEPS = $(BASEDIR)/$(src)/defs.h $(BASEDIR)/include/xen/stdbool.h
> > +nocov-y += $(head-objs:.S=.o)
> > +noubsan-y += $(head-objs:.S=.o)
> > +targets += $(head-objs:.S=.o)
> 
> This working right depends on targets initially getting set with := ,
> because of ...

They are because Rules.mk initialise those variables that way.

> > -CMDLINE_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/$(src)/video.h \
> > -	       $(BASEDIR)/include/xen/kconfig.h \
> > -	       $(BASEDIR)/include/generated/autoconf.h
> > +head-objs := $(addprefix $(obj)/, $(head-objs))
> 
> ... this subsequent adjustment to the variable. Might it be more future
> proof for start with
> 
> head-srcs := cmdline.S reloc.S
> 
> and then derive head-objs only here?

I'll give it a try.

> > -RELOC_DEPS = $(DEFS_H_DEPS) \
> > -	     $(BASEDIR)/include/generated/autoconf.h \
> > -	     $(BASEDIR)/include/xen/kconfig.h \
> > -	     $(BASEDIR)/include/xen/multiboot.h \
> > -	     $(BASEDIR)/include/xen/multiboot2.h \
> > -	     $(BASEDIR)/include/xen/const.h \
> > -	     $(BASEDIR)/include/public/arch-x86/hvm/start_info.h
> > +$(obj)/head.o: $(head-objs)
> >  
> > -$(obj)/head.o: $(obj)/cmdline.S $(obj)/reloc.S
> > +LDFLAGS_DIRECT_OpenBSD = _obsd
> > +LDFLAGS_DIRECT_FreeBSD = _fbsd
> 
> This is somewhat ugly - it means needing to change things in two places
> when config/x86_32.mk would change (e.g. to add another build OS). How
> about ...
> 
> > +$(head-objs:.S=.lnk): LDFLAGS_DIRECT := -melf_i386$(LDFLAGS_DIRECT_$(XEN_OS))
> 
> ... instead:
> 
> $(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> 
> ? Or if deemed still too broad
> 
> $(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst elf_x86_64,elf_i386,$(LDFLAGS_DIRECT))
> 
> ?

Sounds good, I'll do that.

> > -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds
> > -	$(MAKE) -f $(BASEDIR)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)"
> > +CFLAGS_x86_32 := -m32 -march=i686
> > +CFLAGS_x86_32 += -fno-strict-aliasing
> > +CFLAGS_x86_32 += -std=gnu99
> > +CFLAGS_x86_32 += -Wall -Wstrict-prototypes
> > +$(call cc-option-add,CFLAGS_x86_32,CC,-Wdeclaration-after-statement)
> > +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-but-set-variable)
> > +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-local-typedefs)
> > +$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > +CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
> > +CFLAGS_x86_32 += -I$(srctree)/include
> 
> I'm afraid I'm not convinced that having to keep this in sync with the
> original is in fair balance with the removal of build32.mk.

Why would this needs to be kept in sync with the original? I would
actually like to get rid of Config.mk, I don't see the point in sharing
CFLAGS between a kernel and userspace binaries. Also, I hope one day
that we can have the hypervisor in its own repo, and thus they won't be
any Config.mk to keep in sync with.

The goal of this patch is less about removing build32.mk, and more about
using the rest of the build system infrastructure to build those few
32bits objects, and been able to use the automatic dependencies
generation and other things without having to duplicate them.

It probably make the build a bit faster as there is two less invocation
of $(MAKE) (which parse Config.mk).

As for a possible change: instead of having those x86_32 flags in
arch/x86/boot/, I could have them in arch/x86/arch.mk. Linux does
something like that were it prepare REALMODE_CFLAGS.

I know it's a bit annoying to have another list x86_32 cflags, but I
don't see how we can extract them from Config.mk (in a Makefile where we
want to use both x86_32 and x86_64 flags).

Thanks,

-- 
Anthony PERARD

Re: [XEN PATCH v7 41/51] build,x86: remove the need for build32.mk
Posted by Jan Beulich 4 years, 3 months ago
On 15.10.2021 17:52, Anthony PERARD wrote:
> On Thu, Oct 14, 2021 at 10:32:05AM +0200, Jan Beulich wrote:
>> On 24.08.2021 12:50, Anthony PERARD wrote:
>>> -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds
>>> -	$(MAKE) -f $(BASEDIR)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)"
>>> +CFLAGS_x86_32 := -m32 -march=i686
>>> +CFLAGS_x86_32 += -fno-strict-aliasing
>>> +CFLAGS_x86_32 += -std=gnu99
>>> +CFLAGS_x86_32 += -Wall -Wstrict-prototypes
>>> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wdeclaration-after-statement)
>>> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-but-set-variable)
>>> +$(call cc-option-add,CFLAGS_x86_32,CC,-Wno-unused-local-typedefs)
>>> +$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
>>> +CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float
>>> +CFLAGS_x86_32 += -I$(srctree)/include
>>
>> I'm afraid I'm not convinced that having to keep this in sync with the
>> original is in fair balance with the removal of build32.mk.
> 
> Why would this needs to be kept in sync with the original? I would
> actually like to get rid of Config.mk, I don't see the point in sharing
> CFLAGS between a kernel and userspace binaries. Also, I hope one day
> that we can have the hypervisor in its own repo, and thus they won't be
> any Config.mk to keep in sync with.

My point wasn't about Config.mk as the reference in particular, but ...

> The goal of this patch is less about removing build32.mk, and more about
> using the rest of the build system infrastructure to build those few
> 32bits objects, and been able to use the automatic dependencies
> generation and other things without having to duplicate them.
> 
> It probably make the build a bit faster as there is two less invocation
> of $(MAKE) (which parse Config.mk).
> 
> As for a possible change: instead of having those x86_32 flags in
> arch/x86/boot/, I could have them in arch/x86/arch.mk. Linux does
> something like that were it prepare REALMODE_CFLAGS.
> 
> I know it's a bit annoying to have another list x86_32 cflags, but I
> don't see how we can extract them from Config.mk (in a Makefile where we
> want to use both x86_32 and x86_64 flags).

... about the general (apparent) plan to have two sets of flags, which
may not strictly _need_ to remain in sync, but better would. So while
I appreciate the alternative of putting stuff in arch.mk, I'd still
like to see common things like -W... to get probed for and added to
whichever variable just once.

Jan