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