| Message ID | 20211125134006.1076646-27-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
On 25.11.2021 14:39, Anthony PERARD wrote: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -171,6 +171,10 @@ export LEX = $(if $(FLEX),$(FLEX),flex) > # Default file for 'make defconfig'. > export KBUILD_DEFCONFIG := $(ARCH)_defconfig > > +# Copy CFLAGS generated by "Config.mk" so they can be reused later without > +# reparsing Config.mk by e.g. arch/x86/boot/. > +export XEN_COMMON_CFLAGS := $(CFLAGS) For my own understanding (it's hard to check being half way through the series): At this point there are no further adjustments expected to CFLAGS? > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -1,25 +1,45 @@ > obj-bin-y += head.o > +head-objs := cmdline.S reloc.S Is "-objs" really a suitable part of the name for a list of *.S? > -DEFS_H_DEPS = $(abs_srctree)/$(src)/defs.h $(abs_srctree)/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) $(abs_srctree)/$(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 > +$(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst x86_64,i386,$(LDFLAGS_DIRECT)) Considering there's just a single use of LDFLAGS_DIRECT below, wouldn't it make sense to avoid overriding the variable and doing the $(subst ...) right at the use site instead? > -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds > - $(MAKE) -f $(abs_srctree)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)" > +CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_COMMON_CFLAGS)) I can't seem to be able to spot -march=i686 in the old code. Or wait - Is this duplicating what config/x86_32.mk has? > +$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) > +CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float You did inherit -Werror and -fno-builtin from $(XEN_COMMON_CFLAGS) already, so I don't think you need to specify these again? > +CFLAGS_x86_32 += -I$(srctree)/include Isn't this present in $(XEN_COMMON_CFLAGS) as well? > -$(obj)/reloc.S: $(src)/reloc.c $(RELOC_DEPS) $(src)/build32.lds > - $(MAKE) -f $(abs_srctree)/$(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 -fpic should again already be there. > +$(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 $< $@ > + > + Nit: Please no double blank lines. Jan
On Tue, Dec 21, 2021 at 02:33:18PM +0100, Jan Beulich wrote: > On 25.11.2021 14:39, Anthony PERARD wrote: > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -171,6 +171,10 @@ export LEX = $(if $(FLEX),$(FLEX),flex) > > # Default file for 'make defconfig'. > > export KBUILD_DEFCONFIG := $(ARCH)_defconfig > > > > +# Copy CFLAGS generated by "Config.mk" so they can be reused later without > > +# reparsing Config.mk by e.g. arch/x86/boot/. > > +export XEN_COMMON_CFLAGS := $(CFLAGS) > > For my own understanding (it's hard to check being half way through the > series): At this point there are no further adjustments expected to > CFLAGS? I'm not sure what you mean. The comment should be explicit. CFLAGS is going to be adjusted after the copy, for the benefit of all the 64bit code (when building x86_64). The "renamed" to XEN_CFLAGS to be useable by the rest of the tree. The name "XEN_COMMON_CFLAGS" might not be the right one, it is only common to everything in xen.git, so same flags that the toolstack would work with. > > --- a/xen/arch/x86/boot/Makefile > > +++ b/xen/arch/x86/boot/Makefile > > @@ -1,25 +1,45 @@ > > obj-bin-y += head.o > > +head-objs := cmdline.S reloc.S > > Is "-objs" really a suitable part of the name for a list of *.S? Maybe not. I think this "x-objs" naming is used in Linux to build modules with more than one object. But I guess here we have an object with more than one source. Maybe "head-srcs" is better, even if it's compiled code made ready to include into a .S source file. > > -DEFS_H_DEPS = $(abs_srctree)/$(src)/defs.h $(abs_srctree)/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) $(abs_srctree)/$(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 > > +$(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst x86_64,i386,$(LDFLAGS_DIRECT)) > > Considering there's just a single use of LDFLAGS_DIRECT below, wouldn't > it make sense to avoid overriding the variable and doing the $(subst ...) > right at the use site instead? Yes, that might be ok to do. > > -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds > > - $(MAKE) -f $(abs_srctree)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)" > > +CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_COMMON_CFLAGS)) > > I can't seem to be able to spot -march=i686 in the old code. Or wait - > Is this duplicating what config/x86_32.mk has? Yes. > > +$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > +CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float > > You did inherit -Werror and -fno-builtin from $(XEN_COMMON_CFLAGS) > already, so I don't think you need to specify these again? No, because I didn't want to change the CFLAGS used to build the 32bits binaries in this patch. So XEN_COMMON_CFLAGS just hold the CFLAGS produced by "Config.mk" for the x86_32 arch. So XEN_COMMON_CFLAGS is different from XEN_CFLAGS. If we want to use the same CFLAGS to build head.o, then I think it would be better to do in it's own patch. I can probably do it before or after this patch as XEN_CFLAGS is already available. Thanks,
On 18.01.2022 11:50, Anthony PERARD wrote: > On Tue, Dec 21, 2021 at 02:33:18PM +0100, Jan Beulich wrote: >> On 25.11.2021 14:39, Anthony PERARD wrote: >>> --- a/xen/Makefile >>> +++ b/xen/Makefile >>> @@ -171,6 +171,10 @@ export LEX = $(if $(FLEX),$(FLEX),flex) >>> # Default file for 'make defconfig'. >>> export KBUILD_DEFCONFIG := $(ARCH)_defconfig >>> >>> +# Copy CFLAGS generated by "Config.mk" so they can be reused later without >>> +# reparsing Config.mk by e.g. arch/x86/boot/. >>> +export XEN_COMMON_CFLAGS := $(CFLAGS) >> >> For my own understanding (it's hard to check being half way through the >> series): At this point there are no further adjustments expected to >> CFLAGS? > > I'm not sure what you mean. The comment should be explicit. > > CFLAGS is going to be adjusted after the copy, for the benefit of all > the 64bit code (when building x86_64). The "renamed" to XEN_CFLAGS to be > useable by the rest of the tree. I guess I misread the comment, sorry. > The name "XEN_COMMON_CFLAGS" might not be the right one, it is only > common to everything in xen.git, so same flags that the toolstack would > work with. XEN_GLOBAL_CFLAGS would come to mind, but is unlikely to be viewed as significantly better. Maybe XEN_TREEWIDE_CFLAGS? Jan
diff --git a/xen/Makefile b/xen/Makefile index deae319c8a5a..90e8191f51ce 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -171,6 +171,10 @@ export LEX = $(if $(FLEX),$(FLEX),flex) # Default file for 'make defconfig'. export KBUILD_DEFCONFIG := $(ARCH)_defconfig +# Copy CFLAGS generated by "Config.mk" so they can be reused later without +# reparsing Config.mk by e.g. arch/x86/boot/. +export XEN_COMMON_CFLAGS := $(CFLAGS) + # CLANG_FLAGS needs to be calculated before calling Kconfig ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),) CLANG_FLAGS := diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 1ac8cb435e0e..5395aae5d57e 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -1,25 +1,45 @@ obj-bin-y += head.o +head-objs := cmdline.S reloc.S -DEFS_H_DEPS = $(abs_srctree)/$(src)/defs.h $(abs_srctree)/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) $(abs_srctree)/$(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 +$(head-objs:.S=.lnk): LDFLAGS_DIRECT := $(subst x86_64,i386,$(LDFLAGS_DIRECT)) -$(obj)/cmdline.S: $(src)/cmdline.c $(CMDLINE_DEPS) $(src)/build32.lds - $(MAKE) -f $(abs_srctree)/$(src)/build32.mk -C $(obj) $(@F) CMDLINE_DEPS="$(CMDLINE_DEPS)" +CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_COMMON_CFLAGS)) +$(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 $(abs_srctree)/$(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 $@ $< clean-files := cmdline.S reloc.S *.lnk *.bin 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. We introduce XEN_COMMON_CFLAGS which can be reused in boot/Makefile, and avoid the need to reparse Config.mk with a different value for XEN_TARGET_ARCH. From this new $(XEN_COMMON_CFLAGS), we only need to change -m64 to have the 32bit flags. Then those are applied only to "cmdline.o" and "reloc.o". Specifically 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 symbol 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> --- Notes: v8: - avoid the need to list CFLAGS from Config.mk a second time by introducing XEN_COMMON_CFLAGS, and using it in boot/ - improve LDFLAGS_DIRECT, by just substitute x64 to i368 from x86 LDFLAGS_DIRECT. And thus avoid copying the logic from Config.mk. xen/Makefile | 4 +++ xen/arch/x86/boot/Makefile | 52 +++++++++++++++++++++++++----------- xen/arch/x86/boot/build32.mk | 40 --------------------------- 3 files changed, 40 insertions(+), 56 deletions(-) delete mode 100644 xen/arch/x86/boot/build32.mk