[v3,1/8] x86: determine HAVE_AS_* just once
diff mbox series

Message ID 9f4b57e9-c7a4-78e9-32c1-b25530c550f4@suse.com
State New
Headers show
Series
  • x86emul: further work
Related show

Commit Message

Jan Beulich Jan. 6, 2020, 4:34 p.m. UTC
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.

Comments

Andrew Cooper Jan. 6, 2020, 4:41 p.m. UTC | #1
On 06/01/2020 16:34, 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.

Is it v3 or v4?

Also, is it intended for just backport?  It is largely redundant with
Anthony's Kconfig/Kbuild efforts, as moving these (and other checks)
into the Kconfig step is the ultimate goal.

~Andrew
Jan Beulich Jan. 6, 2020, 4:56 p.m. UTC | #2
On 06.01.2020 17:41, Andrew Cooper wrote:
> On 06/01/2020 16:34, 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.
> 
> Is it v3 or v4?

v4, as said in reply to the cover letter.

> Also, is it intended for just backport?  It is largely redundant with
> Anthony's Kconfig/Kbuild efforts, as moving these (and other checks)
> into the Kconfig step is the ultimate goal.

Is it? Looking at current Linux I still see e.g.

# do binutils support CFI?
cfi := $(call as-instr,.cfi_startproc\n.cfi_rel_offset $(sp-y)$(comma)0\n.cfi_endproc,-DCONFIG_AS_CFI=1)
# is .cfi_signal_frame supported too?
cfi-sigframe := $(call as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1)
cfi-sections := $(call as-instr,.cfi_sections .debug_frame,-DCONFIG_AS_CFI_SECTIONS=1)

# does binutils support specific instructions?
asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1)
avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1)
avx512_instr :=$(call as-instr,vpmovm2b %k1$(comma)%zmm5,-DCONFIG_AS_AVX512=1)
sha1_ni_instr :=$(call as-instr,sha1msg1 %xmm0$(comma)%xmm1,-DCONFIG_AS_SHA1_NI=1)
sha256_ni_instr :=$(call as-instr,sha256msg1 %xmm0$(comma)%xmm1,-DCONFIG_AS_SHA256_NI=1)

KBUILD_AFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) $(avx512_instr) $(sha1_ni_instr) $(sha256_ni_instr)
KBUILD_CFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) $(avx512_instr) $(sha1_ni_instr) $(sha256_ni_instr)

in arch/x86/Makefile. I am, btw, also unconvinced that such belongs
into .config in the first place: The configuration doesn't change
if I swap my tool chain. Of course I realized there's the grey area
of user visible options depending on tool chain capabilities (if
one means to allow such).

Jan
Roger Pau Monné Jan. 20, 2020, 12:04 p.m. UTC | #3
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.
Jan Beulich Jan. 20, 2020, 12:36 p.m. UTC | #4
On 20.01.2020 13:04, Roger Pau Monné wrote:
> On Mon, Jan 06, 2020 at 05:34:45PM +0100, Jan Beulich wrote:
>> --- 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?

The whole point of the change is to filter out _both_ config.h (as
well as any future one) - the one under include/xen/ and the one
under include/generated/. Hence the widening of what the pattern
would match.

>> @@ -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.

If your remark was about just the last line - yes, perhaps (and
looking at it again I don't even know why it ended up in the
place it's in right now). But I'm told the original mechanism is
going to be replaced now by a Kconfig one anyway. If this is
going to happen soon, the patch here would be of no further
interest.

Jan

Patch
diff mbox series

--- 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)) \
                               -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
+
+# GAS's idea of true is -1.  Clang's idea is 1.
+NEGATIVE_TRUE-insn := .if ((1 > 0) > 0); .error \"\"; .endif
+
+# Check to see whether the assembler supports the .nop directive.
+NOPS_DIRECTIVE-insn := .L1: .L2: .nops (.L2 - .L1),9
+
+$(BASEDIR)/include/generated/config.h: FORCE
+	echo '/* Generated header, do not edit. */' >$@.new
+	$(foreach f,$(as-features-list), \
+	  $(if $($(f)-insn),,$(error $(f)-insn is empty)) \
+	  echo '#$(call as-insn,$(CC) $(CFLAGS),"$($(f)-insn)", \
+	           define, \
+	           undef) HAVE_AS_$(f) /* $($(f)-insn) */' >>$@.new;)
+	$(call move-if-changed,$@.new,$@)
+
+endif
+
 xen.lds: xen.lds.S
 	$(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(AFLAGS)) -o $@ $<
 	sed -e 's/.*\.lds\.o:/$(@F):/g' <.$(@F).d >.$(@F).d.new
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -3,7 +3,7 @@ 
 
 XEN_IMG_OFFSET := 0x200000
 
-CFLAGS += -I$(BASEDIR)/include
+CFLAGS += $(if $(filter asm-macros.% %/generated/config.h,$@),,-include generated/config.h)
 CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
 CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default
 CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)
@@ -38,26 +38,9 @@  endif
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 $(call cc-option-add,CFLAGS,CC,-Wnested-externs)
-$(call as-option-add,CFLAGS,CC,"vmcall",-DHAVE_AS_VMX)
-$(call as-option-add,CFLAGS,CC,"crc32 %eax$$(comma)%eax",-DHAVE_AS_SSE4_2)
-$(call as-option-add,CFLAGS,CC,"invept (%rax)$$(comma)%rax",-DHAVE_AS_EPT)
-$(call as-option-add,CFLAGS,CC,"rdrand %eax",-DHAVE_AS_RDRAND)
-$(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE)
-$(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT)
-$(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
-$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
 $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1", \
                      -U__OBJECT_LABEL__ -DHAVE_AS_QUOTED_SYM \
                      '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
-$(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID)
-
-# GAS's idea of true is -1.  Clang's idea is 1
-$(call as-option-add,CFLAGS,CC,\
-    ".if ((1 > 0) < 0); .error \"\";.endif",,-DHAVE_AS_NEGATIVE_TRUE)
-
-# Check to see whether the assmbler supports the .nop directive.
-$(call as-option-add,CFLAGS,CC,\
-    ".L1: .L2: .nops (.L2 - .L1)$$(comma)9",-DHAVE_AS_NOPS_DIRECTIVE)
 
 CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -66,7 +66,7 @@  compat/%.h: compat/%.i Makefile $(BASEDI
 	mv -f $@.new $@
 
 compat/%.i: compat/%.c Makefile
-	$(CPP) $(filter-out -Wa$(comma)% -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
+	$(CPP) $(filter-out -Wa$(comma)% -M% %.d -include %/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
 
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)