All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/2] introduce UNSUPPORTED
@ 2021-01-25 21:27 Stefano Stabellini
  2021-01-25 21:27 ` [PATCH v4 1/2] xen: EXPERT clean-up and " Stefano Stabellini
  2021-01-25 21:27 ` [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate Stefano Stabellini
  0 siblings, 2 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-25 21:27 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, andrew.cooper3, george.dunlap, iwj, jbeulich, julien, wl

Hi all,

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 series makes things easier by doing the following:

- 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

Cheers,

Stefano



Stefano Stabellini (2):
      xen: EXPERT clean-up and introduce UNSUPPORTED
      xen: add (EXPERT) to one-line descriptions when appropriate

 xen/Kconfig              | 11 ++++++++++-
 xen/arch/arm/Kconfig     | 10 +++++-----
 xen/arch/x86/Kconfig     |  8 ++++----
 xen/common/Kconfig       | 14 +++++++-------
 xen/common/sched/Kconfig |  8 ++++----
 5 files changed, 30 insertions(+), 21 deletions(-)



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-25 21:27 [PATCH v4 0/2] introduce UNSUPPORTED Stefano Stabellini
@ 2021-01-25 21:27 ` Stefano Stabellini
  2021-01-26  9:22   ` Jan Beulich
  2021-01-26 13:51   ` Julien Grall
  2021-01-25 21:27 ` [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate Stefano Stabellini
  1 sibling, 2 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-25 21:27 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, Stefano Stabellini, andrew.cooper3, george.dunlap,
	iwj, jbeulich, julien, wl

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 v4:
- clarify support statement of UNSUPPORTED
- move UNSUPPORTED past EXPERT
- add default EXPERT to UNSUPPORTED

Changes in v3:
- improve UNSUPPORTED text description
- avoid changing XEN_SHSTK and EFI_SET_VIRTUAL_ADDRESS_MAP
- update HVM_FEP to be UNSUPPORTED

Changes in v2:
- introduce UNSUPPORTED
- don't switch all EXPERT options to UNSUPPORTED

See as reference the v2 thread here:
https://marc.info/?l=xen-devel&m=160566066013723
---
 xen/Kconfig              | 11 ++++++++++-
 xen/arch/arm/Kconfig     | 10 +++++-----
 xen/arch/x86/Kconfig     |  6 +++---
 xen/common/Kconfig       |  2 +-
 xen/common/sched/Kconfig |  6 +++---
 5 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 34c318bfa2..bcbd2758e5 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -35,7 +35,7 @@ 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
@@ -45,6 +45,15 @@ config EXPERT
 	  supported.
 	default n
 
+config UNSUPPORTED
+	bool "Configure UNSUPPORTED features"
+	default EXPERT
+	help
+	  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. (Note that if an option
+	  doesn't depend on UNSUPPORTED it doesn't imply that is supported.)
+
 config LTO
 	bool "Link Time Optimisation"
 	depends on BROKEN
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index c3eb13ea73..cca76040e5 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
@@ -77,7 +77,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
@@ -87,7 +87,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
@@ -104,7 +104,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 78f351f94b..302334d3e4 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -147,7 +147,7 @@ config BIGMEM
 	  If unsure, say N.
 
 config HVM_FEP
-	bool "HVM Forced Emulation Prefix support" if EXPERT
+	bool "HVM Forced Emulation Prefix support (UNSUPPORTED)" if UNSUPPORTED
 	default DEBUG
 	depends on HVM
 	---help---
@@ -166,7 +166,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---
@@ -252,7 +252,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 b5c91a1664..39451e8350 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -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



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate
  2021-01-25 21:27 [PATCH v4 0/2] introduce UNSUPPORTED Stefano Stabellini
  2021-01-25 21:27 ` [PATCH v4 1/2] xen: EXPERT clean-up and " Stefano Stabellini
@ 2021-01-25 21:27 ` Stefano Stabellini
  2021-01-26  9:26   ` Jan Beulich
  2021-01-26 11:08   ` Bertrand Marquis
  1 sibling, 2 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-25 21:27 UTC (permalink / raw)
  To: xen-devel
  Cc: sstabellini, Stefano Stabellini, andrew.cooper3, george.dunlap,
	iwj, jbeulich, julien, wl

Add an "(EXPERT)" tag to the one-line description of Kconfig options
that depend on EXPERT.

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 v4:
- new patch
---
 xen/arch/x86/Kconfig     |  2 +-
 xen/common/Kconfig       | 12 ++++++------
 xen/common/sched/Kconfig |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 302334d3e4..3f630b89e8 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -103,7 +103,7 @@ config HVM
 	  If unsure, say Y.
 
 config XEN_SHSTK
-	bool "Supervisor Shadow Stacks"
+	bool "Supervisor Shadow Stacks (EXPERT)"
 	depends on HAS_AS_CET_SS && EXPERT
 	default y
 	---help---
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 39451e8350..b49127463d 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -12,7 +12,7 @@ config CORE_PARKING
 	bool
 
 config GRANT_TABLE
-	bool "Grant table support" if EXPERT
+	bool "Grant table support (EXPERT)" if EXPERT
 	default y
 	---help---
 	  Grant table provides a generic mechanism to memory sharing
@@ -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() (EXPERT)" if 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 +162,7 @@ config EFI_SET_VIRTUAL_ADDRESS_MAP
 
 config XENOPROF
 	def_bool y
-	prompt "Xen Oprofile Support" if EXPERT
+	prompt "Xen Oprofile Support (EXPERT)" if EXPERT
 	depends on X86
 	---help---
 	  Xen OProfile (Xenoprof) is a system-wide profiler for Xen virtual
@@ -199,7 +199,7 @@ 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 (EXPERT)" if EXPERT
 	depends on XSM_FLASK
 	---help---
 	  Maintain counters on the access vector cache that can be viewed using
@@ -344,7 +344,7 @@ 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 (EXPERT)" if EXPERT
 	default ""
 	---help---
 	  Enter arguments here that should be compiled into the hypervisor
@@ -377,7 +377,7 @@ 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 (EXPERT)" if 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 94c9e20139..b6020a83c6 100644
--- a/xen/common/sched/Kconfig
+++ b/xen/common/sched/Kconfig
@@ -1,4 +1,4 @@
-menu "Schedulers"
+menu "Schedulers (EXPERT)"
 	visible if EXPERT
 
 config SCHED_CREDIT
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-25 21:27 ` [PATCH v4 1/2] xen: EXPERT clean-up and " Stefano Stabellini
@ 2021-01-26  9:22   ` Jan Beulich
  2021-01-26 11:06     ` Bertrand Marquis
  2021-01-26 13:51   ` Julien Grall
  1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2021-01-26  9:22 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Stefano Stabellini, andrew.cooper3, george.dunlap, iwj, julien,
	wl, xen-devel

On 25.01.2021 22:27, Stefano Stabellini wrote:
> 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>

Non-Arm bits
Reviewed-by: Jan Beulich <jbeulich@suse.com>
However, I have questions on the Arm ones (sorry for not noticing
earlier, as I assume it was this way before already):

> @@ -77,7 +77,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
> @@ -87,7 +87,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

Both of these default to y and have their _prompt_
conditional upon EXPERT. Which means only an expert can turn them
_off_. Which doesn't make it look like these are unsupported? Or
if anything, turning them off is unsupported?

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate
  2021-01-25 21:27 ` [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate Stefano Stabellini
@ 2021-01-26  9:26   ` Jan Beulich
  2021-01-26 18:36     ` Stefano Stabellini
  2021-01-26 11:08   ` Bertrand Marquis
  1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2021-01-26  9:26 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Stefano Stabellini, andrew.cooper3, george.dunlap, iwj, julien,
	wl, xen-devel

On 25.01.2021 22:27, Stefano Stabellini wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -103,7 +103,7 @@ config HVM
>  	  If unsure, say Y.
>  
>  config XEN_SHSTK
> -	bool "Supervisor Shadow Stacks"
> +	bool "Supervisor Shadow Stacks (EXPERT)"
>  	depends on HAS_AS_CET_SS && EXPERT
>  	default y
>  	---help---

I agree with this addition, but I'm afraid I'm at least uncertain
about all the other ones made here, where ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -12,7 +12,7 @@ config CORE_PARKING
>  	bool
>  
>  config GRANT_TABLE
> -	bool "Grant table support" if EXPERT
> +	bool "Grant table support (EXPERT)" if EXPERT
>  	default y

... like e.g. here, it's only the prompt that's conditional. IOW
like with the respective uses of UNSUPPORTED in some of the Arm
changes in patch 1, especially when the option's default is "yes"
it's not the feature that is an expert one, but its turning off
is considered an expert change. Which I don't think should be
expressed this way.

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26  9:22   ` Jan Beulich
@ 2021-01-26 11:06     ` Bertrand Marquis
  2021-01-26 11:11       ` Jan Beulich
  0 siblings, 1 reply; 20+ messages in thread
From: Bertrand Marquis @ 2021-01-26 11:06 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3,
	george.dunlap, iwj, julien, wl, xen-devel

Hi,

> On 26 Jan 2021, at 09:22, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 25.01.2021 22:27, Stefano Stabellini wrote:
>> 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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Answering to Jan...

> 
> Non-Arm bits
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> However, I have questions on the Arm ones (sorry for not noticing
> earlier, as I assume it was this way before already):
> 
>> @@ -77,7 +77,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
>> @@ -87,7 +87,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
> 
> Both of these default to y and have their _prompt_
> conditional upon EXPERT. Which means only an expert can turn them
> _off_. Which doesn't make it look like these are unsupported? Or
> if anything, turning them off is unsupported?

...You could see that as EXPERT/UNSUPPORTED options can only be
 “modified” from their default value if EXPERT/UNSUPPORTED is activated.
So I find the current solution ok.

If this is a problem we could also change those options to be default to _off_ by renaming them to config DISABLE_xxxx

Cheers
Bertrand

> 
> Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate
  2021-01-25 21:27 ` [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate Stefano Stabellini
  2021-01-26  9:26   ` Jan Beulich
@ 2021-01-26 11:08   ` Bertrand Marquis
  1 sibling, 0 replies; 20+ messages in thread
From: Bertrand Marquis @ 2021-01-26 11:08 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: Xen-devel, Stefano Stabellini, andrew.cooper3, george.dunlap,
	iwj, jbeulich, julien, wl

Hi,

> On 25 Jan 2021, at 21:27, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> Add an "(EXPERT)" tag to the one-line description of Kconfig options
> that depend on EXPERT.
> 
> 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

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> ---
> Changes in v4:
> - new patch
> ---
> xen/arch/x86/Kconfig     |  2 +-
> xen/common/Kconfig       | 12 ++++++------
> xen/common/sched/Kconfig |  2 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 302334d3e4..3f630b89e8 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -103,7 +103,7 @@ config HVM
> 	  If unsure, say Y.
> 
> config XEN_SHSTK
> -	bool "Supervisor Shadow Stacks"
> +	bool "Supervisor Shadow Stacks (EXPERT)"
> 	depends on HAS_AS_CET_SS && EXPERT
> 	default y
> 	---help---
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 39451e8350..b49127463d 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -12,7 +12,7 @@ config CORE_PARKING
> 	bool
> 
> config GRANT_TABLE
> -	bool "Grant table support" if EXPERT
> +	bool "Grant table support (EXPERT)" if EXPERT
> 	default y
> 	---help---
> 	  Grant table provides a generic mechanism to memory sharing
> @@ -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() (EXPERT)" if 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 +162,7 @@ config EFI_SET_VIRTUAL_ADDRESS_MAP
> 
> config XENOPROF
> 	def_bool y
> -	prompt "Xen Oprofile Support" if EXPERT
> +	prompt "Xen Oprofile Support (EXPERT)" if EXPERT
> 	depends on X86
> 	---help---
> 	  Xen OProfile (Xenoprof) is a system-wide profiler for Xen virtual
> @@ -199,7 +199,7 @@ 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 (EXPERT)" if EXPERT
> 	depends on XSM_FLASK
> 	---help---
> 	  Maintain counters on the access vector cache that can be viewed using
> @@ -344,7 +344,7 @@ 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 (EXPERT)" if EXPERT
> 	default ""
> 	---help---
> 	  Enter arguments here that should be compiled into the hypervisor
> @@ -377,7 +377,7 @@ 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 (EXPERT)" if 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 94c9e20139..b6020a83c6 100644
> --- a/xen/common/sched/Kconfig
> +++ b/xen/common/sched/Kconfig
> @@ -1,4 +1,4 @@
> -menu "Schedulers"
> +menu "Schedulers (EXPERT)"
> 	visible if EXPERT
> 
> config SCHED_CREDIT
> -- 
> 2.17.1
> 
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 11:06     ` Bertrand Marquis
@ 2021-01-26 11:11       ` Jan Beulich
  2021-01-26 11:17         ` Bertrand Marquis
  0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2021-01-26 11:11 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3,
	george.dunlap, iwj, julien, wl, xen-devel

On 26.01.2021 12:06, Bertrand Marquis wrote:
>> On 26 Jan 2021, at 09:22, Jan Beulich <jbeulich@suse.com> wrote:
>> On 25.01.2021 22:27, Stefano Stabellini wrote:
>>> @@ -77,7 +77,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
>>> @@ -87,7 +87,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
>>
>> Both of these default to y and have their _prompt_
>> conditional upon EXPERT. Which means only an expert can turn them
>> _off_. Which doesn't make it look like these are unsupported? Or
>> if anything, turning them off is unsupported?
> 
> ...You could see that as EXPERT/UNSUPPORTED options can only be
>  “modified” from their default value if EXPERT/UNSUPPORTED is activated.

But this is nothing you can recognize when configuring Xen
(i.e. seeing just these prompts).

> If this is a problem we could also change those options to be default
> to _off_ by renaming them to config DISABLE_xxxx

Yes, this would be a possibility, albeit not necessarily
one I would like.

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 11:11       ` Jan Beulich
@ 2021-01-26 11:17         ` Bertrand Marquis
  2021-01-26 13:08           ` Jan Beulich
  0 siblings, 1 reply; 20+ messages in thread
From: Bertrand Marquis @ 2021-01-26 11:17 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3,
	george.dunlap, iwj, julien, wl, xen-devel



> On 26 Jan 2021, at 11:11, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.01.2021 12:06, Bertrand Marquis wrote:
>>> On 26 Jan 2021, at 09:22, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 25.01.2021 22:27, Stefano Stabellini wrote:
>>>> @@ -77,7 +77,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
>>>> @@ -87,7 +87,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
>>> 
>>> Both of these default to y and have their _prompt_
>>> conditional upon EXPERT. Which means only an expert can turn them
>>> _off_. Which doesn't make it look like these are unsupported? Or
>>> if anything, turning them off is unsupported?
>> 
>> ...You could see that as EXPERT/UNSUPPORTED options can only be
>> “modified” from their default value if EXPERT/UNSUPPORTED is activated.
> 
> But this is nothing you can recognize when configuring Xen
> (i.e. seeing just these prompts).

Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
config parameters instead ?
We could also make that more clear in the help of such parameters directly.

I do not see how we could make that more clear directly in the prompt (as
making it too long is not a good solution).

> 
>> If this is a problem we could also change those options to be default
>> to _off_ by renaming them to config DISABLE_xxxx
> 
> Yes, this would be a possibility, albeit not necessarily
> one I would like.

This is not one I would like either.

Bertrand

> 
> Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 11:17         ` Bertrand Marquis
@ 2021-01-26 13:08           ` Jan Beulich
  2021-01-26 13:17             ` Ian Jackson
  2021-01-26 18:28             ` Stefano Stabellini
  0 siblings, 2 replies; 20+ messages in thread
From: Jan Beulich @ 2021-01-26 13:08 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3,
	george.dunlap, iwj, julien, wl, xen-devel

On 26.01.2021 12:17, Bertrand Marquis wrote:
> 
> 
>> On 26 Jan 2021, at 11:11, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.01.2021 12:06, Bertrand Marquis wrote:
>>>> On 26 Jan 2021, at 09:22, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 25.01.2021 22:27, Stefano Stabellini wrote:
>>>>> @@ -77,7 +77,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
>>>>> @@ -87,7 +87,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
>>>>
>>>> Both of these default to y and have their _prompt_
>>>> conditional upon EXPERT. Which means only an expert can turn them
>>>> _off_. Which doesn't make it look like these are unsupported? Or
>>>> if anything, turning them off is unsupported?
>>>
>>> ...You could see that as EXPERT/UNSUPPORTED options can only be
>>> “modified” from their default value if EXPERT/UNSUPPORTED is activated.
>>
>> But this is nothing you can recognize when configuring Xen
>> (i.e. seeing just these prompts).
> 
> Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
> config parameters instead ?
> We could also make that more clear in the help of such parameters directly.
> 
> I do not see how we could make that more clear directly in the prompt (as
> making it too long is not a good solution).

My main request is that such tags be added only if there's
absolutely no ambiguity. Anything else (requiring longer
explanations in many cases) should be expressed in the help
text of the option, or in yet other ways (a referral to
SUPPORT.md comes to mind).

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:08           ` Jan Beulich
@ 2021-01-26 13:17             ` Ian Jackson
  2021-01-26 13:19               ` Jan Beulich
  2021-01-26 15:23               ` Jan Beulich
  2021-01-26 18:28             ` Stefano Stabellini
  1 sibling, 2 replies; 20+ messages in thread
From: Ian Jackson @ 2021-01-26 13:17 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bertrand Marquis, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, julien, wl, xen-devel

Jan Beulich writes ("Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED"):
> On 26.01.2021 12:17, Bertrand Marquis wrote:
> > Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
> > config parameters instead ?
> > We could also make that more clear in the help of such parameters directly.
> > 
> > I do not see how we could make that more clear directly in the prompt (as
> > making it too long is not a good solution).
> 
> My main request is that such tags be added only if there's
> absolutely no ambiguity. Anything else (requiring longer
> explanations in many cases) should be expressed in the help
> text of the option, or in yet other ways (a referral to
> SUPPORT.md comes to mind).

Is

>>>>> +	bool "Harden the branch predictor against aliasing attacks (disabling UNSUPPORTED)" if UNSUPPORTED

too long ?

Ian.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:17             ` Ian Jackson
@ 2021-01-26 13:19               ` Jan Beulich
  2021-01-26 13:20                 ` Bertrand Marquis
  2021-01-26 15:23               ` Jan Beulich
  1 sibling, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2021-01-26 13:19 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Bertrand Marquis, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, julien, wl, xen-devel

On 26.01.2021 14:17, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED"):
>> On 26.01.2021 12:17, Bertrand Marquis wrote:
>>> Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
>>> config parameters instead ?
>>> We could also make that more clear in the help of such parameters directly.
>>>
>>> I do not see how we could make that more clear directly in the prompt (as
>>> making it too long is not a good solution).
>>
>> My main request is that such tags be added only if there's
>> absolutely no ambiguity. Anything else (requiring longer
>> explanations in many cases) should be expressed in the help
>> text of the option, or in yet other ways (a referral to
>> SUPPORT.md comes to mind).
> 
> Is
> 
>>>>>> +	bool "Harden the branch predictor against aliasing attacks (disabling UNSUPPORTED)" if UNSUPPORTED
> 
> too long ?

It may be tolerable, but it is getting longish imo.

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:19               ` Jan Beulich
@ 2021-01-26 13:20                 ` Bertrand Marquis
  2021-01-26 15:18                   ` Marek Marczykowski-Górecki
  0 siblings, 1 reply; 20+ messages in thread
From: Bertrand Marquis @ 2021-01-26 13:20 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Ian Jackson, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, julien, wl, xen-devel



> On 26 Jan 2021, at 13:19, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 26.01.2021 14:17, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED"):
>>> On 26.01.2021 12:17, Bertrand Marquis wrote:
>>>> Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
>>>> config parameters instead ?
>>>> We could also make that more clear in the help of such parameters directly.
>>>> 
>>>> I do not see how we could make that more clear directly in the prompt (as
>>>> making it too long is not a good solution).
>>> 
>>> My main request is that such tags be added only if there's
>>> absolutely no ambiguity. Anything else (requiring longer
>>> explanations in many cases) should be expressed in the help
>>> text of the option, or in yet other ways (a referral to
>>> SUPPORT.md comes to mind).
>> 
>> Is
>> 
>>>>>>> +	bool "Harden the branch predictor against aliasing attacks (disabling UNSUPPORTED)" if UNSUPPORTED
>> 
>> too long ?
> 
> It may be tolerable, but it is getting longish imo.

I am also not quite sure this is making things clearer.

Bertrand

> 
> Jan



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-25 21:27 ` [PATCH v4 1/2] xen: EXPERT clean-up and " Stefano Stabellini
  2021-01-26  9:22   ` Jan Beulich
@ 2021-01-26 13:51   ` Julien Grall
  2021-01-26 14:23     ` Bertrand Marquis
  2021-01-26 18:26     ` Stefano Stabellini
  1 sibling, 2 replies; 20+ messages in thread
From: Julien Grall @ 2021-01-26 13:51 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel
  Cc: Stefano Stabellini, andrew.cooper3, george.dunlap, iwj, jbeulich, wl

Hi Stefano,

On 25/01/2021 21:27, Stefano Stabellini wrote:
>   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
> @@ -87,7 +87,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

I read through the back and forth between Bertrand and Jan about 
"UNSUPPORTED". However, I still don't understand why those two options 
are moved to UNSUPPORTED.

Both options will only build the code to enable the mitigation. The 
decision is still based on the processor you are running on.

In addition to that, ARM_SSBD can also be forced enabled/disabled on the 
command line.

A user may want to compile out the code if the target processor is not 
the affected by the two issues. This wouldn't be much different to Xen 
deciding to not enabling the mitigation.

I would view the two options as supported but not security supported. So 
this seems to fit exactly in the definition of EXPERT rather than 
UNSUPPORTED.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:51   ` Julien Grall
@ 2021-01-26 14:23     ` Bertrand Marquis
  2021-01-26 18:26     ` Stefano Stabellini
  1 sibling, 0 replies; 20+ messages in thread
From: Bertrand Marquis @ 2021-01-26 14:23 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, xen-devel, Stefano Stabellini,
	andrew.cooper3, george.dunlap, iwj, jbeulich, wl

Hi Julien,

> On 26 Jan 2021, at 13:51, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 25/01/2021 21:27, Stefano Stabellini wrote:
>>  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
>> @@ -87,7 +87,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
> 
> I read through the back and forth between Bertrand and Jan about "UNSUPPORTED". However, I still don't understand why those two options are moved to UNSUPPORTED.

Discussion was more on what to do for options which have a default y and can only be turned off with UNSUPPORTED or EXPERT selected.

> 
> Both options will only build the code to enable the mitigation. The decision is still based on the processor you are running on.
> 
> In addition to that, ARM_SSBD can also be forced enabled/disabled on the command line.
> 
> A user may want to compile out the code if the target processor is not the affected by the two issues. This wouldn't be much different to Xen deciding to not enabling the mitigation.
> 
> I would view the two options as supported but not security supported. So this seems to fit exactly in the definition of EXPERT rather than UNSUPPORTED.

I think you are right here, not security supported should be only available to EXPERT.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:20                 ` Bertrand Marquis
@ 2021-01-26 15:18                   ` Marek Marczykowski-Górecki
  0 siblings, 0 replies; 20+ messages in thread
From: Marek Marczykowski-Górecki @ 2021-01-26 15:18 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Jan Beulich, Ian Jackson, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, julien, wl, xen-devel

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

On Tue, Jan 26, 2021 at 01:20:14PM +0000, Bertrand Marquis wrote:
> 
> 
> > On 26 Jan 2021, at 13:19, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> > On 26.01.2021 14:17, Ian Jackson wrote:
> >> Jan Beulich writes ("Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED"):
> >>> On 26.01.2021 12:17, Bertrand Marquis wrote:
> >>>> Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
> >>>> config parameters instead ?
> >>>> We could also make that more clear in the help of such parameters directly.
> >>>> 
> >>>> I do not see how we could make that more clear directly in the prompt (as
> >>>> making it too long is not a good solution).
> >>> 
> >>> My main request is that such tags be added only if there's
> >>> absolutely no ambiguity. Anything else (requiring longer
> >>> explanations in many cases) should be expressed in the help
> >>> text of the option, or in yet other ways (a referral to
> >>> SUPPORT.md comes to mind).
> >> 
> >> Is
> >> 
> >>>>>>> +	bool "Harden the branch predictor against aliasing attacks (disabling UNSUPPORTED)" if UNSUPPORTED
> >> 
> >> too long ?
> > 
> > It may be tolerable, but it is getting longish imo.
> 
> I am also not quite sure this is making things clearer.

IMO it the original version strongly suggested that _enabling_ the
option is unsupported (and quite confusing why it was enabled by
default). When browsing the menu, it isn't really clear what is the
default value, and even if it would be, it's totally not obvious that the
meaning of "(UNSUPPORTED)" depends on default option value.

So, yes, I think "(disabling UNSUPPORTED)" is significantly better for
such cases.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:17             ` Ian Jackson
  2021-01-26 13:19               ` Jan Beulich
@ 2021-01-26 15:23               ` Jan Beulich
  1 sibling, 0 replies; 20+ messages in thread
From: Jan Beulich @ 2021-01-26 15:23 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Bertrand Marquis, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, julien, wl, xen-devel

On 26.01.2021 14:17, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED"):
>> On 26.01.2021 12:17, Bertrand Marquis wrote:
>>> Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
>>> config parameters instead ?
>>> We could also make that more clear in the help of such parameters directly.
>>>
>>> I do not see how we could make that more clear directly in the prompt (as
>>> making it too long is not a good solution).
>>
>> My main request is that such tags be added only if there's
>> absolutely no ambiguity. Anything else (requiring longer
>> explanations in many cases) should be expressed in the help
>> text of the option, or in yet other ways (a referral to
>> SUPPORT.md comes to mind).
> 
> Is
> 
>>>>>> +	bool "Harden the branch predictor against aliasing attacks (disabling UNSUPPORTED)" if UNSUPPORTED
> 
> too long ?

One more consideration, beyond the "too long" aspect. To me (as a
non-native speaker) this wording might allow inferring (by people
not knowing kconfig sufficiently well) that selecting this option
will turn off UNSUPPORTED. IOW if anything I'd see us use the yet
slightly longer "... (UNSUPPORTED if disabled)" or some such.

Jan


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:51   ` Julien Grall
  2021-01-26 14:23     ` Bertrand Marquis
@ 2021-01-26 18:26     ` Stefano Stabellini
  1 sibling, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-26 18:26 UTC (permalink / raw)
  To: Julien Grall
  Cc: Stefano Stabellini, xen-devel, Stefano Stabellini,
	andrew.cooper3, george.dunlap, iwj, jbeulich, wl

On Tue, 26 Jan 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/01/2021 21:27, Stefano Stabellini wrote:
> >   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
> > @@ -87,7 +87,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
> 
> I read through the back and forth between Bertrand and Jan about
> "UNSUPPORTED". However, I still don't understand why those two options are
> moved to UNSUPPORTED.
> 
> Both options will only build the code to enable the mitigation. The decision
> is still based on the processor you are running on.
> 
> In addition to that, ARM_SSBD can also be forced enabled/disabled on the
> command line.

Yes, you are right. HARDEN_BRANCH_PREDICTOR and ARM_SSBD should remain
EXPERT as they are today. It was a mistake to change them to
UNSUPPORTED.


> A user may want to compile out the code if the target processor is not the
> affected by the two issues. This wouldn't be much different to Xen deciding to
> not enabling the mitigation.
> 
> I would view the two options as supported but not security supported. So this
> seems to fit exactly in the definition of EXPERT rather than UNSUPPORTED.

Yes, I'll leave them unmodified.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 1/2] xen: EXPERT clean-up and introduce UNSUPPORTED
  2021-01-26 13:08           ` Jan Beulich
  2021-01-26 13:17             ` Ian Jackson
@ 2021-01-26 18:28             ` Stefano Stabellini
  1 sibling, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-26 18:28 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bertrand Marquis, Stefano Stabellini, Stefano Stabellini,
	andrew.cooper3, george.dunlap, iwj, julien, wl, xen-devel

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Tue, 26 Jan 2021, Jan Beulich wrote:
> On 26.01.2021 12:17, Bertrand Marquis wrote:
> > 
> > 
> >> On 26 Jan 2021, at 11:11, Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 26.01.2021 12:06, Bertrand Marquis wrote:
> >>>> On 26 Jan 2021, at 09:22, Jan Beulich <jbeulich@suse.com> wrote:
> >>>> On 25.01.2021 22:27, Stefano Stabellini wrote:
> >>>>> @@ -77,7 +77,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
> >>>>> @@ -87,7 +87,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
> >>>>
> >>>> Both of these default to y and have their _prompt_
> >>>> conditional upon EXPERT. Which means only an expert can turn them
> >>>> _off_. Which doesn't make it look like these are unsupported? Or
> >>>> if anything, turning them off is unsupported?
> >>>
> >>> ...You could see that as EXPERT/UNSUPPORTED options can only be
> >>> “modified” from their default value if EXPERT/UNSUPPORTED is activated.
> >>
> >> But this is nothing you can recognize when configuring Xen
> >> (i.e. seeing just these prompts).
> > 
> > Maybe something we could explain more clearly in the UNSUPPORTED/EXPERT
> > config parameters instead ?
> > We could also make that more clear in the help of such parameters directly.
> > 
> > I do not see how we could make that more clear directly in the prompt (as
> > making it too long is not a good solution).
> 
> My main request is that such tags be added only if there's
> absolutely no ambiguity. Anything else (requiring longer
> explanations in many cases) should be expressed in the help
> text of the option, or in yet other ways (a referral to
> SUPPORT.md comes to mind).

I actually agree completely with you. In the case of ARM_SSBD and
HARDEN_BRANCH_PREDICTOR, they should remain as they are today I think.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate
  2021-01-26  9:26   ` Jan Beulich
@ 2021-01-26 18:36     ` Stefano Stabellini
  0 siblings, 0 replies; 20+ messages in thread
From: Stefano Stabellini @ 2021-01-26 18:36 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Stefano Stabellini, andrew.cooper3,
	george.dunlap, iwj, julien, wl, xen-devel

On Tue, 26 Jan 2021, Jan Beulich wrote:
> On 25.01.2021 22:27, Stefano Stabellini wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -103,7 +103,7 @@ config HVM
> >  	  If unsure, say Y.
> >  
> >  config XEN_SHSTK
> > -	bool "Supervisor Shadow Stacks"
> > +	bool "Supervisor Shadow Stacks (EXPERT)"
> >  	depends on HAS_AS_CET_SS && EXPERT
> >  	default y
> >  	---help---
> 
> I agree with this addition, but I'm afraid I'm at least uncertain
> about all the other ones made here, where ...
> 
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -12,7 +12,7 @@ config CORE_PARKING
> >  	bool
> >  
> >  config GRANT_TABLE
> > -	bool "Grant table support" if EXPERT
> > +	bool "Grant table support (EXPERT)" if EXPERT
> >  	default y
> 
> ... like e.g. here, it's only the prompt that's conditional. IOW
> like with the respective uses of UNSUPPORTED in some of the Arm
> changes in patch 1, especially when the option's default is "yes"
> it's not the feature that is an expert one, but its turning off
> is considered an expert change. Which I don't think should be
> expressed this way.

That's a good point actually. It makes sense to add the EXPERT tag to
the one-line description of options that depends on EXPERT. Not where
only the prompt depends on EXPERT and the option is actually default y.

Which really narrows it down to XEN_SHSTK only. I'll reduce the patch
to do just that.


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-01-26 18:36 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25 21:27 [PATCH v4 0/2] introduce UNSUPPORTED Stefano Stabellini
2021-01-25 21:27 ` [PATCH v4 1/2] xen: EXPERT clean-up and " Stefano Stabellini
2021-01-26  9:22   ` Jan Beulich
2021-01-26 11:06     ` Bertrand Marquis
2021-01-26 11:11       ` Jan Beulich
2021-01-26 11:17         ` Bertrand Marquis
2021-01-26 13:08           ` Jan Beulich
2021-01-26 13:17             ` Ian Jackson
2021-01-26 13:19               ` Jan Beulich
2021-01-26 13:20                 ` Bertrand Marquis
2021-01-26 15:18                   ` Marek Marczykowski-Górecki
2021-01-26 15:23               ` Jan Beulich
2021-01-26 18:28             ` Stefano Stabellini
2021-01-26 13:51   ` Julien Grall
2021-01-26 14:23     ` Bertrand Marquis
2021-01-26 18:26     ` Stefano Stabellini
2021-01-25 21:27 ` [PATCH v4 2/2] xen: add (EXPERT) to one-line descriptions when appropriate Stefano Stabellini
2021-01-26  9:26   ` Jan Beulich
2021-01-26 18:36     ` Stefano Stabellini
2021-01-26 11:08   ` Bertrand Marquis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.