Re: [RFC PATCH] xen: EXPERT clean-up

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.11.2020 02:14, Stefano Stabellini wrote:
> On Wed, 4 Nov 2020, Bertrand Marquis wrote:
>>> On 4 Nov 2020, at 07:34, Jan Beulich <jbeulich@xxxxxxxx> 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