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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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