Re: [XEN PATCH v8 26/47] build,x86: remove the need for build32.mk

[Jan Beulich <jbeulich@xxxxxxxx>]

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