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

[Rich Persaud <persaur@xxxxxxxxx>]

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.

Rich

> And ideally we would
> also tag features like ACPI as UNSUPPORTED as I suggested above.
>