Re: [PATCH v2] xen: EXPERT clean-up and introduce UNSUPPORTED

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2020 01:50, Stefano Stabellini wrote:
> 1) It is not obvious that "Configure standard Xen features (expert
> users)" is actually the famous EXPERT we keep talking about on xen-devel

Which can be addressed by simply changing the one prompt line.

> 2) It is not obvious when we need to enable EXPERT to get a specific
> feature
> 
> In particular if you want to enable ACPI support so that you can boot
> Xen on an ACPI platform, you have to enable EXPERT first. But searching
> through the kconfig menu it is really not clear (type '/' and "ACPI"):
> nothing in the description tells you that you need to enable EXPERT to
> get the option.

And what causes this to be different once you switch to UNSUPPORTED?

> So this patch makes things easier by doing two things:
> 
> - introduce a new kconfig option UNSUPPORTED which is clearly to enable
>   UNSUPPORTED features as defined by SUPPORT.md
> 
> - change EXPERT options to UNSUPPORTED where it makes sense: keep
>   depending on EXPERT for features made for experts
> 
> - tag unsupported features by adding (UNSUPPORTED) to the one-line
>   description

I am, btw, not fully convinced of the need for this redundancy. Wouldn't
it be enough to have just EXPERT as a setting, but varying (<reason>)
tokens in the prompt text?

> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -34,8 +34,17 @@ config DEFCONFIG_LIST
>       option defconfig_list
>       default ARCH_DEFCONFIG
>  
> +config UNSUPPORTED
> +     bool "Configure UNSUPPORTED features"
> +     help
> +       This option allows unsupported Xen options to be enabled, which

I'd recommend against "enabled" - a control may also be there to allow
disabling something.

> +       includes non-security-supported, experimental, and tech preview
> +       features as defined by SUPPORT.md. Xen binaries built with this
> +       option enabled are not security supported.

Overall I'm a little afraid of possible inverse implications: Anything
_not_ dependent upon this option (and in particular anything not
dependent upon any Kconfig control) may be considered supported then.

Also the last sentence is already present for EXPERT, 

> +     default n

I realize you likely merely copied what EXPERT has, but this "default n"
is pretty pointless and hence would better be omitted imo.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -102,8 +102,8 @@ config HVM
>         If unsure, say Y.
>  
>  config XEN_SHSTK
> -     bool "Supervisor Shadow Stacks"
> -     depends on HAS_AS_CET_SS && EXPERT
> +     bool "Supervisor Shadow Stacks (UNSUPPORTED)"
> +     depends on HAS_AS_CET_SS && UNSUPPORTED
>       default y

Andrew, I think I did ask on v1 already: Do we need to continue to
consider this unsupported? While perhaps not a change to make right in
this patch, it should perhaps be a pre-patch then to avoid the need to
touch it here.

> @@ -165,7 +165,7 @@ config HVM_FEP

Seeing just the patch context here, I think HVM_FEP may also want
converting.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -151,7 +151,7 @@ config KEXEC
>         If unsure, say Y.
>  
>  config EFI_SET_VIRTUAL_ADDRESS_MAP
> -    bool "EFI: call SetVirtualAddressMap()" if EXPERT
> +    bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED

I have to admit I'm pretty unsure about what to do with this one.

> @@ -272,7 +272,7 @@ config LATE_HWDOM
>         If unsure, say N.
>  
>  config ARGO
> -     bool "Argo: hypervisor-mediated interdomain communication" if EXPERT
> +     bool "Argo: hypervisor-mediated interdomain communication 
> (UNSUPPORTED)" if UNSUPPORTED

Perhaps better (EXPERIMENTAL)?

> --- a/xen/common/sched/Kconfig
> +++ b/xen/common/sched/Kconfig
> @@ -15,7 +15,7 @@ config SCHED_CREDIT2
>         optimized for lower latency and higher VM density.
>  
>  config SCHED_RTDS
> -     bool "RTDS scheduler support (EXPERIMENTAL)"
> +     bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED
>       default y
>       ---help---
>         The RTDS scheduler is a soft and firm real-time scheduler for
> @@ -23,14 +23,14 @@ config SCHED_RTDS
>         in the cloud, and general low-latency workloads.
>  
>  config SCHED_ARINC653
> -     bool "ARINC653 scheduler support (EXPERIMENTAL)"
> +     bool "ARINC653 scheduler support (UNSUPPORTED)" if UNSUPPORTED
>       default DEBUG
>       ---help---
>         The ARINC653 scheduler is a hard real-time scheduler for single
>         cores, targeted for avionics, drones, and medical devices.
>  
>  config SCHED_NULL
> -     bool "Null scheduler support (EXPERIMENTAL)"
> +     bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED
>       default y
>       ---help---
>         The null scheduler is a static, zero overhead scheduler,

I'd like to see (EXPERIMENTAL) stay everywhere here.

Jan