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

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



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



 


Rackspace

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