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

Stefano Stabellini posted 1 patch 1 week, 3 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20201118005051.26115-1-sstabellini@kernel.org
Maintainers: Ian Jackson <iwj@xenproject.org>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>, Wei Liu <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>, Julien Grall <julien@xen.org>, George Dunlap <george.dunlap@citrix.com>, Stefano Stabellini <sstabellini@kernel.org>, Jan Beulich <jbeulich@suse.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Dario Faggioli <dfaggioli@suse.com>
xen/Kconfig              | 11 ++++++++++-
xen/arch/arm/Kconfig     | 10 +++++-----
xen/arch/x86/Kconfig     |  8 ++++----
xen/common/Kconfig       |  4 ++--
xen/common/sched/Kconfig |  6 +++---
5 files changed, 24 insertions(+), 15 deletions(-)

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

Posted by Stefano Stabellini 1 week, 3 days ago
From: Stefano Stabellini <stefano.stabellini@xilinx.com>

A recent thread [1] has exposed a couple of issues with our current way
of handling EXPERT.

1) It is not obvious that "Configure standard Xen features (expert
users)" is actually the famous EXPERT we keep talking about on xen-devel

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.

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

- clarify the EXPERT one-line description

[1] https://marc.info/?l=xen-devel&m=160333101228981

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: andrew.cooper3@citrix.com
CC: george.dunlap@citrix.com
CC: iwj@xenproject.org
CC: jbeulich@suse.com
CC: julien@xen.org
CC: wl@xen.org

---
Changes in v2:
- introduce UNSUPPORTED as a separate new option
- don't switch all EXPERT options to UNSUPPORTED
---
 xen/Kconfig              | 11 ++++++++++-
 xen/arch/arm/Kconfig     | 10 +++++-----
 xen/arch/x86/Kconfig     |  8 ++++----
 xen/common/Kconfig       |  4 ++--
 xen/common/sched/Kconfig |  6 +++---
 5 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 34c318bfa2..59400c4788 100644
--- 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
+	  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.
+	default n
+
 config EXPERT
-	bool "Configure standard Xen features (expert users)"
+	bool "Configure EXPERT features"
 	help
 	  This option allows certain base Xen options and settings
 	  to be disabled or tweaked. This is for specialized environments
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index f938dd21bd..5981e7380d 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -32,7 +32,7 @@ menu "Architecture Features"
 source "arch/Kconfig"
 
 config ACPI
-	bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT
+	bool "ACPI (Advanced Configuration and Power Interface) Support (UNSUPPORTED)" if UNSUPPORTED
 	depends on ARM_64
 	---help---
 
@@ -49,7 +49,7 @@ config GICV3
 	  If unsure, say Y
 
 config HAS_ITS
-        bool "GICv3 ITS MSI controller support" if EXPERT
+        bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
         depends on GICV3 && !NEW_VGIC
 
 config HVM
@@ -79,7 +79,7 @@ config SBSA_VUART_CONSOLE
 	  SBSA Generic UART implements a subset of ARM PL011 UART.
 
 config ARM_SSBD
-	bool "Speculative Store Bypass Disable" if EXPERT
+	bool "Speculative Store Bypass Disable (UNSUPPORTED)" if UNSUPPORTED
 	depends on HAS_ALTERNATIVE
 	default y
 	help
@@ -89,7 +89,7 @@ config ARM_SSBD
 	  If unsure, say Y.
 
 config HARDEN_BRANCH_PREDICTOR
-	bool "Harden the branch predictor against aliasing attacks" if EXPERT
+	bool "Harden the branch predictor against aliasing attacks (UNSUPPORTED)" if UNSUPPORTED
 	default y
 	help
 	  Speculation attacks against some high-performance processors rely on
@@ -106,7 +106,7 @@ config HARDEN_BRANCH_PREDICTOR
 	  If unsure, say Y.
 
 config TEE
-	bool "Enable TEE mediators support" if EXPERT
+	bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
 	default n
 	help
 	  This option enables generic TEE mediators support. It allows guests
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 24868aa6ad..d4e20e9d31 100644
--- 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
 	---help---
 	  Control-flow Enforcement Technology (CET) is a set of features in
@@ -165,7 +165,7 @@ config HVM_FEP
 	  If unsure, say N.
 
 config TBOOT
-	bool "Xen tboot support" if EXPERT
+	bool "Xen tboot support (UNSUPPORTED)" if UNSUPPORTED
 	default y if !PV_SHIM_EXCLUSIVE
 	select CRYPTO
 	---help---
@@ -251,7 +251,7 @@ config HYPERV_GUEST
 endif
 
 config MEM_SHARING
-	bool "Xen memory sharing support" if EXPERT
+	bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED
 	depends on HVM
 
 endmenu
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 3e2cf25088..beed507727 100644
--- 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
     ---help---
       Call EFI SetVirtualAddressMap() runtime service to setup memory map for
       further runtime services. According to UEFI spec, it isn't strictly
@@ -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
 	---help---
 	  Enables a hypercall for domains to ask the hypervisor to perform
 	  data transfer of messages between domains.
diff --git a/xen/common/sched/Kconfig b/xen/common/sched/Kconfig
index 61231aacaa..94c9e20139 100644
--- 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,
-- 
2.17.1


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

Posted by Bertrand Marquis 1 week, 3 days ago
Hi Stefano,

> On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> A recent thread [1] has exposed a couple of issues with our current way
> of handling EXPERT.
> 
> 1) It is not obvious that "Configure standard Xen features (expert
> users)" is actually the famous EXPERT we keep talking about on xen-devel
> 
> 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.

This is a great change that makes configuration more clear.

> 
> 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
> 
> - clarify the EXPERT one-line description

Should we also follow the scheme and add (EXPERT) in the text for expert options ?

and one small fix

> 
> [1] https://marc.info/?l=xen-devel&m=160333101228981
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: andrew.cooper3@citrix.com
> CC: george.dunlap@citrix.com
> CC: iwj@xenproject.org
> CC: jbeulich@suse.com
> CC: julien@xen.org
> CC: wl@xen.org
> 
> ---
> Changes in v2:
> - introduce UNSUPPORTED as a separate new option
> - don't switch all EXPERT options to UNSUPPORTED
> ---
> xen/Kconfig              | 11 ++++++++++-
> xen/arch/arm/Kconfig     | 10 +++++-----
> xen/arch/x86/Kconfig     |  8 ++++----
> xen/common/Kconfig       |  4 ++--
> xen/common/sched/Kconfig |  6 +++---
> 5 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/Kconfig b/xen/Kconfig
> index 34c318bfa2..59400c4788 100644
> --- 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
> +	  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.
> +	default n
> +
> config EXPERT
> -	bool "Configure standard Xen features (expert users)"
> +	bool "Configure EXPERT features"
> 	help
> 	  This option allows certain base Xen options and settings
> 	  to be disabled or tweaked. This is for specialized environments
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index f938dd21bd..5981e7380d 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -32,7 +32,7 @@ menu "Architecture Features"
> source "arch/Kconfig"
> 
> config ACPI
> -	bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT
> +	bool "ACPI (Advanced Configuration and Power Interface) Support (UNSUPPORTED)" if UNSUPPORTED
> 	depends on ARM_64
> 	---help---
> 
> @@ -49,7 +49,7 @@ config GICV3
> 	  If unsure, say Y
> 
> config HAS_ITS
> -        bool "GICv3 ITS MSI controller support" if EXPERT
> +        bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
>         depends on GICV3 && !NEW_VGIC
> 
> config HVM
> @@ -79,7 +79,7 @@ config SBSA_VUART_CONSOLE
> 	  SBSA Generic UART implements a subset of ARM PL011 UART.
> 
> config ARM_SSBD
> -	bool "Speculative Store Bypass Disable" if EXPERT
> +	bool "Speculative Store Bypass Disable (UNSUPPORTED)" if UNSUPPORTED
> 	depends on HAS_ALTERNATIVE
> 	default y
> 	help
> @@ -89,7 +89,7 @@ config ARM_SSBD
> 	  If unsure, say Y.
> 
> config HARDEN_BRANCH_PREDICTOR
> -	bool "Harden the branch predictor against aliasing attacks" if EXPERT
> +	bool "Harden the branch predictor against aliasing attacks (UNSUPPORTED)" if UNSUPPORTED
> 	default y
> 	help
> 	  Speculation attacks against some high-performance processors rely on
> @@ -106,7 +106,7 @@ config HARDEN_BRANCH_PREDICTOR
> 	  If unsure, say Y.
> 
> config TEE
> -	bool "Enable TEE mediators support" if EXPERT
> +	bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED
> 	default n
> 	help
> 	  This option enables generic TEE mediators support. It allows guests
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 24868aa6ad..d4e20e9d31 100644
> --- 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

This one is not following the standard scheme with “if UNSUPPORTED"

Cheers
Bertrand

> 	default y
> 	---help---
> 	  Control-flow Enforcement Technology (CET) is a set of features in
> @@ -165,7 +165,7 @@ config HVM_FEP
> 	  If unsure, say N.
> 
> config TBOOT
> -	bool "Xen tboot support" if EXPERT
> +	bool "Xen tboot support (UNSUPPORTED)" if UNSUPPORTED
> 	default y if !PV_SHIM_EXCLUSIVE
> 	select CRYPTO
> 	---help---
> @@ -251,7 +251,7 @@ config HYPERV_GUEST
> endif
> 
> config MEM_SHARING
> -	bool "Xen memory sharing support" if EXPERT
> +	bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED
> 	depends on HVM
> 
> endmenu
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 3e2cf25088..beed507727 100644
> --- 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
>     ---help---
>       Call EFI SetVirtualAddressMap() runtime service to setup memory map for
>       further runtime services. According to UEFI spec, it isn't strictly
> @@ -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
> 	---help---
> 	  Enables a hypercall for domains to ask the hypervisor to perform
> 	  data transfer of messages between domains.
> diff --git a/xen/common/sched/Kconfig b/xen/common/sched/Kconfig
> index 61231aacaa..94c9e20139 100644
> --- 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,
> -- 
> 2.17.1
> 

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

Posted by Jan Beulich 1 week, 3 days ago
On 18.11.2020 09:45, Bertrand Marquis wrote:
>> On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> --- 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
> 
> This one is not following the standard scheme with “if UNSUPPORTED"

There's no standard scheme here: There's one case where the entire
option depends on some other setting (e.g. UNSUPPORTED) and another
where just the prompt (i.e. giving the user a choice) does. The
difference becomes obvious when the option has a default other than
"no": Despite the invisible prompt, it may get turned on. In the
case here (serving as a good example), "default y" would mean the
feature gets turned on when "if UNSUPPORTED" would be added to the
prompt and when UNSUPPORTED is itself off.

Jan

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

Posted by Bertrand Marquis 1 week, 3 days ago
Hi Jan,

> On 18 Nov 2020, at 08:50, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 18.11.2020 09:45, Bertrand Marquis wrote:
>>> On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> --- 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
>> 
>> This one is not following the standard scheme with “if UNSUPPORTED"
> 
> There's no standard scheme here: There's one case where the entire
> option depends on some other setting (e.g. UNSUPPORTED) and another
> where just the prompt (i.e. giving the user a choice) does. The
> difference becomes obvious when the option has a default other than
> "no": Despite the invisible prompt, it may get turned on. In the
> case here (serving as a good example), "default y" would mean the
> feature gets turned on when "if UNSUPPORTED" would be added to the
> prompt and when UNSUPPORTED is itself off.
> 

It makes sense, thanks for the explanation

Bertrand

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

Posted by Jan Beulich 1 week, 3 days ago
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

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

Posted by Stefano Stabellini 1 week, 3 days ago
On Wed, 18 Nov 2020, Jan Beulich wrote:
> 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?

Two things: firstly, it doesn't and shouldn't take an expert to enable
ACPI support, even if ACPI support is experimental. So calling it
UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig
options changed by this patch. Secondly, this patch is adding
"(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match
it with the option you need to enable.


> > 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?

I don't think so, for the same reasons written above: EXPERT should not
be gating things like ACPI. Moreover, the advantage of the tag in the
oneline prompt is that you can search for an option and figure out that
you need to enable UNSUPPORTED. It doesn't work if we use a different
tag. Just to get the idea, try to do "make menuconfig" and search for
"ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for
"UNSUPPORTED" you can find what you need to enable.


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

I can change that.


> > +	  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, 

I am happy to rephrase it. What about:

"This option allows certain unsupported Xen options to be changed, which
includes non-security-supported, experimental, and tech preview features
as defined by SUPPORT.md."


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

OK


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

Of course I have no opinion on this. I am happy to do as instructed.


> > @@ -165,7 +165,7 @@ config HVM_FEP
> 
> Seeing just the patch context here, I think HVM_FEP may also want
> converting.

OK


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

Yeah, similarly to XEN_SHSTK, I don't have an opinion here either.  I am
happy to change it or leave it as.


> > @@ -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)?

For this and also the schedulers options below, although it is true that
(EXPERIMENTAL) is a more accurate description, then the problem is that
it is not easy to match against UNSUPPORTED. In other words, if you
search for "ARGO" in make menuconfig and it is marked with
(EXPERIMENTAL), it is not obvious that you need to enable UNSUPPORTED to
get it as an option. For this reason, I think it is better to use
(UNSUPPORTED) both here and below for SCHED_ARINC653 and SCHED_NULL.


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


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

Posted by Jan Beulich 1 week, 2 days ago
On 18.11.2020 22:00, Stefano Stabellini wrote:
> On Wed, 18 Nov 2020, Jan Beulich wrote:
>> 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?
> 
> Two things: firstly, it doesn't and shouldn't take an expert to enable
> ACPI support, even if ACPI support is experimental. So calling it
> UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig
> options changed by this patch. Secondly, this patch is adding
> "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match
> it with the option you need to enable.

There's redundancy here then, which I think is in almost all cases
better to avoid. That's first and foremost because the two places
can go out of sync. Therefore, if the primary thing is to help
"make menuconfig" (which I admit I don't normally use, as it's
nothing that gets invoked implicitly by the build process afaict,
i.e. one has to actively invoke it), perhaps we should enhance
kconfig to attach at least a pre-determined subset of labels to
the prompts automatically?

And second, also in reply to what you've been saying further down,
perhaps we would better go with a hierarchy of controls here, e.g.
EXPERT -> EXPERIMENTAL -> 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?
> 
> I don't think so, for the same reasons written above: EXPERT should not
> be gating things like ACPI.

Different views are possible here, I suppose. Turning on anything
that's unsupported requires people to know what they're doing (and
be ready to pick up the pieces themselves). I'd consider this to
fall under "expert".

> Moreover, the advantage of the tag in the
> oneline prompt is that you can search for an option and figure out that
> you need to enable UNSUPPORTED. It doesn't work if we use a different
> tag. Just to get the idea, try to do "make menuconfig" and search for
> "ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for
> "UNSUPPORTED" you can find what you need to enable.

Implying that textual representation and Kconfig option name match,
see above. Even a simple spelling mistake would break this model.

>>> --- 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.
> 
> I can change that.
> 
> 
>>> +	  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, 
> 
> I am happy to rephrase it. What about:
> 
> "This option allows certain unsupported Xen options to be changed, which
> includes non-security-supported, experimental, and tech preview features
> as defined by SUPPORT.md."

Sounds better to me.

>>> --- 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.
> 
> Yeah, similarly to XEN_SHSTK, I don't have an opinion here either.  I am
> happy to change it or leave it as.

I guess at least for the first cut I'd like to ask to just leave it
alone.

Jan

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

Posted by Stefano Stabellini 1 week, 2 days ago
On Thu, 19 Nov 2020, Jan Beulich wrote:
> On 18.11.2020 22:00, Stefano Stabellini wrote:
> > On Wed, 18 Nov 2020, Jan Beulich wrote:
> >> 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?
> > 
> > Two things: firstly, it doesn't and shouldn't take an expert to enable
> > ACPI support, even if ACPI support is experimental. So calling it
> > UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig
> > options changed by this patch. Secondly, this patch is adding
> > "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match
> > it with the option you need to enable.
> 
> There's redundancy here then, which I think is in almost all cases
> better to avoid. That's first and foremost because the two places
> can go out of sync. Therefore, if the primary thing is to help
> "make menuconfig" (which I admit I don't normally use, as it's
> nothing that gets invoked implicitly by the build process afaict,
> i.e. one has to actively invoke it), perhaps we should enhance
> kconfig to attach at least a pre-determined subset of labels to
> the prompts automatically?
> 
> And second, also in reply to what you've been saying further down,
> perhaps we would better go with a hierarchy of controls here, e.g.
> EXPERT -> EXPERIMENTAL -> UNSUPPORTED?

Both these are good ideas worth discussing; somebody else made a similar
suggestion some time back. I was already thinking this could be a great
candidate for one of the first "working groups" as defined by George
during the last community call because the topic is not purely
technical: a working group could help getting alignment and make
progress faster. We can propose it to George when he is back.

However, I don't think we need the working group to make progress on
this limited patch that only addresses the lowest hanging fruit.

I'd like to suggest to make progress on this patch in its current form,
and in parallel start a longer term discussion on how to do something
like you suggested above.


> >>> 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?
> > 
> > I don't think so, for the same reasons written above: EXPERT should not
> > be gating things like ACPI.
> 
> Different views are possible here, I suppose. Turning on anything
> that's unsupported requires people to know what they're doing (and
> be ready to pick up the pieces themselves). I'd consider this to
> fall under "expert".

For some features it is as you wrote, but it is not true in all cases.
Let's take ACPI as an example: it doesn't take an expert to enable it
and if it breaks you are in no worse situation than if you didn't,
because if you don't enable it you can't boot at all on the platform.

Also, that's not how EXPERT is commonly used in other projects.
Typically EXPERT is to enable advanced debugging features, and I have
been told recently that it is confusing the way we use it in Xen.

These are the two things that I would like to fix as soon as possible.


> > Moreover, the advantage of the tag in the
> > oneline prompt is that you can search for an option and figure out that
> > you need to enable UNSUPPORTED. It doesn't work if we use a different
> > tag. Just to get the idea, try to do "make menuconfig" and search for
> > "ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for
> > "UNSUPPORTED" you can find what you need to enable.
> 
> Implying that textual representation and Kconfig option name match,
> see above. Even a simple spelling mistake would break this model.

True, a spelling mistake would cause problems, but we do reviews, and
we can make sure there are no spelling mistakes in this patch.

 
> >>> --- 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.
> > 
> > I can change that.
> > 
> > 
> >>> +	  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, 
> > 
> > I am happy to rephrase it. What about:
> > 
> > "This option allows certain unsupported Xen options to be changed, which
> > includes non-security-supported, experimental, and tech preview features
> > as defined by SUPPORT.md."
> 
> Sounds better to me.

I'll use it


> >>> --- 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.
> > 
> > Yeah, similarly to XEN_SHSTK, I don't have an opinion here either.  I am
> > happy to change it or leave it as.
> 
> I guess at least for the first cut I'd like to ask to just leave it
> alone.

OK

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

Posted by Jan Beulich 1 week, 1 day ago
On 19.11.2020 22:40, Stefano Stabellini wrote:
> On Thu, 19 Nov 2020, Jan Beulich wrote:
>> On 18.11.2020 22:00, Stefano Stabellini wrote:
>>> On Wed, 18 Nov 2020, Jan Beulich wrote:
>>>> 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?
>>>
>>> Two things: firstly, it doesn't and shouldn't take an expert to enable
>>> ACPI support, even if ACPI support is experimental. So calling it
>>> UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig
>>> options changed by this patch. Secondly, this patch is adding
>>> "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match
>>> it with the option you need to enable.
>>
>> There's redundancy here then, which I think is in almost all cases
>> better to avoid. That's first and foremost because the two places
>> can go out of sync. Therefore, if the primary thing is to help
>> "make menuconfig" (which I admit I don't normally use, as it's
>> nothing that gets invoked implicitly by the build process afaict,
>> i.e. one has to actively invoke it), perhaps we should enhance
>> kconfig to attach at least a pre-determined subset of labels to
>> the prompts automatically?
>>
>> And second, also in reply to what you've been saying further down,
>> perhaps we would better go with a hierarchy of controls here, e.g.
>> EXPERT -> EXPERIMENTAL -> UNSUPPORTED?
> 
> Both these are good ideas worth discussing; somebody else made a similar
> suggestion some time back. I was already thinking this could be a great
> candidate for one of the first "working groups" as defined by George
> during the last community call because the topic is not purely
> technical: a working group could help getting alignment and make
> progress faster. We can propose it to George when he is back.
> 
> However, I don't think we need the working group to make progress on
> this limited patch that only addresses the lowest hanging fruit.
> 
> I'd like to suggest to make progress on this patch in its current form,
> and in parallel start a longer term discussion on how to do something
> like you suggested above.

Okay, I guess I can accept this. So FAOD I'm not objecting to the
change (with some suitable adjustments, as discussed), but I'm
then also not going to be the one to ack it. Nevertheless I'd like
to point out that doing such a partial solution may end up adding
confusion rather than reducing it. Much depends on how exactly
consumers interpret what we hand to them.

Jan

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

Posted by Stefano Stabellini 4 days, 23 hours ago
On Fri, 20 Nov 2020, Jan Beulich wrote:
> On 19.11.2020 22:40, Stefano Stabellini wrote:
> > On Thu, 19 Nov 2020, Jan Beulich wrote:
> >> On 18.11.2020 22:00, Stefano Stabellini wrote:
> >>> On Wed, 18 Nov 2020, Jan Beulich wrote:
> >>>> 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?
> >>>
> >>> Two things: firstly, it doesn't and shouldn't take an expert to enable
> >>> ACPI support, even if ACPI support is experimental. So calling it
> >>> UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig
> >>> options changed by this patch. Secondly, this patch is adding
> >>> "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match
> >>> it with the option you need to enable.
> >>
> >> There's redundancy here then, which I think is in almost all cases
> >> better to avoid. That's first and foremost because the two places
> >> can go out of sync. Therefore, if the primary thing is to help
> >> "make menuconfig" (which I admit I don't normally use, as it's
> >> nothing that gets invoked implicitly by the build process afaict,
> >> i.e. one has to actively invoke it), perhaps we should enhance
> >> kconfig to attach at least a pre-determined subset of labels to
> >> the prompts automatically?
> >>
> >> And second, also in reply to what you've been saying further down,
> >> perhaps we would better go with a hierarchy of controls here, e.g.
> >> EXPERT -> EXPERIMENTAL -> UNSUPPORTED?
> > 
> > Both these are good ideas worth discussing; somebody else made a similar
> > suggestion some time back. I was already thinking this could be a great
> > candidate for one of the first "working groups" as defined by George
> > during the last community call because the topic is not purely
> > technical: a working group could help getting alignment and make
> > progress faster. We can propose it to George when he is back.
> > 
> > However, I don't think we need the working group to make progress on
> > this limited patch that only addresses the lowest hanging fruit.
> > 
> > I'd like to suggest to make progress on this patch in its current form,
> > and in parallel start a longer term discussion on how to do something
> > like you suggested above.
> 
> Okay, I guess I can accept this. So FAOD I'm not objecting to the
> change (with some suitable adjustments, as discussed), but I'm
> then also not going to be the one to ack it. Nevertheless I'd like
> to point out that doing such a partial solution may end up adding
> confusion rather than reducing it. Much depends on how exactly
> consumers interpret what we hand to them.

Thank you Jan. I'll clarify the patch and address your comments. I'll
also try to get the attention of one of the other maintainers for the
ack.