| Message ID | 20200219081126.29534-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add hypervisor sysfs-like support | expand |
On 19.02.2020 09:11, Juergen Gross wrote: > --- a/docs/misc/hypfs-paths.pandoc > +++ b/docs/misc/hypfs-paths.pandoc > @@ -152,3 +152,12 @@ The major version of Xen. > #### /buildinfo/version/minor = INTEGER > > The minor version of Xen. > + > +#### /params/ > + > +A directory of runtime parameters. > + > +#### /params/* > + > +The single parameters. The description of the different parameters can be s/single/individual/? > --- a/xen/arch/arm/xen.lds.S > +++ b/xen/arch/arm/xen.lds.S > @@ -89,6 +89,11 @@ SECTIONS > __start_schedulers_array = .; > *(.data.schedulers) > __end_schedulers_array = .; > + > + . = ALIGN(8); > + __paramhypfs_start = .; > + *(.data.paramhypfs) > + __paramhypfs_end = .; Do you really need 8-byte alignment even on 32-bit Arm? > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -70,6 +70,17 @@ integer_param("ple_window", ple_window); > static bool __read_mostly opt_ept_pml = true; > static s8 __read_mostly opt_ept_ad = -1; > int8_t __read_mostly opt_ept_exec_sp = -1; > +static char opt_ept_setting[16] = "pml=1"; This is dangerous imo - such strings would better be populated during boot by invoking the same function that also does so after updating. Otherwise it won't take long until reported and actual settings will be out of sync, until first modified via this new interface. > + > + > +static void update_ept_param(void) No double blank lines please. > @@ -31,10 +32,12 @@ static int parse_pcid(const char *s) > { > case 0: > opt_pcid = PCID_OFF; > + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "off"); > break; > > case 1: > opt_pcid = PCID_ALL; > + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "on"); > break; > > default: > @@ -42,10 +45,12 @@ static int parse_pcid(const char *s) > { > case 0: > opt_pcid = PCID_NOXPTI; > + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "noxpti"); > break; > > case 1: > opt_pcid = PCID_XPTI; > + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "xpti"); > break; Pretty expensive to use snprintf() here - how about strlcpy()? > @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const char *arg, > return -ERANGE; > > *valp = val; > + snprintf(par_val, PAR_VAL_SZ, "%lu", val); > > return 0; > } > > unsigned int __read_mostly opt_max_grant_frames = 64; > +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64"; This and the other option are plain integer ones from a presentation pov, so it would be nice to get away here without the extra buffers. > @@ -289,6 +290,33 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf, > return 0; > } > > +int hypfs_write_custom(struct hypfs_entry_leaf *leaf, > + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) > +{ > + struct param_hypfs *p; > + char *buf; > + int ret; > + > + buf = xzalloc_array(char, ulen); > + if ( !buf ) > + return -ENOMEM; > + > + ret = -EFAULT; > + if ( copy_from_guest(buf, uaddr, ulen) ) > + goto out; > + > + ret = -EDOM; > + if ( buf[ulen - 1] ) Perhaps memchr() again. > @@ -23,10 +24,17 @@ struct kernel_param { > } par; > }; > > +struct param_hypfs { > + const struct kernel_param *param; As long as this is here, I don't think ... > @@ -76,40 +84,87 @@ extern const struct kernel_param __param_start[], __param_end[]; > .type = OPT_IGNORE } > > #define __rtparam __param(__dataparam) > +#define __paramfs static __paramhypfs \ > + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs ... you need the alignment attribute here. But see below. > -#define custom_runtime_only_param(_name, _var) \ > +#define custom_runtime_only_param(_name, _var, contvar) \ > __rtparam __rtpar_##_var = \ > { .name = _name, \ > .type = OPT_CUSTOM, \ > - .par.func = _var } > + .par.func = _var }; \ > + __paramfs __parfs_##_var = \ > + { .param = &__rtpar_##_var, \ Instead of a pointer, can't the param struct be part of this bigger structure? > + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \ > + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ > + .hypfs.e.name = _name, \ > + .hypfs.e.size = sizeof(contvar), \ This will go wrong if contvar is not an array. I guess you want ARRAY_SIZE(contvar) * sizeof(*(convar)) here, and perhaps also ... > + .hypfs.e.read = hypfs_read_leaf, \ > + .hypfs.e.write = hypfs_write_custom, \ > + .hypfs.content = &contvar } ... omit the & here. > @@ -123,4 +178,7 @@ extern const struct kernel_param __param_start[], __param_end[]; > string_param(_name, _var); \ > string_runtime_only_param(_name, _var) > > +#define param_append_str(var, fmt, val) \ > + snprintf(var + strlen(var), sizeof(var) - strlen(var), fmt, val) The sizeof() here again isn't safe against var not being of array type. Also again perhaps cheaper to use strlcat()? Jan
On 19.02.20 17:44, Jan Beulich wrote: > On 19.02.2020 09:11, Juergen Gross wrote: >> --- a/docs/misc/hypfs-paths.pandoc >> +++ b/docs/misc/hypfs-paths.pandoc >> @@ -152,3 +152,12 @@ The major version of Xen. >> #### /buildinfo/version/minor = INTEGER >> >> The minor version of Xen. >> + >> +#### /params/ >> + >> +A directory of runtime parameters. >> + >> +#### /params/* >> + >> +The single parameters. The description of the different parameters can be > > s/single/individual/? Yes, this is better. > >> --- a/xen/arch/arm/xen.lds.S >> +++ b/xen/arch/arm/xen.lds.S >> @@ -89,6 +89,11 @@ SECTIONS >> __start_schedulers_array = .; >> *(.data.schedulers) >> __end_schedulers_array = .; >> + >> + . = ALIGN(8); >> + __paramhypfs_start = .; >> + *(.data.paramhypfs) >> + __paramhypfs_end = .; > > Do you really need 8-byte alignment even on 32-bit Arm? I just followed the general pattern in this file. > >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -70,6 +70,17 @@ integer_param("ple_window", ple_window); >> static bool __read_mostly opt_ept_pml = true; >> static s8 __read_mostly opt_ept_ad = -1; >> int8_t __read_mostly opt_ept_exec_sp = -1; >> +static char opt_ept_setting[16] = "pml=1"; > > This is dangerous imo - such strings would better be populated > during boot by invoking the same function that also does so > after updating. Otherwise it won't take long until reported > and actual settings will be out of sync, until first modified > via this new interface. Hmm, this would require a general update_value() function for each custom runtime parameter. I can see how this would look like. > >> + >> + >> +static void update_ept_param(void) > > No double blank lines please. Oh, sorry. > >> @@ -31,10 +32,12 @@ static int parse_pcid(const char *s) >> { >> case 0: >> opt_pcid = PCID_OFF; >> + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "off"); >> break; >> >> case 1: >> opt_pcid = PCID_ALL; >> + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "on"); >> break; >> >> default: >> @@ -42,10 +45,12 @@ static int parse_pcid(const char *s) >> { >> case 0: >> opt_pcid = PCID_NOXPTI; >> + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "noxpti"); >> break; >> >> case 1: >> opt_pcid = PCID_XPTI; >> + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "xpti"); >> break; > > Pretty expensive to use snprintf() here - how about strlcpy()? Oh, of course. > >> @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const char *arg, >> return -ERANGE; >> >> *valp = val; >> + snprintf(par_val, PAR_VAL_SZ, "%lu", val); >> >> return 0; >> } >> >> unsigned int __read_mostly opt_max_grant_frames = 64; >> +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64"; > > This and the other option are plain integer ones from a presentation > pov, so it would be nice to get away here without the extra buffers. There is more than pure integer semantics here: the value is limited, so I can't just use the normal integer assignment. Its not as if those functions are performance critical, and special casing for sparing the buffer memory will probably waste more memory due to larger code size. > >> @@ -289,6 +290,33 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf, >> return 0; >> } >> >> +int hypfs_write_custom(struct hypfs_entry_leaf *leaf, >> + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) >> +{ >> + struct param_hypfs *p; >> + char *buf; >> + int ret; >> + >> + buf = xzalloc_array(char, ulen); >> + if ( !buf ) >> + return -ENOMEM; >> + >> + ret = -EFAULT; >> + if ( copy_from_guest(buf, uaddr, ulen) ) >> + goto out; >> + >> + ret = -EDOM; >> + if ( buf[ulen - 1] ) > > Perhaps memchr() again. Okay. > >> @@ -23,10 +24,17 @@ struct kernel_param { >> } par; >> }; >> >> +struct param_hypfs { >> + const struct kernel_param *param; > > As long as this is here, I don't think ... > >> @@ -76,40 +84,87 @@ extern const struct kernel_param __param_start[], __param_end[]; >> .type = OPT_IGNORE } >> >> #define __rtparam __param(__dataparam) >> +#define __paramfs static __paramhypfs \ >> + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs > > ... you need the alignment attribute here. But see below. > >> -#define custom_runtime_only_param(_name, _var) \ >> +#define custom_runtime_only_param(_name, _var, contvar) \ >> __rtparam __rtpar_##_var = \ >> { .name = _name, \ >> .type = OPT_CUSTOM, \ >> - .par.func = _var } >> + .par.func = _var }; \ >> + __paramfs __parfs_##_var = \ >> + { .param = &__rtpar_##_var, \ > > Instead of a pointer, can't the param struct be part of this > bigger structure? I have some further patches in my pipeline which will do even more, by removing the sysctl for setting parameters and using hypfs for that purpose in libxl. This will remove the need for the runtime copy of struct kernel_param completely. But r > >> + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \ >> + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ >> + .hypfs.e.name = _name, \ >> + .hypfs.e.size = sizeof(contvar), \ > > This will go wrong if contvar is not an array. I guess you want > ARRAY_SIZE(contvar) * sizeof(*(convar)) here, and perhaps also Ah, nice idea. > ... > >> + .hypfs.e.read = hypfs_read_leaf, \ >> + .hypfs.e.write = hypfs_write_custom, \ >> + .hypfs.content = &contvar } > > ... omit the & here. Okay. > >> @@ -123,4 +178,7 @@ extern const struct kernel_param __param_start[], __param_end[]; >> string_param(_name, _var); \ >> string_runtime_only_param(_name, _var) >> >> +#define param_append_str(var, fmt, val) \ >> + snprintf(var + strlen(var), sizeof(var) - strlen(var), fmt, val) > > The sizeof() here again isn't safe against var not being of array > type. Also again perhaps cheaper to use strlcat()? Yes. Juergen
On 20.02.2020 09:22, Jürgen Groß wrote: > On 19.02.20 17:44, Jan Beulich wrote: >> On 19.02.2020 09:11, Juergen Gross wrote: >>> --- a/xen/arch/arm/xen.lds.S >>> +++ b/xen/arch/arm/xen.lds.S >>> @@ -89,6 +89,11 @@ SECTIONS >>> __start_schedulers_array = .; >>> *(.data.schedulers) >>> __end_schedulers_array = .; >>> + >>> + . = ALIGN(8); >>> + __paramhypfs_start = .; >>> + *(.data.paramhypfs) >>> + __paramhypfs_end = .; >> >> Do you really need 8-byte alignment even on 32-bit Arm? > > I just followed the general pattern in this file. Well, it'll be the Arm maintainers to judge anyway. It merely caught my eye. >>> @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const char *arg, >>> return -ERANGE; >>> >>> *valp = val; >>> + snprintf(par_val, PAR_VAL_SZ, "%lu", val); >>> >>> return 0; >>> } >>> >>> unsigned int __read_mostly opt_max_grant_frames = 64; >>> +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64"; >> >> This and the other option are plain integer ones from a presentation >> pov, so it would be nice to get away here without the extra buffers. > > There is more than pure integer semantics here: the value is limited, > so I can't just use the normal integer assignment. Which is why I've said "from a presentation pov", i.e. when consuming the value for passing back as a string. > Its not as if those functions are performance critical, and special > casing for sparing the buffer memory will probably waste more memory > due to larger code size. Well, it's not so much the memory savings, but the reduction of redundant information being kept. But if it's not sufficiently simple to generalize, so be it then. >>> @@ -23,10 +24,17 @@ struct kernel_param { >>> } par; >>> }; >>> >>> +struct param_hypfs { >>> + const struct kernel_param *param; >> >> As long as this is here, I don't think ... >> >>> @@ -76,40 +84,87 @@ extern const struct kernel_param __param_start[], __param_end[]; >>> .type = OPT_IGNORE } >>> >>> #define __rtparam __param(__dataparam) >>> +#define __paramfs static __paramhypfs \ >>> + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs >> >> ... you need the alignment attribute here. But see below. >> >>> -#define custom_runtime_only_param(_name, _var) \ >>> +#define custom_runtime_only_param(_name, _var, contvar) \ >>> __rtparam __rtpar_##_var = \ >>> { .name = _name, \ >>> .type = OPT_CUSTOM, \ >>> - .par.func = _var } >>> + .par.func = _var }; \ >>> + __paramfs __parfs_##_var = \ >>> + { .param = &__rtpar_##_var, \ >> >> Instead of a pointer, can't the param struct be part of this >> bigger structure? > > I have some further patches in my pipeline which will do even more, > by removing the sysctl for setting parameters and using hypfs for > that purpose in libxl. This will remove the need for the runtime > copy of struct kernel_param completely. Ah, good to know. While reviewing this I was wondering about exactly this. > But r ??? Jan
On 20.02.20 09:49, Jan Beulich wrote: > On 20.02.2020 09:22, Jürgen Groß wrote: >> On 19.02.20 17:44, Jan Beulich wrote: >>> On 19.02.2020 09:11, Juergen Gross wrote: >>>> --- a/xen/arch/arm/xen.lds.S >>>> +++ b/xen/arch/arm/xen.lds.S >>>> @@ -89,6 +89,11 @@ SECTIONS >>>> __start_schedulers_array = .; >>>> *(.data.schedulers) >>>> __end_schedulers_array = .; >>>> + >>>> + . = ALIGN(8); >>>> + __paramhypfs_start = .; >>>> + *(.data.paramhypfs) >>>> + __paramhypfs_end = .; >>> >>> Do you really need 8-byte alignment even on 32-bit Arm? >> >> I just followed the general pattern in this file. > > Well, it'll be the Arm maintainers to judge anyway. It merely > caught my eye. > >>>> @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const char *arg, >>>> return -ERANGE; >>>> >>>> *valp = val; >>>> + snprintf(par_val, PAR_VAL_SZ, "%lu", val); >>>> >>>> return 0; >>>> } >>>> >>>> unsigned int __read_mostly opt_max_grant_frames = 64; >>>> +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64"; >>> >>> This and the other option are plain integer ones from a presentation >>> pov, so it would be nice to get away here without the extra buffers. >> >> There is more than pure integer semantics here: the value is limited, >> so I can't just use the normal integer assignment. > > Which is why I've said "from a presentation pov", i.e. when consuming > the value for passing back as a string. Hmm, this might even be possible via not using the common macro. I'll have a look into it. Juergen
diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc index 1faebcccbc..7d03852ccd 100644 --- a/docs/misc/hypfs-paths.pandoc +++ b/docs/misc/hypfs-paths.pandoc @@ -152,3 +152,12 @@ The major version of Xen. #### /buildinfo/version/minor = INTEGER The minor version of Xen. + +#### /params/ + +A directory of runtime parameters. + +#### /params/* + +The single parameters. The description of the different parameters can be +found in `docs/misc/xen-command-line.pandoc`. diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index a497f6a48d..0061a8dfea 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -89,6 +89,11 @@ SECTIONS __start_schedulers_array = .; *(.data.schedulers) __end_schedulers_array = .; + + . = ALIGN(8); + __paramhypfs_start = .; + *(.data.paramhypfs) + __paramhypfs_end = .; *(.data.rel) *(.data.rel.*) CONSTRUCTORS diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 65445afeb0..36ddccfb73 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -70,6 +70,17 @@ integer_param("ple_window", ple_window); static bool __read_mostly opt_ept_pml = true; static s8 __read_mostly opt_ept_ad = -1; int8_t __read_mostly opt_ept_exec_sp = -1; +static char opt_ept_setting[16] = "pml=1"; + + +static void update_ept_param(void) +{ + snprintf(opt_ept_setting, sizeof(opt_ept_setting), "pml=%d", opt_ept_pml); + if ( opt_ept_ad >= 0 ) + param_append_str(opt_ept_setting, ",ad=%d", opt_ept_ad); + if ( opt_ept_exec_sp >= 0 ) + param_append_str(opt_ept_setting, ",exec-sp=%d", opt_ept_exec_sp); +} static int __init parse_ept_param(const char *s) { @@ -93,6 +104,8 @@ static int __init parse_ept_param(const char *s) s = ss + 1; } while ( *ss ); + update_ept_param(); + return rc; } custom_param("ept", parse_ept_param); @@ -115,6 +128,8 @@ static int parse_ept_param_runtime(const char *s) opt_ept_exec_sp = val; + update_ept_param(); + rcu_read_lock(&domlist_read_lock); for_each_domain ( d ) { @@ -144,7 +159,7 @@ static int parse_ept_param_runtime(const char *s) return 0; } -custom_runtime_only_param("ept", parse_ept_param_runtime); +custom_runtime_only_param("ept", parse_ept_param_runtime, opt_ept_setting); /* Dynamic (run-time adjusted) execution control flags. */ u32 vmx_pin_based_exec_control __read_mostly; diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c index 0b37653b12..71e1f69ff8 100644 --- a/xen/arch/x86/pv/domain.c +++ b/xen/arch/x86/pv/domain.c @@ -22,6 +22,7 @@ static __read_mostly enum { PCID_XPTI, PCID_NOXPTI } opt_pcid = PCID_XPTI; +static char opt_pcid_val[7] = "xpti"; static int parse_pcid(const char *s) { @@ -31,10 +32,12 @@ static int parse_pcid(const char *s) { case 0: opt_pcid = PCID_OFF; + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "off"); break; case 1: opt_pcid = PCID_ALL; + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "on"); break; default: @@ -42,10 +45,12 @@ static int parse_pcid(const char *s) { case 0: opt_pcid = PCID_NOXPTI; + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "noxpti"); break; case 1: opt_pcid = PCID_XPTI; + snprintf(opt_pcid_val, sizeof(opt_pcid_val), "xpti"); break; default: @@ -57,7 +62,7 @@ static int parse_pcid(const char *s) return rc; } -custom_runtime_param("pcid", parse_pcid); +custom_runtime_param("pcid", parse_pcid, opt_pcid_val); static void noreturn continue_nonidle_domain(struct vcpu *v) { diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 7f9459d683..21a37f0f57 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -279,6 +279,11 @@ SECTIONS __start_schedulers_array = .; *(.data.schedulers) __end_schedulers_array = .; + + . = ALIGN(8); + __paramhypfs_start = .; + *(.data.paramhypfs) + __paramhypfs_end = .; } :text DECL_SECTION(.data) { diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index bc37acae0e..e8dcc9e568 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -85,8 +85,10 @@ struct grant_table { struct grant_table_arch arch; }; +#define PAR_VAL_SZ 12 + static int parse_gnttab_limit(const char *param, const char *arg, - unsigned int *valp) + unsigned int *valp, char *par_val) { const char *e; unsigned long val; @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const char *arg, return -ERANGE; *valp = val; + snprintf(par_val, PAR_VAL_SZ, "%lu", val); return 0; } unsigned int __read_mostly opt_max_grant_frames = 64; +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64"; static int parse_gnttab_max_frames(const char *arg) { return parse_gnttab_limit("gnttab_max_frames", arg, - &opt_max_grant_frames); + &opt_max_grant_frames, gnttab_max_frames_val); } -custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames); +custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames, + gnttab_max_frames_val); static unsigned int __read_mostly opt_max_maptrack_frames = 1024; +static char max_maptrack_frames_val[PAR_VAL_SZ] = "1024"; static int parse_gnttab_max_maptrack_frames(const char *arg) { return parse_gnttab_limit("gnttab_max_maptrack_frames", arg, - &opt_max_maptrack_frames); + &opt_max_maptrack_frames, + max_maptrack_frames_val); } custom_runtime_param("gnttab_max_maptrack_frames", - parse_gnttab_max_maptrack_frames); + parse_gnttab_max_maptrack_frames, max_maptrack_frames_val); #ifndef GNTTAB_MAX_VERSION #define GNTTAB_MAX_VERSION 2 diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 0a32d45979..e568338002 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -10,6 +10,7 @@ #include <xen/hypercall.h> #include <xen/hypfs.h> #include <xen/lib.h> +#include <xen/param.h> #include <xen/rwlock.h> #include <public/hypfs.h> @@ -289,6 +290,33 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf, return 0; } +int hypfs_write_custom(struct hypfs_entry_leaf *leaf, + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) +{ + struct param_hypfs *p; + char *buf; + int ret; + + buf = xzalloc_array(char, ulen); + if ( !buf ) + return -ENOMEM; + + ret = -EFAULT; + if ( copy_from_guest(buf, uaddr, ulen) ) + goto out; + + ret = -EDOM; + if ( buf[ulen - 1] ) + goto out; + + p = container_of(leaf, struct param_hypfs, hypfs); + ret = p->param->par.func(buf); + + out: + xfree(buf); + return ret; +} + static int hypfs_write(struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) { @@ -355,3 +383,13 @@ long do_hypfs_op(unsigned int cmd, return ret; } + +void hypfs_write_lock(void) +{ + write_lock(&hypfs_lock); +} + +void hypfs_write_unlock(void) +{ + write_unlock(&hypfs_lock); +} diff --git a/xen/common/kernel.c b/xen/common/kernel.c index 3d496bb9e6..e62a1680c7 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -198,7 +198,13 @@ static void __init _cmdline_parse(const char *cmdline) int runtime_parse(const char *line) { - return parse_params(line, __param_start, __param_end); + int ret; + + hypfs_write_lock(); + ret = parse_params(line, __param_start, __param_end); + hypfs_write_unlock(); + + return ret; } /** @@ -428,6 +434,21 @@ static int __init buildinfo_init(void) } __initcall(buildinfo_init); +static HYPFS_DIR_INIT(params, "params"); + +static int __init param_init(void) +{ + struct param_hypfs *param; + + hypfs_add_dir(&hypfs_root, ¶ms, true); + + for ( param = __paramhypfs_start; param < __paramhypfs_end; param++ ) + hypfs_add_leaf(¶ms, ¶m->hypfs, true); + + return 0; +} +__initcall(param_init); + # define DO(fn) long do_##fn #endif diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 913ae1b66a..5440145549 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -78,9 +78,11 @@ enum con_timestamp_mode }; static enum con_timestamp_mode __read_mostly opt_con_timestamp_mode = TSM_NONE; +static char con_timestamp_mode_val[7] = "none"; static int parse_console_timestamps(const char *s); -custom_runtime_param("console_timestamps", parse_console_timestamps); +custom_runtime_param("console_timestamps", parse_console_timestamps, + con_timestamp_mode_val); /* conring_size: allows a large console ring than default (16kB). */ static uint32_t __initdata opt_conring_size; @@ -118,13 +120,17 @@ static DEFINE_SPINLOCK(console_lock); #ifdef NDEBUG #define XENLOG_UPPER_THRESHOLD 2 /* Do not print INFO and DEBUG */ #define XENLOG_LOWER_THRESHOLD 2 /* Always print ERR and WARNING */ +#define XENLOG_DEFAULT_VAL "warning/warning" #define XENLOG_GUEST_UPPER_THRESHOLD 2 /* Do not print INFO and DEBUG */ #define XENLOG_GUEST_LOWER_THRESHOLD 0 /* Rate-limit ERR and WARNING */ +#define XENLOG_GUEST_DEFAULT_VAL "none/warning" #else #define XENLOG_UPPER_THRESHOLD 4 /* Do not discard anything */ #define XENLOG_LOWER_THRESHOLD 4 /* Print everything */ +#define XENLOG_DEFAULT_VAL "all/all" #define XENLOG_GUEST_UPPER_THRESHOLD 4 /* Do not discard anything */ #define XENLOG_GUEST_LOWER_THRESHOLD 4 /* Print everything */ +#define XENLOG_GUEST_DEFAULT_VAL "all/all" #endif /* * The XENLOG_DEFAULT is the default given to printks that @@ -133,16 +139,20 @@ static DEFINE_SPINLOCK(console_lock); #define XENLOG_DEFAULT 1 /* XENLOG_WARNING */ #define XENLOG_GUEST_DEFAULT 1 /* XENLOG_WARNING */ +#define LOGLVL_VAL_SZ 16 static int __read_mostly xenlog_upper_thresh = XENLOG_UPPER_THRESHOLD; static int __read_mostly xenlog_lower_thresh = XENLOG_LOWER_THRESHOLD; +static char xenlog_val[LOGLVL_VAL_SZ] = XENLOG_DEFAULT_VAL; static int __read_mostly xenlog_guest_upper_thresh = XENLOG_GUEST_UPPER_THRESHOLD; static int __read_mostly xenlog_guest_lower_thresh = XENLOG_GUEST_LOWER_THRESHOLD; +static char xenlog_guest_val[LOGLVL_VAL_SZ] = XENLOG_GUEST_DEFAULT_VAL; static int parse_loglvl(const char *s); static int parse_guest_loglvl(const char *s); +static char *lvl2opt[] = { "none", "error", "warning", "info", "all" }; /* * <lvl> := none|error|warning|info|debug|all * loglvl=<lvl_print_always>[/<lvl_print_ratelimit>] @@ -151,8 +161,8 @@ static int parse_guest_loglvl(const char *s); * Similar definitions for guest_loglvl, but applies to guest tracing. * Defaults: loglvl=warning ; guest_loglvl=none/warning */ -custom_runtime_param("loglvl", parse_loglvl); -custom_runtime_param("guest_loglvl", parse_guest_loglvl); +custom_runtime_param("loglvl", parse_loglvl, xenlog_val); +custom_runtime_param("guest_loglvl", parse_guest_loglvl, xenlog_guest_val); static atomic_t print_everything = ATOMIC_INIT(0); @@ -173,7 +183,7 @@ static int __parse_loglvl(const char *s, const char **ps) return 2; /* sane fallback */ } -static int _parse_loglvl(const char *s, int *lower, int *upper) +static int _parse_loglvl(const char *s, int *lower, int *upper, char *val) { *lower = *upper = __parse_loglvl(s, &s); if ( *s == '/' ) @@ -181,18 +191,21 @@ static int _parse_loglvl(const char *s, int *lower, int *upper) if ( *upper < *lower ) *upper = *lower; + snprintf(val, LOGLVL_VAL_SZ, "%s/%s", lvl2opt[*lower], lvl2opt[*upper]); + return *s ? -EINVAL : 0; } static int parse_loglvl(const char *s) { - return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh); + return _parse_loglvl(s, &xenlog_lower_thresh, &xenlog_upper_thresh, + xenlog_val); } static int parse_guest_loglvl(const char *s) { return _parse_loglvl(s, &xenlog_guest_lower_thresh, - &xenlog_guest_upper_thresh); + &xenlog_guest_upper_thresh, xenlog_guest_val); } static char *loglvl_str(int lvl) @@ -731,22 +744,46 @@ static int parse_console_timestamps(const char *s) { case 0: opt_con_timestamp_mode = TSM_NONE; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "none"); return 0; case 1: opt_con_timestamp_mode = TSM_DATE; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "date"); return 0; } if ( *s == '\0' || /* Compat for old booleanparam() */ !strcmp(s, "date") ) + { opt_con_timestamp_mode = TSM_DATE; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "date"); + } else if ( !strcmp(s, "datems") ) + { opt_con_timestamp_mode = TSM_DATE_MS; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "datems"); + } else if ( !strcmp(s, "boot") ) + { opt_con_timestamp_mode = TSM_BOOT; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "boot"); + } else if ( !strcmp(s, "raw") ) + { opt_con_timestamp_mode = TSM_RAW; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "raw"); + } else if ( !strcmp(s, "none") ) + { opt_con_timestamp_mode = TSM_NONE; + snprintf(con_timestamp_mode_val, sizeof(con_timestamp_mode_val), + "none"); + } else return -EINVAL; diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h index 5b26b2e611..2b6ed8ae23 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -104,5 +104,9 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen); int hypfs_write_bool(struct hypfs_entry_leaf *leaf, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen); +int hypfs_write_custom(struct hypfs_entry_leaf *leaf, + XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen); +void hypfs_write_lock(void); +void hypfs_write_unlock(void); #endif /* __XEN_HYPFS_H__ */ diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h index 75471eb4ad..b4ef3e7a22 100644 --- a/xen/include/xen/param.h +++ b/xen/include/xen/param.h @@ -1,6 +1,7 @@ #ifndef _XEN_PARAM_H #define _XEN_PARAM_H +#include <xen/hypfs.h> #include <xen/init.h> /* @@ -23,10 +24,17 @@ struct kernel_param { } par; }; +struct param_hypfs { + const struct kernel_param *param; + struct hypfs_entry_leaf hypfs; +}; + extern const struct kernel_param __setup_start[], __setup_end[]; extern const struct kernel_param __param_start[], __param_end[]; +extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[]; #define __dataparam __used_section(".data.param") +#define __paramhypfs __used_section(".data.paramhypfs") #define __param(att) static const att \ __attribute__((__aligned__(sizeof(void *)))) struct kernel_param @@ -76,40 +84,87 @@ extern const struct kernel_param __param_start[], __param_end[]; .type = OPT_IGNORE } #define __rtparam __param(__dataparam) +#define __paramfs static __paramhypfs \ + __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs -#define custom_runtime_only_param(_name, _var) \ +#define custom_runtime_only_param(_name, _var, contvar) \ __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_CUSTOM, \ - .par.func = _var } + .par.func = _var }; \ + __paramfs __parfs_##_var = \ + { .param = &__rtpar_##_var, \ + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \ + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .hypfs.e.name = _name, \ + .hypfs.e.size = sizeof(contvar), \ + .hypfs.e.read = hypfs_read_leaf, \ + .hypfs.e.write = hypfs_write_custom, \ + .hypfs.content = &contvar } #define boolean_runtime_only_param(_name, _var) \ __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_BOOL, \ .len = sizeof(_var), \ - .par.var = &_var } + .par.var = &_var }; \ + __paramfs __parfs_##_var = \ + { .param = &__rtpar_##_var, \ + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \ + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .hypfs.e.name = _name, \ + .hypfs.e.size = sizeof(_var), \ + .hypfs.e.read = hypfs_read_leaf, \ + .hypfs.e.write = hypfs_write_bool, \ + .hypfs.content = &_var } #define integer_runtime_only_param(_name, _var) \ __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_UINT, \ .len = sizeof(_var), \ - .par.var = &_var } + .par.var = &_var }; \ + __paramfs __parfs_##_var = \ + { .param = &__rtpar_##_var, \ + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \ + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .hypfs.e.name = _name, \ + .hypfs.e.size = sizeof(_var), \ + .hypfs.e.read = hypfs_read_leaf, \ + .hypfs.e.write = hypfs_write_leaf, \ + .hypfs.content = &_var } #define size_runtime_only_param(_name, _var) \ __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_SIZE, \ .len = sizeof(_var), \ - .par.var = &_var } + .par.var = &_var }; \ + __paramfs __parfs_##_var = \ + { .param = &__rtpar_##_var, \ + .hypfs.e.type = XEN_HYPFS_TYPE_UINT, \ + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .hypfs.e.name = _name, \ + .hypfs.e.size = sizeof(_var), \ + .hypfs.e.read = hypfs_read_leaf, \ + .hypfs.e.write = hypfs_write_leaf, \ + .hypfs.content = &_var } #define string_runtime_only_param(_name, _var) \ __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_STR, \ .len = sizeof(_var), \ - .par.var = &_var } + .par.var = &_var }; \ + __paramfs __parfs_##_var = \ + { .param = &__rtpar_##_var, \ + .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \ + .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \ + .hypfs.e.name = _name, \ + .hypfs.e.size = sizeof(_var), \ + .hypfs.e.read = hypfs_read_leaf, \ + .hypfs.e.write = hypfs_write_leaf, \ + .hypfs.content = &_var } -#define custom_runtime_param(_name, _var) \ +#define custom_runtime_param(_name, _var, contvar) \ custom_param(_name, _var); \ - custom_runtime_only_param(_name, _var) + custom_runtime_only_param(_name, _var, contvar) #define boolean_runtime_param(_name, _var) \ boolean_param(_name, _var); \ boolean_runtime_only_param(_name, _var) @@ -123,4 +178,7 @@ extern const struct kernel_param __param_start[], __param_end[]; string_param(_name, _var); \ string_runtime_only_param(_name, _var) +#define param_append_str(var, fmt, val) \ + snprintf(var + strlen(var), sizeof(var) - strlen(var), fmt, val) + #endif /* _XEN_PARAM_H */
Add support to read and modify values of hypervisor runtime parameters via the hypervisor file system. As runtime parameters can be modified via a sysctl, too, this path has to take the hypfs rw_lock as writer. For custom runtime parameters the resulting parameter value needs to be stored in a string buffer for being consumable by the file system. Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - complete rework - support custom parameters, too - support parameter writing --- docs/misc/hypfs-paths.pandoc | 9 ++++++ xen/arch/arm/xen.lds.S | 5 +++ xen/arch/x86/hvm/vmx/vmcs.c | 17 +++++++++- xen/arch/x86/pv/domain.c | 7 ++++- xen/arch/x86/xen.lds.S | 5 +++ xen/common/grant_table.c | 17 +++++++--- xen/common/hypfs.c | 38 +++++++++++++++++++++++ xen/common/kernel.c | 23 +++++++++++++- xen/drivers/char/console.c | 49 +++++++++++++++++++++++++---- xen/include/xen/hypfs.h | 4 +++ xen/include/xen/param.h | 74 +++++++++++++++++++++++++++++++++++++++----- 11 files changed, 226 insertions(+), 22 deletions(-)