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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan