Re: [XEN PATCH v7 49/51] build: adding out-of-tree support to the xen build

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Oct 14, 2021 at 12:14:29PM +0200, Jan Beulich wrote:
> On 24.08.2021 12:50, Anthony PERARD wrote:
> > This implement out-of-tree support, there's two ways to create an
> > out-of-tree build tree (after that, `make` in that new directory
> > works):
> >     make O=build
> >     mkdir build; cd build; make -f ../Makefile
> > also works with an absolute path for both.
> > 
> > This implementation only works if the source tree is clean, as we use
> > VPATH.
> > 
> > This patch copies most new code with handling out-of-tree build from
> > Linux v5.12.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > ---
> >  xen/Makefile                | 173 +++++++++++++++++++++++++++++-------
> >  xen/Rules.mk                |  11 ++-
> >  xen/arch/arm/efi/Makefile   |   3 +
> >  xen/arch/x86/arch.mk        |   3 +
> >  xen/arch/x86/boot/Makefile  |   5 +-
> >  xen/arch/x86/efi/Makefile   |   3 +
> >  xen/include/Makefile        |   9 +-
> >  xen/scripts/mkmakefile      |  17 ++++
> >  xen/test/livepatch/Makefile |   2 +
> >  xen/xsm/flask/Makefile      |   3 +
> >  xen/xsm/flask/ss/Makefile   |   3 +
> >  11 files changed, 194 insertions(+), 38 deletions(-)
> >  create mode 100755 xen/scripts/mkmakefile
> 
> Linux have done away with this script just recently; I don't think we
> should introduce it.

Will change.

> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -1,3 +1,7 @@
> > +# $(lastword,) for GNU Make older than 0.81
> 
> DYM 3.81?

Yes.

> > +lastword = $(word $(words $(1)),$(1))
> > +this-makefile := $(call lastword,$(MAKEFILE_LIST))
> 
> Oh - is it documented somewhere that this would always be last?

Yes, it is even in Make's documentation about the variable:
    "if the first thing a makefile does is examine the last word in this
    variable, it will be the name of the current makefile."
    (taken from Make 3.80 documentation)

> > @@ -17,33 +21,18 @@ export XEN_BUILD_HOST   ?= $(shell hostname)
> >  PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 
> > 2>/dev/null) python)
> >  export PYTHON              ?= $(PYTHON_INTERPRETER)
> >  
> > -export XEN_ROOT := $(CURDIR)/..
> > +$(if $(filter __%, $(MAKECMDGOALS)), \
> > +   $(error targets prefixed with '__' are only for internal use))
> >  
> > -srctree := .
> > -objtree := .
> > -export srctree objtree
> > +# That's our default target when none is given on the command line
> > +PHONY := __all
> > +__all:
> >  
> >  # Do not use make's built-in rules and variables
> >  MAKEFLAGS += -rR
> >  
> >  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
> >  
> > -ARCH=$(XEN_TARGET_ARCH)
> > -SRCARCH=$(shell echo $(ARCH) | \
> > -          sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> > -              -e s'/riscv.*/riscv/g')
> > -export ARCH SRCARCH
> > -
> > -# Don't break if the build process wasn't called from the top level
> > -# we need XEN_TARGET_ARCH to generate the proper config
> > -include $(XEN_ROOT)/Config.mk
> > -
> > -# Set ARCH/SUBARCH appropriately.
> > -export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
> > -export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
> > -                            sed -e 's/x86.*/x86/' -e 
> > s'/arm\(32\|64\)/arm/g' \
> > -                                -e s'/riscv.*/riscv/g')
> > -
> >  # Allow someone to change their config file
> >  export KCONFIG_CONFIG ?= .config
> 
> Would it be possible to split off some of the pure movement of pieces,
> to make it easier to see what (if anything) is actually changed at the
> same time as getting moved?

I'll give it a try.

> > @@ -51,14 +40,9 @@ export CC CXX LD
> >  
> >  export TARGET := xen
> >  
> > -.PHONY: default
> > -default: build
> > -
> >  .PHONY: dist
> >  dist: install
> >  
> > -include scripts/Kbuild.include
> > -
> >  ifneq ($(root-make-done),y)
> >  # section to run before calling Rules.mk, but only once.
> >  
> > @@ -130,14 +114,93 @@ endif
> >  
> >  ifneq ($(findstring s,$(filter-out --%,$(MAKEFLAGS))),)
> >      quiet := silent_
> > +    KBUILD_VERBOSE = 0
> >  endif
> 
> In how far is this related here? Doesn't this belong in an earlier patch?

That probably belong to a patch that have been committed some time ago,
and I've notice it was missing when comparing our makefile to Linux's
one. (Or maybe it wasn't set as well in Linux originally). I'll look if
that can be added to another patch, or just have a patch for this single
line.

> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -38,7 +38,7 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
> >                           $(foreach r,rel rel.ro,data.$(r).local)
> >  
> >  # The filename build.mk has precedence over Makefile
> > -mk-dir := $(src)
> > +mk-dir := $(srctree)/$(src)
> >  include $(if $(wildcard 
> > $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile)
> 
> Perhaps already when it was changed to $(src) the name has become
> slightly misleading, at least imo: I would rather expect a variable
> with this name to refer to the build dir/tree. Maybe "srcdir" or
> even shorted "sd" right from the start? (Reaching here I can finally
> see why having a shorthand is helpful.)

I have to think about that. I've made some further progress in order to
be able to build the Xen pvhshim without a link farm and notice that
nearly every source file needs to use "$(srctree)/$(src)" and I'm not
sure "$(src)" could be use alone. So having a single variable which have
both would be useful.

"srcdir" might be to close to "srctree" and one might mistake it for the
other, so "sd" might be better.

> > --- a/xen/arch/x86/efi/Makefile
> > +++ b/xen/arch/x86/efi/Makefile
> > @@ -1,5 +1,8 @@
> >  CFLAGS-y += -fshort-wchar
> >  CFLAGS-y += -I$(srctree)/common/efi
> > +ifdef building_out_of_srctree
> > +CFLAGS-y += -I$(srctree)/$(src)
> > +endif
> 
> At the example of this (where perhaps -iquote could be used again) - is
> it strictly necessary to have the ifdef-s around such? I.e. would things
> fail to work for an in-tree build without them?

ifdef isn't really necessary, gcc always check that directory first
anyway (for quote-included headers), so it is just a duplicate of gcc's
path list, but it would work fine. So I'll remove the ifdef.

> > --- a/xen/xsm/flask/ss/Makefile
> > +++ b/xen/xsm/flask/ss/Makefile
> > @@ -9,3 +9,6 @@ obj-y += conditional.o
> >  obj-y += mls.o
> >  
> >  CFLAGS-y += -I$(srctree)/xsm/flask/include
> > +ifdef building_out_of_srctree
> > +    CFLAGS-y += -I$(objtree)/xsm/flask/include
> 
> There's no header in $(srctree)/xsm/flask/include in this case, so if you
> use "ifdef" here, shouldn't that other part move into an "else"?

Actually, there are headers in the source, like "security.h", and `git
ls-files xen/xsm/flask/include` shows plenty other headers.

Thanks,

-- 
Anthony PERARD