[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 09/12] xen: add runtime parameter access support to hypfs
On 04.03.2020 16:07, Jürgen Groß wrote: > On 04.03.20 12:32, Jan Beulich wrote: >> On 26.02.2020 13:47, Juergen Gross wrote: >>> +static void update_ept_param_append(const char *str, int val) >>> +{ >>> + char *pos = opt_ept_setting + strlen(opt_ept_setting); >>> + >>> + snprintf(pos, sizeof(opt_ept_setting) - (pos - opt_ept_setting), >>> + ",%s=%d", str, val); >>> +} >>> + >>> +static void update_ept_param(void) >>> +{ >>> + snprintf(opt_ept_setting, sizeof(opt_ept_setting), "pml=%d", >>> opt_ept_pml); >>> + if ( opt_ept_ad >= 0 ) >>> + update_ept_param_append("ad", opt_ept_ad); >> >> This won't correctly reflect reality: If you look at >> vmx_init_vmcs_config(), even a negative value means "true" here, >> unless on a specific Atom model. I think init_ept_param() wants >> to have that erratum workaround logic moved there, such that >> you can then assme the value to be non-negative here. > > But isn't not mentioning it in the -1 case correct? -1 means: do the > correct thing on the current hardware. Well, I think the output here should represent effective settings, and a sub-item should be suppressed only if a setting has no effect at all in the current setup, like ... >>> + if ( opt_ept_exec_sp >= 0 ) >>> + update_ept_param_append("exec-sp", opt_ept_exec_sp); >> >> I agree for this one - if the value is still -1, it has neither >> been set nor is its value of any interest. ... here. >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -85,8 +85,10 @@ struct grant_table { >>> struct grant_table_arch arch; >>> }; >>> >>> -static int parse_gnttab_limit(const char *param, const char *arg, >>> - unsigned int *valp) >>> +#define GRANT_CUSTOM_VAL_SZ 12 >>> + >>> +static int parse_gnttab_limit(const char *arg, unsigned int *valp, >>> + char *parval) >>> { >>> const char *e; >>> unsigned long val; >>> @@ -99,28 +101,47 @@ static int parse_gnttab_limit(const char *param, const >>> char *arg, >>> return -ERANGE; >>> >>> *valp = val; >>> + snprintf(parval, GRANT_CUSTOM_VAL_SZ, "%lu", val); >>> >>> return 0; >>> } >>> >>> unsigned int __read_mostly opt_max_grant_frames = 64; >>> +static char __read_mostly opt_max_grant_frames_val[GRANT_CUSTOM_VAL_SZ]; >>> + >>> +static void __init gnttab_max_frames_init(struct param_hypfs *par) >>> +{ >>> + custom_runtime_set_var(par, opt_max_grant_frames_val); >> >> You still use a custom string buffer here. Can this "set-var" >> operation record that the variable (for presentation purposes) >> is simply of UINT type, handing a pointer to the actual >> variable? > > No, this would result in the need to set a custom parameter via a > binary value passed in from user land. So I'd need to convert this > binary into a string to be parseable by the custom function. Hmm, not very fortunate, but I can see what you're saying. >>> --- 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> >>> >>> @@ -281,6 +282,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 ( !memchr(buf, 0, ulen) ) >> >> Once again " != buf + ulen - 1"? (EDOM also looks like an odd >> error code to use in this case, but I guess there's no really >> good one.) > > " != buf + ulen - 1" is a logical choice with the change of patch 4. I'm afraid I don't understand. You want to parse a string here. The caller should tell you what the string length is (including the nul again), not what its buffer size may be. >>> @@ -79,41 +88,94 @@ 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_set_var(parfs, var) \ >>> + { \ >>> + (parfs)->hypfs.write_ptr = &(var); \ >>> + (parfs)->hypfs.e.size = sizeof(var); \ >> >> All users of this use char[]. Why sizeof() rather than strlen(), > > That is the maximum string length. Otherwise I wouldn't know I am > allowed to replace e.g. "on" by "noxpti". As said elsewhere - if e.size is the buffer size, then the reading function wants adjusting, and it needs to be clarified how buffer size and payload size can be told apart for BLOBs. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |