| Message ID | 20201118005051.26115-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] xen: EXPERT clean-up and introduce UNSUPPORTED | expand |
On 18.11.2020 01:50, Stefano Stabellini wrote: > 1) It is not obvious that "Configure standard Xen features (expert > users)" is actually the famous EXPERT we keep talking about on xen-devel Which can be addressed by simply changing the one prompt line. > 2) It is not obvious when we need to enable EXPERT to get a specific > feature > > In particular if you want to enable ACPI support so that you can boot > Xen on an ACPI platform, you have to enable EXPERT first. But searching > through the kconfig menu it is really not clear (type '/' and "ACPI"): > nothing in the description tells you that you need to enable EXPERT to > get the option. And what causes this to be different once you switch to UNSUPPORTED? > So this patch makes things easier by doing two things: > > - introduce a new kconfig option UNSUPPORTED which is clearly to enable > UNSUPPORTED features as defined by SUPPORT.md > > - change EXPERT options to UNSUPPORTED where it makes sense: keep > depending on EXPERT for features made for experts > > - tag unsupported features by adding (UNSUPPORTED) to the one-line > description I am, btw, not fully convinced of the need for this redundancy. Wouldn't it be enough to have just EXPERT as a setting, but varying (<reason>) tokens in the prompt text? > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -34,8 +34,17 @@ config DEFCONFIG_LIST > option defconfig_list > default ARCH_DEFCONFIG > > +config UNSUPPORTED > + bool "Configure UNSUPPORTED features" > + help > + This option allows unsupported Xen options to be enabled, which I'd recommend against "enabled" - a control may also be there to allow disabling something. > + includes non-security-supported, experimental, and tech preview > + features as defined by SUPPORT.md. Xen binaries built with this > + option enabled are not security supported. Overall I'm a little afraid of possible inverse implications: Anything _not_ dependent upon this option (and in particular anything not dependent upon any Kconfig control) may be considered supported then. Also the last sentence is already present for EXPERT, > + default n I realize you likely merely copied what EXPERT has, but this "default n" is pretty pointless and hence would better be omitted imo. > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -102,8 +102,8 @@ config HVM > If unsure, say Y. > > config XEN_SHSTK > - bool "Supervisor Shadow Stacks" > - depends on HAS_AS_CET_SS && EXPERT > + bool "Supervisor Shadow Stacks (UNSUPPORTED)" > + depends on HAS_AS_CET_SS && UNSUPPORTED > default y Andrew, I think I did ask on v1 already: Do we need to continue to consider this unsupported? While perhaps not a change to make right in this patch, it should perhaps be a pre-patch then to avoid the need to touch it here. > @@ -165,7 +165,7 @@ config HVM_FEP Seeing just the patch context here, I think HVM_FEP may also want converting. > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -151,7 +151,7 @@ config KEXEC > If unsure, say Y. > > config EFI_SET_VIRTUAL_ADDRESS_MAP > - bool "EFI: call SetVirtualAddressMap()" if EXPERT > + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED I have to admit I'm pretty unsure about what to do with this one. > @@ -272,7 +272,7 @@ config LATE_HWDOM > If unsure, say N. > > config ARGO > - bool "Argo: hypervisor-mediated interdomain communication" if EXPERT > + bool "Argo: hypervisor-mediated interdomain communication (UNSUPPORTED)" if UNSUPPORTED Perhaps better (EXPERIMENTAL)? > --- a/xen/common/sched/Kconfig > +++ b/xen/common/sched/Kconfig > @@ -15,7 +15,7 @@ config SCHED_CREDIT2 > optimized for lower latency and higher VM density. > > config SCHED_RTDS > - bool "RTDS scheduler support (EXPERIMENTAL)" > + bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED > default y > ---help--- > The RTDS scheduler is a soft and firm real-time scheduler for > @@ -23,14 +23,14 @@ config SCHED_RTDS > in the cloud, and general low-latency workloads. > > config SCHED_ARINC653 > - bool "ARINC653 scheduler support (EXPERIMENTAL)" > + bool "ARINC653 scheduler support (UNSUPPORTED)" if UNSUPPORTED > default DEBUG > ---help--- > The ARINC653 scheduler is a hard real-time scheduler for single > cores, targeted for avionics, drones, and medical devices. > > config SCHED_NULL > - bool "Null scheduler support (EXPERIMENTAL)" > + bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED > default y > ---help--- > The null scheduler is a static, zero overhead scheduler, I'd like to see (EXPERIMENTAL) stay everywhere here. Jan
Hi Stefano, > On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote: > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > A recent thread [1] has exposed a couple of issues with our current way > of handling EXPERT. > > 1) It is not obvious that "Configure standard Xen features (expert > users)" is actually the famous EXPERT we keep talking about on xen-devel > > 2) It is not obvious when we need to enable EXPERT to get a specific > feature > > In particular if you want to enable ACPI support so that you can boot > Xen on an ACPI platform, you have to enable EXPERT first. But searching > through the kconfig menu it is really not clear (type '/' and "ACPI"): > nothing in the description tells you that you need to enable EXPERT to > get the option. This is a great change that makes configuration more clear. > > So this patch makes things easier by doing two things: > > - introduce a new kconfig option UNSUPPORTED which is clearly to enable > UNSUPPORTED features as defined by SUPPORT.md > > - change EXPERT options to UNSUPPORTED where it makes sense: keep > depending on EXPERT for features made for experts > > - tag unsupported features by adding (UNSUPPORTED) to the one-line > description > > - clarify the EXPERT one-line description Should we also follow the scheme and add (EXPERT) in the text for expert options ? and one small fix > > [1] https://marc.info/?l=xen-devel&m=160333101228981 > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > CC: andrew.cooper3@citrix.com > CC: george.dunlap@citrix.com > CC: iwj@xenproject.org > CC: jbeulich@suse.com > CC: julien@xen.org > CC: wl@xen.org > > --- > Changes in v2: > - introduce UNSUPPORTED as a separate new option > - don't switch all EXPERT options to UNSUPPORTED > --- > xen/Kconfig | 11 ++++++++++- > xen/arch/arm/Kconfig | 10 +++++----- > xen/arch/x86/Kconfig | 8 ++++---- > xen/common/Kconfig | 4 ++-- > xen/common/sched/Kconfig | 6 +++--- > 5 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/xen/Kconfig b/xen/Kconfig > index 34c318bfa2..59400c4788 100644 > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -34,8 +34,17 @@ config DEFCONFIG_LIST > option defconfig_list > default ARCH_DEFCONFIG > > +config UNSUPPORTED > + bool "Configure UNSUPPORTED features" > + help > + This option allows unsupported Xen options to be enabled, which > + includes non-security-supported, experimental, and tech preview > + features as defined by SUPPORT.md. Xen binaries built with this > + option enabled are not security supported. > + default n > + > config EXPERT > - bool "Configure standard Xen features (expert users)" > + bool "Configure EXPERT features" > help > This option allows certain base Xen options and settings > to be disabled or tweaked. This is for specialized environments > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index f938dd21bd..5981e7380d 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -32,7 +32,7 @@ menu "Architecture Features" > source "arch/Kconfig" > > config ACPI > - bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT > + bool "ACPI (Advanced Configuration and Power Interface) Support (UNSUPPORTED)" if UNSUPPORTED > depends on ARM_64 > ---help--- > > @@ -49,7 +49,7 @@ config GICV3 > If unsure, say Y > > config HAS_ITS > - bool "GICv3 ITS MSI controller support" if EXPERT > + bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED > depends on GICV3 && !NEW_VGIC > > config HVM > @@ -79,7 +79,7 @@ config SBSA_VUART_CONSOLE > SBSA Generic UART implements a subset of ARM PL011 UART. > > config ARM_SSBD > - bool "Speculative Store Bypass Disable" if EXPERT > + bool "Speculative Store Bypass Disable (UNSUPPORTED)" if UNSUPPORTED > depends on HAS_ALTERNATIVE > default y > help > @@ -89,7 +89,7 @@ config ARM_SSBD > If unsure, say Y. > > config HARDEN_BRANCH_PREDICTOR > - bool "Harden the branch predictor against aliasing attacks" if EXPERT > + bool "Harden the branch predictor against aliasing attacks (UNSUPPORTED)" if UNSUPPORTED > default y > help > Speculation attacks against some high-performance processors rely on > @@ -106,7 +106,7 @@ config HARDEN_BRANCH_PREDICTOR > If unsure, say Y. > > config TEE > - bool "Enable TEE mediators support" if EXPERT > + bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED > default n > help > This option enables generic TEE mediators support. It allows guests > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 24868aa6ad..d4e20e9d31 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -102,8 +102,8 @@ config HVM > If unsure, say Y. > > config XEN_SHSTK > - bool "Supervisor Shadow Stacks" > - depends on HAS_AS_CET_SS && EXPERT > + bool "Supervisor Shadow Stacks (UNSUPPORTED)" > + depends on HAS_AS_CET_SS && UNSUPPORTED This one is not following the standard scheme with “if UNSUPPORTED" Cheers Bertrand > default y > ---help--- > Control-flow Enforcement Technology (CET) is a set of features in > @@ -165,7 +165,7 @@ config HVM_FEP > If unsure, say N. > > config TBOOT > - bool "Xen tboot support" if EXPERT > + bool "Xen tboot support (UNSUPPORTED)" if UNSUPPORTED > default y if !PV_SHIM_EXCLUSIVE > select CRYPTO > ---help--- > @@ -251,7 +251,7 @@ config HYPERV_GUEST > endif > > config MEM_SHARING > - bool "Xen memory sharing support" if EXPERT > + bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED > depends on HVM > > endmenu > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index 3e2cf25088..beed507727 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -151,7 +151,7 @@ config KEXEC > If unsure, say Y. > > config EFI_SET_VIRTUAL_ADDRESS_MAP > - bool "EFI: call SetVirtualAddressMap()" if EXPERT > + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED > ---help--- > Call EFI SetVirtualAddressMap() runtime service to setup memory map for > further runtime services. According to UEFI spec, it isn't strictly > @@ -272,7 +272,7 @@ config LATE_HWDOM > If unsure, say N. > > config ARGO > - bool "Argo: hypervisor-mediated interdomain communication" if EXPERT > + bool "Argo: hypervisor-mediated interdomain communication (UNSUPPORTED)" if UNSUPPORTED > ---help--- > Enables a hypercall for domains to ask the hypervisor to perform > data transfer of messages between domains. > diff --git a/xen/common/sched/Kconfig b/xen/common/sched/Kconfig > index 61231aacaa..94c9e20139 100644 > --- a/xen/common/sched/Kconfig > +++ b/xen/common/sched/Kconfig > @@ -15,7 +15,7 @@ config SCHED_CREDIT2 > optimized for lower latency and higher VM density. > > config SCHED_RTDS > - bool "RTDS scheduler support (EXPERIMENTAL)" > + bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED > default y > ---help--- > The RTDS scheduler is a soft and firm real-time scheduler for > @@ -23,14 +23,14 @@ config SCHED_RTDS > in the cloud, and general low-latency workloads. > > config SCHED_ARINC653 > - bool "ARINC653 scheduler support (EXPERIMENTAL)" > + bool "ARINC653 scheduler support (UNSUPPORTED)" if UNSUPPORTED > default DEBUG > ---help--- > The ARINC653 scheduler is a hard real-time scheduler for single > cores, targeted for avionics, drones, and medical devices. > > config SCHED_NULL > - bool "Null scheduler support (EXPERIMENTAL)" > + bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED > default y > ---help--- > The null scheduler is a static, zero overhead scheduler, > -- > 2.17.1 >
On 18.11.2020 09:45, Bertrand Marquis wrote: >> On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -102,8 +102,8 @@ config HVM >> If unsure, say Y. >> >> config XEN_SHSTK >> - bool "Supervisor Shadow Stacks" >> - depends on HAS_AS_CET_SS && EXPERT >> + bool "Supervisor Shadow Stacks (UNSUPPORTED)" >> + depends on HAS_AS_CET_SS && UNSUPPORTED > > This one is not following the standard scheme with “if UNSUPPORTED" There's no standard scheme here: There's one case where the entire option depends on some other setting (e.g. UNSUPPORTED) and another where just the prompt (i.e. giving the user a choice) does. The difference becomes obvious when the option has a default other than "no": Despite the invisible prompt, it may get turned on. In the case here (serving as a good example), "default y" would mean the feature gets turned on when "if UNSUPPORTED" would be added to the prompt and when UNSUPPORTED is itself off. Jan
Hi Jan, > On 18 Nov 2020, at 08:50, Jan Beulich <jbeulich@suse.com> wrote: > > On 18.11.2020 09:45, Bertrand Marquis wrote: >>> On 18 Nov 2020, at 00:50, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -102,8 +102,8 @@ config HVM >>> If unsure, say Y. >>> >>> config XEN_SHSTK >>> - bool "Supervisor Shadow Stacks" >>> - depends on HAS_AS_CET_SS && EXPERT >>> + bool "Supervisor Shadow Stacks (UNSUPPORTED)" >>> + depends on HAS_AS_CET_SS && UNSUPPORTED >> >> This one is not following the standard scheme with “if UNSUPPORTED" > > There's no standard scheme here: There's one case where the entire > option depends on some other setting (e.g. UNSUPPORTED) and another > where just the prompt (i.e. giving the user a choice) does. The > difference becomes obvious when the option has a default other than > "no": Despite the invisible prompt, it may get turned on. In the > case here (serving as a good example), "default y" would mean the > feature gets turned on when "if UNSUPPORTED" would be added to the > prompt and when UNSUPPORTED is itself off. > It makes sense, thanks for the explanation Bertrand
On Wed, 18 Nov 2020, Jan Beulich wrote: > On 18.11.2020 01:50, Stefano Stabellini wrote: > > 1) It is not obvious that "Configure standard Xen features (expert > > users)" is actually the famous EXPERT we keep talking about on xen-devel > > Which can be addressed by simply changing the one prompt line. > > > 2) It is not obvious when we need to enable EXPERT to get a specific > > feature > > > > In particular if you want to enable ACPI support so that you can boot > > Xen on an ACPI platform, you have to enable EXPERT first. But searching > > through the kconfig menu it is really not clear (type '/' and "ACPI"): > > nothing in the description tells you that you need to enable EXPERT to > > get the option. > > And what causes this to be different once you switch to UNSUPPORTED? Two things: firstly, it doesn't and shouldn't take an expert to enable ACPI support, even if ACPI support is experimental. So calling it UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig options changed by this patch. Secondly, this patch is adding "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match it with the option you need to enable. > > So this patch makes things easier by doing two things: > > > > - introduce a new kconfig option UNSUPPORTED which is clearly to enable > > UNSUPPORTED features as defined by SUPPORT.md > > > > - change EXPERT options to UNSUPPORTED where it makes sense: keep > > depending on EXPERT for features made for experts > > > > - tag unsupported features by adding (UNSUPPORTED) to the one-line > > description > > I am, btw, not fully convinced of the need for this redundancy. Wouldn't > it be enough to have just EXPERT as a setting, but varying (<reason>) > tokens in the prompt text? I don't think so, for the same reasons written above: EXPERT should not be gating things like ACPI. Moreover, the advantage of the tag in the oneline prompt is that you can search for an option and figure out that you need to enable UNSUPPORTED. It doesn't work if we use a different tag. Just to get the idea, try to do "make menuconfig" and search for "ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for "UNSUPPORTED" you can find what you need to enable. > > --- a/xen/Kconfig > > +++ b/xen/Kconfig > > @@ -34,8 +34,17 @@ config DEFCONFIG_LIST > > option defconfig_list > > default ARCH_DEFCONFIG > > > > +config UNSUPPORTED > > + bool "Configure UNSUPPORTED features" > > + help > > + This option allows unsupported Xen options to be enabled, which > > I'd recommend against "enabled" - a control may also be there to allow > disabling something. I can change that. > > + includes non-security-supported, experimental, and tech preview > > + features as defined by SUPPORT.md. Xen binaries built with this > > + option enabled are not security supported. > > Overall I'm a little afraid of possible inverse implications: Anything > _not_ dependent upon this option (and in particular anything not > dependent upon any Kconfig control) may be considered supported then. > > Also the last sentence is already present for EXPERT, I am happy to rephrase it. What about: "This option allows certain unsupported Xen options to be changed, which includes non-security-supported, experimental, and tech preview features as defined by SUPPORT.md." > > + default n > > I realize you likely merely copied what EXPERT has, but this "default n" > is pretty pointless and hence would better be omitted imo. OK > > --- a/xen/arch/x86/Kconfig > > +++ b/xen/arch/x86/Kconfig > > @@ -102,8 +102,8 @@ config HVM > > If unsure, say Y. > > > > config XEN_SHSTK > > - bool "Supervisor Shadow Stacks" > > - depends on HAS_AS_CET_SS && EXPERT > > + bool "Supervisor Shadow Stacks (UNSUPPORTED)" > > + depends on HAS_AS_CET_SS && UNSUPPORTED > > default y > > Andrew, I think I did ask on v1 already: Do we need to continue to > consider this unsupported? While perhaps not a change to make right in > this patch, it should perhaps be a pre-patch then to avoid the need to > touch it here. Of course I have no opinion on this. I am happy to do as instructed. > > @@ -165,7 +165,7 @@ config HVM_FEP > > Seeing just the patch context here, I think HVM_FEP may also want > converting. OK > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -151,7 +151,7 @@ config KEXEC > > If unsure, say Y. > > > > config EFI_SET_VIRTUAL_ADDRESS_MAP > > - bool "EFI: call SetVirtualAddressMap()" if EXPERT > > + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED > > I have to admit I'm pretty unsure about what to do with this one. Yeah, similarly to XEN_SHSTK, I don't have an opinion here either. I am happy to change it or leave it as. > > @@ -272,7 +272,7 @@ config LATE_HWDOM > > If unsure, say N. > > > > config ARGO > > - bool "Argo: hypervisor-mediated interdomain communication" if EXPERT > > + bool "Argo: hypervisor-mediated interdomain communication (UNSUPPORTED)" if UNSUPPORTED > > Perhaps better (EXPERIMENTAL)? For this and also the schedulers options below, although it is true that (EXPERIMENTAL) is a more accurate description, then the problem is that it is not easy to match against UNSUPPORTED. In other words, if you search for "ARGO" in make menuconfig and it is marked with (EXPERIMENTAL), it is not obvious that you need to enable UNSUPPORTED to get it as an option. For this reason, I think it is better to use (UNSUPPORTED) both here and below for SCHED_ARINC653 and SCHED_NULL. > > --- a/xen/common/sched/Kconfig > > +++ b/xen/common/sched/Kconfig > > @@ -15,7 +15,7 @@ config SCHED_CREDIT2 > > optimized for lower latency and higher VM density. > > > > config SCHED_RTDS > > - bool "RTDS scheduler support (EXPERIMENTAL)" > > + bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED > > default y > > ---help--- > > The RTDS scheduler is a soft and firm real-time scheduler for > > @@ -23,14 +23,14 @@ config SCHED_RTDS > > in the cloud, and general low-latency workloads. > > > > config SCHED_ARINC653 > > - bool "ARINC653 scheduler support (EXPERIMENTAL)" > > + bool "ARINC653 scheduler support (UNSUPPORTED)" if UNSUPPORTED > > default DEBUG > > ---help--- > > The ARINC653 scheduler is a hard real-time scheduler for single > > cores, targeted for avionics, drones, and medical devices. > > > > config SCHED_NULL > > - bool "Null scheduler support (EXPERIMENTAL)" > > + bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED > > default y > > ---help--- > > The null scheduler is a static, zero overhead scheduler, > > I'd like to see (EXPERIMENTAL) stay everywhere here.
On 18.11.2020 22:00, Stefano Stabellini wrote: > On Wed, 18 Nov 2020, Jan Beulich wrote: >> On 18.11.2020 01:50, Stefano Stabellini wrote: >>> 1) It is not obvious that "Configure standard Xen features (expert >>> users)" is actually the famous EXPERT we keep talking about on xen-devel >> >> Which can be addressed by simply changing the one prompt line. >> >>> 2) It is not obvious when we need to enable EXPERT to get a specific >>> feature >>> >>> In particular if you want to enable ACPI support so that you can boot >>> Xen on an ACPI platform, you have to enable EXPERT first. But searching >>> through the kconfig menu it is really not clear (type '/' and "ACPI"): >>> nothing in the description tells you that you need to enable EXPERT to >>> get the option. >> >> And what causes this to be different once you switch to UNSUPPORTED? > > Two things: firstly, it doesn't and shouldn't take an expert to enable > ACPI support, even if ACPI support is experimental. So calling it > UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig > options changed by this patch. Secondly, this patch is adding > "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match > it with the option you need to enable. There's redundancy here then, which I think is in almost all cases better to avoid. That's first and foremost because the two places can go out of sync. Therefore, if the primary thing is to help "make menuconfig" (which I admit I don't normally use, as it's nothing that gets invoked implicitly by the build process afaict, i.e. one has to actively invoke it), perhaps we should enhance kconfig to attach at least a pre-determined subset of labels to the prompts automatically? And second, also in reply to what you've been saying further down, perhaps we would better go with a hierarchy of controls here, e.g. EXPERT -> EXPERIMENTAL -> UNSUPPORTED? >>> So this patch makes things easier by doing two things: >>> >>> - introduce a new kconfig option UNSUPPORTED which is clearly to enable >>> UNSUPPORTED features as defined by SUPPORT.md >>> >>> - change EXPERT options to UNSUPPORTED where it makes sense: keep >>> depending on EXPERT for features made for experts >>> >>> - tag unsupported features by adding (UNSUPPORTED) to the one-line >>> description >> >> I am, btw, not fully convinced of the need for this redundancy. Wouldn't >> it be enough to have just EXPERT as a setting, but varying (<reason>) >> tokens in the prompt text? > > I don't think so, for the same reasons written above: EXPERT should not > be gating things like ACPI. Different views are possible here, I suppose. Turning on anything that's unsupported requires people to know what they're doing (and be ready to pick up the pieces themselves). I'd consider this to fall under "expert". > Moreover, the advantage of the tag in the > oneline prompt is that you can search for an option and figure out that > you need to enable UNSUPPORTED. It doesn't work if we use a different > tag. Just to get the idea, try to do "make menuconfig" and search for > "ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for > "UNSUPPORTED" you can find what you need to enable. Implying that textual representation and Kconfig option name match, see above. Even a simple spelling mistake would break this model. >>> --- a/xen/Kconfig >>> +++ b/xen/Kconfig >>> @@ -34,8 +34,17 @@ config DEFCONFIG_LIST >>> option defconfig_list >>> default ARCH_DEFCONFIG >>> >>> +config UNSUPPORTED >>> + bool "Configure UNSUPPORTED features" >>> + help >>> + This option allows unsupported Xen options to be enabled, which >> >> I'd recommend against "enabled" - a control may also be there to allow >> disabling something. > > I can change that. > > >>> + includes non-security-supported, experimental, and tech preview >>> + features as defined by SUPPORT.md. Xen binaries built with this >>> + option enabled are not security supported. >> >> Overall I'm a little afraid of possible inverse implications: Anything >> _not_ dependent upon this option (and in particular anything not >> dependent upon any Kconfig control) may be considered supported then. >> >> Also the last sentence is already present for EXPERT, > > I am happy to rephrase it. What about: > > "This option allows certain unsupported Xen options to be changed, which > includes non-security-supported, experimental, and tech preview features > as defined by SUPPORT.md." Sounds better to me. >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -151,7 +151,7 @@ config KEXEC >>> If unsure, say Y. >>> >>> config EFI_SET_VIRTUAL_ADDRESS_MAP >>> - bool "EFI: call SetVirtualAddressMap()" if EXPERT >>> + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED >> >> I have to admit I'm pretty unsure about what to do with this one. > > Yeah, similarly to XEN_SHSTK, I don't have an opinion here either. I am > happy to change it or leave it as. I guess at least for the first cut I'd like to ask to just leave it alone. Jan
On Thu, 19 Nov 2020, Jan Beulich wrote: > On 18.11.2020 22:00, Stefano Stabellini wrote: > > On Wed, 18 Nov 2020, Jan Beulich wrote: > >> On 18.11.2020 01:50, Stefano Stabellini wrote: > >>> 1) It is not obvious that "Configure standard Xen features (expert > >>> users)" is actually the famous EXPERT we keep talking about on xen-devel > >> > >> Which can be addressed by simply changing the one prompt line. > >> > >>> 2) It is not obvious when we need to enable EXPERT to get a specific > >>> feature > >>> > >>> In particular if you want to enable ACPI support so that you can boot > >>> Xen on an ACPI platform, you have to enable EXPERT first. But searching > >>> through the kconfig menu it is really not clear (type '/' and "ACPI"): > >>> nothing in the description tells you that you need to enable EXPERT to > >>> get the option. > >> > >> And what causes this to be different once you switch to UNSUPPORTED? > > > > Two things: firstly, it doesn't and shouldn't take an expert to enable > > ACPI support, even if ACPI support is experimental. So calling it > > UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig > > options changed by this patch. Secondly, this patch is adding > > "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match > > it with the option you need to enable. > > There's redundancy here then, which I think is in almost all cases > better to avoid. That's first and foremost because the two places > can go out of sync. Therefore, if the primary thing is to help > "make menuconfig" (which I admit I don't normally use, as it's > nothing that gets invoked implicitly by the build process afaict, > i.e. one has to actively invoke it), perhaps we should enhance > kconfig to attach at least a pre-determined subset of labels to > the prompts automatically? > > And second, also in reply to what you've been saying further down, > perhaps we would better go with a hierarchy of controls here, e.g. > EXPERT -> EXPERIMENTAL -> UNSUPPORTED? Both these are good ideas worth discussing; somebody else made a similar suggestion some time back. I was already thinking this could be a great candidate for one of the first "working groups" as defined by George during the last community call because the topic is not purely technical: a working group could help getting alignment and make progress faster. We can propose it to George when he is back. However, I don't think we need the working group to make progress on this limited patch that only addresses the lowest hanging fruit. I'd like to suggest to make progress on this patch in its current form, and in parallel start a longer term discussion on how to do something like you suggested above. > >>> So this patch makes things easier by doing two things: > >>> > >>> - introduce a new kconfig option UNSUPPORTED which is clearly to enable > >>> UNSUPPORTED features as defined by SUPPORT.md > >>> > >>> - change EXPERT options to UNSUPPORTED where it makes sense: keep > >>> depending on EXPERT for features made for experts > >>> > >>> - tag unsupported features by adding (UNSUPPORTED) to the one-line > >>> description > >> > >> I am, btw, not fully convinced of the need for this redundancy. Wouldn't > >> it be enough to have just EXPERT as a setting, but varying (<reason>) > >> tokens in the prompt text? > > > > I don't think so, for the same reasons written above: EXPERT should not > > be gating things like ACPI. > > Different views are possible here, I suppose. Turning on anything > that's unsupported requires people to know what they're doing (and > be ready to pick up the pieces themselves). I'd consider this to > fall under "expert". For some features it is as you wrote, but it is not true in all cases. Let's take ACPI as an example: it doesn't take an expert to enable it and if it breaks you are in no worse situation than if you didn't, because if you don't enable it you can't boot at all on the platform. Also, that's not how EXPERT is commonly used in other projects. Typically EXPERT is to enable advanced debugging features, and I have been told recently that it is confusing the way we use it in Xen. These are the two things that I would like to fix as soon as possible. > > Moreover, the advantage of the tag in the > > oneline prompt is that you can search for an option and figure out that > > you need to enable UNSUPPORTED. It doesn't work if we use a different > > tag. Just to get the idea, try to do "make menuconfig" and search for > > "ARGO" with '/': you'll see "(UNSUPPORTED)". Then, if you search for > > "UNSUPPORTED" you can find what you need to enable. > > Implying that textual representation and Kconfig option name match, > see above. Even a simple spelling mistake would break this model. True, a spelling mistake would cause problems, but we do reviews, and we can make sure there are no spelling mistakes in this patch. > >>> --- a/xen/Kconfig > >>> +++ b/xen/Kconfig > >>> @@ -34,8 +34,17 @@ config DEFCONFIG_LIST > >>> option defconfig_list > >>> default ARCH_DEFCONFIG > >>> > >>> +config UNSUPPORTED > >>> + bool "Configure UNSUPPORTED features" > >>> + help > >>> + This option allows unsupported Xen options to be enabled, which > >> > >> I'd recommend against "enabled" - a control may also be there to allow > >> disabling something. > > > > I can change that. > > > > > >>> + includes non-security-supported, experimental, and tech preview > >>> + features as defined by SUPPORT.md. Xen binaries built with this > >>> + option enabled are not security supported. > >> > >> Overall I'm a little afraid of possible inverse implications: Anything > >> _not_ dependent upon this option (and in particular anything not > >> dependent upon any Kconfig control) may be considered supported then. > >> > >> Also the last sentence is already present for EXPERT, > > > > I am happy to rephrase it. What about: > > > > "This option allows certain unsupported Xen options to be changed, which > > includes non-security-supported, experimental, and tech preview features > > as defined by SUPPORT.md." > > Sounds better to me. I'll use it > >>> --- a/xen/common/Kconfig > >>> +++ b/xen/common/Kconfig > >>> @@ -151,7 +151,7 @@ config KEXEC > >>> If unsure, say Y. > >>> > >>> config EFI_SET_VIRTUAL_ADDRESS_MAP > >>> - bool "EFI: call SetVirtualAddressMap()" if EXPERT > >>> + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED > >> > >> I have to admit I'm pretty unsure about what to do with this one. > > > > Yeah, similarly to XEN_SHSTK, I don't have an opinion here either. I am > > happy to change it or leave it as. > > I guess at least for the first cut I'd like to ask to just leave it > alone. OK
On 19.11.2020 22:40, Stefano Stabellini wrote: > On Thu, 19 Nov 2020, Jan Beulich wrote: >> On 18.11.2020 22:00, Stefano Stabellini wrote: >>> On Wed, 18 Nov 2020, Jan Beulich wrote: >>>> On 18.11.2020 01:50, Stefano Stabellini wrote: >>>>> 1) It is not obvious that "Configure standard Xen features (expert >>>>> users)" is actually the famous EXPERT we keep talking about on xen-devel >>>> >>>> Which can be addressed by simply changing the one prompt line. >>>> >>>>> 2) It is not obvious when we need to enable EXPERT to get a specific >>>>> feature >>>>> >>>>> In particular if you want to enable ACPI support so that you can boot >>>>> Xen on an ACPI platform, you have to enable EXPERT first. But searching >>>>> through the kconfig menu it is really not clear (type '/' and "ACPI"): >>>>> nothing in the description tells you that you need to enable EXPERT to >>>>> get the option. >>>> >>>> And what causes this to be different once you switch to UNSUPPORTED? >>> >>> Two things: firstly, it doesn't and shouldn't take an expert to enable >>> ACPI support, even if ACPI support is experimental. So calling it >>> UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig >>> options changed by this patch. Secondly, this patch is adding >>> "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match >>> it with the option you need to enable. >> >> There's redundancy here then, which I think is in almost all cases >> better to avoid. That's first and foremost because the two places >> can go out of sync. Therefore, if the primary thing is to help >> "make menuconfig" (which I admit I don't normally use, as it's >> nothing that gets invoked implicitly by the build process afaict, >> i.e. one has to actively invoke it), perhaps we should enhance >> kconfig to attach at least a pre-determined subset of labels to >> the prompts automatically? >> >> And second, also in reply to what you've been saying further down, >> perhaps we would better go with a hierarchy of controls here, e.g. >> EXPERT -> EXPERIMENTAL -> UNSUPPORTED? > > Both these are good ideas worth discussing; somebody else made a similar > suggestion some time back. I was already thinking this could be a great > candidate for one of the first "working groups" as defined by George > during the last community call because the topic is not purely > technical: a working group could help getting alignment and make > progress faster. We can propose it to George when he is back. > > However, I don't think we need the working group to make progress on > this limited patch that only addresses the lowest hanging fruit. > > I'd like to suggest to make progress on this patch in its current form, > and in parallel start a longer term discussion on how to do something > like you suggested above. Okay, I guess I can accept this. So FAOD I'm not objecting to the change (with some suitable adjustments, as discussed), but I'm then also not going to be the one to ack it. Nevertheless I'd like to point out that doing such a partial solution may end up adding confusion rather than reducing it. Much depends on how exactly consumers interpret what we hand to them. Jan
On Fri, 20 Nov 2020, Jan Beulich wrote: > On 19.11.2020 22:40, Stefano Stabellini wrote: > > On Thu, 19 Nov 2020, Jan Beulich wrote: > >> On 18.11.2020 22:00, Stefano Stabellini wrote: > >>> On Wed, 18 Nov 2020, Jan Beulich wrote: > >>>> On 18.11.2020 01:50, Stefano Stabellini wrote: > >>>>> 1) It is not obvious that "Configure standard Xen features (expert > >>>>> users)" is actually the famous EXPERT we keep talking about on xen-devel > >>>> > >>>> Which can be addressed by simply changing the one prompt line. > >>>> > >>>>> 2) It is not obvious when we need to enable EXPERT to get a specific > >>>>> feature > >>>>> > >>>>> In particular if you want to enable ACPI support so that you can boot > >>>>> Xen on an ACPI platform, you have to enable EXPERT first. But searching > >>>>> through the kconfig menu it is really not clear (type '/' and "ACPI"): > >>>>> nothing in the description tells you that you need to enable EXPERT to > >>>>> get the option. > >>>> > >>>> And what causes this to be different once you switch to UNSUPPORTED? > >>> > >>> Two things: firstly, it doesn't and shouldn't take an expert to enable > >>> ACPI support, even if ACPI support is experimental. So calling it > >>> UNSUPPORTED helps a lot. This is particularly relevant to the ARM Kconfig > >>> options changed by this patch. Secondly, this patch is adding > >>> "(UNSUPPORTED)" in the oneline prompt so that it becomes easy to match > >>> it with the option you need to enable. > >> > >> There's redundancy here then, which I think is in almost all cases > >> better to avoid. That's first and foremost because the two places > >> can go out of sync. Therefore, if the primary thing is to help > >> "make menuconfig" (which I admit I don't normally use, as it's > >> nothing that gets invoked implicitly by the build process afaict, > >> i.e. one has to actively invoke it), perhaps we should enhance > >> kconfig to attach at least a pre-determined subset of labels to > >> the prompts automatically? > >> > >> And second, also in reply to what you've been saying further down, > >> perhaps we would better go with a hierarchy of controls here, e.g. > >> EXPERT -> EXPERIMENTAL -> UNSUPPORTED? > > > > Both these are good ideas worth discussing; somebody else made a similar > > suggestion some time back. I was already thinking this could be a great > > candidate for one of the first "working groups" as defined by George > > during the last community call because the topic is not purely > > technical: a working group could help getting alignment and make > > progress faster. We can propose it to George when he is back. > > > > However, I don't think we need the working group to make progress on > > this limited patch that only addresses the lowest hanging fruit. > > > > I'd like to suggest to make progress on this patch in its current form, > > and in parallel start a longer term discussion on how to do something > > like you suggested above. > > Okay, I guess I can accept this. So FAOD I'm not objecting to the > change (with some suitable adjustments, as discussed), but I'm > then also not going to be the one to ack it. Nevertheless I'd like > to point out that doing such a partial solution may end up adding > confusion rather than reducing it. Much depends on how exactly > consumers interpret what we hand to them. Thank you Jan. I'll clarify the patch and address your comments. I'll also try to get the attention of one of the other maintainers for the ack.
diff --git a/xen/Kconfig b/xen/Kconfig index 34c318bfa2..59400c4788 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -34,8 +34,17 @@ config DEFCONFIG_LIST option defconfig_list default ARCH_DEFCONFIG +config UNSUPPORTED + bool "Configure UNSUPPORTED features" + help + This option allows unsupported Xen options to be enabled, which + includes non-security-supported, experimental, and tech preview + features as defined by SUPPORT.md. Xen binaries built with this + option enabled are not security supported. + default n + config EXPERT - bool "Configure standard Xen features (expert users)" + bool "Configure EXPERT features" help This option allows certain base Xen options and settings to be disabled or tweaked. This is for specialized environments diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index f938dd21bd..5981e7380d 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -32,7 +32,7 @@ menu "Architecture Features" source "arch/Kconfig" config ACPI - bool "ACPI (Advanced Configuration and Power Interface) Support" if EXPERT + bool "ACPI (Advanced Configuration and Power Interface) Support (UNSUPPORTED)" if UNSUPPORTED depends on ARM_64 ---help--- @@ -49,7 +49,7 @@ config GICV3 If unsure, say Y config HAS_ITS - bool "GICv3 ITS MSI controller support" if EXPERT + bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED depends on GICV3 && !NEW_VGIC config HVM @@ -79,7 +79,7 @@ config SBSA_VUART_CONSOLE SBSA Generic UART implements a subset of ARM PL011 UART. config ARM_SSBD - bool "Speculative Store Bypass Disable" if EXPERT + bool "Speculative Store Bypass Disable (UNSUPPORTED)" if UNSUPPORTED depends on HAS_ALTERNATIVE default y help @@ -89,7 +89,7 @@ config ARM_SSBD If unsure, say Y. config HARDEN_BRANCH_PREDICTOR - bool "Harden the branch predictor against aliasing attacks" if EXPERT + bool "Harden the branch predictor against aliasing attacks (UNSUPPORTED)" if UNSUPPORTED default y help Speculation attacks against some high-performance processors rely on @@ -106,7 +106,7 @@ config HARDEN_BRANCH_PREDICTOR If unsure, say Y. config TEE - bool "Enable TEE mediators support" if EXPERT + bool "Enable TEE mediators support (UNSUPPORTED)" if UNSUPPORTED default n help This option enables generic TEE mediators support. It allows guests diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 24868aa6ad..d4e20e9d31 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -102,8 +102,8 @@ config HVM If unsure, say Y. config XEN_SHSTK - bool "Supervisor Shadow Stacks" - depends on HAS_AS_CET_SS && EXPERT + bool "Supervisor Shadow Stacks (UNSUPPORTED)" + depends on HAS_AS_CET_SS && UNSUPPORTED default y ---help--- Control-flow Enforcement Technology (CET) is a set of features in @@ -165,7 +165,7 @@ config HVM_FEP If unsure, say N. config TBOOT - bool "Xen tboot support" if EXPERT + bool "Xen tboot support (UNSUPPORTED)" if UNSUPPORTED default y if !PV_SHIM_EXCLUSIVE select CRYPTO ---help--- @@ -251,7 +251,7 @@ config HYPERV_GUEST endif config MEM_SHARING - bool "Xen memory sharing support" if EXPERT + bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED depends on HVM endmenu diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 3e2cf25088..beed507727 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -151,7 +151,7 @@ config KEXEC If unsure, say Y. config EFI_SET_VIRTUAL_ADDRESS_MAP - bool "EFI: call SetVirtualAddressMap()" if EXPERT + bool "EFI: call SetVirtualAddressMap() (UNSUPPORTED)" if UNSUPPORTED ---help--- Call EFI SetVirtualAddressMap() runtime service to setup memory map for further runtime services. According to UEFI spec, it isn't strictly @@ -272,7 +272,7 @@ config LATE_HWDOM If unsure, say N. config ARGO - bool "Argo: hypervisor-mediated interdomain communication" if EXPERT + bool "Argo: hypervisor-mediated interdomain communication (UNSUPPORTED)" if UNSUPPORTED ---help--- Enables a hypercall for domains to ask the hypervisor to perform data transfer of messages between domains. diff --git a/xen/common/sched/Kconfig b/xen/common/sched/Kconfig index 61231aacaa..94c9e20139 100644 --- a/xen/common/sched/Kconfig +++ b/xen/common/sched/Kconfig @@ -15,7 +15,7 @@ config SCHED_CREDIT2 optimized for lower latency and higher VM density. config SCHED_RTDS - bool "RTDS scheduler support (EXPERIMENTAL)" + bool "RTDS scheduler support (UNSUPPORTED)" if UNSUPPORTED default y ---help--- The RTDS scheduler is a soft and firm real-time scheduler for @@ -23,14 +23,14 @@ config SCHED_RTDS in the cloud, and general low-latency workloads. config SCHED_ARINC653 - bool "ARINC653 scheduler support (EXPERIMENTAL)" + bool "ARINC653 scheduler support (UNSUPPORTED)" if UNSUPPORTED default DEBUG ---help--- The ARINC653 scheduler is a hard real-time scheduler for single cores, targeted for avionics, drones, and medical devices. config SCHED_NULL - bool "Null scheduler support (EXPERIMENTAL)" + bool "Null scheduler support (UNSUPPORTED)" if UNSUPPORTED default y ---help--- The null scheduler is a static, zero overhead scheduler,