Re: [Xen-devel] [PATCH v3 1/8] x86: determine HAVE_AS_* just once

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Konrad Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v3 1/8] x86: determine HAVE_AS_* just once
Date: Mon, 20 Jan 2020 12:04:44 +0000	[thread overview]
Message-ID: <20200120120444.GY11756@Air-de-Roger> (raw)
In-Reply-To: <9f4b57e9-c7a4-78e9-32c1-b25530c550f4@suse.com>

On Mon, Jan 06, 2020 at 05:34:45PM +0100, Jan Beulich wrote:
> With the exception of HAVE_AS_QUOTED_SYM, populate the results into a
> generated header instead of (at least once per [sub]directory) into
> CFLAGS. This results in proper rebuilds (via make dependencies) in case
> the compiler used changes between builds. It additionally eases
> inspection of which assembler features were actually found usable.
> 
> Some trickery is needed to avoid header generation itself to try to
> include the to-be/not-yet-generated header.
> 
> Since the definitions in generated/config.h, previously having been
> command line options, might even affect xen/config.h or its descendants,
> move adding of the -include option for the latter after inclusion of the
> per-arch Rules.mk. Use the occasion to also move the most general -I
> option to the common Rules.mk.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: New.
> ---
> An alternative to the $(MAKECMDGOALS) trickery would be to make
> generation of generated/config.h part of the asm-offsets.s rule, instead
> of adding it as a dependency there. Not sure whether either is truly
> better than the other.
> 
> --- a/Config.mk
> +++ b/Config.mk
> @@ -151,7 +151,7 @@ endif
>  # as-insn: Check whether assembler supports an instruction.
>  # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no)
>  as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
> -                       | $(filter-out -M% %.d -include %/include/xen/config.h,$(1)) \
> +                       | $(filter-out -M% %.d -include %/config.h,$(1)) \

Don't you need to filter out -include xen/config.h as added to CLFAGS
below?

>                                -c -x c -o /dev/null - 2>&1),$(4),$(3))
>  
>  # as-option-add: Conditionally add options to flags
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -59,7 +59,7 @@ endif
>  CFLAGS += -nostdinc -fno-builtin -fno-common
>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
>  $(call cc-option-add,CFLAGS,CC,-Wvla)
> -CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
> +CFLAGS += -pipe -D__XEN__ -I$(BASEDIR)/include
>  CFLAGS-$(CONFIG_DEBUG_INFO) += -g
>  CFLAGS += '-D__OBJECT_FILE__="$@"'
>  
> @@ -97,6 +97,9 @@ LDFLAGS += $(LDFLAGS-y)
>  
>  include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
>  
> +# Allow the arch to use -include ahead of this one.
> +CFLAGS += -include xen/config.h
> +
>  DEPS = .*.d
>  
>  include Makefile
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -6,8 +6,6 @@
>  # 'make clean' before rebuilding.
>  #
>  
> -CFLAGS += -I$(BASEDIR)/include
> -
>  $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>  $(call cc-option-add,CFLAGS,CC,-Wnested-externs)
>  
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -224,7 +224,8 @@ endif
>  efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: $(BASEDIR)/arch/x86/efi/built_in.o
>  efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: ;
>  
> -asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h
> +asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h \
> +	$(BASEDIR)/include/generated/config.h
>  	$(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $<
>  
>  asm-macros.i: CFLAGS += -D__ASSEMBLY__ -P
> @@ -240,6 +241,45 @@ $(BASEDIR)/include/asm-x86/asm-macros.h:
>  	echo '#endif' >>$@.new
>  	$(call move-if-changed,$@.new,$@)
>  
> +# There are multiple invocations of this Makefile, one each for asm-offset.s,
> +# $(TARGET), built_in.o, and several more from the rules building $(TARGET)
> +# and $(TARGET).efi. The 2nd and 3rd may race with one another, and we don't
> +# want to re-generate config.h in that case anyway, so guard the logic
> +# accordingly. (We do want to have the FORCE dependency on the rule, to be
> +# sure we pick up changes when the compiler used has changed.)
> +ifeq ($(MAKECMDGOALS),asm-offsets.s)
> +
> +as-ISA-list := CLWB EPT FSGSBASE INVPCID RDRAND RDSEED SSE4_2 VMX XSAVEOPT
> +
> +CLWB-insn	:= clwb (%rax)
> +EPT-insn	:= invept (%rax),%rax
> +FSGSBASE-insn	:= rdfsbase %rax
> +INVPCID-insn	:= invpcid (%rax),%rax
> +RDRAND-insn	:= rdrand %eax
> +RDSEED-insn	:= rdseed %eax
> +SSE4_2-insn	:= crc32 %eax,%eax
> +VMX-insn	:= vmcall
> +XSAVEOPT-insn	:= xsaveopt (%rax)
> +
> +as-features-list := $(as-ISA-list) NEGATIVE_TRUE NOPS_DIRECTIVE

I think it would be clearer to place this below the NEGATIVE_TRUE and
NOPS_DIRECTIVE definitions below? So that all FOO-insn are together.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel