| Message ID | 20210824105038.1257926-42-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
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
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,
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
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
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