Re: [PATCH v2 2/4] x86: allow Kconfig control over psABI level

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2023 14:28, Jan Beulich wrote:
> On 19.07.2023 12:04, Andrew Cooper wrote:
>> On 19/07/2023 10:44 am, Jan Beulich wrote:
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -118,6 +118,36 @@ config HVM
>>>  
>>>       If unsure, say Y.
>>>  
>>> +choice
>>> +   prompt "base psABI level"
>>> +   default X86_64_BASELINE
>>> +   help
>>> +     The psABI defines 4 levels of ISA extension sets as a coarse granular
>>> +     way of identifying advanced functionality that would be uniformly
>>> +     available in respectively newer hardware.  While v4 is not really of
>>> +     interest for Xen, the others can be selected here, making the
>>> +     resulting Xen no longer work on older hardware.  This option won't
>>> +     have any effect if the toolchain doesn't support the distinction.
>>> +
>>> +     If unsure, stick to the default.
>>> +
>>> +config X86_64_BASELINE
>>> +   bool "baseline"
>>> +
>>> +config X86_64_V2
>>> +   bool "v2"
>>> +   help
>>> +     This enables POPCNT and CX16, besides other extensions which are of
>>> +     no interest here.
>>> +
>>> +config X86_64_V3
>>> +   bool "v3"
>>> +   help
>>> +     This enables BMI, BMI2, LZCNT, MOVBE, and XSAVE, besides other
>>> +     extensions which are of no interest here.
>>> +
>>> +endchoice
>>> +
>>>  config XEN_SHSTK
>>>     bool "Supervisor Shadow Stacks"
>>>     depends on HAS_AS_CET_SS
>>> --- a/xen/arch/x86/arch.mk
>>> +++ b/xen/arch/x86/arch.mk
>>> @@ -36,6 +36,10 @@ CFLAGS += -mno-red-zone -fpic
>>>  # the SSE setup for variadic function calls.
>>>  CFLAGS += -mno-mmx -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
>>>  
>>> +# Enable the selected baseline ABI, if supported by the compiler.
>>> +CFLAGS-$(CONFIG_X86_64_V2) += $(call cc-option,$(CC),-march=x86-64-v2)
>>> +CFLAGS-$(CONFIG_X86_64_V3) += $(call cc-option,$(CC),-march=x86-64-v3)
>>
>> I know we're having severe disagreements over Kconfig compiler checking,
>> but this patch cannot cannot go in in this form.
>>
>> You're asking the user unconditionally for the psABI level, then
>> ignoring the answer on toolchains which don't understand it.
>>
>> The makefile needs to be unconditional, and the Kconfig options need to
>> depend on suitable toolchain support.  This is the only way we don't get
>> a false statement written into the .config, and embedded in hypfs.
> 
> I was tempted to base this on "x86: convert CET tool chain feature checks
> to mixed Kconfig/Makefile model", but then it likely wouldn't have stood
> a chance to go in either. The technical issues aside that need solving in
> that other patch, I still haven't had any feedback on the conceptual
> aspects. Yet as said in other contexts, without having the conceptual
> side (largely) settled, there's no incentive for me to invest time in
> dealing with the technical issues (which surely are solvable).
> 
> When raising this aspect, did you pay attention to the first of the TBDs
> in the patch? If we were to force build errors (for no real reason, see
> below), we should first try those fallbacks, to limit the possible
> damage. As mentioned there, support for these -march= forms isn't all
> that old.
> 
> As to forcing build errors in the first place, that goes against the
> intentions with the mixed Kconfig / Makefile checking model. There we
> would only issue warnings. Albeit as mentioned in that patch, that's up
> for discussion, and a majority may view things differently than I do.
> Especially here there's no reason to outright fail builds, though:
> .config / hypfs wouldn't really state anything wrong - the binary merely
> wouldn't make use of newer insns despite being permitted to.

In an attempt to fit both your and my expectations, what about another
prereq patch along the lines of the below one, of course then accompanied
by adjustments to this patch (to first try the fallback mentioned, and
then complain - as configured - if that's also not successful)?

Cc-ing other people as well which would be Cc-ed on an eventual proper
submission.

Jan

build: permit Kconfig control over how to deal with unsatisfiable choices

Some options we allow the build admin to select may require new enough
tool chain components to fulfill (partly or entirely). Provide yet
another control to pick what action to take at the end of the build
process - be silent about this, warn, or fail the build.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
---
This may not be fine grained enough: Optimization settings (like added
by "x86: allow Kconfig control over psABI level") may want dealing with
differently than security relevant ones (like XEN_SHSTK or XEN_IBT).

Whether to do this uniformly at the end of the build is up for
discussion: In the "warn" case we will want the resulting output late,
so it is more likely to be noticed. In the "fail build" case though we
may want the failure to occur early.

--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -64,6 +64,25 @@ config UNSUPPORTED
          preview features as defined by SUPPORT.md. (Note that if an option
          doesn't depend on UNSUPPORTED it doesn't imply that is supported.)
 
+choice
+       prompt "How to deal with settings which cannot be satisified"
+       default UNSATISFIED_WARNING
+       help
+         Some selectable options may depend on e.g. tool chain functionality.
+         Select here how to deal with such when actually building a such
+         configured hypervisor.
+
+config UNSATISFIED_SILENT
+       bool "silent"
+
+config UNSATISFIED_WARNING
+       bool "emit warnings"
+
+config UNSATISFIED_ERROR
+       bool "fail the build"
+
+endchoice
+
 config LTO
        bool "Link Time Optimisation"
        depends on BROKEN
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -588,6 +588,10 @@ $(TARGET): outputmakefile FORCE
        $(Q)$(MAKE) $(build)=arch/$(SRCARCH) include
        $(Q)$(MAKE) $(build)=. arch/$(SRCARCH)/include/asm/asm-offsets.h
        $(Q)$(MAKE) $(build)=. MKRELOC=$(MKRELOC) 'ALL_OBJS=$(ALL_OBJS-y)' 
'ALL_LIBS=$(ALL_LIBS-y)' $@
+ifneq ($(CONFIG_UNSATISFIED_SILENT),y)
+       $(Q)$(if $(strip $(XEN_CONFIG_UNSATISFIED)),$(MAKE),:) \
+           $(build)=. 'XEN_CONFIG_UNSATISFIED=$(XEN_CONFIG_UNSATISFIED)' 
check_unsatisfied
+endif
 
 SUBDIRS = xsm arch common crypto drivers lib test
 define all_sources
--- a/xen/build.mk
+++ b/xen/build.mk
@@ -88,3 +88,11 @@ targets += prelink.o
 
 $(TARGET): prelink.o FORCE
        $(Q)$(MAKE) $(build)=arch/$(SRCARCH) $@
+
+PHONY += check_unsatisfied
+check_unsatisfied:
+       $(Q): $(if $(filter y,$(CONFIG_UNSATISFIED_WARNING)), \
+                  $(warning The following selections could not be satisfied:), 
\
+                  $(shell echo 'The following selections could not be 
satisfied:' >&2)) \
+             $(foreach c,$(sort $(XEN_CONFIG_UNSATISFIED)),$(shell echo ' - 
CONFIG_$c' >&2)) \
+             $(if $(filter y,$(CONFIG_UNSATISFIED_ERROR)),$(error Failing 
build))