Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:20, Jan Beulich wrote:
>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>> domains or cpupools this patch series adds support for generic text
>>>> based parameter parsing.
>>>>
>>>> Parameters are defined via new macros similar to those of boot
>>>> parameters. Parsing of parameter strings is done via the already
>>>> existing boot parameter parsing function which is extended a little
>>>> bit.
>>>>
>>>> Parameter settings can either be specified in configuration files of
>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>
>>> Without having looked at any of the patches yet (not even their
>>> descriptions) I'm still wondering what the benefit of textual parameters
>>> really is: Just like "binary" ones, they become part of the public
>>> interface, and hence subsequently can't be changed any more or
>>> less than the ones we currently have (in particular, anything valid in
>>> a guest config file will imo need to remain to be valid and meaningful
>>> down the road).
>>
>> So lets look what would be needed for adding something like the
>> per-domain xpti parameter using binary interfaces:
>>
>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>   interface version
>> 2 adding the logic to domctl handling
>> 3 adding libxc support
>> 4 adding libxl support
>> 5 adding a new xl sub-command
>> 6 adding domain config support
>> 7 adding documentation
>>
>> With my approach only 2 (in a modified form, parameter handling instead
>> of domctl, but comparable in the needed effort) and 7 are needed.
>>
>> So once the framework is in place it is _much_ easier to add new
>> features.
> 
> All the above would hold if the individual options were expressed as
> e.g. flags in an extensible bit vector.

Who would translate the new option into a bit vector? This would be the
tools (libxc/libxl/xl), so those need to be modified for each new bit.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 12:32 PM, Juergen Gross wrote:
> On 18/09/18 13:20, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>> domains or cpupools this patch series adds support for generic text
>>>>> based parameter parsing.
>>>>>
>>>>> Parameters are defined via new macros similar to those of boot
>>>>> parameters. Parsing of parameter strings is done via the already
>>>>> existing boot parameter parsing function which is extended a little
>>>>> bit.
>>>>>
>>>>> Parameter settings can either be specified in configuration files of
>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>
>>>> Without having looked at any of the patches yet (not even their
>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>> really is: Just like "binary" ones, they become part of the public
>>>> interface, and hence subsequently can't be changed any more or
>>>> less than the ones we currently have (in particular, anything valid in
>>>> a guest config file will imo need to remain to be valid and meaningful
>>>> down the road).
>>>
>>> So lets look what would be needed for adding something like the
>>> per-domain xpti parameter using binary interfaces:
>>>
>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>   interface version
>>> 2 adding the logic to domctl handling
>>> 3 adding libxc support
>>> 4 adding libxl support
>>> 5 adding a new xl sub-command
>>> 6 adding domain config support
>>> 7 adding documentation
>>>
>>> With my approach only 2 (in a modified form, parameter handling instead
>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>
>>> So once the framework is in place it is _much_ easier to add new
>>> features.
>>
>> All the above would hold if the individual options were expressed as
>> e.g. flags in an extensible bit vector.
> 
> Who would translate the new option into a bit vector? This would be the
> tools (libxc/libxl/xl), so those need to be modified for each new bit.

A bit vector would only allow on/off; it wouldn't allow you to set
numeric parameters, for example.

I like the idea of being able to add configuration parameters without
having a huge amount of boilerplate; and also of being able to backport
parameters like xpti without having to worry so much about compatibility.

But I'm not a fan of the idea of using a "string blob" to accomplish
that.  It's convenient for the exact use case you seem to be
contemplating: having a user add the string into the xl config file, and
having nobody but the hypervisor interpret it.  But what about tools
that may want to set that parameter?  Or tools that want to query the
parameter, or "introspect" on the domain settings or whatever?  Now they
have to have a bunch of code to generate and parse the string code.

Could we have a reasonably generic structure / union, with a parameter
number, that we could pass in instead?  Something like:

struct domain_parameter {
  int param_num;
  int val;
}

And then have a list somewhere of string values -> parameter numbers
that callers could use to translate strings into values?

That way the above list would look like:

1. Add new parameter in Xen
2. Add a string name -> parameter number in a header somewhere
3. Add a libxl #define with that parameter number

You'd have to recompile xl against the new header, but you were probably
going to do that anyway.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Tue, Sep 18, 2018 at 02:25:04PM +0100, George Dunlap wrote:
> On 09/18/2018 12:32 PM, Juergen Gross wrote:
> > On 18/09/18 13:20, Jan Beulich wrote:
> >>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
> >>> On 18/09/18 12:32, Jan Beulich wrote:
> >>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
> >>>>> Instead of using binary hypervisor interfaces for new parameters of
> >>>>> domains or cpupools this patch series adds support for generic text
> >>>>> based parameter parsing.
> >>>>>
> >>>>> Parameters are defined via new macros similar to those of boot
> >>>>> parameters. Parsing of parameter strings is done via the already
> >>>>> existing boot parameter parsing function which is extended a little
> >>>>> bit.
> >>>>>
> >>>>> Parameter settings can either be specified in configuration files of
> >>>>> domains or cpupools, or they can be set via new xl sub-commands.
> >>>>
> >>>> Without having looked at any of the patches yet (not even their
> >>>> descriptions) I'm still wondering what the benefit of textual parameters
> >>>> really is: Just like "binary" ones, they become part of the public
> >>>> interface, and hence subsequently can't be changed any more or
> >>>> less than the ones we currently have (in particular, anything valid in
> >>>> a guest config file will imo need to remain to be valid and meaningful
> >>>> down the road).
> >>>
> >>> So lets look what would be needed for adding something like the
> >>> per-domain xpti parameter using binary interfaces:
> >>>
> >>> 1 an extension of some domctl interface, maybe bumping of the domctl
> >>>   interface version
> >>> 2 adding the logic to domctl handling
> >>> 3 adding libxc support
> >>> 4 adding libxl support
> >>> 5 adding a new xl sub-command
> >>> 6 adding domain config support
> >>> 7 adding documentation
> >>>
> >>> With my approach only 2 (in a modified form, parameter handling instead
> >>> of domctl, but comparable in the needed effort) and 7 are needed.
> >>>
> >>> So once the framework is in place it is _much_ easier to add new
> >>> features.
> >>
> >> All the above would hold if the individual options were expressed as
> >> e.g. flags in an extensible bit vector.
> > 
> > Who would translate the new option into a bit vector? This would be the
> > tools (libxc/libxl/xl), so those need to be modified for each new bit.
> 
> A bit vector would only allow on/off; it wouldn't allow you to set
> numeric parameters, for example.
> 
> I like the idea of being able to add configuration parameters without
> having a huge amount of boilerplate; and also of being able to backport
> parameters like xpti without having to worry so much about compatibility.
> 
> But I'm not a fan of the idea of using a "string blob" to accomplish
> that.  It's convenient for the exact use case you seem to be
> contemplating: having a user add the string into the xl config file, and
> having nobody but the hypervisor interpret it.  But what about tools
> that may want to set that parameter?  Or tools that want to query the
> parameter, or "introspect" on the domain settings or whatever?  Now they
> have to have a bunch of code to generate and parse the string code.
> 

Having the ability to query parameters is a must. Suppose you change
some settings while the domain is running, in order to re-create domain
with the same parameter (migration) there must be a way for toolstack to
query the current settings of that domain. I think most if not all
information is retrieved from xen using binary interface.

Furthermore, if the string blob is not stored in xen, and there isn't a
binary interface for *setting* parameters, toolstack will have to
translate the retrieved binary information into strings again.

I'm not picky about formats, but please make get and set interfaces
symmetric (use the same representation).

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 15:25, George Dunlap wrote:
> On 09/18/2018 12:32 PM, Juergen Gross wrote:
>> On 18/09/18 13:20, Jan Beulich wrote:
>>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>> based parameter parsing.
>>>>>>
>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>> existing boot parameter parsing function which is extended a little
>>>>>> bit.
>>>>>>
>>>>>> Parameter settings can either be specified in configuration files of
>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>
>>>>> Without having looked at any of the patches yet (not even their
>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>> really is: Just like "binary" ones, they become part of the public
>>>>> interface, and hence subsequently can't be changed any more or
>>>>> less than the ones we currently have (in particular, anything valid in
>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>> down the road).
>>>>
>>>> So lets look what would be needed for adding something like the
>>>> per-domain xpti parameter using binary interfaces:
>>>>
>>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>>   interface version
>>>> 2 adding the logic to domctl handling
>>>> 3 adding libxc support
>>>> 4 adding libxl support
>>>> 5 adding a new xl sub-command
>>>> 6 adding domain config support
>>>> 7 adding documentation
>>>>
>>>> With my approach only 2 (in a modified form, parameter handling instead
>>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>>
>>>> So once the framework is in place it is _much_ easier to add new
>>>> features.
>>>
>>> All the above would hold if the individual options were expressed as
>>> e.g. flags in an extensible bit vector.
>>
>> Who would translate the new option into a bit vector? This would be the
>> tools (libxc/libxl/xl), so those need to be modified for each new bit.
> 
> A bit vector would only allow on/off; it wouldn't allow you to set
> numeric parameters, for example.
> 
> I like the idea of being able to add configuration parameters without
> having a huge amount of boilerplate; and also of being able to backport
> parameters like xpti without having to worry so much about compatibility.
> 
> But I'm not a fan of the idea of using a "string blob" to accomplish
> that.  It's convenient for the exact use case you seem to be
> contemplating: having a user add the string into the xl config file, and
> having nobody but the hypervisor interpret it.  But what about tools
> that may want to set that parameter?  Or tools that want to query the
> parameter, or "introspect" on the domain settings or whatever?  Now they
> have to have a bunch of code to generate and parse the string code.
> 
> Could we have a reasonably generic structure / union, with a parameter
> number, that we could pass in instead?  Something like:
> 
> struct domain_parameter {
>   int param_num;
>   int val;
> }
> 
> And then have a list somewhere of string values -> parameter numbers
> that callers could use to translate strings into values?
> 
> That way the above list would look like:
> 
> 1. Add new parameter in Xen
> 2. Add a string name -> parameter number in a header somewhere
> 3. Add a libxl #define with that parameter number
> 
> You'd have to recompile xl against the new header, but you were probably
> going to do that anyway.

The string variant is much more flexible.

It is easy possible to e.g. add a per-domain trace parameter to specify
rather complex trace instrumentations. Doing something like that via a
struct based interface is in the best case complicated.

Another advantage of the string based variant is that you don't need a
central header. You can define the parameters where you are implementing
them. No need to expand switch statements and headers, just a local
definition and maybe a handler function.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 02:36 PM, Juergen Gross wrote:
> On 18/09/18 15:25, George Dunlap wrote:
>> On 09/18/2018 12:32 PM, Juergen Gross wrote:
>>> On 18/09/18 13:20, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>> based parameter parsing.
>>>>>>>
>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>> bit.
>>>>>>>
>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>
>>>>>> Without having looked at any of the patches yet (not even their
>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>> down the road).
>>>>>
>>>>> So lets look what would be needed for adding something like the
>>>>> per-domain xpti parameter using binary interfaces:
>>>>>
>>>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>>>   interface version
>>>>> 2 adding the logic to domctl handling
>>>>> 3 adding libxc support
>>>>> 4 adding libxl support
>>>>> 5 adding a new xl sub-command
>>>>> 6 adding domain config support
>>>>> 7 adding documentation
>>>>>
>>>>> With my approach only 2 (in a modified form, parameter handling instead
>>>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>>>
>>>>> So once the framework is in place it is _much_ easier to add new
>>>>> features.
>>>>
>>>> All the above would hold if the individual options were expressed as
>>>> e.g. flags in an extensible bit vector.
>>>
>>> Who would translate the new option into a bit vector? This would be the
>>> tools (libxc/libxl/xl), so those need to be modified for each new bit.
>>
>> A bit vector would only allow on/off; it wouldn't allow you to set
>> numeric parameters, for example.
>>
>> I like the idea of being able to add configuration parameters without
>> having a huge amount of boilerplate; and also of being able to backport
>> parameters like xpti without having to worry so much about compatibility.
>>
>> But I'm not a fan of the idea of using a "string blob" to accomplish
>> that.  It's convenient for the exact use case you seem to be
>> contemplating: having a user add the string into the xl config file, and
>> having nobody but the hypervisor interpret it.  But what about tools
>> that may want to set that parameter?  Or tools that want to query the
>> parameter, or "introspect" on the domain settings or whatever?  Now they
>> have to have a bunch of code to generate and parse the string code.
>>
>> Could we have a reasonably generic structure / union, with a parameter
>> number, that we could pass in instead?  Something like:
>>
>> struct domain_parameter {
>>   int param_num;
>>   int val;
>> }
>>
>> And then have a list somewhere of string values -> parameter numbers
>> that callers could use to translate strings into values?
>>
>> That way the above list would look like:
>>
>> 1. Add new parameter in Xen
>> 2. Add a string name -> parameter number in a header somewhere
>> 3. Add a libxl #define with that parameter number
>>
>> You'd have to recompile xl against the new header, but you were probably
>> going to do that anyway.
> 
> The string variant is much more flexible.
> 
> It is easy possible to e.g. add a per-domain trace parameter to specify
> rather complex trace instrumentations. Doing something like that via a
> struct based interface is in the best case complicated.

...or, for instance, specifying the runqueue layout of a cpupool (for
schedulers like credit2 which allow such things).  Yes, that is true;
but probably a very niche case.

> Another advantage of the string based variant is that you don't need a
> central header. You can define the parameters where you are implementing
> them. No need to expand switch statements and headers, just a local
> definition and maybe a handler function.

I don't see the lack of central header as a big advantage -- how hard is
it to add a single line to a list somewhere?

*Not* having a language-level construct around (either an enum or a
#define) means that programs can't take advantage of the preprocessor /
type system to catch bugs; someone calling
libxl_domain_param("xptl=off"); won't get a compile-time error, only a
run-time one; someone calling
libxl_domain_param(LIBXL_DOMAIN_PARAM_XPTL, 0) will.

I'm not completely opposed to the "string blob" idea, but it would be
nice if at least for the common case of simple boolean / integer values,
we could avoid having a string blob.

What about

struct parameter {
  int param_number;
  union {
    int val;
    char special[]
  };
}

Or something like that?  That would give flexibility for special cases
like mentioned above, while allowing the common case to avoid special
encoding / decoding &c.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3055 bytes --]

[Hey, is it me/my mailer, or threading is weird for this series? :-O]

On Tue, 2018-09-18 at 14:57 +0100, George Dunlap wrote:
> On 09/18/2018 02:36 PM, Juergen Gross wrote:
> > 
> > The string variant is much more flexible.
> > 
> > It is easy possible to e.g. add a per-domain trace parameter to
> > specify
> > rather complex trace instrumentations. Doing something like that
> > via a
> > struct based interface is in the best case complicated.
> 
> ...or, for instance, specifying the runqueue layout of a cpupool (for
> schedulers like credit2 which allow such things).  Yes, that is true;
> but probably a very niche case.
> 
Exactly. As another example, I want to follow up on this:

https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg01644.html

More precisely, I want to add a per-cpupool "smt=[on|off|force]" (or
cpupool-smt, or something like that), with the following meaning:
- smt=on: cpus that are hyperthread siblings can be added to the 
  cpupool. Adding a cpu whose sibling is in another pool would fail;
- smt=off: only one cpu per core can be added to the cpupool. Adding a 
  cpu whose sibling is already in the pool would fail. Adding a cpu 
  whose sibling is in another pool would also fail;
- smt=force: (and I particularly dislike the name, but let's ignore it 
  for now) any cpu can be added to any pool

What I was putting together was something along the lines of:

https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg01552.html

And then there will be the support for having a line like this, in a
cpupool config file:

 smt = "off"

With this approach, instead, there will have to be a line like this in
there:

 parameters = "smt=off"

Is that right?

And when we will also have the support for, say, per-cpupool runqueue
arrangement for Credit2, it will look like this:

 parameters = "credit2_runqueues=socket smt=off"

Right again?

If yes, I think I'm fine with this.

The per-cpupool parameters case is, I think, probably less
controversial than the per-domain case. In facte, the parsing of, e.g.,
credit2_runqueue=, happens in Xen already. And most (if not all) of the
scheduling parameters that we allow as command line options, also make
sense as per-cpupool parameters, so... :-)

I'm not sure where to draw the line, assuming we even want to draw one.
I.e., if we take this approach and these patches, _any_ new parameter
will have to be handled like this? If no, how do we decide which ones
better use the "hypervisor centric" string blobs, and which ones better
use the current "toolstack centric" one? About this (and especially for
per-domain params), I've much less clear ideas.

But as far as per-cpupools parameters are concerned, I do like this.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 15:57, George Dunlap wrote:
> On 09/18/2018 02:36 PM, Juergen Gross wrote:
>> On 18/09/18 15:25, George Dunlap wrote:
>>> On 09/18/2018 12:32 PM, Juergen Gross wrote:
>>>> On 18/09/18 13:20, Jan Beulich wrote:
>>>>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>>> based parameter parsing.
>>>>>>>>
>>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>>> bit.
>>>>>>>>
>>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>>
>>>>>>> Without having looked at any of the patches yet (not even their
>>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>>> down the road).
>>>>>>
>>>>>> So lets look what would be needed for adding something like the
>>>>>> per-domain xpti parameter using binary interfaces:
>>>>>>
>>>>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>>>>   interface version
>>>>>> 2 adding the logic to domctl handling
>>>>>> 3 adding libxc support
>>>>>> 4 adding libxl support
>>>>>> 5 adding a new xl sub-command
>>>>>> 6 adding domain config support
>>>>>> 7 adding documentation
>>>>>>
>>>>>> With my approach only 2 (in a modified form, parameter handling instead
>>>>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>>>>
>>>>>> So once the framework is in place it is _much_ easier to add new
>>>>>> features.
>>>>>
>>>>> All the above would hold if the individual options were expressed as
>>>>> e.g. flags in an extensible bit vector.
>>>>
>>>> Who would translate the new option into a bit vector? This would be the
>>>> tools (libxc/libxl/xl), so those need to be modified for each new bit.
>>>
>>> A bit vector would only allow on/off; it wouldn't allow you to set
>>> numeric parameters, for example.
>>>
>>> I like the idea of being able to add configuration parameters without
>>> having a huge amount of boilerplate; and also of being able to backport
>>> parameters like xpti without having to worry so much about compatibility.
>>>
>>> But I'm not a fan of the idea of using a "string blob" to accomplish
>>> that.  It's convenient for the exact use case you seem to be
>>> contemplating: having a user add the string into the xl config file, and
>>> having nobody but the hypervisor interpret it.  But what about tools
>>> that may want to set that parameter?  Or tools that want to query the
>>> parameter, or "introspect" on the domain settings or whatever?  Now they
>>> have to have a bunch of code to generate and parse the string code.
>>>
>>> Could we have a reasonably generic structure / union, with a parameter
>>> number, that we could pass in instead?  Something like:
>>>
>>> struct domain_parameter {
>>>   int param_num;
>>>   int val;
>>> }
>>>
>>> And then have a list somewhere of string values -> parameter numbers
>>> that callers could use to translate strings into values?
>>>
>>> That way the above list would look like:
>>>
>>> 1. Add new parameter in Xen
>>> 2. Add a string name -> parameter number in a header somewhere
>>> 3. Add a libxl #define with that parameter number
>>>
>>> You'd have to recompile xl against the new header, but you were probably
>>> going to do that anyway.
>>
>> The string variant is much more flexible.
>>
>> It is easy possible to e.g. add a per-domain trace parameter to specify
>> rather complex trace instrumentations. Doing something like that via a
>> struct based interface is in the best case complicated.
> 
> ...or, for instance, specifying the runqueue layout of a cpupool (for
> schedulers like credit2 which allow such things).  Yes, that is true;
> but probably a very niche case.
> 
>> Another advantage of the string based variant is that you don't need a
>> central header. You can define the parameters where you are implementing
>> them. No need to expand switch statements and headers, just a local
>> definition and maybe a handler function.
> 
> I don't see the lack of central header as a big advantage -- how hard is
> it to add a single line to a list somewhere?

That's not very hard.

You need additional entries for connecting the domctl with the parameter
setting:

/* central header: */
#define PARAM_XPTI 13

/* domctl handling: */
switch (param) {
case PARAM_XPTI: ret = do_param_xpti_setting(value);
    break;

/* pv-dom header: */
int do_param_xpti_setting(int val);

/* pv-dom handler: */
int do_param_xpti_setting(int val)
{
...
}

So you need to touch at least four files in the hypervisor, plus the
parsing added in xl.

The string-only variant needs:

/* pv-dom handler: */
static int do_param_xpti_setting(...)
{
...
}
custom_domain_param("xpti", ...);

And that's all. See the difference?

> 
> *Not* having a language-level construct around (either an enum or a
> #define) means that programs can't take advantage of the preprocessor /
> type system to catch bugs; someone calling
> libxl_domain_param("xptl=off"); won't get a compile-time error, only a
> run-time one; someone calling
> libxl_domain_param(LIBXL_DOMAIN_PARAM_XPTL, 0) will.

Yes. We should be very careful which parameters should be supported
that way. I wouldn't want to do standard domain configuration stuff
(e.g. number of vcpus, memory size, ...) this way, just additional
stuff like xpti, l1tf mitigation, maybe test interfaces like tracing,
setting of special thresholds.

> I'm not completely opposed to the "string blob" idea, but it would be
> nice if at least for the common case of simple boolean / integer values,
> we could avoid having a string blob.
> 
> What about
> 
> struct parameter {
>   int param_number;
>   union {
>     int val;
>     char special[]
>   };
> }

I think this would be a good interface for replacing current domctl
or sysctl interfaces which are used implicitly e.g. during domain
creation. This would bring us a step closer to stable sysctl and domctl
interfaces.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 03:57 PM, Juergen Gross wrote:
> On 18/09/18 15:57, George Dunlap wrote:
>> On 09/18/2018 02:36 PM, Juergen Gross wrote:
>>> On 18/09/18 15:25, George Dunlap wrote:
>>>> On 09/18/2018 12:32 PM, Juergen Gross wrote:
>>>>> On 18/09/18 13:20, Jan Beulich wrote:
>>>>>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>>>> based parameter parsing.
>>>>>>>>>
>>>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>>>> bit.
>>>>>>>>>
>>>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>>>
>>>>>>>> Without having looked at any of the patches yet (not even their
>>>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>>>> down the road).
>>>>>>>
>>>>>>> So lets look what would be needed for adding something like the
>>>>>>> per-domain xpti parameter using binary interfaces:
>>>>>>>
>>>>>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>>>>>   interface version
>>>>>>> 2 adding the logic to domctl handling
>>>>>>> 3 adding libxc support
>>>>>>> 4 adding libxl support
>>>>>>> 5 adding a new xl sub-command
>>>>>>> 6 adding domain config support
>>>>>>> 7 adding documentation
>>>>>>>
>>>>>>> With my approach only 2 (in a modified form, parameter handling instead
>>>>>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>>>>>
>>>>>>> So once the framework is in place it is _much_ easier to add new
>>>>>>> features.
>>>>>>
>>>>>> All the above would hold if the individual options were expressed as
>>>>>> e.g. flags in an extensible bit vector.
>>>>>
>>>>> Who would translate the new option into a bit vector? This would be the
>>>>> tools (libxc/libxl/xl), so those need to be modified for each new bit.
>>>>
>>>> A bit vector would only allow on/off; it wouldn't allow you to set
>>>> numeric parameters, for example.
>>>>
>>>> I like the idea of being able to add configuration parameters without
>>>> having a huge amount of boilerplate; and also of being able to backport
>>>> parameters like xpti without having to worry so much about compatibility.
>>>>
>>>> But I'm not a fan of the idea of using a "string blob" to accomplish
>>>> that.  It's convenient for the exact use case you seem to be
>>>> contemplating: having a user add the string into the xl config file, and
>>>> having nobody but the hypervisor interpret it.  But what about tools
>>>> that may want to set that parameter?  Or tools that want to query the
>>>> parameter, or "introspect" on the domain settings or whatever?  Now they
>>>> have to have a bunch of code to generate and parse the string code.
>>>>
>>>> Could we have a reasonably generic structure / union, with a parameter
>>>> number, that we could pass in instead?  Something like:
>>>>
>>>> struct domain_parameter {
>>>>   int param_num;
>>>>   int val;
>>>> }
>>>>
>>>> And then have a list somewhere of string values -> parameter numbers
>>>> that callers could use to translate strings into values?
>>>>
>>>> That way the above list would look like:
>>>>
>>>> 1. Add new parameter in Xen
>>>> 2. Add a string name -> parameter number in a header somewhere
>>>> 3. Add a libxl #define with that parameter number
>>>>
>>>> You'd have to recompile xl against the new header, but you were probably
>>>> going to do that anyway.
>>>
>>> The string variant is much more flexible.
>>>
>>> It is easy possible to e.g. add a per-domain trace parameter to specify
>>> rather complex trace instrumentations. Doing something like that via a
>>> struct based interface is in the best case complicated.
>>
>> ...or, for instance, specifying the runqueue layout of a cpupool (for
>> schedulers like credit2 which allow such things).  Yes, that is true;
>> but probably a very niche case.
>>
>>> Another advantage of the string based variant is that you don't need a
>>> central header. You can define the parameters where you are implementing
>>> them. No need to expand switch statements and headers, just a local
>>> definition and maybe a handler function.
>>
>> I don't see the lack of central header as a big advantage -- how hard is
>> it to add a single line to a list somewhere?
> 
> That's not very hard.
> 
> You need additional entries for connecting the domctl with the parameter
> setting:
> 
> /* central header: */
> #define PARAM_XPTI 13
> 
> /* domctl handling: */
> switch (param) {
> case PARAM_XPTI: ret = do_param_xpti_setting(value);
>     break;
> 
> /* pv-dom header: */
> int do_param_xpti_setting(int val);
> 
> /* pv-dom handler: */
> int do_param_xpti_setting(int val)
> {
> ...
> }
> 
> So you need to touch at least four files in the hypervisor, plus the
> parsing added in xl.
> 
> The string-only variant needs:
> 
> /* pv-dom handler: */
> static int do_param_xpti_setting(...)
> {
> ...
> }
> custom_domain_param("xpti", ...);
> 
> And that's all. See the difference?

I don't think we need the function prototype or a switch statement.

params.h:
#define PARAM_XPTI 13

[later]
   [PARAM_XPTI]="xpti"

pv-dom.c:

static int do_param_xpti_setting(...)
{
...
}
custom_domain_param(XPTI, do_param_xpti_setting);

Multiplexing over the parameter values would be done the same way as
multiplexing over the string values.

Sure it's a tiny bit more convenient not to have to edit params.h; but
having a parameter number, and for most values an integer value, makes
doing things with these programmaticaly in the toolstack a lot easier
and more robust.

Like I said, I'm not 100% opposed to "string blobs" if other people
think it's a good idea; I just think we can do a bit better.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 26/09/18 17:10, Dario Faggioli wrote:
> [Hey, is it me/my mailer, or threading is weird for this series? :-O]
> 
> On Tue, 2018-09-18 at 14:57 +0100, George Dunlap wrote:
>> On 09/18/2018 02:36 PM, Juergen Gross wrote:
>>>
>>> The string variant is much more flexible.
>>>
>>> It is easy possible to e.g. add a per-domain trace parameter to
>>> specify
>>> rather complex trace instrumentations. Doing something like that
>>> via a
>>> struct based interface is in the best case complicated.
>>
>> ...or, for instance, specifying the runqueue layout of a cpupool (for
>> schedulers like credit2 which allow such things).  Yes, that is true;
>> but probably a very niche case.
>>
> Exactly. As another example, I want to follow up on this:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2018-08/msg01644.html
> 
> More precisely, I want to add a per-cpupool "smt=[on|off|force]" (or
> cpupool-smt, or something like that), with the following meaning:
> - smt=on: cpus that are hyperthread siblings can be added to the 
>   cpupool. Adding a cpu whose sibling is in another pool would fail;
> - smt=off: only one cpu per core can be added to the cpupool. Adding a 
>   cpu whose sibling is already in the pool would fail. Adding a cpu 
>   whose sibling is in another pool would also fail;
> - smt=force: (and I particularly dislike the name, but let's ignore it 
>   for now) any cpu can be added to any pool
> 
> What I was putting together was something along the lines of:
> 
> https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg01552.html
> 
> And then there will be the support for having a line like this, in a
> cpupool config file:
> 
>  smt = "off"
> 
> With this approach, instead, there will have to be a line like this in
> there:
> 
>  parameters = "smt=off"
> 
> Is that right?
> 
> And when we will also have the support for, say, per-cpupool runqueue
> arrangement for Credit2, it will look like this:
> 
>  parameters = "credit2_runqueues=socket smt=off"
> 
> Right again?

Right now, yes :-)

I could imagine to modify config file parsing to treat all unknown
key-value pairs as parameters, so you could use "smt=off" again. This
would have the effect to filter out all unsupported lines in the config
file.

> If yes, I think I'm fine with this.
> 
> The per-cpupool parameters case is, I think, probably less
> controversial than the per-domain case. In facte, the parsing of, e.g.,
> credit2_runqueue=, happens in Xen already. And most (if not all) of the
> scheduling parameters that we allow as command line options, also make
> sense as per-cpupool parameters, so... :-)
> 
> I'm not sure where to draw the line, assuming we even want to draw one.

All parameters _needed_ at cpupool creation time (e.g. scheduler) can't
be handled this way. And I don't think it is a good idea to handle cpu
assignments that way.

> I.e., if we take this approach and these patches, _any_ new parameter
> will have to be handled like this? If no, how do we decide which ones
> better use the "hypervisor centric" string blobs, and which ones better
> use the current "toolstack centric" one? About this (and especially for
> per-domain params), I've much less clear ideas.
> 
> But as far as per-cpupools parameters are concerned, I do like this.

Thanks.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 20/09/18 18:06, Wei Liu wrote:
> On Wed, Sep 19, 2018 at 07:58:50PM +0200, Juergen Gross wrote:
>>
>> Did you look into the patches, especially patch 10? The parameters set
>> are all stored in domain config via libxl__arch_domain_save_config().
> 
> No, I didn't.
> 
> I think the general idea of what you do in patch 10 should work. However
> I want to comment on the implementation.
> 
> It appears that the implementation in patch 10 concatenates the new
> settings to the old ones. It is not very nice imo.
> 
> If for the life time of the domain you set X times the same parameter
> you get a string of foo=bar1 foo=bar2 in the saved config file.
> 
> There is probably a simple solution: make the parameter list in IDL a
> key value list. You then update the list accordingly.

The problem with that approach are parameters with sub-parameters:

par=sub1=no,sub2=yes
par=sub2=yes

would remove the sub1=no setting.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
> On 20/09/18 18:06, Wei Liu wrote:
> > On Wed, Sep 19, 2018 at 07:58:50PM +0200, Juergen Gross wrote:
> >>
> >> Did you look into the patches, especially patch 10? The parameters set
> >> are all stored in domain config via libxl__arch_domain_save_config().
> > 
> > No, I didn't.
> > 
> > I think the general idea of what you do in patch 10 should work. However
> > I want to comment on the implementation.
> > 
> > It appears that the implementation in patch 10 concatenates the new
> > settings to the old ones. It is not very nice imo.
> > 
> > If for the life time of the domain you set X times the same parameter
> > you get a string of foo=bar1 foo=bar2 in the saved config file.
> > 
> > There is probably a simple solution: make the parameter list in IDL a
> > key value list. You then update the list accordingly.
> 
> The problem with that approach are parameters with sub-parameters:
> 
> par=sub1=no,sub2=yes
> par=sub2=yes

That means the value type of the top level key value list should ideally
be another key value list. I do notice the limitation in the key value
list type: the value can only be string.

There is another way to solve this: further parse the sub-parameters.
This doesn't require any parameter specific knowledge and there are
already functions to split strings.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1487 bytes --]

On Fri, 2018-09-21 at 09:52 +0100, Wei Liu wrote:
> On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
> > On 20/09/18 18:06, Wei Liu wrote:
> > > 
> > > It appears that the implementation in patch 10 concatenates the
> > > new
> > > settings to the old ones. It is not very nice imo.
> > > 
> > > If for the life time of the domain you set X times the same
> > > parameter
> > > you get a string of foo=bar1 foo=bar2 in the saved config file.
> > > 
> > > There is probably a simple solution: make the parameter list in
> > > IDL a
> > > key value list. You then update the list accordingly.
> > 
> > The problem with that approach are parameters with sub-parameters:
> > 
> > par=sub1=no,sub2=yes
> > par=sub2=yes
> 
> There is another way to solve this: further parse the sub-parameters.
> This doesn't require any parameter specific knowledge and there are
> already functions to split strings.
> 
I'm not sure whether we're saying the same thing or not, but can't we,
when parameter 'foo', which has been set to 'bar1' already, is being
set to 'bar2', search d_config.b_info.parameters for the substring
containing 'foo=bar1', replace it with 'foo=bar2', and save d_config
again?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Wed, Sep 26, 2018 at 07:30:38PM +0200, Dario Faggioli wrote:
> On Fri, 2018-09-21 at 09:52 +0100, Wei Liu wrote:
> > On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
> > > On 20/09/18 18:06, Wei Liu wrote:
> > > > 
> > > > It appears that the implementation in patch 10 concatenates the
> > > > new
> > > > settings to the old ones. It is not very nice imo.
> > > > 
> > > > If for the life time of the domain you set X times the same
> > > > parameter
> > > > you get a string of foo=bar1 foo=bar2 in the saved config file.
> > > > 
> > > > There is probably a simple solution: make the parameter list in
> > > > IDL a
> > > > key value list. You then update the list accordingly.
> > > 
> > > The problem with that approach are parameters with sub-parameters:
> > > 
> > > par=sub1=no,sub2=yes
> > > par=sub2=yes
> > 
> > There is another way to solve this: further parse the sub-parameters.
> > This doesn't require any parameter specific knowledge and there are
> > already functions to split strings.
> > 
> I'm not sure whether we're saying the same thing or not, but can't we,
> when parameter 'foo', which has been set to 'bar1' already, is being
> set to 'bar2', search d_config.b_info.parameters for the substring
> containing 'foo=bar1', replace it with 'foo=bar2', and save d_config
> again?

This can do, too. It is still parsing so the amount of work needed is
more or less the same to me.

Wei.

> 
> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Software Engineer @ SUSE https://www.suse.com/



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 03/10/2018 13:00, Wei Liu wrote:
> On Wed, Sep 26, 2018 at 07:30:38PM +0200, Dario Faggioli wrote:
>> On Fri, 2018-09-21 at 09:52 +0100, Wei Liu wrote:
>>> On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
>>>> On 20/09/18 18:06, Wei Liu wrote:
>>>>>
>>>>> It appears that the implementation in patch 10 concatenates the
>>>>> new
>>>>> settings to the old ones. It is not very nice imo.
>>>>>
>>>>> If for the life time of the domain you set X times the same
>>>>> parameter
>>>>> you get a string of foo=bar1 foo=bar2 in the saved config file.
>>>>>
>>>>> There is probably a simple solution: make the parameter list in
>>>>> IDL a
>>>>> key value list. You then update the list accordingly.
>>>>
>>>> The problem with that approach are parameters with sub-parameters:
>>>>
>>>> par=sub1=no,sub2=yes
>>>> par=sub2=yes
>>>
>>> There is another way to solve this: further parse the sub-parameters.
>>> This doesn't require any parameter specific knowledge and there are
>>> already functions to split strings.
>>>
>> I'm not sure whether we're saying the same thing or not, but can't we,
>> when parameter 'foo', which has been set to 'bar1' already, is being
>> set to 'bar2', search d_config.b_info.parameters for the substring
>> containing 'foo=bar1', replace it with 'foo=bar2', and save d_config
>> again?
> 
> This can do, too. It is still parsing so the amount of work needed is
> more or less the same to me.

No, this isn't always correct. Think of console=tty0 console=hvc0 in
the linux kernel. You don't want hvc0 to replace tty0, but to have
both.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Wed, Oct 03, 2018 at 01:07:30PM +0200, Juergen Gross wrote:
> On 03/10/2018 13:00, Wei Liu wrote:
> > On Wed, Sep 26, 2018 at 07:30:38PM +0200, Dario Faggioli wrote:
> >> On Fri, 2018-09-21 at 09:52 +0100, Wei Liu wrote:
> >>> On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
> >>>> On 20/09/18 18:06, Wei Liu wrote:
> >>>>>
> >>>>> It appears that the implementation in patch 10 concatenates the
> >>>>> new
> >>>>> settings to the old ones. It is not very nice imo.
> >>>>>
> >>>>> If for the life time of the domain you set X times the same
> >>>>> parameter
> >>>>> you get a string of foo=bar1 foo=bar2 in the saved config file.
> >>>>>
> >>>>> There is probably a simple solution: make the parameter list in
> >>>>> IDL a
> >>>>> key value list. You then update the list accordingly.
> >>>>
> >>>> The problem with that approach are parameters with sub-parameters:
> >>>>
> >>>> par=sub1=no,sub2=yes
> >>>> par=sub2=yes
> >>>
> >>> There is another way to solve this: further parse the sub-parameters.
> >>> This doesn't require any parameter specific knowledge and there are
> >>> already functions to split strings.
> >>>
> >> I'm not sure whether we're saying the same thing or not, but can't we,
> >> when parameter 'foo', which has been set to 'bar1' already, is being
> >> set to 'bar2', search d_config.b_info.parameters for the substring
> >> containing 'foo=bar1', replace it with 'foo=bar2', and save d_config
> >> again?
> > 
> > This can do, too. It is still parsing so the amount of work needed is
> > more or less the same to me.
> 
> No, this isn't always correct. Think of console=tty0 console=hvc0 in
> the linux kernel. You don't want hvc0 to replace tty0, but to have
> both.

Good point.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

Instead of using binary hypervisor interfaces for new parameters of
domains or cpupools this patch series adds support for generic text
based parameter parsing.

Parameters are defined via new macros similar to those of boot
parameters. Parsing of parameter strings is done via the already
existing boot parameter parsing function which is extended a little
bit.

Parameter settings can either be specified in configuration files of
domains or cpupools, or they can be set via new xl sub-commands.

As an example how the infrastructure is being used a per-domain
parameter for switching xpti on a per-domain basis is added in the
last patch of the series.

Juergen Gross (12):
  xen: use macros for filling parameter definition blocks
  xen: use a structure to define parsing parameters
  xen: add support for parameter scopes
  xen: add a generic flags field to parameter definitions
  xen: add hypercall interfaces for domain and cpupool parameter setting
  xen: add domain specific parameter support
  xen: add domain specific parameter support
  tools/libxc: add per domain/cpupool parameter support
  tools/xl: add support for setting generic per-cpupool parameters
  tools/xl: add support for setting generic per-domain parameters
  x86: add domain type flags for domain parameters
  x86/xpti: add per-domain parameter for controlling xpti

 docs/man/xl.cfg.pod.5.in            |  22 +++++++
 docs/man/xl.pod.1.in                |  25 ++++++++
 docs/man/xlcpupool.cfg.pod.5        |  12 ++++
 tools/flask/policy/modules/dom0.te  |   2 +-
 tools/libxc/include/xenctrl.h       |  18 ++++++
 tools/libxc/xc_cpupool.c            |  23 +++++++
 tools/libxc/xc_domain.c             |  23 +++++++
 tools/libxc/xc_misc.c               |   4 +-
 tools/libxl/libxl.h                 |  18 ++++++
 tools/libxl/libxl_cpupool.c         |  16 +++++
 tools/libxl/libxl_create.c          |  10 +++
 tools/libxl/libxl_domain.c          |  47 +++++++++++++++
 tools/libxl/libxl_types.idl         |   1 +
 tools/xl/xl.h                       |   2 +
 tools/xl/xl_cmdtable.c              |  11 ++++
 tools/xl/xl_cpupool.c               |  39 ++++++++++++
 tools/xl/xl_parse.c                 |   3 +
 tools/xl/xl_vmcontrol.c             |  25 ++++++++
 xen/arch/arm/domain.c               |   5 ++
 xen/arch/x86/domain.c               |   9 +++
 xen/arch/x86/pv/domain.c            |  37 ++++++++++++
 xen/common/cpupool.c                |  10 +++
 xen/common/domain.c                 |  10 +++
 xen/common/domctl.c                 |  46 ++++++++++++++
 xen/common/kernel.c                 |  93 ++++++++++++++++++++++------
 xen/common/sysctl.c                 |  28 ++++++++-
 xen/include/asm-x86/domain.h        |   6 ++
 xen/include/public/domctl.h         |  20 +++++-
 xen/include/public/sysctl.h         |   8 ++-
 xen/include/xen/domain.h            |   3 +
 xen/include/xen/init.h              | 117 ++++++++++++++++++++++++------------
 xen/include/xen/lib.h               |   3 +
 xen/include/xen/sched.h             |   1 +
 xen/xsm/flask/hooks.c               |   3 +
 xen/xsm/flask/policy/access_vectors |   2 +
 35 files changed, 640 insertions(+), 62 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 01/12] xen: use macros for filling parameter definition blocks

[Juergen Gross]

Define macros for filling struct kernel_param when defining parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/xen/init.h | 58 +++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 38 deletions(-)

diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index db06c76fdf..d0b07b3d39 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -101,72 +101,54 @@ extern const struct kernel_param __param_start[], __param_end[];
     __attribute__((__aligned__(1))) char
 #define __kparam          __param(__initsetup)
 
+#define def_custom_param(_name, _func) \
+    { .name = _name, \
+      .type = OPT_CUSTOM, \
+      .par.func = _func }
+#define def_var_param(_name, _type, _var) \
+    { .name = _name, \
+      .type = _type, \
+      .len = sizeof(_var), \
+      .par.var = &_var }
+
 #define custom_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        { .name = __setup_str_##_var, \
-          .type = OPT_CUSTOM, \
-          .par.func = _var }
+        def_custom_param(__setup_str_##_var, _var)
 #define boolean_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        { .name = __setup_str_##_var, \
-          .type = OPT_BOOL, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(__setup_str_##_var, OPT_BOOL, _var)
 #define integer_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        { .name = __setup_str_##_var, \
-          .type = OPT_UINT, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(__setup_str_##_var, OPT_UINT, _var)
 #define size_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        { .name = __setup_str_##_var, \
-          .type = OPT_SIZE, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(__setup_str_##_var, OPT_SIZE, _var)
 #define string_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        { .name = __setup_str_##_var, \
-          .type = OPT_STR, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(__setup_str_##_var, OPT_STR, _var)
 
 #define __rtparam         __param(__dataparam)
 
 #define custom_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-      { .name = _name, \
-          .type = OPT_CUSTOM, \
-          .par.func = _var }
+        def_custom_param(_name, _var)
 #define boolean_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        { .name = _name, \
-          .type = OPT_BOOL, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(_name, OPT_BOOL, _var)
 #define integer_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        { .name = _name, \
-          .type = OPT_UINT, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(_name, OPT_UINT, _var)
 #define size_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        { .name = _name, \
-          .type = OPT_SIZE, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(_name, OPT_SIZE, _var)
 #define string_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        { .name = _name, \
-          .type = OPT_STR, \
-          .len = sizeof(_var), \
-          .par.var = &_var }
+        def_var_param(_name, OPT_STR, _var)
 
 #define custom_runtime_param(_name, _var) \
     custom_param(_name, _var); \
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/12] xen: use macros for filling parameter definition blocks

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 502 bytes --]

On Tue, 2018-09-18 at 08:02 +0200, Juergen Gross wrote:
> Define macros for filling struct kernel_param when defining
> parameters.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 01/12] xen: use macros for filling parameter definition blocks

[Jan Beulich]

>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
> Define macros for filling struct kernel_param when defining parameters.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/include/xen/init.h | 58 +++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 38 deletions(-)

The diffstat perhaps makes this a worthwhile patch even independent
of the rest of the series (i.e. if and how much of a re-work will be needed).

> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -101,72 +101,54 @@ extern const struct kernel_param __param_start[], 
> __param_end[];
>      __attribute__((__aligned__(1))) char
>  #define __kparam          __param(__initsetup)
>  
> +#define def_custom_param(_name, _func) \
> +    { .name = _name, \
> +      .type = OPT_CUSTOM, \
> +      .par.func = _func }
> +#define def_var_param(_name, _type, _var) \
> +    { .name = _name, \
> +      .type = _type, \
> +      .len = sizeof(_var), \
> +      .par.var = &_var }

May I ask that at least in the new macros you introduce you do
away with the bogus leading underscores? Personally I have no
issue with single character macro parameter names (I say that
because simply dropping the underscores will obviously not work).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 02/12] xen: use a structure to define parsing parameters

[Juergen Gross]

Instead of passing the start and end pointers of the parameter
definition array to the parsing function use a struct containing that
information. This will allow to add other parameters to control the
parsing later.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/kernel.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 5766a0f784..a7e82453c2 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -18,6 +18,11 @@
 
 #ifndef COMPAT
 
+struct parse_data {
+    const struct kernel_param *start;
+    const struct kernel_param *end;
+};
+
 enum system_state system_state = SYS_STATE_early_boot;
 
 xen_commandline_t saved_cmdline;
@@ -52,8 +57,7 @@ static int assign_integer_param(const struct kernel_param *param, uint64_t val)
     return 0;
 }
 
-static int parse_params(const char *cmdline, const struct kernel_param *start,
-                        const struct kernel_param *end)
+static int parse_params(const char *cmdline, const struct parse_data *data)
 {
     char opt[128], *optval, *optkey, *q;
     const char *p = cmdline, *key;
@@ -100,7 +104,7 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
 
         rc = 0;
         found = false;
-        for ( param = start; param < end; param++ )
+        for ( param = data->start; param < data->end; param++ )
         {
             int rctmp;
             const char *s;
@@ -187,14 +191,24 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
     return final_rc;
 }
 
+static const struct parse_data boot_parse_data = {
+    .start  = __setup_start,
+    .end    = __setup_end,
+};
+
+static const struct parse_data runtime_parse_data = {
+    .start  = __param_start,
+    .end    = __param_end,
+};
+
 static void __init _cmdline_parse(const char *cmdline)
 {
-    parse_params(cmdline, __setup_start, __setup_end);
+    parse_params(cmdline, &boot_parse_data);
 }
 
 int runtime_parse(const char *line)
 {
-    return parse_params(line, __param_start, __param_end);
+    return parse_params(line, &runtime_parse_data);
 }
 
 /**
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 02/12] xen: use a structure to define parsing parameters

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 652 bytes --]

On Tue, 2018-09-18 at 08:02 +0200, Juergen Gross wrote:
> Instead of passing the start and end pointers of the parameter
> definition array to the parsing function use a struct containing that
> information. This will allow to add other parameters to control the
> parsing later.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 02/12] xen: use a structure to define parsing parameters

[Jan Beulich]

>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
> @@ -187,14 +191,24 @@ static int parse_params(const char *cmdline, const struct kernel_param *start,
>      return final_rc;
>  }
>  
> +static const struct parse_data boot_parse_data = {

__initconstrel

With this
Acked-by: Jan Beulich <jbeulich@suse.com>
provided it will continue to be needed (i.e. I wouldn't want to see this
go in on its own).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 03/12] xen: add support for parameter scopes

[Juergen Gross]

In order to prepare support for domain-specific parameters similar to
boot- and runtime-parameters of the hypervisor, add generic support for
those to the parameter parsing framework.

To each parameter definition a scope (global only for now) is added.
Global parameters keep using pointers to the related variables in the
struct kernel_param of their definitions, while those of narrower scope
will use the offset in the structure (e.g. struct domain) for that
purpose.

This scheme requires passing the pointer to the structure instance to
the parsing function and the parameter type specific subfunctions, of
course.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/kernel.c    | 38 ++++++++++++++++++++++++--------------
 xen/include/xen/init.h | 32 ++++++++++++++++++++------------
 2 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index a7e82453c2..f7f4e0dbff 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -19,6 +19,7 @@
 #ifndef COMPAT
 
 struct parse_data {
+    enum param_scope scope;
     const struct kernel_param *start;
     const struct kernel_param *end;
 };
@@ -28,27 +29,28 @@ enum system_state system_state = SYS_STATE_early_boot;
 xen_commandline_t saved_cmdline;
 static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE;
 
-static int assign_integer_param(const struct kernel_param *param, uint64_t val)
+static int assign_integer_param(const struct kernel_param *param,
+                                void *ptr, uint64_t val)
 {
     switch ( param->len )
     {
     case sizeof(uint8_t):
         if ( val > UINT8_MAX && val < (uint64_t)INT8_MIN )
             return -EOVERFLOW;
-        *(uint8_t *)param->par.var = val;
+        *(uint8_t *)ptr = val;
         break;
     case sizeof(uint16_t):
         if ( val > UINT16_MAX && val < (uint64_t)INT16_MIN )
             return -EOVERFLOW;
-        *(uint16_t *)param->par.var = val;
+        *(uint16_t *)ptr = val;
         break;
     case sizeof(uint32_t):
         if ( val > UINT32_MAX && val < (uint64_t)INT32_MIN )
             return -EOVERFLOW;
-        *(uint32_t *)param->par.var = val;
+        *(uint32_t *)ptr = val;
         break;
     case sizeof(uint64_t):
-        *(uint64_t *)param->par.var = val;
+        *(uint64_t *)ptr = val;
         break;
     default:
         BUG();
@@ -57,7 +59,8 @@ static int assign_integer_param(const struct kernel_param *param, uint64_t val)
     return 0;
 }
 
-static int parse_params(const char *cmdline, const struct parse_data *data)
+static int parse_params(const char *cmdline, const struct parse_data *data,
+                        void *instance)
 {
     char opt[128], *optval, *optkey, *q;
     const char *p = cmdline, *key;
@@ -108,6 +111,10 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
         {
             int rctmp;
             const char *s;
+            void *ptr;
+
+            if ( param->scope != data->scope )
+                continue;
 
             if ( strcmp(param->name, optkey) )
             {
@@ -117,7 +124,7 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
                 {
                     found = true;
                     optval[-1] = '=';
-                    rctmp = param->par.func(q);
+                    rctmp = param->par.call(q, instance);
                     optval[-1] = '\0';
                     if ( !rc )
                         rc = rctmp;
@@ -127,14 +134,15 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
 
             rctmp = 0;
             found = true;
+            ptr = (void *)((uint64_t)param->par.var + (uint64_t)instance);
             switch ( param->type )
             {
             case OPT_STR:
-                strlcpy(param->par.var, optval, param->len);
+                strlcpy(ptr, optval, param->len);
                 break;
             case OPT_UINT:
                 rctmp = assign_integer_param(
-                    param,
+                    param, ptr,
                     simple_strtoll(optval, &s, 0));
                 if ( *s )
                     rctmp = -EINVAL;
@@ -146,11 +154,11 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
                 if ( !rctmp )
                     bool_assert = !bool_assert;
                 rctmp = 0;
-                assign_integer_param(param, bool_assert);
+                assign_integer_param(param, ptr, bool_assert);
                 break;
             case OPT_SIZE:
                 rctmp = assign_integer_param(
-                    param,
+                    param, ptr,
                     parse_size_and_unit(optval, &s));
                 if ( *s )
                     rctmp = -EINVAL;
@@ -164,7 +172,7 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
                     safe_strcpy(opt, "no");
                     optval = opt;
                 }
-                rctmp = param->par.func(optval);
+                rctmp = param->par.call(optval, instance);
                 break;
             default:
                 BUG();
@@ -192,23 +200,25 @@ static int parse_params(const char *cmdline, const struct parse_data *data)
 }
 
 static const struct parse_data boot_parse_data = {
+    .scope  = SCOPE_GLOBAL,
     .start  = __setup_start,
     .end    = __setup_end,
 };
 
 static const struct parse_data runtime_parse_data = {
+    .scope  = SCOPE_GLOBAL,
     .start  = __param_start,
     .end    = __param_end,
 };
 
 static void __init _cmdline_parse(const char *cmdline)
 {
-    parse_params(cmdline, &boot_parse_data);
+    parse_params(cmdline, &boot_parse_data, NULL);
 }
 
 int runtime_parse(const char *line)
 {
-    return parse_params(line, &runtime_parse_data);
+    return parse_params(line, &runtime_parse_data, NULL);
 }
 
 /**
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index d0b07b3d39..b04534c11a 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -73,6 +73,10 @@ void do_initcalls(void);
 /*
  * Used for kernel command line parameter setup
  */
+enum param_scope {
+    SCOPE_GLOBAL
+};
+
 struct kernel_param {
     const char *name;
     enum {
@@ -83,9 +87,11 @@ struct kernel_param {
         OPT_CUSTOM
     } type;
     unsigned int len;
+    enum param_scope scope;
     union {
         void *var;
         int (*func)(const char *);
+        int (*call)(const char *, void *);
     } par;
 };
 
@@ -101,54 +107,56 @@ extern const struct kernel_param __param_start[], __param_end[];
     __attribute__((__aligned__(1))) char
 #define __kparam          __param(__initsetup)
 
-#define def_custom_param(_name, _func) \
+#define def_custom_param(_name, _scope, _func) \
     { .name = _name, \
       .type = OPT_CUSTOM, \
+      .scope = _scope, \
       .par.func = _func }
-#define def_var_param(_name, _type, _var) \
+#define def_var_param(_name, _type, _scope, _var) \
     { .name = _name, \
       .type = _type, \
       .len = sizeof(_var), \
+      .scope = _scope, \
       .par.var = &_var }
 
 #define custom_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_custom_param(__setup_str_##_var, _var)
+        def_custom_param(__setup_str_##_var, SCOPE_GLOBAL, _var)
 #define boolean_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_BOOL, _var)
+        def_var_param(__setup_str_##_var, OPT_BOOL, SCOPE_GLOBAL, _var)
 #define integer_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_UINT, _var)
+        def_var_param(__setup_str_##_var, OPT_UINT, SCOPE_GLOBAL, _var)
 #define size_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_SIZE, _var)
+        def_var_param(__setup_str_##_var, OPT_SIZE, SCOPE_GLOBAL, _var)
 #define string_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_STR, _var)
+        def_var_param(__setup_str_##_var, OPT_STR, SCOPE_GLOBAL, _var)
 
 #define __rtparam         __param(__dataparam)
 
 #define custom_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_custom_param(_name, _var)
+        def_custom_param(_name, SCOPE_GLOBAL, _var)
 #define boolean_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_BOOL, _var)
+        def_var_param(_name, OPT_BOOL, SCOPE_GLOBAL, _var)
 #define integer_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_UINT, _var)
+        def_var_param(_name, OPT_UINT, SCOPE_GLOBAL, _var)
 #define size_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_SIZE, _var)
+        def_var_param(_name, OPT_SIZE, SCOPE_GLOBAL, _var)
 #define string_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_STR, _var)
+        def_var_param(_name, OPT_STR, SCOPE_GLOBAL, _var)
 
 #define custom_runtime_param(_name, _var) \
     custom_param(_name, _var); \
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 04/12] xen: add a generic flags field to parameter definitions

[Juergen Gross]

In preparation for domain specific parameters add a flags field to
struct kernel_param. Add a common flag definition for support of
changing the related parameter at runtime (in case of domains while
the domain is already running in contrast to domain creation time).

As other flags will be scope specific add a generic check function
pointer to struct parse_data which will be called by parameter parsing
to verify the parameter is allowed on the specified instance.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/kernel.c    | 15 +++++++++++++--
 xen/include/xen/init.h | 28 ++++++++++++++++------------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f7f4e0dbff..0d3d7f6135 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -22,6 +22,7 @@ struct parse_data {
     enum param_scope scope;
     const struct kernel_param *start;
     const struct kernel_param *end;
+    int (*check)(void *instance, unsigned int flags);
 };
 
 enum system_state system_state = SYS_STATE_early_boot;
@@ -124,7 +125,10 @@ static int parse_params(const char *cmdline, const struct parse_data *data,
                 {
                     found = true;
                     optval[-1] = '=';
-                    rctmp = param->par.call(q, instance);
+                    rctmp = data->check
+                            ? data->check(instance, param->flags) : 0;
+                    if ( !rctmp )
+                        rctmp = param->par.call(q, instance);
                     optval[-1] = '\0';
                     if ( !rc )
                         rc = rctmp;
@@ -132,7 +136,14 @@ static int parse_params(const char *cmdline, const struct parse_data *data,
                 continue;
             }
 
-            rctmp = 0;
+            rctmp = data->check ? data->check(instance, param->flags) : 0;
+            if ( rctmp )
+            {
+                if ( !rc )
+                    rc = rctmp;
+                continue;
+            }
+
             found = true;
             ptr = (void *)((uint64_t)param->par.var + (uint64_t)instance);
             switch ( param->type )
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index b04534c11a..3975964ee8 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -88,6 +88,8 @@ struct kernel_param {
     } type;
     unsigned int len;
     enum param_scope scope;
+    unsigned int flags;
+#define PARAM_FLAG_RUNTIME    0x80000000
     union {
         void *var;
         int (*func)(const char *);
@@ -107,56 +109,58 @@ extern const struct kernel_param __param_start[], __param_end[];
     __attribute__((__aligned__(1))) char
 #define __kparam          __param(__initsetup)
 
-#define def_custom_param(_name, _scope, _func) \
+#define def_custom_param(_name, _scope, _flags, _func) \
     { .name = _name, \
       .type = OPT_CUSTOM, \
       .scope = _scope, \
+      .flags = _flags, \
       .par.func = _func }
-#define def_var_param(_name, _type, _scope, _var) \
+#define def_var_param(_name, _type, _scope, _flags, _var) \
     { .name = _name, \
       .type = _type, \
       .len = sizeof(_var), \
       .scope = _scope, \
+      .flags = _flags, \
       .par.var = &_var }
 
 #define custom_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_custom_param(__setup_str_##_var, SCOPE_GLOBAL, _var)
+        def_custom_param(__setup_str_##_var, SCOPE_GLOBAL, 0, _var)
 #define boolean_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_BOOL, SCOPE_GLOBAL, _var)
+        def_var_param(__setup_str_##_var, OPT_BOOL, SCOPE_GLOBAL, 0, _var)
 #define integer_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_UINT, SCOPE_GLOBAL, _var)
+        def_var_param(__setup_str_##_var, OPT_UINT, SCOPE_GLOBAL, 0, _var)
 #define size_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_SIZE, SCOPE_GLOBAL, _var)
+        def_var_param(__setup_str_##_var, OPT_SIZE, SCOPE_GLOBAL, 0, _var)
 #define string_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_var_param(__setup_str_##_var, OPT_STR, SCOPE_GLOBAL, _var)
+        def_var_param(__setup_str_##_var, OPT_STR, SCOPE_GLOBAL, 0, _var)
 
 #define __rtparam         __param(__dataparam)
 
 #define custom_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_custom_param(_name, SCOPE_GLOBAL, _var)
+        def_custom_param(_name, SCOPE_GLOBAL, 0, _var)
 #define boolean_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_BOOL, SCOPE_GLOBAL, _var)
+        def_var_param(_name, OPT_BOOL, SCOPE_GLOBAL, 0, _var)
 #define integer_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_UINT, SCOPE_GLOBAL, _var)
+        def_var_param(_name, OPT_UINT, SCOPE_GLOBAL, 0, _var)
 #define size_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_SIZE, SCOPE_GLOBAL, _var)
+        def_var_param(_name, OPT_SIZE, SCOPE_GLOBAL, 0, _var)
 #define string_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_var_param(_name, OPT_STR, SCOPE_GLOBAL, _var)
+        def_var_param(_name, OPT_STR, SCOPE_GLOBAL, 0, _var)
 
 #define custom_runtime_param(_name, _var) \
     custom_param(_name, _var); \
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 05/12] xen: add hypercall interfaces for domain and cpupool parameter setting

[Juergen Gross]

Add a new domctl for setting domain specific parameters similar to
XEN_SYSCTL_set_parameter for global hypervisor parameters.

Enhance XEN_SYSCTL_set_parameter to be usable for setting cpupool
specific parameters, too. For now do only extended parameter checking.
The cpupool parameter setting will be added later.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/flask/policy/modules/dom0.te  |  2 +-
 tools/libxc/xc_misc.c               |  4 +++-
 xen/common/sysctl.c                 | 14 +++++++++++---
 xen/include/public/domctl.h         | 20 +++++++++++++++++++-
 xen/include/public/sysctl.h         |  8 +++++++-
 xen/xsm/flask/hooks.c               |  3 +++
 xen/xsm/flask/policy/access_vectors |  2 ++
 7 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index dfdcdcd128..64a328570a 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -39,7 +39,7 @@ allow dom0_t dom0_t:domain {
 };
 allow dom0_t dom0_t:domain2 {
 	set_cpuid gettsc settsc setscheduler set_vnumainfo
-	get_vnumainfo psr_cmt_op psr_alloc
+	get_vnumainfo psr_cmt_op psr_alloc set_parameter
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 5e6714ae2b..655c2329b1 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -199,7 +199,9 @@ int xc_set_parameters(xc_interface *xch, char *params)
     sysctl.cmd = XEN_SYSCTL_set_parameter;
     set_xen_guest_handle(sysctl.u.set_parameter.params, params);
     sysctl.u.set_parameter.size = len;
-    memset(sysctl.u.set_parameter.pad, 0, sizeof(sysctl.u.set_parameter.pad));
+    sysctl.u.set_parameter.scope = XEN_SYSCTL_SETPAR_SCOPE_GLOBAL;
+    sysctl.u.set_parameter.pad = 0;
+    sysctl.u.set_parameter.instance = 0;
 
     ret = do_sysctl(xch, &sysctl);
 
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c0aa6bde4e..f10cd279f7 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -471,8 +471,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 #define XEN_SET_PARAMETER_MAX_SIZE 1023
         char *params;
 
-        if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
-             op->u.set_parameter.pad[2] )
+        if ( op->u.set_parameter.pad )
         {
             ret = -EINVAL;
             break;
@@ -494,7 +493,16 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         else
         {
             params[op->u.set_parameter.size] = 0;
-            ret = runtime_parse(params);
+            switch ( op->u.set_parameter.scope )
+            {
+            case XEN_SYSCTL_SETPAR_SCOPE_GLOBAL:
+                ret = op->u.set_parameter.instance
+                      ? -EINVAL : runtime_parse(params);
+                break;
+            default:
+                ret = -EINVAL;
+                break;
+            }
         }
 
         xfree(params);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 82b696798c..3d6f8b27ab 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -38,7 +38,7 @@
 #include "hvm/save.h"
 #include "memory.h"
 
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000010
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000011
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
@@ -1098,6 +1098,22 @@ struct xen_domctl_vuart_op {
                                  */
 };
 
+/*
+ * XEN_DOMCTL_set_parameter
+ *
+ * Change domain parameters at runtime.
+ * The input string is parsed similar to the boot parameters.
+ * Parameters are a single string terminated by a NUL byte of max. size
+ * characters. Multiple settings can be specified by separating them
+ * with blanks.
+ */
+
+struct xen_domctl_set_parameter {
+    XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to parameters. */
+    uint16_t size;                          /* IN: size of parameters. */
+    uint16_t pad[3];                        /* IN: MUST be zero. */
+};
+
 struct xen_domctl {
     uint32_t cmd;
 #define XEN_DOMCTL_createdomain                   1
@@ -1177,6 +1193,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_soft_reset                    79
 /* #define XEN_DOMCTL_set_gnttab_limits          80 - Moved into XEN_DOMCTL_createdomain */
 #define XEN_DOMCTL_vuart_op                      81
+#define XEN_DOMCTL_set_parameter                 82
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
@@ -1237,6 +1254,7 @@ struct xen_domctl {
         struct xen_domctl_monitor_op        monitor_op;
         struct xen_domctl_psr_alloc         psr_alloc;
         struct xen_domctl_vuart_op          vuart_op;
+        struct xen_domctl_set_parameter     set_parameter;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 8cd0a9cb0d..a6246c4ca7 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1055,12 +1055,18 @@ struct xen_sysctl_livepatch_op {
  * Parameters are a single string terminated by a NUL byte of max. size
  * characters. Multiple settings can be specified by separating them
  * with blanks.
+ * Scope can be either global (like boot parameters) or cpupool.
  */
 
 struct xen_sysctl_set_parameter {
     XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to parameters. */
     uint16_t size;                          /* IN: size of parameters. */
-    uint16_t pad[3];                        /* IN: MUST be zero. */
+    uint8_t  scope;                         /* IN: scope of parameters. */
+#define XEN_SYSCTL_SETPAR_SCOPE_GLOBAL   0
+#define XEN_SYSCTL_SETPAR_SCOPE_CPUPOOL  1
+    uint8_t  pad;                           /* IN: MUST be zero. */
+    uint32_t instance;                      /* IN: scope global: must be zero */
+                                            /*     scope cpupool: cpupool id */
 };
 
 struct xen_sysctl {
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 6da2773aa9..d382883394 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -742,6 +742,9 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_soft_reset:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET);
 
+    case XEN_DOMCTL_set_parameter:
+        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_PARAMETER);
+
     default:
         return avc_unknown_permission("domctl", cmd);
     }
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index d01a7a0d03..36874c3452 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -248,6 +248,8 @@ class domain2
     psr_alloc
 # XENMEM_resource_map
     resource_map
+# XEN_DOMCTL_set_parameter
+    set_parameter
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/12] xen: add hypercall interfaces for domain and cpupool parameter setting

[Daniel De Graaf]

On 09/18/2018 02:03 AM, Juergen Gross wrote:
> Add a new domctl for setting domain specific parameters similar to
> XEN_SYSCTL_set_parameter for global hypervisor parameters.
> 
> Enhance XEN_SYSCTL_set_parameter to be usable for setting cpupool
> specific parameters, too. For now do only extended parameter checking.
> The cpupool parameter setting will be added later.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

You also need to add the permission to the create_domain_common macro
in xen.if (currently you only let dom0 modify its own settings).  If
you intend this interface to be used to modify running domains, adding
it to the manage_domain macro would also be useful, but that's not as
critical.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/12] xen: add hypercall interfaces for domain and cpupool parameter setting

[Juergen Gross]

On 18/09/18 23:23, Daniel De Graaf wrote:
> On 09/18/2018 02:03 AM, Juergen Gross wrote:
>> Add a new domctl for setting domain specific parameters similar to
>> XEN_SYSCTL_set_parameter for global hypervisor parameters.
>>
>> Enhance XEN_SYSCTL_set_parameter to be usable for setting cpupool
>> specific parameters, too. For now do only extended parameter checking.
>> The cpupool parameter setting will be added later.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> You also need to add the permission to the create_domain_common macro
> in xen.if (currently you only let dom0 modify its own settings).  If
> you intend this interface to be used to modify running domains, adding
> it to the manage_domain macro would also be useful, but that's not as
> critical.

Thanks, will do.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 05/12] xen: add hypercall interfaces for domain and cpupool parameter setting

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1997 bytes --]

On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
> Add a new domctl for setting domain specific parameters similar to
> XEN_SYSCTL_set_parameter for global hypervisor parameters.
> 
> Enhance XEN_SYSCTL_set_parameter to be usable for setting cpupool
> specific parameters, too. For now do only extended parameter
> checking.
> The cpupool parameter setting will be added later.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

I mean...

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1055,12 +1055,18 @@ struct xen_sysctl_livepatch_op {
>   * Parameters are a single string terminated by a NUL byte of max.
> size
>   * characters. Multiple settings can be specified by separating them
>   * with blanks.
> + * Scope can be either global (like boot parameters) or cpupool.
>   */
>  
>  struct xen_sysctl_set_parameter {
>      XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to
> parameters. */
>      uint16_t size;                          /* IN: size of
> parameters. */
> -    uint16_t pad[3];                        /* IN: MUST be zero. */
> +    uint8_t  scope;                         /* IN: scope of
> parameters. */
> +#define XEN_SYSCTL_SETPAR_SCOPE_GLOBAL   0
> +#define XEN_SYSCTL_SETPAR_SCOPE_CPUPOOL  1
> +    uint8_t  pad;                           /* IN: MUST be zero. */
> +    uint32_t instance;                      /* IN: scope global:
> must be zero */
> +                                            /*     scope cpupool:
> cpupool id */
>
... I don't particularly like the name 'instance', but I've not been
able to come up with anything else which sounds better... :-/

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 06/12] xen: add domain specific parameter support

[Juergen Gross]

Add the framework for being able to define domain specific parameters.
This includes the needed macros for defining a parameter and the
minimal set of functions for doing parameter parsing.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/domain.c    |  5 +++++
 xen/arch/x86/domain.c    |  4 ++++
 xen/common/domain.c      | 10 ++++++++++
 xen/common/domctl.c      | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/common/kernel.c      | 12 ++++++++++++
 xen/include/xen/domain.h |  3 +++
 xen/include/xen/init.h   | 34 +++++++++++++++++++++++++++++-----
 xen/include/xen/lib.h    |  1 +
 8 files changed, 110 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index feebbf5a92..3f8bd6bdcc 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1011,6 +1011,11 @@ void vcpu_kick(struct vcpu *vcpu)
     }
 }
 
+int arch_domain_check_parflags(struct domain *d, unsigned int flags)
+{
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index d67a0478f6..8e57e7a181 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2106,6 +2106,10 @@ static int __init init_vcpu_kick_softirq(void)
 }
 __initcall(init_vcpu_kick_softirq);
 
+int arch_domain_check_parflags(struct domain *d, unsigned int flags)
+{
+    return 0;
+}
 
 /*
  * Local variables:
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 65151e2ac4..ac0ad97638 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1647,6 +1647,16 @@ int continue_hypercall_on_cpu(
     return 0;
 }
 
+int domain_check_parflags(void *instance, unsigned int flags)
+{
+    struct domain *d = instance;
+
+    if ( !(flags & PARAM_FLAG_RUNTIME) && d->creation_finished )
+         return -EBUSY;
+
+    return arch_domain_check_parflags(d, flags);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index b2948814aa..149f0ed9d8 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1075,6 +1075,52 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
+    case XEN_DOMCTL_set_parameter:
+    {
+#define XEN_SET_PARAMETER_MAX_SIZE 1023
+        char *params;
+
+        if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
+             op->u.set_parameter.pad[2] )
+        {
+            ret = -EINVAL;
+            break;
+        }
+        if ( d == current->domain )
+        {
+            printk("domain %u can't set parameter for itself!\n",
+                   d->domain_id);
+            ret = -EBUSY;
+            break;
+        }
+        if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
+        {
+            printk("parameter size too big!\n");
+            ret = -E2BIG;
+            break;
+        }
+        params = xmalloc_bytes(op->u.set_parameter.size + 1);
+        if ( !params )
+        {
+            ret = -ENOMEM;
+            break;
+        }
+        if ( copy_from_guest(params, op->u.set_parameter.params,
+                             op->u.set_parameter.size) )
+            ret = -EFAULT;
+        else
+        {
+            domain_pause(d);
+            params[op->u.set_parameter.size] = 0;
+            ret = domain_param_parse(d, params);
+            domain_unpause(d);
+        }
+
+        xfree(params);
+
+        break;
+    }
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 0d3d7f6135..d0b3af1453 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -222,6 +222,13 @@ static const struct parse_data runtime_parse_data = {
     .end    = __param_end,
 };
 
+static const struct parse_data domain_parse_data = {
+    .scope = SCOPE_DOMAIN,
+    .start = __param_start,
+    .end   = __param_end,
+    .check = domain_check_parflags,
+};
+
 static void __init _cmdline_parse(const char *cmdline)
 {
     parse_params(cmdline, &boot_parse_data, NULL);
@@ -232,6 +239,11 @@ int runtime_parse(const char *line)
     return parse_params(line, &runtime_parse_data, NULL);
 }
 
+int domain_param_parse(struct domain *d, const char *line)
+{
+    return parse_params(line, &domain_parse_data, d);
+}
+
 /**
  *    cmdline_parse -- parses the xen command line.
  * If CONFIG_CMDLINE is set, it would be parsed prior to @cmdline.
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 5e393fd7f2..944ed5beeb 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -72,6 +72,9 @@ int arch_domain_soft_reset(struct domain *d);
 
 void arch_p2m_set_access_required(struct domain *d, bool access_required);
 
+int domain_check_parflags(void *instance, unsigned int flags);
+int arch_domain_check_parflags(struct domain *d, unsigned int flags);
+
 int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
 void arch_get_info_guest(struct vcpu *, vcpu_guest_context_u);
 
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 3975964ee8..13cf5214b3 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -54,6 +54,8 @@
 
 #ifndef __ASSEMBLY__
 
+struct domain;
+
 /*
  * Used for initialization calls..
  */
@@ -74,7 +76,8 @@ void do_initcalls(void);
  * Used for kernel command line parameter setup
  */
 enum param_scope {
-    SCOPE_GLOBAL
+    SCOPE_GLOBAL,
+    SCOPE_DOMAIN
 };
 
 struct kernel_param {
@@ -93,6 +96,7 @@ struct kernel_param {
     union {
         void *var;
         int (*func)(const char *);
+        int (*func_domain)(const char *, struct domain *);
         int (*call)(const char *, void *);
     } par;
 };
@@ -109,12 +113,12 @@ extern const struct kernel_param __param_start[], __param_end[];
     __attribute__((__aligned__(1))) char
 #define __kparam          __param(__initsetup)
 
-#define def_custom_param(_name, _scope, _flags, _func) \
+#define def_custom_param(_name, _scope, _flags, _ptr, _func) \
     { .name = _name, \
       .type = OPT_CUSTOM, \
       .scope = _scope, \
       .flags = _flags, \
-      .par.func = _func }
+      .par._ptr = _func }
 #define def_var_param(_name, _type, _scope, _flags, _var) \
     { .name = _name, \
       .type = _type, \
@@ -126,7 +130,7 @@ extern const struct kernel_param __param_start[], __param_end[];
 #define custom_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-        def_custom_param(__setup_str_##_var, SCOPE_GLOBAL, 0, _var)
+        def_custom_param(__setup_str_##_var, SCOPE_GLOBAL, 0, func, _var)
 #define boolean_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
@@ -148,7 +152,7 @@ extern const struct kernel_param __param_start[], __param_end[];
 
 #define custom_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
-        def_custom_param(_name, SCOPE_GLOBAL, 0, _var)
+        def_custom_param(_name, SCOPE_GLOBAL, 0, func, _var)
 #define boolean_runtime_only_param(_name, _var) \
     __rtparam __rtpar_##_var = \
         def_var_param(_name, OPT_BOOL, SCOPE_GLOBAL, 0, _var)
@@ -178,6 +182,26 @@ extern const struct kernel_param __param_start[], __param_end[];
     string_param(_name, _var); \
     string_runtime_only_param(_name, _var)
 
+#define custom_domain_param(_name, _flags, _var) \
+    __rtparam __domain_par_##_var = \
+        def_custom_param(_name, SCOPE_DOMAIN, _flags, func_domain, _var)
+#define boolean_domain_param(_name, _flags, _var) \
+    __rtparam __domain_par_##_var = \
+        def_var_param(_name, OPT_BOOL, SCOPE_DOMAIN, _flags, \
+                      (struct domain *)NULL->_var)
+#define integer_domain_param(_name, _flags, _var) \
+    __rtparam __domain_par_##_var = \
+        def_var_param(_name, OPT_UINT, SCOPE_DOMAIN, _flags, \
+                      (struct domain *)NULL->_var)
+#define size_domain_param(_name, _flags, _var) \
+    __rtparam __domain_par_##_var = \
+        def_var_param(_name, OPT_SIZE, SCOPE_DOMAIN, _flags, \
+                      (struct domain *)NULL->_var)
+#define string_domain_param(_name, _flags, _var) \
+    __rtparam __domain_par_##_var = \
+        def_var_param(_name, OPT_STR, SCOPE_DOMAIN, _flags, \
+                      (struct domain *)NULL->_var)
+
 #endif /* __ASSEMBLY__ */
 
 #ifdef CONFIG_LATE_HWDOM
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 972fc843fa..7ba5929ed5 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -70,6 +70,7 @@ struct domain;
 
 void cmdline_parse(const char *cmdline);
 int runtime_parse(const char *line);
+int domain_param_parse(struct domain *d, const char *line);
 int parse_bool(const char *s, const char *e);
 
 /**
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 07/12] xen: add domain specific parameter support

[Juergen Gross]

Add the framework for being able to define cpupool specific parameters.
This includes the needed macros for defining a parameter and the
minimal set of functions for doing parameter parsing.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c    | 10 ++++++++++
 xen/common/kernel.c     | 12 ++++++++++++
 xen/common/sysctl.c     | 14 ++++++++++++++
 xen/include/xen/init.h  | 23 +++++++++++++++++++++++
 xen/include/xen/lib.h   |  2 ++
 xen/include/xen/sched.h |  1 +
 6 files changed, 62 insertions(+)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 1e8edcbd57..0a9f875602 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -808,6 +808,16 @@ static int __init cpupool_presmp_init(void)
 }
 presmp_initcall(cpupool_presmp_init);
 
+int cpupool_check_parflags(void *instance, unsigned int flags)
+{
+    struct cpupool *c = instance;
+
+    if ( !(flags & PARAM_FLAG_RUNTIME) && (c->n_dom != 0) )
+        return -EBUSY;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index d0b3af1453..81155d5ebe 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -222,6 +222,13 @@ static const struct parse_data runtime_parse_data = {
     .end    = __param_end,
 };
 
+static const struct parse_data cpupool_parse_data = {
+    .scope = SCOPE_CPUPOOL,
+    .start = __param_start,
+    .end   = __param_end,
+    .check = cpupool_check_parflags,
+};
+
 static const struct parse_data domain_parse_data = {
     .scope = SCOPE_DOMAIN,
     .start = __param_start,
@@ -239,6 +246,11 @@ int runtime_parse(const char *line)
     return parse_params(line, &runtime_parse_data, NULL);
 }
 
+int cpupool_param_parse(struct cpupool *c, const char *line)
+{
+    return parse_params(line, &cpupool_parse_data, c);
+}
+
 int domain_param_parse(struct domain *d, const char *line)
 {
     return parse_params(line, &domain_parse_data, d);
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index f10cd279f7..575248301e 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -499,6 +499,20 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
                 ret = op->u.set_parameter.instance
                       ? -EINVAL : runtime_parse(params);
                 break;
+            case XEN_SYSCTL_SETPAR_SCOPE_CPUPOOL:
+            {
+                struct cpupool *c;
+
+                c = cpupool_get_by_id(op->u.set_parameter.instance);
+                if ( c == NULL )
+                    ret = -ESRCH;
+                else
+                {
+                    ret = cpupool_param_parse(c, params);
+                    cpupool_put(c);
+                }
+                break;
+            }
             default:
                 ret = -EINVAL;
                 break;
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 13cf5214b3..598ae0b5d4 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -54,6 +54,7 @@
 
 #ifndef __ASSEMBLY__
 
+struct cpupool;
 struct domain;
 
 /*
@@ -77,6 +78,7 @@ void do_initcalls(void);
  */
 enum param_scope {
     SCOPE_GLOBAL,
+    SCOPE_CPUPOOL,
     SCOPE_DOMAIN
 };
 
@@ -96,6 +98,7 @@ struct kernel_param {
     union {
         void *var;
         int (*func)(const char *);
+        int (*func_cpupool)(const char *, struct cpupool *);
         int (*func_domain)(const char *, struct domain *);
         int (*call)(const char *, void *);
     } par;
@@ -182,6 +185,26 @@ extern const struct kernel_param __param_start[], __param_end[];
     string_param(_name, _var); \
     string_runtime_only_param(_name, _var)
 
+#define custom_cpupool_param(_name, _flags, _var) \
+    __rtparam __cpupool_par_##_var = \
+        def_custom_param(_name, SCOPE_CPUPOOL, _flags, func_cpupool, _var)
+#define boolean_cpupool_param(_name, _flags, _var) \
+    __rtparam __cpupool_par_##_var = \
+        def_var_param(_name, OPT_BOOL, SCOPE_CPUPOOL, _flags, \
+                      (struct cpupool *)NULL->_var)
+#define integer_cpupool_param(_name, _flags, _var) \
+    __rtparam __cpupool_par_##_var = \
+        def_var_param(_name, OPT_UINT, SCOPE_CPUPOOL, _flags, \
+                      (struct cpupool *)NULL->_var)
+#define size_cpupool_param(_name, _flags, _var) \
+    __rtparam __cpupool_par_##_var = \
+        def_var_param(_name, OPT_SIZE, SCOPE_CPUPOOL, _flags, \
+                      (struct cpupool *)NULL->_var)
+#define string_cpupool_param(_name, _flags, _var) \
+    __rtparam __cpupool_par_##_var = \
+        def_var_param(_name, OPT_STR, SCOPE_CPUPOOL, _flags, \
+                      (struct cpupool *)NULL->_var)
+
 #define custom_domain_param(_name, _flags, _var) \
     __rtparam __domain_par_##_var = \
         def_custom_param(_name, SCOPE_DOMAIN, _flags, func_domain, _var)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 7ba5929ed5..17f60607ca 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -66,10 +66,12 @@
 
 #define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
 
+struct cpupool;
 struct domain;
 
 void cmdline_parse(const char *cmdline);
 int runtime_parse(const char *line);
+int cpupool_param_parse(struct cpupool *c, const char *line);
 int domain_param_parse(struct domain *d, const char *line);
 int parse_bool(const char *s, const char *e);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 0ba80cb1a8..97f838b7c0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -919,6 +919,7 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c);
 int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
 void schedule_dump(struct cpupool *c);
 extern void dump_runq(unsigned char key);
+int cpupool_check_parflags(void *instance, unsigned int flags);
 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
 
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 07/12] xen: add domain specific parameter support

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 709 bytes --]

I think the subject wants to say "cpupool specific" ?

With this adjusted...

On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
> Add the framework for being able to define cpupool specific
> parameters.
> This includes the needed macros for defining a parameter and the
> minimal set of functions for doing parameter parsing.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 08/12] tools/libxc: add per domain/cpupool parameter support

[Juergen Gross]

Add support for setting per domain and per cpupool parameters to libxc.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxc/include/xenctrl.h | 18 ++++++++++++++++++
 tools/libxc/xc_cpupool.c      | 23 +++++++++++++++++++++++
 tools/libxc/xc_domain.c       | 23 +++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index bb75bcc84d..d22da5ee61 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1081,6 +1081,15 @@ int xc_domain_set_access_required(xc_interface *xch,
  */
 int xc_domain_set_virq_handler(xc_interface *xch, uint32_t domid, int virq);
 
+/**
+ * Set generic domain parameters.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm domid id of the domain
+ * @parm params parameter string to be passed to the hypervisor
+ */
+int xc_set_domain_parameters(xc_interface *xch, uint32_t domid, char *params);
+
 /*
  * CPUPOOL MANAGEMENT FUNCTIONS
  */
@@ -1178,6 +1187,15 @@ int xc_cpupool_movedomain(xc_interface *xch,
  */
 xc_cpumap_t xc_cpupool_freeinfo(xc_interface *xch);
 
+/**
+ * Set generic cpupool parameters.
+ *
+ * @parm xch a handle to an open hypervisor interface
+ * @parm poolid id of the cpupool
+ * @parm params parameter string to be passed to the hypervisor
+ */
+int xc_set_cpupool_parameters(xc_interface *xch, uint32_t poolid, char *params);
+
 /*
  * EVENT CHANNEL FUNCTIONS
  *
diff --git a/tools/libxc/xc_cpupool.c b/tools/libxc/xc_cpupool.c
index fbd8cc9d03..4df775884b 100644
--- a/tools/libxc/xc_cpupool.c
+++ b/tools/libxc/xc_cpupool.c
@@ -217,3 +217,26 @@ out:
     xc_hypercall_buffer_free(xch, local);
     return cpumap;
 }
+
+int xc_set_cpupool_parameters(xc_interface *xch, uint32_t poolid, char *params)
+{
+    int ret, len = strlen(params);
+    DECLARE_SYSCTL;
+    DECLARE_HYPERCALL_BOUNCE(params, len, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    if ( xc_hypercall_bounce_pre(xch, params) )
+        return -1;
+
+    sysctl.cmd = XEN_SYSCTL_set_parameter;
+    set_xen_guest_handle(sysctl.u.set_parameter.params, params);
+    sysctl.u.set_parameter.size = len;
+    sysctl.u.set_parameter.scope = XEN_SYSCTL_SETPAR_SCOPE_CPUPOOL;
+    sysctl.u.set_parameter.pad = 0;
+    sysctl.u.set_parameter.instance = poolid;
+
+    ret = do_sysctl(xch, &sysctl);
+
+    xc_hypercall_bounce_post(xch, params);
+
+    return ret;
+}
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 05d771f2ce..265f018210 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2386,6 +2386,29 @@ int xc_domain_soft_reset(xc_interface *xch,
     domctl.domain = domid;
     return do_domctl(xch, &domctl);
 }
+
+int xc_set_domain_parameters(xc_interface *xch, uint32_t domid, char *params)
+{
+    int ret, len = strlen(params);
+    DECLARE_DOMCTL;
+    DECLARE_HYPERCALL_BOUNCE(params, len, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    if ( xc_hypercall_bounce_pre(xch, params) )
+        return -1;
+
+    domctl.cmd = XEN_DOMCTL_set_parameter;
+    domctl.domain = domid;
+    set_xen_guest_handle(domctl.u.set_parameter.params, params);
+    domctl.u.set_parameter.size = len;
+    memset(domctl.u.set_parameter.pad, 0, sizeof(domctl.u.set_parameter.pad));
+
+    ret = do_domctl(xch, &domctl);
+
+    xc_hypercall_bounce_post(xch, params);
+
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 09/12] tools/xl: add support for setting generic per-cpupool parameters

[Juergen Gross]

Add a new xl command "cpupool-set-parameters" and cpupool config file
support for setting per-cpupool generic parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.pod.1.in         |  6 ++++++
 docs/man/xlcpupool.cfg.pod.5 | 12 ++++++++++++
 tools/libxl/libxl.h          |  8 ++++++++
 tools/libxl/libxl_cpupool.c  | 16 ++++++++++++++++
 tools/xl/xl.h                |  1 +
 tools/xl/xl_cmdtable.c       |  5 +++++
 tools/xl/xl_cpupool.c        | 39 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 87 insertions(+)

diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index b74764dcd3..a2ddc4b2e0 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -1319,6 +1319,12 @@ Domain-0 can't be moved to another cpu-pool.
 
 Splits up the machine into one cpu-pool per numa node.
 
+=item B<cpupool-set-parameters> I<cpu-pool> I<params>
+
+Sets generic parameters I<params> for I<cpu-pool>.
+
+See the L<xlcpupool.cfg(5)> manpage for supported parameters.
+
 =back
 
 =head1 VIRTUAL DEVICE COMMANDS
diff --git a/docs/man/xlcpupool.cfg.pod.5 b/docs/man/xlcpupool.cfg.pod.5
index 792cf4f425..4f92a68209 100644
--- a/docs/man/xlcpupool.cfg.pod.5
+++ b/docs/man/xlcpupool.cfg.pod.5
@@ -113,6 +113,18 @@ of cpus 10,11,12,13 will be memeber of the cpupool.
 If neither B<nodes> nor B<cpus> are specified only the first free cpu
 found will be allocated in the new cpupool.
 
+=item B<parameters="PARAMS">
+
+Specifies generic parameters for the cpupool. C<PARAMS> is a list of
+parameter settings in the form of "name[=value]" separated by blanks.
+The following parameter settings are supported:
+
+=over 4
+
+NONE
+
+=back
+
 =back
 
 =head1 FILES
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 2cfc1b08ad..bd26d9fd4a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1164,6 +1164,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_PVCALLS 1
 
+/*
+ * LIBXL_HAVE_CPUPOOL_SET_PARAMETERS
+ *
+ * If this is defined setting per-cpupool parameters is supported.
+ */
+#define LIBXL_HAVE_CPUPOOL_SET_PARAMETERS 1
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -2352,6 +2359,7 @@ int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
                                    const libxl_bitmap *cpumap);
 int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
 int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
+int libxl_cpupool_set_parameters(libxl_ctx *ctx, uint32_t poolid, char *params);
 
 int libxl_domid_valid_guest(uint32_t domid);
 
diff --git a/tools/libxl/libxl_cpupool.c b/tools/libxl/libxl_cpupool.c
index 85b06882db..e2db3fd5cb 100644
--- a/tools/libxl/libxl_cpupool.c
+++ b/tools/libxl/libxl_cpupool.c
@@ -443,6 +443,22 @@ int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid)
     return 0;
 }
 
+int libxl_cpupool_set_parameters(libxl_ctx *ctx, uint32_t poolid, char *params)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = xc_set_cpupool_parameters(ctx->xch, poolid, params);
+    if (rc) {
+        LOGEV(ERROR, rc, "Error setting cpupool parameters");
+        GC_FREE;
+        return ERROR_FAIL;
+    }
+
+    GC_FREE;
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index cf4202bc89..a51acc4256 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -201,6 +201,7 @@ int main_cpupoolcpuadd(int argc, char **argv);
 int main_cpupoolcpuremove(int argc, char **argv);
 int main_cpupoolmigrate(int argc, char **argv);
 int main_cpupoolnumasplit(int argc, char **argv);
+int main_cpupoolsetparameters(int argc, char **argv);
 int main_getenforce(int argc, char **argv);
 int main_setenforce(int argc, char **argv);
 int main_loadpolicy(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 89716badcb..3a469dacc3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -530,6 +530,11 @@ struct cmd_spec cmd_table[] = {
       "Splits up the machine into one CPU pool per NUMA node",
       "",
     },
+    { "cpupool-set-parameters",
+      &main_cpupoolsetparameters, 0, 1,
+      "Sets generic parameters for a CPU pool",
+      "<CPU Pool> <Params>",
+    },
     { "getenforce",
       &main_getenforce, 0, 0,
       "Returns the current enforcing mode of the Flask Xen security module",
diff --git a/tools/xl/xl_cpupool.c b/tools/xl/xl_cpupool.c
index 273811b663..9e0fce1961 100644
--- a/tools/xl/xl_cpupool.c
+++ b/tools/xl/xl_cpupool.c
@@ -41,6 +41,7 @@ int main_cpupoolcreate(int argc, char **argv)
     XLU_Config *config;
     const char *buf;
     char *name = NULL;
+    char *params = NULL;
     uint32_t poolid;
     libxl_scheduler sched = 0;
     XLU_ConfigList *cpus;
@@ -146,6 +147,9 @@ int main_cpupoolcreate(int argc, char **argv)
         sched = rc;
     }
 
+    if (!xlu_cfg_get_string (config, "parameters", &buf, 0))
+        params = strdup(buf);
+
     if (libxl_get_freecpus(ctx, &freemap)) {
         fprintf(stderr, "libxl_get_freecpus failed\n");
         goto out_cfg;
@@ -213,6 +217,8 @@ int main_cpupoolcreate(int argc, char **argv)
     printf("cpupool name:   %s\n", name);
     printf("scheduler:      %s\n", libxl_scheduler_to_string(sched));
     printf("number of cpus: %d\n", n_cpus);
+    if (params)
+         printf("parameters:     %s\n", params);
 
     if (!dryrun_only) {
         poolid = LIBXL_CPUPOOL_POOLID_ANY;
@@ -220,6 +226,10 @@ int main_cpupoolcreate(int argc, char **argv)
             fprintf(stderr, "error on creating cpupool\n");
             goto out_cfg;
         }
+        if (params) {
+            if (libxl_cpupool_set_parameters(ctx, poolid, params))
+                fprintf(stderr, "error setting parameters, ignored\n");
+        }
     }
     /* We made it! */
     rc = EXIT_SUCCESS;
@@ -615,6 +625,35 @@ out:
     return rc;
 }
 
+int main_cpupoolsetparameters(int argc, char **argv)
+{
+    int opt;
+    const char *pool;
+    char *params;
+    uint32_t poolid;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-set-parameters", 2) {
+        /* No options */
+    }
+
+    pool = argv[optind++];
+    if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid, NULL) ||
+        !libxl_cpupoolid_is_valid(ctx, poolid)) {
+        fprintf(stderr, "unknown cpupool '%s'\n", pool);
+        return EXIT_FAILURE;
+    }
+
+    params = argv[optind];
+
+    if (libxl_cpupool_set_parameters(ctx, poolid, params)) {
+        fprintf(stderr, "cannot set parameters: %s\n", params);
+        fprintf(stderr, "Use \"xl dmesg\" to look for possible reason.\n");
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 09/12] tools/xl: add support for setting generic per-cpupool parameters

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2052 bytes --]

On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
> Add a new xl command "cpupool-set-parameters" and cpupool config file
> support for setting per-cpupool generic parameters.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Seems good to me. Couple of questions.

Question one: what about getting (and displaying, I guess in
cpupoolinfo) the cpupool parameters?

> --- a/tools/xl/xl_cpupool.c
> +++ b/tools/xl/xl_cpupool.c
> @@ -615,6 +625,35 @@ out:
>      return rc;
>  }
>  
> +int main_cpupoolsetparameters(int argc, char **argv)
> +{
> +    int opt;
> +    const char *pool;
> +    char *params;
> +    uint32_t poolid;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-set-parameters", 2) {
> +        /* No options */
> +    }
> +
> +    pool = argv[optind++];
> +    if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid,
> NULL) ||
> +        !libxl_cpupoolid_is_valid(ctx, poolid)) {
> +        fprintf(stderr, "unknown cpupool '%s'\n", pool);
> +        return EXIT_FAILURE;
> +    }
> +
Since we know that we, AFAIUI, never allow changing the parameters for
a cpupool with domains in it already, shall we test this here, and bail
early, with a specific error message, without even trying going down in
Xen?

I know it's sort-of duplicating checks with what's in the hypervisor,
but I think it would be a common mistake, that it's worth trying to
prevent/address specifically.

> +    params = argv[optind];
> +
> +    if (libxl_cpupool_set_parameters(ctx, poolid, params)) {
> +        fprintf(stderr, "cannot set parameters: %s\n", params);
> +        fprintf(stderr, "Use \"xl dmesg\" to look for possible
> reason.\n");
> +        return EXIT_FAILURE;
> +    }
> +
> +    return EXIT_SUCCESS;
> +}
>
Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 09/12] tools/xl: add support for setting generic per-cpupool parameters

[Juergen Gross]

On 26/09/18 19:17, Dario Faggioli wrote:
> On Tue, 2018-09-18 at 08:03 +0200, Juergen Gross wrote:
>> Add a new xl command "cpupool-set-parameters" and cpupool config file
>> support for setting per-cpupool generic parameters.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> Seems good to me. Couple of questions.
> 
> Question one: what about getting (and displaying, I guess in
> cpupoolinfo) the cpupool parameters?
> 
>> --- a/tools/xl/xl_cpupool.c
>> +++ b/tools/xl/xl_cpupool.c
>> @@ -615,6 +625,35 @@ out:
>>      return rc;
>>  }
>>  
>> +int main_cpupoolsetparameters(int argc, char **argv)
>> +{
>> +    int opt;
>> +    const char *pool;
>> +    char *params;
>> +    uint32_t poolid;
>> +
>> +    SWITCH_FOREACH_OPT(opt, "", NULL, "cpupool-set-parameters", 2) {
>> +        /* No options */
>> +    }
>> +
>> +    pool = argv[optind++];
>> +    if (libxl_cpupool_qualifier_to_cpupoolid(ctx, pool, &poolid,
>> NULL) ||
>> +        !libxl_cpupoolid_is_valid(ctx, poolid)) {
>> +        fprintf(stderr, "unknown cpupool '%s'\n", pool);
>> +        return EXIT_FAILURE;
>> +    }
>> +
> Since we know that we, AFAIUI, never allow changing the parameters for
> a cpupool with domains in it already, shall we test this here, and bail
> early, with a specific error message, without even trying going down in
> Xen?
> 
> I know it's sort-of duplicating checks with what's in the hypervisor,
> but I think it would be a common mistake, that it's worth trying to
> prevent/address specifically.

That's exactly what the PARAM_FLAG_RUNTIME is meant for. I could think
of parameters which might be changeable even with active domains in the
cpupool. So I wouldn't like to test that in the tools as we would need
to add the knowledge of each parameter to the tools.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 10/12] tools/xl: add support for setting generic per-domain parameters

[Juergen Gross]

Add a new xl command "domain-set-parameters" and domain config file
support for setting per-domain generic parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.cfg.pod.5.in    | 12 ++++++++++++
 docs/man/xl.pod.1.in        | 19 ++++++++++++++++++
 tools/libxl/libxl.h         | 10 ++++++++++
 tools/libxl/libxl_create.c  | 10 ++++++++++
 tools/libxl/libxl_domain.c  | 47 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl |  1 +
 tools/xl/xl.h               |  1 +
 tools/xl/xl_cmdtable.c      |  6 ++++++
 tools/xl/xl_parse.c         |  3 +++
 tools/xl/xl_vmcontrol.c     | 25 ++++++++++++++++++++++++
 10 files changed, 134 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b72718151b..c97ae77129 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -604,6 +604,18 @@ option should only be used with a trusted device tree.
 Note that the partial device tree should avoid using the phandle 65000
 which is reserved by the toolstack.
 
+=item B<parameters="PARAMS">
+
+Specifies generic parameters for the domain. C<PARAMS> is a list of
+parameter settings in the form of "name[=value]" separated by blanks.
+The following parameter settings are supported:
+
+=over 4
+
+NONE
+
+=back
+
 =back
 
 =head2 Devices
diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index a2ddc4b2e0..7ab3e304d7 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -263,6 +263,25 @@ using a hardware domain separated from domain 0.
 
 =back
 
+=item B<domain-set-parameters> [I<OPTIONS>] I<domain-id> I<params>
+
+Sets generic parameters I<params> for I<domain-id>.
+
+See the L<xl.cfg(5)> manpage for supported parameters.
+
+B<OPTIONS>
+
+=over 4
+
+=item I<-t>
+
+Set the parameter only temporary. The parameter isn't added to the domain
+config data, so it won't persist across migration or domain save/restore.
+
+=back
+
+=back
+
 =item B<domid> I<domain-name>
 
 Converts a domain name to a domain id.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index bd26d9fd4a..a3c87407a5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1164,6 +1164,13 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_PVCALLS 1
 
+/*
+ * LIBXL_HAVE_DOMAIN_SET_PARAMETERS
+ *
+ * If this is defined setting per-domain parameters is supported.
+ */
+#define LIBXL_HAVE_DOMAIN_SET_PARAMETERS 1
+
 /*
  * LIBXL_HAVE_CPUPOOL_SET_PARAMETERS
  *
@@ -1554,6 +1561,9 @@ int libxl_domain_destroy(libxl_ctx *ctx, uint32_t domid,
                          LIBXL_EXTERNAL_CALLERS_ONLY;
 int libxl_domain_preserve(libxl_ctx *ctx, uint32_t domid, libxl_domain_create_info *info, const char *name_suffix, libxl_uuid new_uuid);
 
+int libxl_domain_set_parameters(libxl_ctx *ctx, uint32_t domid, char *params,
+                                bool temp);
+
 /* get max. number of cpus supported by hypervisor */
 int libxl_get_max_cpus(libxl_ctx *ctx);
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index dcfde7787e..1eb0e8f639 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -597,6 +597,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             goto out;
         }
 
+        if (d_config->b_info.parameters) {
+            ret = xc_set_domain_parameters(ctx->xch, *domid,
+                                           d_config->b_info.parameters);
+            if (ret < 0) {
+                LOGED(ERROR, *domid, "fail to set domain parameters");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+        }
+
         rc = libxl__arch_domain_save_config(gc, d_config, state, &create);
         if (rc < 0)
             goto out;
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 3377bba994..e0f78ac489 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1773,6 +1773,53 @@ out:
     return rc;
 }
 
+int libxl_domain_set_parameters(libxl_ctx *ctx, uint32_t domid, char *params,
+                                bool temp)
+{
+    libxl__domain_userdata_lock *lock = NULL;
+    libxl_domain_config d_config;
+    GC_INIT(ctx);
+    int rc;
+
+    libxl_domain_config_init(&d_config);
+
+    rc = xc_set_domain_parameters(ctx->xch, domid, params);
+    if (rc) {
+        LOGEV(ERROR, rc, "Error setting domain parameters");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (!temp) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
+
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
+
+        if (d_config.b_info.parameters) {
+            d_config.b_info.parameters =
+                libxl__realloc(gc, d_config.b_info.parameters,
+                               strlen(d_config.b_info.parameters) +
+                               strlen(params) + 2);
+            strcat(d_config.b_info.parameters, " ");
+            strcat(d_config.b_info.parameters, params);
+        } else
+            d_config.b_info.parameters = libxl__strdup(gc, params);
+
+        rc = libxl__set_domain_configuration(gc, domid, &d_config);
+    }
+
+out:
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_domain_config_dispose(&d_config);
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 2cceb8c057..382a6c1d6e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -488,6 +488,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
     ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+    ("parameters",      string),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index a51acc4256..3153a35f22 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -151,6 +151,7 @@ int main_sched_credit2(int argc, char **argv);
 int main_sched_rtds(int argc, char **argv);
 int main_domid(int argc, char **argv);
 int main_domname(int argc, char **argv);
+int main_domainsetparameters(int argc, char **argv);
 int main_rename(int argc, char **argv);
 int main_trigger(int argc, char **argv);
 int main_sysrq(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 3a469dacc3..41e5bd27a3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -285,6 +285,12 @@ struct cmd_spec cmd_table[] = {
       "-b BUDGET, --budget=BUDGET     Budget (us)\n"
       "-e Extratime, --extratime=Extratime Extratime (1=yes, 0=no)\n"
     },
+    { "domain-set-parameters",
+      &main_domainsetparameters, 0, 1,
+      "Sets generic parameters for a domain",
+      "[-t] <Domain> <Params>",
+      "  -t                           Set parameter only temporary",
+    },
     { "domid",
       &main_domid, 0, 0,
       "Convert a domain name to domain id",
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 971ec1bc56..0a6fe141b9 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1558,6 +1558,9 @@ void parse_config_data(const char *config_source,
 
     xlu_cfg_get_defbool(config, "nestedhvm", &b_info->nested_hvm, 0);
 
+    if (!xlu_cfg_get_string (config, "parameters", &buf, 0))
+        xlu_cfg_replace_string(config, "parameters", &b_info->parameters, 0);
+
     switch(b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         kernel_basename = libxl_basename(b_info->kernel);
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index a1d633795c..8fa2338621 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -275,6 +275,31 @@ int main_reboot(int argc, char **argv)
     return main_shutdown_or_reboot(1, argc, argv);
 }
 
+int main_domainsetparameters(int argc, char **argv)
+{
+    int opt;
+    char *params;
+    uint32_t domid;
+    bool temp = false;
+
+    SWITCH_FOREACH_OPT(opt, "t", NULL, "domain-set-parameters", 2) {
+    case 't':
+        temp = true;
+        break;
+    }
+
+    domid = find_domain(argv[optind]);
+    params = argv[++optind];
+
+    if (libxl_domain_set_parameters(ctx, domid, params, temp)) {
+        fprintf(stderr, "cannot set parameters: %s\n", params);
+        fprintf(stderr, "Use \"xl dmesg\" to look for possible reason.\n");
+        return EXIT_FAILURE;
+    }
+
+    return EXIT_SUCCESS;
+}
+
 static void evdisable_disk_ejects(libxl_evgen_disk_eject **diskws,
                                  int num_disks)
 {
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 11/12] x86: add domain type flags for domain parameters

[Juergen Gross]

Add flags for excluding pv- or hvm-domains for domain parameters.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/domain.c        | 5 +++++
 xen/include/asm-x86/domain.h | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8e57e7a181..557ebd1b87 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2108,6 +2108,11 @@ __initcall(init_vcpu_kick_softirq);
 
 int arch_domain_check_parflags(struct domain *d, unsigned int flags)
 {
+    if ( flags & (is_hvm_domain(d) ? PARAM_FLAG_NOHVM
+                                   : (is_pv_32bit_domain(d) ?
+                                      PARAM_FLAG_NOPV32 : PARAM_FLAG_NOPV64)) )
+        return -EINVAL;
+
     return 0;
 }
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4da4353de7..57d15ed3cf 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -286,6 +286,12 @@ struct monitor_write_data {
     uint64_t cr4;
 };
 
+/* Flags for excluding domain types from per-domain parameters. */
+#define PARAM_FLAG_NOHVM   0x00000001
+#define PARAM_FLAG_NOPV32  0x00000002
+#define PARAM_FLAG_NOPV64  0x00000004
+#define PARAM_FLAG_NOPV    (PARAM_FLAG_NOPV32 | PARAM_FLAG_NOPV64)
+
 struct arch_domain
 {
     struct page_info *perdomain_l3_pg;
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 12/12] x86/xpti: add per-domain parameter for controlling xpti

[Juergen Gross]

Add a per-domain parameter to switch xpti for single pv domains on or
off.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.cfg.pod.5.in | 12 +++++++++++-
 xen/arch/x86/pv/domain.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index c97ae77129..524f9f23d4 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -612,7 +612,17 @@ The following parameter settings are supported:
 
 =over 4
 
-NONE
+=item B<xpti=BOOLEAN>
+
+Override default selection of whether to isolate 64-bit PV guest page
+tables of a domain.
+
+B<true> activates page table isolation even on hardware not vulnerable by
+Meltdown for the domain.
+
+B<false> deactivates page table isolation on all systems for the domain.
+
+Can be modified at runtime.
 
 =back
 
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 355f320fa3..86d9f8fb67 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -287,6 +287,43 @@ int pv_domain_initialise(struct domain *d)
     return rc;
 }
 
+static int dompar_xpti(const char *s, struct domain *d)
+{
+    switch ( parse_bool(s, NULL) )
+    {
+    case 0:
+        d->arch.pv.xpti = false;
+        break;
+
+    case 1:
+        d->arch.pv.xpti = true;
+        break;
+
+    default:
+        return -EINVAL;
+    }
+
+    switch ( opt_pcid )
+    {
+    case PCID_XPTI:
+        d->arch.pv.pcid = d->arch.pv.xpti;
+        break;
+
+    case PCID_NOXPTI:
+        d->arch.pv.pcid = !d->arch.pv.xpti;
+        break;
+
+    default:
+        break;
+    }
+
+    return 0;
+}
+
+custom_domain_param("xpti",
+                    PARAM_FLAG_RUNTIME | PARAM_FLAG_NOHVM | PARAM_FLAG_NOPV32,
+                    dompar_xpti);
+
 bool __init xpti_pcid_enabled(void)
 {
     return use_invpcid && cpu_has_pcid &&
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
> Instead of using binary hypervisor interfaces for new parameters of
> domains or cpupools this patch series adds support for generic text
> based parameter parsing.
> 
> Parameters are defined via new macros similar to those of boot
> parameters. Parsing of parameter strings is done via the already
> existing boot parameter parsing function which is extended a little
> bit.
> 
> Parameter settings can either be specified in configuration files of
> domains or cpupools, or they can be set via new xl sub-commands.

Without having looked at any of the patches yet (not even their
descriptions) I'm still wondering what the benefit of textual parameters
really is: Just like "binary" ones, they become part of the public
interface, and hence subsequently can't be changed any more or
less than the ones we currently have (in particular, anything valid in
a guest config file will imo need to remain to be valid and meaningful
down the road).

If this is solely or mainly about deferring the parsing from the tool
stack to the hypervisor, then I'm not at all convinced of the approach
(I'd even be tempted to call it backwards).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 12:32, Jan Beulich wrote:
>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>> Instead of using binary hypervisor interfaces for new parameters of
>> domains or cpupools this patch series adds support for generic text
>> based parameter parsing.
>>
>> Parameters are defined via new macros similar to those of boot
>> parameters. Parsing of parameter strings is done via the already
>> existing boot parameter parsing function which is extended a little
>> bit.
>>
>> Parameter settings can either be specified in configuration files of
>> domains or cpupools, or they can be set via new xl sub-commands.
> 
> Without having looked at any of the patches yet (not even their
> descriptions) I'm still wondering what the benefit of textual parameters
> really is: Just like "binary" ones, they become part of the public
> interface, and hence subsequently can't be changed any more or
> less than the ones we currently have (in particular, anything valid in
> a guest config file will imo need to remain to be valid and meaningful
> down the road).

So lets look what would be needed for adding something like the
per-domain xpti parameter using binary interfaces:

1 an extension of some domctl interface, maybe bumping of the domctl
  interface version
2 adding the logic to domctl handling
3 adding libxc support
4 adding libxl support
5 adding a new xl sub-command
6 adding domain config support
7 adding documentation

With my approach only 2 (in a modified form, parameter handling instead
of domctl, but comparable in the needed effort) and 7 are needed.

So once the framework is in place it is _much_ easier to add new
features.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 12:10 PM, Juergen Gross wrote:
> On 18/09/18 12:32, Jan Beulich wrote:
>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>> Instead of using binary hypervisor interfaces for new parameters of
>>> domains or cpupools this patch series adds support for generic text
>>> based parameter parsing.
>>>
>>> Parameters are defined via new macros similar to those of boot
>>> parameters. Parsing of parameter strings is done via the already
>>> existing boot parameter parsing function which is extended a little
>>> bit.
>>>
>>> Parameter settings can either be specified in configuration files of
>>> domains or cpupools, or they can be set via new xl sub-commands.
>>
>> Without having looked at any of the patches yet (not even their
>> descriptions) I'm still wondering what the benefit of textual parameters
>> really is: Just like "binary" ones, they become part of the public
>> interface, and hence subsequently can't be changed any more or
>> less than the ones we currently have (in particular, anything valid in
>> a guest config file will imo need to remain to be valid and meaningful
>> down the road).
> 
> So lets look what would be needed for adding something like the
> per-domain xpti parameter using binary interfaces:
> 
> 1 an extension of some domctl interface, maybe bumping of the domctl
>   interface version
> 2 adding the logic to domctl handling
> 3 adding libxc support
> 4 adding libxl support
> 5 adding a new xl sub-command
> 6 adding domain config support
> 7 adding documentation
> 
> With my approach only 2 (in a modified form, parameter handling instead
> of domctl, but comparable in the needed effort) and 7 are needed.
> 
> So once the framework is in place it is _much_ easier to add new
> features.

So the idea here is that you pass a full hypervisor command-line style
string into a hypercall -- say, "credit2_cap_period_ms=5"?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:18, George Dunlap wrote:
> On 09/18/2018 12:10 PM, Juergen Gross wrote:
>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>> domains or cpupools this patch series adds support for generic text
>>>> based parameter parsing.
>>>>
>>>> Parameters are defined via new macros similar to those of boot
>>>> parameters. Parsing of parameter strings is done via the already
>>>> existing boot parameter parsing function which is extended a little
>>>> bit.
>>>>
>>>> Parameter settings can either be specified in configuration files of
>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>
>>> Without having looked at any of the patches yet (not even their
>>> descriptions) I'm still wondering what the benefit of textual parameters
>>> really is: Just like "binary" ones, they become part of the public
>>> interface, and hence subsequently can't be changed any more or
>>> less than the ones we currently have (in particular, anything valid in
>>> a guest config file will imo need to remain to be valid and meaningful
>>> down the road).
>>
>> So lets look what would be needed for adding something like the
>> per-domain xpti parameter using binary interfaces:
>>
>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>   interface version
>> 2 adding the logic to domctl handling
>> 3 adding libxc support
>> 4 adding libxl support
>> 5 adding a new xl sub-command
>> 6 adding domain config support
>> 7 adding documentation
>>
>> With my approach only 2 (in a modified form, parameter handling instead
>> of domctl, but comparable in the needed effort) and 7 are needed.
>>
>> So once the framework is in place it is _much_ easier to add new
>> features.
> 
> So the idea here is that you pass a full hypervisor command-line style
> string into a hypercall -- say, "credit2_cap_period_ms=5"?

Yes.

I guess it would be questionable to have multiple ways to set a
property, so only new properties should be supported.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
> On 18/09/18 12:32, Jan Beulich wrote:
>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>> Instead of using binary hypervisor interfaces for new parameters of
>>> domains or cpupools this patch series adds support for generic text
>>> based parameter parsing.
>>>
>>> Parameters are defined via new macros similar to those of boot
>>> parameters. Parsing of parameter strings is done via the already
>>> existing boot parameter parsing function which is extended a little
>>> bit.
>>>
>>> Parameter settings can either be specified in configuration files of
>>> domains or cpupools, or they can be set via new xl sub-commands.
>> 
>> Without having looked at any of the patches yet (not even their
>> descriptions) I'm still wondering what the benefit of textual parameters
>> really is: Just like "binary" ones, they become part of the public
>> interface, and hence subsequently can't be changed any more or
>> less than the ones we currently have (in particular, anything valid in
>> a guest config file will imo need to remain to be valid and meaningful
>> down the road).
> 
> So lets look what would be needed for adding something like the
> per-domain xpti parameter using binary interfaces:
> 
> 1 an extension of some domctl interface, maybe bumping of the domctl
>   interface version
> 2 adding the logic to domctl handling
> 3 adding libxc support
> 4 adding libxl support
> 5 adding a new xl sub-command
> 6 adding domain config support
> 7 adding documentation
> 
> With my approach only 2 (in a modified form, parameter handling instead
> of domctl, but comparable in the needed effort) and 7 are needed.
> 
> So once the framework is in place it is _much_ easier to add new
> features.

All the above would hold if the individual options were expressed as
e.g. flags in an extensible bit vector.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 19/09/18 19:28, Wei Liu wrote:
> On Tue, Sep 18, 2018 at 02:25:04PM +0100, George Dunlap wrote:
>> On 09/18/2018 12:32 PM, Juergen Gross wrote:
>>> On 18/09/18 13:20, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 13:10, <jgross@suse.com> wrote:
>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>> based parameter parsing.
>>>>>>>
>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>> bit.
>>>>>>>
>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>
>>>>>> Without having looked at any of the patches yet (not even their
>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>> down the road).
>>>>>
>>>>> So lets look what would be needed for adding something like the
>>>>> per-domain xpti parameter using binary interfaces:
>>>>>
>>>>> 1 an extension of some domctl interface, maybe bumping of the domctl
>>>>>   interface version
>>>>> 2 adding the logic to domctl handling
>>>>> 3 adding libxc support
>>>>> 4 adding libxl support
>>>>> 5 adding a new xl sub-command
>>>>> 6 adding domain config support
>>>>> 7 adding documentation
>>>>>
>>>>> With my approach only 2 (in a modified form, parameter handling instead
>>>>> of domctl, but comparable in the needed effort) and 7 are needed.
>>>>>
>>>>> So once the framework is in place it is _much_ easier to add new
>>>>> features.
>>>>
>>>> All the above would hold if the individual options were expressed as
>>>> e.g. flags in an extensible bit vector.
>>>
>>> Who would translate the new option into a bit vector? This would be the
>>> tools (libxc/libxl/xl), so those need to be modified for each new bit.
>>
>> A bit vector would only allow on/off; it wouldn't allow you to set
>> numeric parameters, for example.
>>
>> I like the idea of being able to add configuration parameters without
>> having a huge amount of boilerplate; and also of being able to backport
>> parameters like xpti without having to worry so much about compatibility.
>>
>> But I'm not a fan of the idea of using a "string blob" to accomplish
>> that.  It's convenient for the exact use case you seem to be
>> contemplating: having a user add the string into the xl config file, and
>> having nobody but the hypervisor interpret it.  But what about tools
>> that may want to set that parameter?  Or tools that want to query the
>> parameter, or "introspect" on the domain settings or whatever?  Now they
>> have to have a bunch of code to generate and parse the string code.
>>
> 
> Having the ability to query parameters is a must. Suppose you change
> some settings while the domain is running, in order to re-create domain
> with the same parameter (migration) there must be a way for toolstack to
> query the current settings of that domain. I think most if not all
> information is retrieved from xen using binary interface.
> 
> Furthermore, if the string blob is not stored in xen, and there isn't a
> binary interface for *setting* parameters, toolstack will have to
> translate the retrieved binary information into strings again.
> 
> I'm not picky about formats, but please make get and set interfaces
> symmetric (use the same representation).

Did you look into the patches, especially patch 10? The parameters set
are all stored in domain config via libxl__arch_domain_save_config().

This is skipped only with the user specifying a special flag for
"xl domain-set-parameters".


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Wed, Sep 19, 2018 at 07:58:50PM +0200, Juergen Gross wrote:
> 
> Did you look into the patches, especially patch 10? The parameters set
> are all stored in domain config via libxl__arch_domain_save_config().

No, I didn't.

I think the general idea of what you do in patch 10 should work. However
I want to comment on the implementation.

It appears that the implementation in patch 10 concatenates the new
settings to the old ones. It is not very nice imo.

If for the life time of the domain you set X times the same parameter
you get a string of foo=bar1 foo=bar2 in the saved config file.

There is probably a simple solution: make the parameter list in IDL a
key value list. You then update the list accordingly.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 21/09/18 10:52, Wei Liu wrote:
> On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
>> On 20/09/18 18:06, Wei Liu wrote:
>>> On Wed, Sep 19, 2018 at 07:58:50PM +0200, Juergen Gross wrote:
>>>>
>>>> Did you look into the patches, especially patch 10? The parameters set
>>>> are all stored in domain config via libxl__arch_domain_save_config().
>>>
>>> No, I didn't.
>>>
>>> I think the general idea of what you do in patch 10 should work. However
>>> I want to comment on the implementation.
>>>
>>> It appears that the implementation in patch 10 concatenates the new
>>> settings to the old ones. It is not very nice imo.
>>>
>>> If for the life time of the domain you set X times the same parameter
>>> you get a string of foo=bar1 foo=bar2 in the saved config file.
>>>
>>> There is probably a simple solution: make the parameter list in IDL a
>>> key value list. You then update the list accordingly.
>>
>> The problem with that approach are parameters with sub-parameters:
>>
>> par=sub1=no,sub2=yes
>> par=sub2=yes
> 
> That means the value type of the top level key value list should ideally
> be another key value list. I do notice the limitation in the key value
> list type: the value can only be string.
> 
> There is another way to solve this: further parse the sub-parameters.
> This doesn't require any parameter specific knowledge and there are
> already functions to split strings.

I don't think this will work for the general case. It might be that

par=no

will switch off all sub-parameters. How would you handle that?

I'm looking into a way to report the current parameter settings.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Wei Liu]

On Thu, Sep 27, 2018 at 07:58:39AM +0200, Juergen Gross wrote:
> On 21/09/18 10:52, Wei Liu wrote:
> > On Fri, Sep 21, 2018 at 07:23:23AM +0200, Juergen Gross wrote:
> >> On 20/09/18 18:06, Wei Liu wrote:
> >>> On Wed, Sep 19, 2018 at 07:58:50PM +0200, Juergen Gross wrote:
> >>>>
> >>>> Did you look into the patches, especially patch 10? The parameters set
> >>>> are all stored in domain config via libxl__arch_domain_save_config().
> >>>
> >>> No, I didn't.
> >>>
> >>> I think the general idea of what you do in patch 10 should work. However
> >>> I want to comment on the implementation.
> >>>
> >>> It appears that the implementation in patch 10 concatenates the new
> >>> settings to the old ones. It is not very nice imo.
> >>>
> >>> If for the life time of the domain you set X times the same parameter
> >>> you get a string of foo=bar1 foo=bar2 in the saved config file.
> >>>
> >>> There is probably a simple solution: make the parameter list in IDL a
> >>> key value list. You then update the list accordingly.
> >>
> >> The problem with that approach are parameters with sub-parameters:
> >>
> >> par=sub1=no,sub2=yes
> >> par=sub2=yes
> > 
> > That means the value type of the top level key value list should ideally
> > be another key value list. I do notice the limitation in the key value
> > list type: the value can only be string.
> > 
> > There is another way to solve this: further parse the sub-parameters.
> > This doesn't require any parameter specific knowledge and there are
> > already functions to split strings.
> 
> I don't think this will work for the general case. It might be that
> 
> par=no
> 
> will switch off all sub-parameters. How would you handle that?

Isn't that what it is supposed to do? Do you want par=no to not turn par
completely off but leave part(s) of par on? Then how do you turn par off
completely?

Wei.

> 
> I'm looking into a way to report the current parameter settings.
> 
> 
> Juergen
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 12:32, Jan Beulich wrote:
>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>> Instead of using binary hypervisor interfaces for new parameters of
>> domains or cpupools this patch series adds support for generic text
>> based parameter parsing.
>>
>> Parameters are defined via new macros similar to those of boot
>> parameters. Parsing of parameter strings is done via the already
>> existing boot parameter parsing function which is extended a little
>> bit.
>>
>> Parameter settings can either be specified in configuration files of
>> domains or cpupools, or they can be set via new xl sub-commands.
> 
> Without having looked at any of the patches yet (not even their
> descriptions) I'm still wondering what the benefit of textual parameters
> really is: Just like "binary" ones, they become part of the public
> interface, and hence subsequently can't be changed any more or
> less than the ones we currently have (in particular, anything valid in
> a guest config file will imo need to remain to be valid and meaningful
> down the road).
> 
> If this is solely or mainly about deferring the parsing from the tool
> stack to the hypervisor, then I'm not at all convinced of the approach
> (I'd even be tempted to call it backwards).

The main advantage is that it would be much easier to backport new
parameters like the xpti per-domain one. No need to bump sysctl/domctl
interface versions for that.

It might be a good idea to support mandatory and optional parameters
in the guest config. Optional parameters not supported by the hypervisor
would then be ignored instead of leading to failure at guest creation
time.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
> On 18/09/18 12:32, Jan Beulich wrote:
>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>> Instead of using binary hypervisor interfaces for new parameters of
>>> domains or cpupools this patch series adds support for generic text
>>> based parameter parsing.
>>>
>>> Parameters are defined via new macros similar to those of boot
>>> parameters. Parsing of parameter strings is done via the already
>>> existing boot parameter parsing function which is extended a little
>>> bit.
>>>
>>> Parameter settings can either be specified in configuration files of
>>> domains or cpupools, or they can be set via new xl sub-commands.
>> 
>> Without having looked at any of the patches yet (not even their
>> descriptions) I'm still wondering what the benefit of textual parameters
>> really is: Just like "binary" ones, they become part of the public
>> interface, and hence subsequently can't be changed any more or
>> less than the ones we currently have (in particular, anything valid in
>> a guest config file will imo need to remain to be valid and meaningful
>> down the road).
>> 
>> If this is solely or mainly about deferring the parsing from the tool
>> stack to the hypervisor, then I'm not at all convinced of the approach
>> (I'd even be tempted to call it backwards).
> 
> The main advantage is that it would be much easier to backport new
> parameters like the xpti per-domain one. No need to bump sysctl/domctl
> interface versions for that.

Additions to sysctl/domctl interfaces don't require such a bump.

> It might be a good idea to support mandatory and optional parameters
> in the guest config. Optional parameters not supported by the hypervisor
> would then be ignored instead of leading to failure at guest creation
> time.

Except that over time opinions may change what is supposed to
be optional vs mandatory.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>> domains or cpupools this patch series adds support for generic text
>>>> based parameter parsing.
>>>>
>>>> Parameters are defined via new macros similar to those of boot
>>>> parameters. Parsing of parameter strings is done via the already
>>>> existing boot parameter parsing function which is extended a little
>>>> bit.
>>>>
>>>> Parameter settings can either be specified in configuration files of
>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>
>>> Without having looked at any of the patches yet (not even their
>>> descriptions) I'm still wondering what the benefit of textual parameters
>>> really is: Just like "binary" ones, they become part of the public
>>> interface, and hence subsequently can't be changed any more or
>>> less than the ones we currently have (in particular, anything valid in
>>> a guest config file will imo need to remain to be valid and meaningful
>>> down the road).
>>>
>>> If this is solely or mainly about deferring the parsing from the tool
>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>> (I'd even be tempted to call it backwards).
>>
>> The main advantage is that it would be much easier to backport new
>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>> interface versions for that.
> 
> Additions to sysctl/domctl interfaces don't require such a bump.
> 
>> It might be a good idea to support mandatory and optional parameters
>> in the guest config. Optional parameters not supported by the hypervisor
>> would then be ignored instead of leading to failure at guest creation
>> time.
> 
> Except that over time opinions may change what is supposed to
> be optional vs mandatory.

I thought the idea would be that the admin would specify which ones were
optional or mandatory.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 13:20, <george.dunlap@citrix.com> wrote:
> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>> domains or cpupools this patch series adds support for generic text
>>>>> based parameter parsing.
>>>>>
>>>>> Parameters are defined via new macros similar to those of boot
>>>>> parameters. Parsing of parameter strings is done via the already
>>>>> existing boot parameter parsing function which is extended a little
>>>>> bit.
>>>>>
>>>>> Parameter settings can either be specified in configuration files of
>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>
>>>> Without having looked at any of the patches yet (not even their
>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>> really is: Just like "binary" ones, they become part of the public
>>>> interface, and hence subsequently can't be changed any more or
>>>> less than the ones we currently have (in particular, anything valid in
>>>> a guest config file will imo need to remain to be valid and meaningful
>>>> down the road).
>>>>
>>>> If this is solely or mainly about deferring the parsing from the tool
>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>> (I'd even be tempted to call it backwards).
>>>
>>> The main advantage is that it would be much easier to backport new
>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>> interface versions for that.
>> 
>> Additions to sysctl/domctl interfaces don't require such a bump.
>> 
>>> It might be a good idea to support mandatory and optional parameters
>>> in the guest config. Optional parameters not supported by the hypervisor
>>> would then be ignored instead of leading to failure at guest creation
>>> time.
>> 
>> Except that over time opinions may change what is supposed to
>> be optional vs mandatory.
> 
> I thought the idea would be that the admin would specify which ones were
> optional or mandatory.

If this was admin controlled, there would be no way to encode in
the hypercall handler which ones to reject when unknown. Even
without admin involvement it's not really clear to me how options
we don't even know of today could be treated as either optional
or mandatory.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[George Dunlap]

On 09/18/2018 12:23 PM, Jan Beulich wrote:
>>>> On 18.09.18 at 13:20, <george.dunlap@citrix.com> wrote:
>> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>> based parameter parsing.
>>>>>>
>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>> existing boot parameter parsing function which is extended a little
>>>>>> bit.
>>>>>>
>>>>>> Parameter settings can either be specified in configuration files of
>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>
>>>>> Without having looked at any of the patches yet (not even their
>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>> really is: Just like "binary" ones, they become part of the public
>>>>> interface, and hence subsequently can't be changed any more or
>>>>> less than the ones we currently have (in particular, anything valid in
>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>> down the road).
>>>>>
>>>>> If this is solely or mainly about deferring the parsing from the tool
>>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>>> (I'd even be tempted to call it backwards).
>>>>
>>>> The main advantage is that it would be much easier to backport new
>>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>>> interface versions for that.
>>>
>>> Additions to sysctl/domctl interfaces don't require such a bump.
>>>
>>>> It might be a good idea to support mandatory and optional parameters
>>>> in the guest config. Optional parameters not supported by the hypervisor
>>>> would then be ignored instead of leading to failure at guest creation
>>>> time.
>>>
>>> Except that over time opinions may change what is supposed to
>>> be optional vs mandatory.
>>
>> I thought the idea would be that the admin would specify which ones were
>> optional or mandatory.
> 
> If this was admin controlled, there would be no way to encode in
> the hypercall handler which ones to reject when unknown. Even
> without admin involvement it's not really clear to me how options
> we don't even know of today could be treated as either optional
> or mandatory.

My interpretation was the hypervisor would always return "-ENOSYS" (or
whatever) when passed an unknown option, and the toolstack would decide
what to do about it -- whether to simply throw a warning or stop
creation of the domain.  That way in some configs you could write:

# Disable xpti if it's available, otherwise just run
optional_params=['xpti=off']

and other configs you could write:

# Only run if we're certain we have xpti enabled
mandatory_params=['xpti=on']

The toolstack would attempt to enable / disable xpti during domain
creation, and DTRT if the hypercall failed.

(I admit I haven't looked at the series to see if that's compatible with
what's planned.)

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:29, George Dunlap wrote:
> On 09/18/2018 12:23 PM, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:20, <george.dunlap@citrix.com> wrote:
>>> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>> based parameter parsing.
>>>>>>>
>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>> bit.
>>>>>>>
>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>
>>>>>> Without having looked at any of the patches yet (not even their
>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>> down the road).
>>>>>>
>>>>>> If this is solely or mainly about deferring the parsing from the tool
>>>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>>>> (I'd even be tempted to call it backwards).
>>>>>
>>>>> The main advantage is that it would be much easier to backport new
>>>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>>>> interface versions for that.
>>>>
>>>> Additions to sysctl/domctl interfaces don't require such a bump.
>>>>
>>>>> It might be a good idea to support mandatory and optional parameters
>>>>> in the guest config. Optional parameters not supported by the hypervisor
>>>>> would then be ignored instead of leading to failure at guest creation
>>>>> time.
>>>>
>>>> Except that over time opinions may change what is supposed to
>>>> be optional vs mandatory.
>>>
>>> I thought the idea would be that the admin would specify which ones were
>>> optional or mandatory.
>>
>> If this was admin controlled, there would be no way to encode in
>> the hypercall handler which ones to reject when unknown. Even
>> without admin involvement it's not really clear to me how options
>> we don't even know of today could be treated as either optional
>> or mandatory.
> 
> My interpretation was the hypervisor would always return "-ENOSYS" (or
> whatever) when passed an unknown option, and the toolstack would decide
> what to do about it -- whether to simply throw a warning or stop
> creation of the domain.  That way in some configs you could write:
> 
> # Disable xpti if it's available, otherwise just run
> optional_params=['xpti=off']
> 
> and other configs you could write:
> 
> # Only run if we're certain we have xpti enabled
> mandatory_params=['xpti=on']
> 
> The toolstack would attempt to enable / disable xpti during domain
> creation, and DTRT if the hypercall failed.

That was the idea, right.

> (I admit I haven't looked at the series to see if that's compatible with
> what's planned.)

The series is implementing mandatory params only at the moment, but that
would be easy to expand.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 13:29, <george.dunlap@citrix.com> wrote:
> On 09/18/2018 12:23 PM, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:20, <george.dunlap@citrix.com> wrote:
>>> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>>> based parameter parsing.
>>>>>>>
>>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>>> existing boot parameter parsing function which is extended a little
>>>>>>> bit.
>>>>>>>
>>>>>>> Parameter settings can either be specified in configuration files of
>>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>>
>>>>>> Without having looked at any of the patches yet (not even their
>>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>>> really is: Just like "binary" ones, they become part of the public
>>>>>> interface, and hence subsequently can't be changed any more or
>>>>>> less than the ones we currently have (in particular, anything valid in
>>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>>> down the road).
>>>>>>
>>>>>> If this is solely or mainly about deferring the parsing from the tool
>>>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>>>> (I'd even be tempted to call it backwards).
>>>>>
>>>>> The main advantage is that it would be much easier to backport new
>>>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>>>> interface versions for that.
>>>>
>>>> Additions to sysctl/domctl interfaces don't require such a bump.
>>>>
>>>>> It might be a good idea to support mandatory and optional parameters
>>>>> in the guest config. Optional parameters not supported by the hypervisor
>>>>> would then be ignored instead of leading to failure at guest creation
>>>>> time.
>>>>
>>>> Except that over time opinions may change what is supposed to
>>>> be optional vs mandatory.
>>>
>>> I thought the idea would be that the admin would specify which ones were
>>> optional or mandatory.
>> 
>> If this was admin controlled, there would be no way to encode in
>> the hypercall handler which ones to reject when unknown. Even
>> without admin involvement it's not really clear to me how options
>> we don't even know of today could be treated as either optional
>> or mandatory.
> 
> My interpretation was the hypervisor would always return "-ENOSYS" (or
> whatever) when passed an unknown option, and the toolstack would decide
> what to do about it -- whether to simply throw a warning or stop
> creation of the domain.  That way in some configs you could write:
> 
> # Disable xpti if it's available, otherwise just run
> optional_params=['xpti=off']
> 
> and other configs you could write:
> 
> # Only run if we're certain we have xpti enabled
> mandatory_params=['xpti=on']
> 
> The toolstack would attempt to enable / disable xpti during domain
> creation, and DTRT if the hypercall failed.

Oh, I see - a completely different meaning of "mandatory": I was
assuming some options (say "xpti") would be considered mandatory
by the implementation.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:20, George Dunlap wrote:
> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>> domains or cpupools this patch series adds support for generic text
>>>>> based parameter parsing.
>>>>>
>>>>> Parameters are defined via new macros similar to those of boot
>>>>> parameters. Parsing of parameter strings is done via the already
>>>>> existing boot parameter parsing function which is extended a little
>>>>> bit.
>>>>>
>>>>> Parameter settings can either be specified in configuration files of
>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>
>>>> Without having looked at any of the patches yet (not even their
>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>> really is: Just like "binary" ones, they become part of the public
>>>> interface, and hence subsequently can't be changed any more or
>>>> less than the ones we currently have (in particular, anything valid in
>>>> a guest config file will imo need to remain to be valid and meaningful
>>>> down the road).
>>>>
>>>> If this is solely or mainly about deferring the parsing from the tool
>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>> (I'd even be tempted to call it backwards).
>>>
>>> The main advantage is that it would be much easier to backport new
>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>> interface versions for that.
>>
>> Additions to sysctl/domctl interfaces don't require such a bump.
>>
>>> It might be a good idea to support mandatory and optional parameters
>>> in the guest config. Optional parameters not supported by the hypervisor
>>> would then be ignored instead of leading to failure at guest creation
>>> time.
>>
>> Except that over time opinions may change what is supposed to
>> be optional vs mandatory.
> 
> I thought the idea would be that the admin would specify which ones were
> optional or mandatory.

Right.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:19, Jan Beulich wrote:
>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>> domains or cpupools this patch series adds support for generic text
>>>> based parameter parsing.
>>>>
>>>> Parameters are defined via new macros similar to those of boot
>>>> parameters. Parsing of parameter strings is done via the already
>>>> existing boot parameter parsing function which is extended a little
>>>> bit.
>>>>
>>>> Parameter settings can either be specified in configuration files of
>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>
>>> Without having looked at any of the patches yet (not even their
>>> descriptions) I'm still wondering what the benefit of textual parameters
>>> really is: Just like "binary" ones, they become part of the public
>>> interface, and hence subsequently can't be changed any more or
>>> less than the ones we currently have (in particular, anything valid in
>>> a guest config file will imo need to remain to be valid and meaningful
>>> down the road).
>>>
>>> If this is solely or mainly about deferring the parsing from the tool
>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>> (I'd even be tempted to call it backwards).
>>
>> The main advantage is that it would be much easier to backport new
>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>> interface versions for that.
> 
> Additions to sysctl/domctl interfaces don't require such a bump.

What are XEN_DOMCTL_INTERFACE_VERSION and XEN_SYSCTL_INTERFACE_VERSION
for then?

> 
>> It might be a good idea to support mandatory and optional parameters
>> in the guest config. Optional parameters not supported by the hypervisor
>> would then be ignored instead of leading to failure at guest creation
>> time.
> 
> Except that over time opinions may change what is supposed to
> be optional vs mandatory.

And that would change how in case of binary interfaces?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Jan Beulich]

>>> On 18.09.18 at 13:26, <jgross@suse.com> wrote:
> On 18/09/18 13:19, Jan Beulich wrote:
>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>> domains or cpupools this patch series adds support for generic text
>>>>> based parameter parsing.
>>>>>
>>>>> Parameters are defined via new macros similar to those of boot
>>>>> parameters. Parsing of parameter strings is done via the already
>>>>> existing boot parameter parsing function which is extended a little
>>>>> bit.
>>>>>
>>>>> Parameter settings can either be specified in configuration files of
>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>
>>>> Without having looked at any of the patches yet (not even their
>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>> really is: Just like "binary" ones, they become part of the public
>>>> interface, and hence subsequently can't be changed any more or
>>>> less than the ones we currently have (in particular, anything valid in
>>>> a guest config file will imo need to remain to be valid and meaningful
>>>> down the road).
>>>>
>>>> If this is solely or mainly about deferring the parsing from the tool
>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>> (I'd even be tempted to call it backwards).
>>>
>>> The main advantage is that it would be much easier to backport new
>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>> interface versions for that.
>> 
>> Additions to sysctl/domctl interfaces don't require such a bump.
> 
> What are XEN_DOMCTL_INTERFACE_VERSION and XEN_SYSCTL_INTERFACE_VERSION
> for then?

To guard _existing_ users of _existing_ interfaces.

>>> It might be a good idea to support mandatory and optional parameters
>>> in the guest config. Optional parameters not supported by the hypervisor
>>> would then be ignored instead of leading to failure at guest creation
>>> time.
>> 
>> Except that over time opinions may change what is supposed to
>> be optional vs mandatory.
> 
> And that would change how in case of binary interfaces?

Optional vs mandatory is an orthogonal discussion.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 00/12] add per-domain and per-cpupool generic parameters

[Juergen Gross]

On 18/09/18 13:23, Jan Beulich wrote:
>>>> On 18.09.18 at 13:20, <george.dunlap@citrix.com> wrote:
>> On 09/18/2018 12:19 PM, Jan Beulich wrote:
>>>>>> On 18.09.18 at 13:02, <jgross@suse.com> wrote:
>>>> On 18/09/18 12:32, Jan Beulich wrote:
>>>>>>>> On 18.09.18 at 08:02, <jgross@suse.com> wrote:
>>>>>> Instead of using binary hypervisor interfaces for new parameters of
>>>>>> domains or cpupools this patch series adds support for generic text
>>>>>> based parameter parsing.
>>>>>>
>>>>>> Parameters are defined via new macros similar to those of boot
>>>>>> parameters. Parsing of parameter strings is done via the already
>>>>>> existing boot parameter parsing function which is extended a little
>>>>>> bit.
>>>>>>
>>>>>> Parameter settings can either be specified in configuration files of
>>>>>> domains or cpupools, or they can be set via new xl sub-commands.
>>>>>
>>>>> Without having looked at any of the patches yet (not even their
>>>>> descriptions) I'm still wondering what the benefit of textual parameters
>>>>> really is: Just like "binary" ones, they become part of the public
>>>>> interface, and hence subsequently can't be changed any more or
>>>>> less than the ones we currently have (in particular, anything valid in
>>>>> a guest config file will imo need to remain to be valid and meaningful
>>>>> down the road).
>>>>>
>>>>> If this is solely or mainly about deferring the parsing from the tool
>>>>> stack to the hypervisor, then I'm not at all convinced of the approach
>>>>> (I'd even be tempted to call it backwards).
>>>>
>>>> The main advantage is that it would be much easier to backport new
>>>> parameters like the xpti per-domain one. No need to bump sysctl/domctl
>>>> interface versions for that.
>>>
>>> Additions to sysctl/domctl interfaces don't require such a bump.
>>>
>>>> It might be a good idea to support mandatory and optional parameters
>>>> in the guest config. Optional parameters not supported by the hypervisor
>>>> would then be ignored instead of leading to failure at guest creation
>>>> time.
>>>
>>> Except that over time opinions may change what is supposed to
>>> be optional vs mandatory.
>>
>> I thought the idea would be that the admin would specify which ones were
>> optional or mandatory.
> 
> If this was admin controlled, there would be no way to encode in
> the hypercall handler which ones to reject when unknown. Even
> without admin involvement it's not really clear to me how options
> we don't even know of today could be treated as either optional
> or mandatory.

This would be in the tools: when you add:

optional_parameters="bla=foo"

the tools know they can ignore a negative rc when trying to set the
parameter.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel