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

[Jürgen Groß <jgross@xxxxxxxx>]

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.

Does it really make sense to make a central control and information
interface conditional?

I'd like at least a second opinion on that topic.

> > > > --- 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?
Hmm, that's an option, yes.


Juergen