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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Stefano,

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

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

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

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

Cheers
Bertrand

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