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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> 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.

Cheers
Bertrand