[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.