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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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,

-- 
Anthony PERARD