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:

- rename the EXPERT description to clarify the option and make sure to
include the word "EXPERT" in the oneliner

- instead of using "if EXPERT" add EXPERT as a dependency so that when
searching for a feature on the menu you are told that you need to enable
EXPERT to get the option

[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
---
 xen/Kconfig              | 13 ++++++-------
 xen/arch/arm/Kconfig     | 18 ++++++++++--------
 xen/arch/x86/Kconfig     | 11 ++++++-----
 xen/common/Kconfig       | 21 ++++++++++++++-------
 xen/common/sched/Kconfig |  2 +-
 5 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 34c318bfa2..5fa2716e2d 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -35,14 +35,13 @@ config DEFCONFIG_LIST
 	default ARCH_DEFCONFIG
 
 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
-	  which can tolerate a "non-standard" Xen.
-	  Only use this if you really know what you are doing.
-	  Xen binaries built with this option enabled are not security
-	  supported.
+	  This option allows certain experimental (see SUPPORT.md) Xen
+	  options and settings to be enabled/disabled. This is for
+	  specialized environments which can tolerate a "non-standard" Xen.
+	  Only use this if you really know what you are doing.  Xen binaries
+	  built with this option enabled are not security supported.
 	default n
 
 config LTO
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2777388265..0223cf11c6 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -32,8 +32,8 @@ menu "Architecture Features"
 source "arch/Kconfig"
 
 config ACPI
-	bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT
-	depends on ARM_64
+	bool "ACPI (Advanced Configuration and Power Interface) Support"
+	depends on ARM_64 && EXPERT
 	---help---
 
 	  Advanced Configuration and Power Interface (ACPI) support for Xen is
@@ -49,8 +49,8 @@ config GICV3
 	  If unsure, say Y
 
 config HAS_ITS
-        bool "GICv3 ITS MSI controller support" if EXPERT
-        depends on GICV3 && !NEW_VGIC
+        bool "GICv3 ITS MSI controller support"
+        depends on GICV3 && !NEW_VGICi && EXPERT
 
 config HVM
         def_bool y
@@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
 	  SBSA Generic UART implements a subset of ARM PL011 UART.
 
 config ARM_SSBD
-	bool "Speculative Store Bypass Disable" if EXPERT
-	depends on HAS_ALTERNATIVE
+	bool "Speculative Store Bypass Disable"
+	depends on HAS_ALTERNATIVE && EXPERT
 	default y
 	help
 	  This enables mitigation of bypassing of previous stores by speculative
@@ -89,7 +89,8 @@ 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"
+	depends on EXPERT
 	default y
 	help
 	  Speculation attacks against some high-performance processors rely on
@@ -106,7 +107,8 @@ config HARDEN_BRANCH_PREDICTOR
 	  If unsure, say Y.
 
 config TEE
-	bool "Enable TEE mediators support" if EXPERT
+	bool "Enable TEE mediators support"
+	depends on EXPERT
 	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..071bfbbc40 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -146,9 +146,9 @@ config BIGMEM
 	  If unsure, say N.
 
 config HVM_FEP
-	bool "HVM Forced Emulation Prefix support" if EXPERT
+	bool "HVM Forced Emulation Prefix support"
 	default DEBUG
-	depends on HVM
+	depends on HVM && EXPERT
 	---help---
 
 	  Compiles in a feature that allows HVM guest to arbitrarily
@@ -165,8 +165,9 @@ config HVM_FEP
 	  If unsure, say N.
 
 config TBOOT
-	bool "Xen tboot support" if EXPERT
+	bool "Xen tboot support"
 	default y if !PV_SHIM_EXCLUSIVE
+	depends on EXPERT
 	select CRYPTO
 	---help---
 	  Allows support for Trusted Boot using the Intel(R) Trusted Execution
@@ -251,8 +252,8 @@ config HYPERV_GUEST
 endif
 
 config MEM_SHARING
-	bool "Xen memory sharing support" if EXPERT
-	depends on HVM
+	bool "Xen memory sharing support"
+	depends on HVM && EXPERT
 
 endmenu
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 3e2cf25088..7a8c54e66c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -12,7 +12,8 @@ config CORE_PARKING
 	bool
 
 config GRANT_TABLE
-	bool "Grant table support" if EXPERT
+	bool "Grant table support"
+	depends on EXPERT
 	default y
 	---help---
 	  Grant table provides a generic mechanism to memory sharing
@@ -151,7 +152,8 @@ config KEXEC
 	  If unsure, say Y.
 
 config EFI_SET_VIRTUAL_ADDRESS_MAP
-    bool "EFI: call SetVirtualAddressMap()" if EXPERT
+    bool "EFI: call SetVirtualAddressMap()"
+    depends on EXPERT
     ---help---
       Call EFI SetVirtualAddressMap() runtime service to setup memory map for
       further runtime services. According to UEFI spec, it isn't strictly
@@ -162,7 +164,8 @@ config EFI_SET_VIRTUAL_ADDRESS_MAP
 
 config XENOPROF
 	def_bool y
-	prompt "Xen Oprofile Support" if EXPERT
+	prompt "Xen Oprofile Support"
+	depends on EXPERT
 	depends on X86
 	---help---
 	  Xen OProfile (Xenoprof) is a system-wide profiler for Xen virtual
@@ -199,7 +202,8 @@ config XSM_FLASK
 
 config XSM_FLASK_AVC_STATS
 	def_bool y
-	prompt "Maintain statistics on the FLASK access vector cache" if EXPERT
+	prompt "Maintain statistics on the FLASK access vector cache"
+	depends on EXPERT
 	depends on XSM_FLASK
 	---help---
 	  Maintain counters on the access vector cache that can be viewed using
@@ -272,7 +276,8 @@ config LATE_HWDOM
 	  If unsure, say N.
 
 config ARGO
-	bool "Argo: hypervisor-mediated interdomain communication" if EXPERT
+	bool "Argo: hypervisor-mediated interdomain communication"
+	depends on EXPERT
 	---help---
 	  Enables a hypercall for domains to ask the hypervisor to perform
 	  data transfer of messages between domains.
@@ -344,7 +349,8 @@ config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
 	  build becoming overly verbose.
 
 config CMDLINE
-	string "Built-in hypervisor command string" if EXPERT
+	string "Built-in hypervisor command string"
+	depends on EXPERT
 	default ""
 	---help---
 	  Enter arguments here that should be compiled into the hypervisor
@@ -377,7 +383,8 @@ config DOM0_MEM
 	  Leave empty if you are not sure what to specify.
 
 config TRACEBUFFER
-	bool "Enable tracing infrastructure" if EXPERT
+	bool "Enable tracing infrastructure"
+	depends on EXPERT
 	default y
 	---help---
 	  Enable tracing infrastructure and pre-defined tracepoints within Xen.
diff --git a/xen/common/sched/Kconfig b/xen/common/sched/Kconfig
index 61231aacaa..ec0385cd07 100644
--- a/xen/common/sched/Kconfig
+++ b/xen/common/sched/Kconfig
@@ -1,5 +1,5 @@
 menu "Schedulers"
-	visible if EXPERT
+	depends on EXPERT
 
 config SCHED_CREDIT
 	bool "Credit scheduler support"
-- 
2.17.1

On 31.10.2020 01:24, Stefano Stabellini wrote:
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -35,14 +35,13 @@ config DEFCONFIG_LIST
>  	default ARCH_DEFCONFIG
>  
>  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
> -	  which can tolerate a "non-standard" Xen.
> -	  Only use this if you really know what you are doing.
> -	  Xen binaries built with this option enabled are not security
> -	  supported.
> +	  This option allows certain experimental (see SUPPORT.md) Xen
> +	  options and settings to be enabled/disabled. This is for
> +	  specialized environments which can tolerate a "non-standard" Xen.
> +	  Only use this if you really know what you are doing.  Xen binaries
> +	  built with this option enabled are not security supported.
>  	default n

I'm definitely in favor of this - it was more than once that I
wondered about the prompt text.

> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>  	  SBSA Generic UART implements a subset of ARM PL011 UART.
>  
>  config ARM_SSBD
> -	bool "Speculative Store Bypass Disable" if EXPERT
> -	depends on HAS_ALTERNATIVE
> +	bool "Speculative Store Bypass Disable"
> +	depends on HAS_ALTERNATIVE && EXPERT
>  	default y

At the example of this, I'm afraid when the default isn't "n"
(or there's no default directive at all, as ought to be
equivalent to and preferred over "default n"), such a
transformation is not functionally identical: Before your
change, with !EXPERT this option defaults to y. After your
change this option is unavailable (which resolves to it being
off for all consuming purposes).

IOW there are reasons to have "if ..." attached to the prompts
(for this construct indeed only making the prompt conditional,
not the entire option), but there are also cases where the
cleanup you do is indeed desirable / helpful.

Jan

On Mon, 2 Nov 2020, Jan Beulich wrote:
> On 31.10.2020 01:24, Stefano Stabellini wrote:
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -35,14 +35,13 @@ config DEFCONFIG_LIST
> >  	default ARCH_DEFCONFIG
> >  
> >  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
> > -	  which can tolerate a "non-standard" Xen.
> > -	  Only use this if you really know what you are doing.
> > -	  Xen binaries built with this option enabled are not security
> > -	  supported.
> > +	  This option allows certain experimental (see SUPPORT.md) Xen
> > +	  options and settings to be enabled/disabled. This is for
> > +	  specialized environments which can tolerate a "non-standard" Xen.
> > +	  Only use this if you really know what you are doing.  Xen binaries
> > +	  built with this option enabled are not security supported.
> >  	default n
> 
> I'm definitely in favor of this - it was more than once that I
> wondered about the prompt text.

Thanks, I agree!


> > @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
> >  	  SBSA Generic UART implements a subset of ARM PL011 UART.
> >  
> >  config ARM_SSBD
> > -	bool "Speculative Store Bypass Disable" if EXPERT
> > -	depends on HAS_ALTERNATIVE
> > +	bool "Speculative Store Bypass Disable"
> > +	depends on HAS_ALTERNATIVE && EXPERT
> >  	default y
> 
> At the example of this, I'm afraid when the default isn't "n"
> (or there's no default directive at all, as ought to be
> equivalent to and preferred over "default n"), such a
> transformation is not functionally identical: Before your
> change, with !EXPERT this option defaults to y. After your
> change this option is unavailable (which resolves to it being
> off for all consuming purposes).
> 
> IOW there are reasons to have "if ..." attached to the prompts
> (for this construct indeed only making the prompt conditional,
> not the entire option), but there are also cases where the
> cleanup you do is indeed desirable / helpful.

Yeah, thanks for catching it, it is obviously a problem.

My intention was just to "tag" somehow the options to EXPERT so that it
would show on the menu. Maybe a better, simpler, way to do it is
to add the word "EXPERT" to the one line prompt:

 config ARM_SSBD
-	bool "Speculative Store Bypass Disable" if EXPERT
+	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
 	depends on HAS_ALTERNATIVE
 	default y
 	help


What do you think?

On 02.11.2020 22:41, Stefano Stabellini wrote:
> On Mon, 2 Nov 2020, Jan Beulich wrote:
>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>  	  SBSA Generic UART implements a subset of ARM PL011 UART.
>>>  
>>>  config ARM_SSBD
>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>> -	depends on HAS_ALTERNATIVE
>>> +	bool "Speculative Store Bypass Disable"
>>> +	depends on HAS_ALTERNATIVE && EXPERT
>>>  	default y
>>
>> At the example of this, I'm afraid when the default isn't "n"
>> (or there's no default directive at all, as ought to be
>> equivalent to and preferred over "default n"), such a
>> transformation is not functionally identical: Before your
>> change, with !EXPERT this option defaults to y. After your
>> change this option is unavailable (which resolves to it being
>> off for all consuming purposes).
>>
>> IOW there are reasons to have "if ..." attached to the prompts
>> (for this construct indeed only making the prompt conditional,
>> not the entire option), but there are also cases where the
>> cleanup you do is indeed desirable / helpful.
> 
> Yeah, thanks for catching it, it is obviously a problem.
> 
> My intention was just to "tag" somehow the options to EXPERT so that it
> would show on the menu. Maybe a better, simpler, way to do it is
> to add the word "EXPERT" to the one line prompt:
> 
>  config ARM_SSBD
> -	bool "Speculative Store Bypass Disable" if EXPERT
> +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>  	depends on HAS_ALTERNATIVE
>  	default y
>  	help
> 
> 
> What do you think?

While on the surface this may look like an improvement, I don't
see how it would actually help: If you read the Kconfig file
itself, the dependency is seen anyway. And on the menu I don't
see the point of telling someone who has enabled EXPERT that a
certain option is (or is not) an expert one. If they think
they're experts, so should they be treated. (It was, after all,
a deliberate decision to make enabling expert mode easier, and
hence easier to use for what one might consider not-really-
experts. I realize saying so may be considered tendentious; I
mean it in a purely technical sense, and I'd like to apologize
in advance to anyone not sharing this as a possible perspective
to take.)

Plus, of course, the addition of such (EXPERT) markers to
future options' prompts is liable to get forgotten now and then,
so sooner or later we'd likely end up with an inconsistent
mixture anyway.

Jan

On Tue, 3 Nov 2020, Jan Beulich wrote:
> On 02.11.2020 22:41, Stefano Stabellini wrote:
> > On Mon, 2 Nov 2020, Jan Beulich wrote:
> >> On 31.10.2020 01:24, Stefano Stabellini wrote:
> >>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
> >>>  	  SBSA Generic UART implements a subset of ARM PL011 UART.
> >>>  
> >>>  config ARM_SSBD
> >>> -	bool "Speculative Store Bypass Disable" if EXPERT
> >>> -	depends on HAS_ALTERNATIVE
> >>> +	bool "Speculative Store Bypass Disable"
> >>> +	depends on HAS_ALTERNATIVE && EXPERT
> >>>  	default y
> >>
> >> At the example of this, I'm afraid when the default isn't "n"
> >> (or there's no default directive at all, as ought to be
> >> equivalent to and preferred over "default n"), such a
> >> transformation is not functionally identical: Before your
> >> change, with !EXPERT this option defaults to y. After your
> >> change this option is unavailable (which resolves to it being
> >> off for all consuming purposes).
> >>
> >> IOW there are reasons to have "if ..." attached to the prompts
> >> (for this construct indeed only making the prompt conditional,
> >> not the entire option), but there are also cases where the
> >> cleanup you do is indeed desirable / helpful.
> > 
> > Yeah, thanks for catching it, it is obviously a problem.
> > 
> > My intention was just to "tag" somehow the options to EXPERT so that it
> > would show on the menu. Maybe a better, simpler, way to do it is
> > to add the word "EXPERT" to the one line prompt:
> > 
> >  config ARM_SSBD
> > -	bool "Speculative Store Bypass Disable" if EXPERT
> > +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
> >  	depends on HAS_ALTERNATIVE
> >  	default y
> >  	help
> > 
> > 
> > What do you think?
> 
> While on the surface this may look like an improvement, I don't
> see how it would actually help: If you read the Kconfig file
> itself, the dependency is seen anyway. And on the menu I don't
> see the point of telling someone who has enabled EXPERT that a
> certain option is (or is not) an expert one. If they think
> they're experts, so should they be treated. (It was, after all,
> a deliberate decision to make enabling expert mode easier, and
> hence easier to use for what one might consider not-really-
> experts. I realize saying so may be considered tendentious; I
> mean it in a purely technical sense, and I'd like to apologize
> in advance to anyone not sharing this as a possible perspective
> to take.)
> 
> Plus, of course, the addition of such (EXPERT) markers to
> future options' prompts is liable to get forgotten now and then,
> so sooner or later we'd likely end up with an inconsistent
> mixture anyway.

I tend to agree with you on everything you wrote. The fundamental issue
is that we are (mis)using EXPERT to tag features that are experimental,
as defined by SUPPORT.md.

It is important to be able to distinguish clearly at the kconfig level
options that are (security) supported from options that are
unsupported/experimental. Today the only way to do it is with EXPERT
which is not great because:

- it doesn't convey the idea that it is for unsupported/experimental
  features
- if you want to enable one unsupported feature, it is not clear what
  you have to do

So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
the Kconfig menu?

It would make it clearer that by enabling UNSUPPORTED you are going to
get a configuration that is not security supported. And ideally we would
also tag features like ACPI as UNSUPPORTED as I suggested above.

On Nov 3, 2020, at 14:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>      SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>> 
>>>>> config ARM_SSBD
>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>> -    depends on HAS_ALTERNATIVE
>>>>> +    bool "Speculative Store Bypass Disable"
>>>>> +    depends on HAS_ALTERNATIVE && EXPERT
>>>>>    default y
>>>> 
>>>> At the example of this, I'm afraid when the default isn't "n"
>>>> (or there's no default directive at all, as ought to be
>>>> equivalent to and preferred over "default n"), such a
>>>> transformation is not functionally identical: Before your
>>>> change, with !EXPERT this option defaults to y. After your
>>>> change this option is unavailable (which resolves to it being
>>>> off for all consuming purposes).
>>>> 
>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>> (for this construct indeed only making the prompt conditional,
>>>> not the entire option), but there are also cases where the
>>>> cleanup you do is indeed desirable / helpful.
>>> 
>>> Yeah, thanks for catching it, it is obviously a problem.
>>> 
>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>> would show on the menu. Maybe a better, simpler, way to do it is
>>> to add the word "EXPERT" to the one line prompt:
>>> 
>>> config ARM_SSBD
>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>> +    bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>    depends on HAS_ALTERNATIVE
>>>    default y
>>>    help
>>> 
>>> 
>>> What do you think?
>> 
>> While on the surface this may look like an improvement, I don't
>> see how it would actually help: If you read the Kconfig file
>> itself, the dependency is seen anyway. And on the menu I don't
>> see the point of telling someone who has enabled EXPERT that a
>> certain option is (or is not) an expert one. If they think
>> they're experts, so should they be treated. (It was, after all,
>> a deliberate decision to make enabling expert mode easier, and
>> hence easier to use for what one might consider not-really-
>> experts. I realize saying so may be considered tendentious; I
>> mean it in a purely technical sense, and I'd like to apologize
>> in advance to anyone not sharing this as a possible perspective
>> to take.)
>> 
>> Plus, of course, the addition of such (EXPERT) markers to
>> future options' prompts is liable to get forgotten now and then,
>> so sooner or later we'd likely end up with an inconsistent
>> mixture anyway.
> 
> I tend to agree with you on everything you wrote. The fundamental issue
> is that we are (mis)using EXPERT to tag features that are experimental,
> as defined by SUPPORT.md.
> 
> It is important to be able to distinguish clearly at the kconfig level
> options that are (security) supported from options that are
> unsupported/experimental. Today the only way to do it is with EXPERT
> which is not great because:
> 
> - it doesn't convey the idea that it is for unsupported/experimental
>  features
> - if you want to enable one unsupported feature, it is not clear what
>  you have to do
> 
> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
> the Kconfig menu?
> 
> It would make it clearer that by enabling UNSUPPORTED you are going to
> get a configuration that is not security supported.

If going down this path, there should be one, authoritative, in-tree definition of feature-level support from which Kconfig (build-time policy enforcement) and SUPPORT.md (documentation) can be derived.  Later, even run-time enforcement can be similarly classified.  FuSA may also wish for documented policy to align with enforcement.

Rich

> And ideally we would
> also tag features like ACPI as UNSUPPORTED as I suggested above.
> 

On Tue, 3 Nov 2020, Rich Persaud wrote:
> On Nov 3, 2020, at 14:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Tue, 3 Nov 2020, Jan Beulich wrote:
> >>> On 02.11.2020 22:41, Stefano Stabellini wrote:
> >>> On Mon, 2 Nov 2020, Jan Beulich wrote:
> >>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
> >>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
> >>>>>      SBSA Generic UART implements a subset of ARM PL011 UART.
> >>>>> 
> >>>>> config ARM_SSBD
> >>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
> >>>>> -    depends on HAS_ALTERNATIVE
> >>>>> +    bool "Speculative Store Bypass Disable"
> >>>>> +    depends on HAS_ALTERNATIVE && EXPERT
> >>>>>    default y
> >>>> 
> >>>> At the example of this, I'm afraid when the default isn't "n"
> >>>> (or there's no default directive at all, as ought to be
> >>>> equivalent to and preferred over "default n"), such a
> >>>> transformation is not functionally identical: Before your
> >>>> change, with !EXPERT this option defaults to y. After your
> >>>> change this option is unavailable (which resolves to it being
> >>>> off for all consuming purposes).
> >>>> 
> >>>> IOW there are reasons to have "if ..." attached to the prompts
> >>>> (for this construct indeed only making the prompt conditional,
> >>>> not the entire option), but there are also cases where the
> >>>> cleanup you do is indeed desirable / helpful.
> >>> 
> >>> Yeah, thanks for catching it, it is obviously a problem.
> >>> 
> >>> My intention was just to "tag" somehow the options to EXPERT so that it
> >>> would show on the menu. Maybe a better, simpler, way to do it is
> >>> to add the word "EXPERT" to the one line prompt:
> >>> 
> >>> config ARM_SSBD
> >>> -    bool "Speculative Store Bypass Disable" if EXPERT
> >>> +    bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
> >>>    depends on HAS_ALTERNATIVE
> >>>    default y
> >>>    help
> >>> 
> >>> 
> >>> What do you think?
> >> 
> >> While on the surface this may look like an improvement, I don't
> >> see how it would actually help: If you read the Kconfig file
> >> itself, the dependency is seen anyway. And on the menu I don't
> >> see the point of telling someone who has enabled EXPERT that a
> >> certain option is (or is not) an expert one. If they think
> >> they're experts, so should they be treated. (It was, after all,
> >> a deliberate decision to make enabling expert mode easier, and
> >> hence easier to use for what one might consider not-really-
> >> experts. I realize saying so may be considered tendentious; I
> >> mean it in a purely technical sense, and I'd like to apologize
> >> in advance to anyone not sharing this as a possible perspective
> >> to take.)
> >> 
> >> Plus, of course, the addition of such (EXPERT) markers to
> >> future options' prompts is liable to get forgotten now and then,
> >> so sooner or later we'd likely end up with an inconsistent
> >> mixture anyway.
> > 
> > I tend to agree with you on everything you wrote. The fundamental issue
> > is that we are (mis)using EXPERT to tag features that are experimental,
> > as defined by SUPPORT.md.
> > 
> > It is important to be able to distinguish clearly at the kconfig level
> > options that are (security) supported from options that are
> > unsupported/experimental. Today the only way to do it is with EXPERT
> > which is not great because:
> > 
> > - it doesn't convey the idea that it is for unsupported/experimental
> >  features
> > - if you want to enable one unsupported feature, it is not clear what
> >  you have to do
> > 
> > So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
> > the Kconfig menu?
> > 
> > It would make it clearer that by enabling UNSUPPORTED you are going to
> > get a configuration that is not security supported.
> 
> If going down this path, there should be one, authoritative, in-tree definition of feature-level support from which Kconfig (build-time policy enforcement) and SUPPORT.md (documentation) can be derived.  Later, even run-time enforcement can be similarly classified.  FuSA may also wish for documented policy to align with enforcement.

The goal is trying to align Kconfig and SUPPORT.md by clarifying the
EXPERT option, which today is a poor implementation of "experimental".

There could be further improvements down the line, for instance we could
taint Xen when UNSUPPORTED is selected and even have separate kconfig
options for UNSUPPORTED, EXPERIMENTAL, and TECHPREVIEW. FuSa is likely
going to need its own SAFETY option too. Like you suggested, we could
even have a single source of feature-level support information for both
Kconfig and SUPPORT.md.

However, I didn't want to increase the scope of this one patch. For now,
it would be a good start if we replaced EXPERT with something that covers
anything not security supported, for which UNSUPPORTED looks like a good
name.

> On Nov 3, 2020, at 16:15, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 3 Nov 2020, Rich Persaud wrote:
>>> On Nov 3, 2020, at 14:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>>>     SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>>> 
>>>>>>> config ARM_SSBD
>>>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>>>> -    depends on HAS_ALTERNATIVE
>>>>>>> +    bool "Speculative Store Bypass Disable"
>>>>>>> +    depends on HAS_ALTERNATIVE && EXPERT
>>>>>>>   default y
>>>>>> 
>>>>>> At the example of this, I'm afraid when the default isn't "n"
>>>>>> (or there's no default directive at all, as ought to be
>>>>>> equivalent to and preferred over "default n"), such a
>>>>>> transformation is not functionally identical: Before your
>>>>>> change, with !EXPERT this option defaults to y. After your
>>>>>> change this option is unavailable (which resolves to it being
>>>>>> off for all consuming purposes).
>>>>>> 
>>>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>>>> (for this construct indeed only making the prompt conditional,
>>>>>> not the entire option), but there are also cases where the
>>>>>> cleanup you do is indeed desirable / helpful.
>>>>> 
>>>>> Yeah, thanks for catching it, it is obviously a problem.
>>>>> 
>>>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>>>> would show on the menu. Maybe a better, simpler, way to do it is
>>>>> to add the word "EXPERT" to the one line prompt:
>>>>> 
>>>>> config ARM_SSBD
>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>> +    bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>>>   depends on HAS_ALTERNATIVE
>>>>>   default y
>>>>>   help
>>>>> 
>>>>> 
>>>>> What do you think?
>>>> 
>>>> While on the surface this may look like an improvement, I don't
>>>> see how it would actually help: If you read the Kconfig file
>>>> itself, the dependency is seen anyway. And on the menu I don't
>>>> see the point of telling someone who has enabled EXPERT that a
>>>> certain option is (or is not) an expert one. If they think
>>>> they're experts, so should they be treated. (It was, after all,
>>>> a deliberate decision to make enabling expert mode easier, and
>>>> hence easier to use for what one might consider not-really-
>>>> experts. I realize saying so may be considered tendentious; I
>>>> mean it in a purely technical sense, and I'd like to apologize
>>>> in advance to anyone not sharing this as a possible perspective
>>>> to take.)
>>>> 
>>>> Plus, of course, the addition of such (EXPERT) markers to
>>>> future options' prompts is liable to get forgotten now and then,
>>>> so sooner or later we'd likely end up with an inconsistent
>>>> mixture anyway.
>>> 
>>> I tend to agree with you on everything you wrote. The fundamental issue
>>> is that we are (mis)using EXPERT to tag features that are experimental,
>>> as defined by SUPPORT.md.
>>> 
>>> It is important to be able to distinguish clearly at the kconfig level
>>> options that are (security) supported from options that are
>>> unsupported/experimental. Today the only way to do it is with EXPERT
>>> which is not great because:
>>> 
>>> - it doesn't convey the idea that it is for unsupported/experimental
>>> features
>>> - if you want to enable one unsupported feature, it is not clear what
>>> you have to do
>>> 
>>> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
>>> the Kconfig menu?
>>> 
>>> It would make it clearer that by enabling UNSUPPORTED you are going to
>>> get a configuration that is not security supported.
>> 
>> If going down this path, there should be one, authoritative, in-tree definition of feature-level support from which Kconfig (build-time policy enforcement) and SUPPORT.md (documentation) can be derived.  Later, even run-time enforcement can be similarly classified.  FuSA may also wish for documented policy to align with enforcement.
> 
> The goal is trying to align Kconfig and SUPPORT.md by clarifying the
> EXPERT option, which today is a poor implementation of "experimental".
> 
> There could be further improvements down the line, for instance we could
> taint Xen when UNSUPPORTED is selected and even have separate kconfig
> options for UNSUPPORTED, EXPERIMENTAL, and TECHPREVIEW. FuSa is likely
> going to need its own SAFETY option too. Like you suggested, we could
> even have a single source of feature-level support information for both
> Kconfig and SUPPORT.md.
> 
> However, I didn't want to increase the scope of this one patch. For now,
> it would be a good start if we replaced EXPERT with something that covers
> anything not security supported, for which UNSUPPORTED looks like a good
> name.

Kconfig UI is aimed at humans. There is a gulf of semantic difference between EXPERT (human assessment based on local expertise and contextual priorities) and UNSUPPORTED (binary policy assertion independent of local context, which can be parsed by computer).  We also have the supported-with-caveat category of features in SUPPORT.md.  

It's not clear that every human-interpreted instance of EXPERT should be replaced by UNSUPPORTED, even if some instances may qualify.  It may be better to have a separate patch which introduces a comprehensive set of labels with matching policy statements.

Another option is to embed/copy SUPPORT.md feature support policy text into the description field of relevant Kconfig menu options.  A human expert can then consider support criteria in their decision, informing consent for use of "supported" (with/without caveats) and "unsupported" features.

Rich

Hi Stefano,

> On 3 Nov 2020, at 21:15, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 3 Nov 2020, Rich Persaud wrote:
>> On Nov 3, 2020, at 14:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>>>     SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>>> 
>>>>>>> config ARM_SSBD
>>>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>>>> -    depends on HAS_ALTERNATIVE
>>>>>>> +    bool "Speculative Store Bypass Disable"
>>>>>>> +    depends on HAS_ALTERNATIVE && EXPERT
>>>>>>>   default y
>>>>>> 
>>>>>> At the example of this, I'm afraid when the default isn't "n"
>>>>>> (or there's no default directive at all, as ought to be
>>>>>> equivalent to and preferred over "default n"), such a
>>>>>> transformation is not functionally identical: Before your
>>>>>> change, with !EXPERT this option defaults to y. After your
>>>>>> change this option is unavailable (which resolves to it being
>>>>>> off for all consuming purposes).
>>>>>> 
>>>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>>>> (for this construct indeed only making the prompt conditional,
>>>>>> not the entire option), but there are also cases where the
>>>>>> cleanup you do is indeed desirable / helpful.
>>>>> 
>>>>> Yeah, thanks for catching it, it is obviously a problem.
>>>>> 
>>>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>>>> would show on the menu. Maybe a better, simpler, way to do it is
>>>>> to add the word "EXPERT" to the one line prompt:
>>>>> 
>>>>> config ARM_SSBD
>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>> +    bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>>>   depends on HAS_ALTERNATIVE
>>>>>   default y
>>>>>   help
>>>>> 
>>>>> 
>>>>> What do you think?
>>>> 
>>>> While on the surface this may look like an improvement, I don't
>>>> see how it would actually help: If you read the Kconfig file
>>>> itself, the dependency is seen anyway. And on the menu I don't
>>>> see the point of telling someone who has enabled EXPERT that a
>>>> certain option is (or is not) an expert one. If they think
>>>> they're experts, so should they be treated. (It was, after all,
>>>> a deliberate decision to make enabling expert mode easier, and
>>>> hence easier to use for what one might consider not-really-
>>>> experts. I realize saying so may be considered tendentious; I
>>>> mean it in a purely technical sense, and I'd like to apologize
>>>> in advance to anyone not sharing this as a possible perspective
>>>> to take.)
>>>> 
>>>> Plus, of course, the addition of such (EXPERT) markers to
>>>> future options' prompts is liable to get forgotten now and then,
>>>> so sooner or later we'd likely end up with an inconsistent
>>>> mixture anyway.
>>> 
>>> I tend to agree with you on everything you wrote. The fundamental issue
>>> is that we are (mis)using EXPERT to tag features that are experimental,
>>> as defined by SUPPORT.md.
>>> 
>>> It is important to be able to distinguish clearly at the kconfig level
>>> options that are (security) supported from options that are
>>> unsupported/experimental. Today the only way to do it is with EXPERT
>>> which is not great because:
>>> 
>>> - it doesn't convey the idea that it is for unsupported/experimental
>>> features
>>> - if you want to enable one unsupported feature, it is not clear what
>>> you have to do
>>> 
>>> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
>>> the Kconfig menu?
>>> 
>>> It would make it clearer that by enabling UNSUPPORTED you are going to
>>> get a configuration that is not security supported.
>> 
>> If going down this path, there should be one, authoritative, in-tree definition of feature-level support from which Kconfig (build-time policy enforcement) and SUPPORT.md (documentation) can be derived.  Later, even run-time enforcement can be similarly classified.  FuSA may also wish for documented policy to align with enforcement.
> 
> The goal is trying to align Kconfig and SUPPORT.md by clarifying the
> EXPERT option, which today is a poor implementation of "experimental".
> 
> There could be further improvements down the line, for instance we could
> taint Xen when UNSUPPORTED is selected and even have separate kconfig
> options for UNSUPPORTED, EXPERIMENTAL, and TECHPREVIEW. FuSa is likely
> going to need its own SAFETY option too. Like you suggested, we could
> even have a single source of feature-level support information for both
> Kconfig and SUPPORT.md.
> 

I do think this is a great idea that could make like easier for users.
There could be some generic options as you suggested that the user
could enable or not at the top level of the configuration and depending
on this some features/drivers would be possible to select or not.

This would also make it easier to generate a fully security-supported system.

Tainting the system when one of this option is activated could also make
it easier to identify when someone is using a feature that is not supported.

Cheers
Bertrand

> However, I didn't want to increase the scope of this one patch. For now,
> it would be a good start if we replaced EXPERT with something that covers
> anything not security supported, for which UNSUPPORTED looks like a good
> name.

On 11/3/20 4:15 PM, Stefano Stabellini wrote:
> On Tue, 3 Nov 2020, Rich Persaud wrote:
>> On Nov 3, 2020, at 14:37, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>
>>> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>>>      SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>>>
>>>>>>> config ARM_SSBD
>>>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>>>> -    depends on HAS_ALTERNATIVE
>>>>>>> +    bool "Speculative Store Bypass Disable"
>>>>>>> +    depends on HAS_ALTERNATIVE && EXPERT
>>>>>>>    default y
>>>>>>
>>>>>> At the example of this, I'm afraid when the default isn't "n"
>>>>>> (or there's no default directive at all, as ought to be
>>>>>> equivalent to and preferred over "default n"), such a
>>>>>> transformation is not functionally identical: Before your
>>>>>> change, with !EXPERT this option defaults to y. After your
>>>>>> change this option is unavailable (which resolves to it being
>>>>>> off for all consuming purposes).
>>>>>>
>>>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>>>> (for this construct indeed only making the prompt conditional,
>>>>>> not the entire option), but there are also cases where the
>>>>>> cleanup you do is indeed desirable / helpful.
>>>>>
>>>>> Yeah, thanks for catching it, it is obviously a problem.
>>>>>
>>>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>>>> would show on the menu. Maybe a better, simpler, way to do it is
>>>>> to add the word "EXPERT" to the one line prompt:
>>>>>
>>>>> config ARM_SSBD
>>>>> -    bool "Speculative Store Bypass Disable" if EXPERT
>>>>> +    bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>>>    depends on HAS_ALTERNATIVE
>>>>>    default y
>>>>>    help
>>>>>
>>>>>
>>>>> What do you think?
>>>>
>>>> While on the surface this may look like an improvement, I don't
>>>> see how it would actually help: If you read the Kconfig file
>>>> itself, the dependency is seen anyway. And on the menu I don't
>>>> see the point of telling someone who has enabled EXPERT that a
>>>> certain option is (or is not) an expert one. If they think
>>>> they're experts, so should they be treated. (It was, after all,
>>>> a deliberate decision to make enabling expert mode easier, and
>>>> hence easier to use for what one might consider not-really-
>>>> experts. I realize saying so may be considered tendentious; I
>>>> mean it in a purely technical sense, and I'd like to apologize
>>>> in advance to anyone not sharing this as a possible perspective
>>>> to take.)
>>>>
>>>> Plus, of course, the addition of such (EXPERT) markers to
>>>> future options' prompts is liable to get forgotten now and then,
>>>> so sooner or later we'd likely end up with an inconsistent
>>>> mixture anyway.
>>>
>>> I tend to agree with you on everything you wrote. The fundamental issue
>>> is that we are (mis)using EXPERT to tag features that are experimental,
>>> as defined by SUPPORT.md.
>>>
>>> It is important to be able to distinguish clearly at the kconfig level
>>> options that are (security) supported from options that are
>>> unsupported/experimental. Today the only way to do it is with EXPERT
>>> which is not great because:
>>>
>>> - it doesn't convey the idea that it is for unsupported/experimental
>>>  features
>>> - if you want to enable one unsupported feature, it is not clear what
>>>  you have to do
>>>
>>> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
>>> the Kconfig menu?
>>>
>>> It would make it clearer that by enabling UNSUPPORTED you are going to
>>> get a configuration that is not security supported.
>>
>> If going down this path, there should be one, authoritative, in-tree definition of feature-level support from which Kconfig (build-time policy enforcement) and SUPPORT.md (documentation) can be derived.  Later, even run-time enforcement can be similarly classified.  FuSA may also wish for documented policy to align with enforcement.
> 
> The goal is trying to align Kconfig and SUPPORT.md by clarifying the
> EXPERT option, which today is a poor implementation of "experimental".

Just a thought but what if it were to be kept as EXPERT for the config
option to expose all of these features and then by convention require
that experimental options be required to carry an EXPERIMENTAL tag at
the beginning of the option's description. This would separate the idea
of EXPERT configuration mode from that of EXPERIMENTAL features which
can only be reached in EXPERT mode. The convention could be carried
through to UNSUPPORTED, TECHPREVIEW, or any new types of tags as they
are needed.

> There could be further improvements down the line, for instance we could
> taint Xen when UNSUPPORTED is selected and even have separate kconfig
> options for UNSUPPORTED, EXPERIMENTAL, and TECHPREVIEW. FuSa is likely
> going to need its own SAFETY option too. Like you suggested, we could
> even have a single source of feature-level support information for both
> Kconfig and SUPPORT.md.
> 
> However, I didn't want to increase the scope of this one patch. For now,
> it would be a good start if we replaced EXPERT with something that covers
> anything not security supported, for which UNSUPPORTED looks like a good
> name.
> 

On 03.11.2020 20:37, Stefano Stabellini wrote:
> On Tue, 3 Nov 2020, Jan Beulich wrote:
>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>  	  SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>  
>>>>>  config ARM_SSBD
>>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>>>> -	depends on HAS_ALTERNATIVE
>>>>> +	bool "Speculative Store Bypass Disable"
>>>>> +	depends on HAS_ALTERNATIVE && EXPERT
>>>>>  	default y
>>>>
>>>> At the example of this, I'm afraid when the default isn't "n"
>>>> (or there's no default directive at all, as ought to be
>>>> equivalent to and preferred over "default n"), such a
>>>> transformation is not functionally identical: Before your
>>>> change, with !EXPERT this option defaults to y. After your
>>>> change this option is unavailable (which resolves to it being
>>>> off for all consuming purposes).
>>>>
>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>> (for this construct indeed only making the prompt conditional,
>>>> not the entire option), but there are also cases where the
>>>> cleanup you do is indeed desirable / helpful.
>>>
>>> Yeah, thanks for catching it, it is obviously a problem.
>>>
>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>> would show on the menu. Maybe a better, simpler, way to do it is
>>> to add the word "EXPERT" to the one line prompt:
>>>
>>>  config ARM_SSBD
>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>> +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>  	depends on HAS_ALTERNATIVE
>>>  	default y
>>>  	help
>>>
>>>
>>> What do you think?
>>
>> While on the surface this may look like an improvement, I don't
>> see how it would actually help: If you read the Kconfig file
>> itself, the dependency is seen anyway. And on the menu I don't
>> see the point of telling someone who has enabled EXPERT that a
>> certain option is (or is not) an expert one. If they think
>> they're experts, so should they be treated. (It was, after all,
>> a deliberate decision to make enabling expert mode easier, and
>> hence easier to use for what one might consider not-really-
>> experts. I realize saying so may be considered tendentious; I
>> mean it in a purely technical sense, and I'd like to apologize
>> in advance to anyone not sharing this as a possible perspective
>> to take.)
>>
>> Plus, of course, the addition of such (EXPERT) markers to
>> future options' prompts is liable to get forgotten now and then,
>> so sooner or later we'd likely end up with an inconsistent
>> mixture anyway.
> 
> I tend to agree with you on everything you wrote. The fundamental issue
> is that we are (mis)using EXPERT to tag features that are experimental,
> as defined by SUPPORT.md.
> 
> It is important to be able to distinguish clearly at the kconfig level
> options that are (security) supported from options that are
> unsupported/experimental. Today the only way to do it is with EXPERT
> which is not great because:
> 
> - it doesn't convey the idea that it is for unsupported/experimental
>   features
> - if you want to enable one unsupported feature, it is not clear what
>   you have to do
> 
> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
> the Kconfig menu?

If you mean this to be added to prompt texts, then yes, I'd view
this as helpful. However, ...

> It would make it clearer that by enabling UNSUPPORTED you are going to
> get a configuration that is not security supported. And ideally we would
> also tag features like ACPI as UNSUPPORTED as I suggested above.

... things will get uglier when (just a simple example) something
is supported on x86, but not on Arm.

Jan

Hi Jan,

> On 4 Nov 2020, at 07:34, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.11.2020 20:37, Stefano Stabellini wrote:
>> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>> 	  SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>> 
>>>>>> config ARM_SSBD
>>>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>>>>> -	depends on HAS_ALTERNATIVE
>>>>>> +	bool "Speculative Store Bypass Disable"
>>>>>> +	depends on HAS_ALTERNATIVE && EXPERT
>>>>>> 	default y
>>>>> 
>>>>> At the example of this, I'm afraid when the default isn't "n"
>>>>> (or there's no default directive at all, as ought to be
>>>>> equivalent to and preferred over "default n"), such a
>>>>> transformation is not functionally identical: Before your
>>>>> change, with !EXPERT this option defaults to y. After your
>>>>> change this option is unavailable (which resolves to it being
>>>>> off for all consuming purposes).
>>>>> 
>>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>>> (for this construct indeed only making the prompt conditional,
>>>>> not the entire option), but there are also cases where the
>>>>> cleanup you do is indeed desirable / helpful.
>>>> 
>>>> Yeah, thanks for catching it, it is obviously a problem.
>>>> 
>>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>>> would show on the menu. Maybe a better, simpler, way to do it is
>>>> to add the word "EXPERT" to the one line prompt:
>>>> 
>>>> config ARM_SSBD
>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>>> +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>> 	depends on HAS_ALTERNATIVE
>>>> 	default y
>>>> 	help
>>>> 
>>>> 
>>>> What do you think?
>>> 
>>> While on the surface this may look like an improvement, I don't
>>> see how it would actually help: If you read the Kconfig file
>>> itself, the dependency is seen anyway. And on the menu I don't
>>> see the point of telling someone who has enabled EXPERT that a
>>> certain option is (or is not) an expert one. If they think
>>> they're experts, so should they be treated. (It was, after all,
>>> a deliberate decision to make enabling expert mode easier, and
>>> hence easier to use for what one might consider not-really-
>>> experts. I realize saying so may be considered tendentious; I
>>> mean it in a purely technical sense, and I'd like to apologize
>>> in advance to anyone not sharing this as a possible perspective
>>> to take.)
>>> 
>>> Plus, of course, the addition of such (EXPERT) markers to
>>> future options' prompts is liable to get forgotten now and then,
>>> so sooner or later we'd likely end up with an inconsistent
>>> mixture anyway.
>> 
>> I tend to agree with you on everything you wrote. The fundamental issue
>> is that we are (mis)using EXPERT to tag features that are experimental,
>> as defined by SUPPORT.md.
>> 
>> It is important to be able to distinguish clearly at the kconfig level
>> options that are (security) supported from options that are
>> unsupported/experimental. Today the only way to do it is with EXPERT
>> which is not great because:
>> 
>> - it doesn't convey the idea that it is for unsupported/experimental
>>  features
>> - if you want to enable one unsupported feature, it is not clear what
>>  you have to do
>> 
>> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
>> the Kconfig menu?
> 
> If you mean this to be added to prompt texts, then yes, I'd view
> this as helpful. However, ...

+1

> 
>> It would make it clearer that by enabling UNSUPPORTED you are going to
>> get a configuration that is not security supported. And ideally we would
>> also tag features like ACPI as UNSUPPORTED as I suggested above.
> 
> ... things will get uglier when (just a simple example) something
> is supported on x86, but not on Arm.

This is true that this could happen but we could easily workaround this
by having arch specific entries selecting the generic one:

CONFIG_PCI
	bool
	default n

CONFIG_X86_PCI
	bool if x86
	select CONFIG_PCI

CONFIG_ARM_PCI
	bool if arm
	depends on UNSUPPORTED
	select CONFIG_PCI

This is not the full syntax or right variables but you get the idea :-)

This is making Kconfig more complex but improves the user configuration experience
so i think this is a win.

Cheers
Bertrand

On Wed, 4 Nov 2020, Bertrand Marquis wrote:
> > On 4 Nov 2020, at 07:34, Jan Beulich <jbeulich@suse.com> wrote:
> > On 03.11.2020 20:37, Stefano Stabellini wrote:
> >> On Tue, 3 Nov 2020, Jan Beulich wrote:
> >>> On 02.11.2020 22:41, Stefano Stabellini wrote:
> >>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
> >>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
> >>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
> >>>>>> 	  SBSA Generic UART implements a subset of ARM PL011 UART.
> >>>>>> 
> >>>>>> config ARM_SSBD
> >>>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
> >>>>>> -	depends on HAS_ALTERNATIVE
> >>>>>> +	bool "Speculative Store Bypass Disable"
> >>>>>> +	depends on HAS_ALTERNATIVE && EXPERT
> >>>>>> 	default y
> >>>>> 
> >>>>> At the example of this, I'm afraid when the default isn't "n"
> >>>>> (or there's no default directive at all, as ought to be
> >>>>> equivalent to and preferred over "default n"), such a
> >>>>> transformation is not functionally identical: Before your
> >>>>> change, with !EXPERT this option defaults to y. After your
> >>>>> change this option is unavailable (which resolves to it being
> >>>>> off for all consuming purposes).
> >>>>> 
> >>>>> IOW there are reasons to have "if ..." attached to the prompts
> >>>>> (for this construct indeed only making the prompt conditional,
> >>>>> not the entire option), but there are also cases where the
> >>>>> cleanup you do is indeed desirable / helpful.
> >>>> 
> >>>> Yeah, thanks for catching it, it is obviously a problem.
> >>>> 
> >>>> My intention was just to "tag" somehow the options to EXPERT so that it
> >>>> would show on the menu. Maybe a better, simpler, way to do it is
> >>>> to add the word "EXPERT" to the one line prompt:
> >>>> 
> >>>> config ARM_SSBD
> >>>> -	bool "Speculative Store Bypass Disable" if EXPERT
> >>>> +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
> >>>> 	depends on HAS_ALTERNATIVE
> >>>> 	default y
> >>>> 	help
> >>>> 
> >>>> 
> >>>> What do you think?
> >>> 
> >>> While on the surface this may look like an improvement, I don't
> >>> see how it would actually help: If you read the Kconfig file
> >>> itself, the dependency is seen anyway. And on the menu I don't
> >>> see the point of telling someone who has enabled EXPERT that a
> >>> certain option is (or is not) an expert one. If they think
> >>> they're experts, so should they be treated. (It was, after all,
> >>> a deliberate decision to make enabling expert mode easier, and
> >>> hence easier to use for what one might consider not-really-
> >>> experts. I realize saying so may be considered tendentious; I
> >>> mean it in a purely technical sense, and I'd like to apologize
> >>> in advance to anyone not sharing this as a possible perspective
> >>> to take.)
> >>> 
> >>> Plus, of course, the addition of such (EXPERT) markers to
> >>> future options' prompts is liable to get forgotten now and then,
> >>> so sooner or later we'd likely end up with an inconsistent
> >>> mixture anyway.
> >> 
> >> I tend to agree with you on everything you wrote. The fundamental issue
> >> is that we are (mis)using EXPERT to tag features that are experimental,
> >> as defined by SUPPORT.md.
> >> 
> >> It is important to be able to distinguish clearly at the kconfig level
> >> options that are (security) supported from options that are
> >> unsupported/experimental. Today the only way to do it is with EXPERT
> >> which is not great because:
> >> 
> >> - it doesn't convey the idea that it is for unsupported/experimental
> >>  features
> >> - if you want to enable one unsupported feature, it is not clear what
> >>  you have to do
> >> 
> >> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
> >> the Kconfig menu?
> > 
> > If you mean this to be added to prompt texts, then yes, I'd view
> > this as helpful. However, ...
> 
> +1
> 
> > 
> >> It would make it clearer that by enabling UNSUPPORTED you are going to
> >> get a configuration that is not security supported. And ideally we would
> >> also tag features like ACPI as UNSUPPORTED as I suggested above.
> > 
> > ... things will get uglier when (just a simple example) something
> > is supported on x86, but not on Arm.
> 
> This is true that this could happen but we could easily workaround this
> by having arch specific entries selecting the generic one:
> 
> CONFIG_PCI
> 	bool
> 	default n
> 
> CONFIG_X86_PCI
> 	bool if x86
> 	select CONFIG_PCI
> 
> CONFIG_ARM_PCI
> 	bool if arm
> 	depends on UNSUPPORTED
> 	select CONFIG_PCI
> 
> This is not the full syntax or right variables but you get the idea :-)
> 
> This is making Kconfig more complex but improves the user configuration experience
> so i think this is a win.

It is good that we have a potential clean solution for this. However,
today this problem is only theoretical because none of the EXPERT
options under xen/commons have a different support status on ARM vs x86.
So that's not an issue.

However, there a few options in xen/common/Kconfig that are honestly
more in the original meaning of EXPERT rather than UNSUPPORTED, such as:
- CMDLINE
- TRACEBUFFER

I don't think we want to change CMDLINE from EXPERT to UNSUPPORTED,
right? Jan, are there any other options, either under xen/common/Kconfig
or xen/arch/x86/Kconfig that you think they should remain EXPERT?


So, I think the plan should be to:

- introduce a new UNSUPPORTED option, alongside EXPERT
- change EXPERT options under xen/arch/arm/Kconfig to UNSUPPORTED
    - ACPI
    - HAS_ITS
    - ARM_SSBD
    - HARDEN_BRANCH_PREDICTOR
    - TEE
- change other EXPERT options to UNSUPPORTED where it makes sense
    - e.g. ARGO
    - EFI_SET_VIRTUAL_ADDRESS_MAP
    - MEM_SHARING
    - TBOOT
    - XEN_SHSTK
    - Jan, anything else?
- add "(UNSUPPORTED)" to the oneline text of every UNSUPPORTED option


Do you guys agree?

On 05.11.2020 02:14, Stefano Stabellini wrote:
> On Wed, 4 Nov 2020, Bertrand Marquis wrote:
>>> On 4 Nov 2020, at 07:34, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 03.11.2020 20:37, Stefano Stabellini wrote:
>>>> On Tue, 3 Nov 2020, Jan Beulich wrote:
>>>>> On 02.11.2020 22:41, Stefano Stabellini wrote:
>>>>>> On Mon, 2 Nov 2020, Jan Beulich wrote:
>>>>>>> On 31.10.2020 01:24, Stefano Stabellini wrote:
>>>>>>>> @@ -79,8 +79,8 @@ config SBSA_VUART_CONSOLE
>>>>>>>> 	  SBSA Generic UART implements a subset of ARM PL011 UART.
>>>>>>>>
>>>>>>>> config ARM_SSBD
>>>>>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>>>>>>> -	depends on HAS_ALTERNATIVE
>>>>>>>> +	bool "Speculative Store Bypass Disable"
>>>>>>>> +	depends on HAS_ALTERNATIVE && EXPERT
>>>>>>>> 	default y
>>>>>>>
>>>>>>> At the example of this, I'm afraid when the default isn't "n"
>>>>>>> (or there's no default directive at all, as ought to be
>>>>>>> equivalent to and preferred over "default n"), such a
>>>>>>> transformation is not functionally identical: Before your
>>>>>>> change, with !EXPERT this option defaults to y. After your
>>>>>>> change this option is unavailable (which resolves to it being
>>>>>>> off for all consuming purposes).
>>>>>>>
>>>>>>> IOW there are reasons to have "if ..." attached to the prompts
>>>>>>> (for this construct indeed only making the prompt conditional,
>>>>>>> not the entire option), but there are also cases where the
>>>>>>> cleanup you do is indeed desirable / helpful.
>>>>>>
>>>>>> Yeah, thanks for catching it, it is obviously a problem.
>>>>>>
>>>>>> My intention was just to "tag" somehow the options to EXPERT so that it
>>>>>> would show on the menu. Maybe a better, simpler, way to do it is
>>>>>> to add the word "EXPERT" to the one line prompt:
>>>>>>
>>>>>> config ARM_SSBD
>>>>>> -	bool "Speculative Store Bypass Disable" if EXPERT
>>>>>> +	bool "Speculative Store Bypass Disable (EXPERT)" if EXPERT
>>>>>> 	depends on HAS_ALTERNATIVE
>>>>>> 	default y
>>>>>> 	help
>>>>>>
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> While on the surface this may look like an improvement, I don't
>>>>> see how it would actually help: If you read the Kconfig file
>>>>> itself, the dependency is seen anyway. And on the menu I don't
>>>>> see the point of telling someone who has enabled EXPERT that a
>>>>> certain option is (or is not) an expert one. If they think
>>>>> they're experts, so should they be treated. (It was, after all,
>>>>> a deliberate decision to make enabling expert mode easier, and
>>>>> hence easier to use for what one might consider not-really-
>>>>> experts. I realize saying so may be considered tendentious; I
>>>>> mean it in a purely technical sense, and I'd like to apologize
>>>>> in advance to anyone not sharing this as a possible perspective
>>>>> to take.)
>>>>>
>>>>> Plus, of course, the addition of such (EXPERT) markers to
>>>>> future options' prompts is liable to get forgotten now and then,
>>>>> so sooner or later we'd likely end up with an inconsistent
>>>>> mixture anyway.
>>>>
>>>> I tend to agree with you on everything you wrote. The fundamental issue
>>>> is that we are (mis)using EXPERT to tag features that are experimental,
>>>> as defined by SUPPORT.md.
>>>>
>>>> It is important to be able to distinguish clearly at the kconfig level
>>>> options that are (security) supported from options that are
>>>> unsupported/experimental. Today the only way to do it is with EXPERT
>>>> which is not great because:
>>>>
>>>> - it doesn't convey the idea that it is for unsupported/experimental
>>>>  features
>>>> - if you want to enable one unsupported feature, it is not clear what
>>>>  you have to do
>>>>
>>>> So maybe we should replace EXPERT with UNSUPPORTED (or EXPERIMENTAL) in
>>>> the Kconfig menu?
>>>
>>> If you mean this to be added to prompt texts, then yes, I'd view
>>> this as helpful. However, ...
>>
>> +1
>>
>>>
>>>> It would make it clearer that by enabling UNSUPPORTED you are going to
>>>> get a configuration that is not security supported. And ideally we would
>>>> also tag features like ACPI as UNSUPPORTED as I suggested above.
>>>
>>> ... things will get uglier when (just a simple example) something
>>> is supported on x86, but not on Arm.
>>
>> This is true that this could happen but we could easily workaround this
>> by having arch specific entries selecting the generic one:
>>
>> CONFIG_PCI
>> 	bool
>> 	default n
>>
>> CONFIG_X86_PCI
>> 	bool if x86
>> 	select CONFIG_PCI
>>
>> CONFIG_ARM_PCI
>> 	bool if arm
>> 	depends on UNSUPPORTED
>> 	select CONFIG_PCI
>>
>> This is not the full syntax or right variables but you get the idea :-)
>>
>> This is making Kconfig more complex but improves the user configuration experience
>> so i think this is a win.
> 
> It is good that we have a potential clean solution for this. However,
> today this problem is only theoretical because none of the EXPERT
> options under xen/commons have a different support status on ARM vs x86.
> So that's not an issue.
> 
> However, there a few options in xen/common/Kconfig that are honestly
> more in the original meaning of EXPERT rather than UNSUPPORTED, such as:
> - CMDLINE
> - TRACEBUFFER
> 
> I don't think we want to change CMDLINE from EXPERT to UNSUPPORTED,
> right? Jan, are there any other options, either under xen/common/Kconfig
> or xen/arch/x86/Kconfig that you think they should remain EXPERT?

GRANT_TABLE and the "Schedulers" menu (As a general rule I'd say
that options defaulting to y and where only the prompt is
conditional are supported. In the "Schedulers" menu this then
means that the sub-options marked experimental are to become
dependent upon UNSUPPORTED in addition to the menu's EXPERT
dependency.)

Not sure about XSM_FLASK_AVC_STATS.

> So, I think the plan should be to:
> 
> - introduce a new UNSUPPORTED option, alongside EXPERT
> - change EXPERT options under xen/arch/arm/Kconfig to UNSUPPORTED
>     - ACPI
>     - HAS_ITS
>     - ARM_SSBD
>     - HARDEN_BRANCH_PREDICTOR
>     - TEE
> - change other EXPERT options to UNSUPPORTED where it makes sense
>     - e.g. ARGO
>     - EFI_SET_VIRTUAL_ADDRESS_MAP
>     - MEM_SHARING
>     - TBOOT
>     - XEN_SHSTK

Andrew, do we mean this last one to be / remain unsupported? I'd
be inclined to suggest EXPERT wants dropping there, or at least
moving to the prompt.

Jan