Re: [PATCH v7 08/12] xen: add /buildinfo/config entry to hypervisor filesystem

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Apr 27, 2020, at 4:40 PM, Jürgen Groß <jgross@xxxxxxxx> wrote:
> 
> Stefano,
> 
> On 06.04.20 14:29, Jan Beulich wrote:
>> On 03.04.2020 17:45, Jürgen Groß wrote:
>>> On 03.04.20 17:33, Jan Beulich wrote:
>>>> On 03.04.2020 17:12, Jürgen Groß wrote:
>>>>> On 03.04.20 16:31, Jan Beulich wrote:
>>>>>> On 02.04.2020 17:46, Juergen Gross wrote:
>>>>>>> --- a/xen/common/Kconfig
>>>>>>> +++ b/xen/common/Kconfig
>>>>>>> @@ -353,6 +353,16 @@ config DOM0_MEM
>>>>>>>            Leave empty if you are not sure what to specify.
>>>>>>>    +config HYPFS_CONFIG
>>>>>>> +    bool "Provide hypervisor .config via hypfs entry"
>>>>>>> +    default y
>>>>>> 
>>>>>> My initial reaction was to ask for "depends on HYPFS", but then
>>>>>> I noticed the earlier patch doesn't introduce such. Am I
>>>>>> mis-remembering that it was agreed to make the whole thing
>>>>>> possible to disable at least in EXPERT mode?
>>>>> 
>>>>> No, I don't remember that agreement.
>>>>> 
>>>>> And TBH I'm not sure this is a good idea, as that would at once make the
>>>>> plan to replace at least some sysctl and/or domctl interfaces impossible
>>>>> (like e.g. the last 3 patches of the series are doing already), or at
>>>>> least would tie the related functionality to CONFIG_HYPFS.
>>>> 
>>>> I think that would be fine - that's what config setting are for.
>>>> Someone caring about space may not care about runtime setting of
>>>> parameters.
>>> 
>>> So right now it would start with a plain hypfs available or not.
>>> 
>>> The next step would be in patch 12 to tell the user he will lose the
>>> capability of setting runtime parameters.
>>> 
>>> Another planned extension would be to control per-cpupool settings,
>>> which would the go away (possibly functionality being unconditionally
>>> available today).
>>> 
>>> Next would be the lack of being able to control per-domain mitigations
>>> like XPTI or L1TF, which I'd like to add.
>>> 
>>> Another thing I wanted to add is some debugging stuff (e.g. to switch
>>> lock profiling using hypfs).
>>> 
>>> And the list will go on.
>> Understood.
>>> Does it really make sense to make a central control and information
>>> interface conditional?
>> None of the above may be of interest to e.g. embedded use cases.
>>> I'd like at least a second opinion on that topic.
>> Yes, further opinions would surely help.
> 
> Any opinion on making hypfs configurable?
> 
> It would be quite some code churn and I want to avoid it in case you
> see no benefit in configuring it out for embedded.

Just to reply with what I said on IRC:

First of all, if it were free, I think that having CONFIG_HYPFS would be 
valuable.  Sub-options should be made available on a case-by-case basis; I 
think CONFIG_HYPFS_CONFIG would be valuable, but I don’t see any point in 
having CONFIG_HYPFS_RUNTIME_PARAMETER.

However,  Juergen seems to think it would require a lot of churn to the current 
series.  I don’t quite understand why; but supposing that’s so: In general, the 
people who want a feature should be the ones who do the work to make it.  I 
think it would be perfectly reasonable for Juergen to say, “This is a lot of 
work that I don’t have time to do before the 4.14 release; if people want to be 
able to disable this feature, they can post their own patches.”  (It’s also 
perfectly reasonable for him to do the work himself just to be helpful.)

The discussion about CONFIG_EXPERT is sort of the same.  If we have 
CONFIG_HYPFS, it would obviously be more valuable if it were *not* in 
CONFIG_EXPERT, so that people could change it while still being security 
supported.

If Jan is OK with it simply being outside CONFIG_EXPERT, then great.  But if he 
insists on some kind of testing for it to be outside of CONFIG_EXPERT, then 
again, the people who want it to be security supported should be the ones who 
do the work to make it happen.

Hope that makes sense. :-)

 -George