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

From: Jan Beulich <jbeulich@suse.com>
To: "Jürgen Groß" <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v7 08/12] xen: add /buildinfo/config entry to hypervisor filesystem
Date: Fri, 3 Apr 2020 17:33:47 +0200	[thread overview]
Message-ID: <f7d1f3aa-3a7e-fcb2-3163-5e67756e8452@suse.com> (raw)
In-Reply-To: <b9ddd1fb-d868-bb69-3b6b-27531beda2fa@suse.com>

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.

>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -389,6 +389,16 @@ static HYPFS_STRING_INIT(compile_date, "compile_date");
>>>   static HYPFS_STRING_INIT(compile_domain, "compile_domain");
>>>   static HYPFS_STRING_INIT(extra, "extra");
>>>   +#ifdef CONFIG_HYPFS_CONFIG
>>> +static struct hypfs_entry_leaf config = {
>>> +    .e.type = XEN_HYPFS_TYPE_STRING,
>>> +    .e.encoding = XEN_HYPFS_ENC_GZIP,
>>> +    .e.name = "config",
>>> +    .e.read = hypfs_read_leaf,
>>> +    .content = &xen_config_data
>>> +};
>>> +#endif
>>
>> Would be really good if no open-coded instantiations like this
>> one would ever have to appear, i.e. if suitable macros were
>> available. What's preventing use of one of the available ones
>> here?
> 
> Right now it is the only case for a non plain encoding. The alternative
> would have been to either specify the encoding explicitly at all other
> node definitions, or to have a macro for this single instance.

Or set the encoding alongside e.size in the init function?

Jan