| Message ID | 20220302221157.17246-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cmdline: Interpret 'vpmu' as a positive boolean | expand |
On 02.03.2022 23:11, Andrew Cooper wrote: > This makes it behave slightly more like a regular boolean option. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > Slightly RFC, because there is no easy way of making the opposite "normal > boolean" case work for no-vpmu. There's nothing to do to make this work afaict: Generic command line handling converts "no-<option>" to "<option>=no" for custom params. Jan
On 03/03/2022 07:44, Jan Beulich wrote: > On 02.03.2022 23:11, Andrew Cooper wrote: >> This makes it behave slightly more like a regular boolean option. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > >> Slightly RFC, because there is no easy way of making the opposite "normal >> boolean" case work for no-vpmu. > There's nothing to do to make this work afaict: Generic command line > handling converts "no-<option>" to "<option>=no" for custom params. Oh - I'd forgotten that, in which case this patch actually wants to be simply: diff --git a/xen/common/kernel.c b/xen/common/kernel.c index adff2d2c77f3..2cea1da781ac 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -162,6 +162,11 @@ static int parse_params(const char *cmdline, const struct kernel_param *start, safe_strcpy(opt, "no"); optval = opt; } + else if ( !*optval ) + { + safe_strcpy(opt, "1"); + optval = opt; + } rctmp = param->par.func(optval); break; case OPT_IGNORE: to turn "option\0" into "option=1", no? ~Andrew
On 03.03.2022 11:48, Andrew Cooper wrote: > On 03/03/2022 07:44, Jan Beulich wrote: >> On 02.03.2022 23:11, Andrew Cooper wrote: >>> This makes it behave slightly more like a regular boolean option. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >>> Slightly RFC, because there is no easy way of making the opposite "normal >>> boolean" case work for no-vpmu. >> There's nothing to do to make this work afaict: Generic command line >> handling converts "no-<option>" to "<option>=no" for custom params. > > Oh - I'd forgotten that, in which case this patch actually wants to be > simply: > > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index adff2d2c77f3..2cea1da781ac 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -162,6 +162,11 @@ static int parse_params(const char *cmdline, const > struct kernel_param *start, > safe_strcpy(opt, "no"); > optval = opt; > } > + else if ( !*optval ) > + { > + safe_strcpy(opt, "1"); > + optval = opt; > + } > rctmp = param->par.func(optval); > break; > case OPT_IGNORE: > > to turn "option\0" into "option=1", no? Iirc extending this to the positive case was deliberately not done, for the risk of breaking custom handlers not expecting the standard boolean forms. We could likely go this route, but only after auditing all custom handlers, I'm afraid. Jan
On 03/03/2022 11:04, Jan Beulich wrote: > On 03.03.2022 11:48, Andrew Cooper wrote: >> On 03/03/2022 07:44, Jan Beulich wrote: >>> On 02.03.2022 23:11, Andrew Cooper wrote: >>>> This makes it behave slightly more like a regular boolean option. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>>> Slightly RFC, because there is no easy way of making the opposite "normal >>>> boolean" case work for no-vpmu. >>> There's nothing to do to make this work afaict: Generic command line >>> handling converts "no-<option>" to "<option>=no" for custom params. >> Oh - I'd forgotten that, in which case this patch actually wants to be >> simply: >> >> diff --git a/xen/common/kernel.c b/xen/common/kernel.c >> index adff2d2c77f3..2cea1da781ac 100644 >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> @@ -162,6 +162,11 @@ static int parse_params(const char *cmdline, const >> struct kernel_param *start, >> safe_strcpy(opt, "no"); >> optval = opt; >> } >> + else if ( !*optval ) >> + { >> + safe_strcpy(opt, "1"); >> + optval = opt; >> + } >> rctmp = param->par.func(optval); >> break; >> case OPT_IGNORE: >> >> to turn "option\0" into "option=1", no? > Iirc extending this to the positive case was deliberately not done, for > the risk of breaking custom handlers not expecting the standard boolean > forms. We could likely go this route, but only after auditing all custom > handlers, I'm afraid. Well - I've already audited them all once recently. What's once more... I'll have a go in due course; I'd definitely prefer to avoid special casing the positive boolean form in individual handlers. ~Andrew
On 03.03.2022 12:15, Andrew Cooper wrote: > On 03/03/2022 11:04, Jan Beulich wrote: >> On 03.03.2022 11:48, Andrew Cooper wrote: >>> On 03/03/2022 07:44, Jan Beulich wrote: >>>> On 02.03.2022 23:11, Andrew Cooper wrote: >>>>> This makes it behave slightly more like a regular boolean option. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> >>>>> Slightly RFC, because there is no easy way of making the opposite "normal >>>>> boolean" case work for no-vpmu. >>>> There's nothing to do to make this work afaict: Generic command line >>>> handling converts "no-<option>" to "<option>=no" for custom params. >>> Oh - I'd forgotten that, in which case this patch actually wants to be >>> simply: >>> >>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c >>> index adff2d2c77f3..2cea1da781ac 100644 >>> --- a/xen/common/kernel.c >>> +++ b/xen/common/kernel.c >>> @@ -162,6 +162,11 @@ static int parse_params(const char *cmdline, const >>> struct kernel_param *start, >>> safe_strcpy(opt, "no"); >>> optval = opt; >>> } >>> + else if ( !*optval ) >>> + { >>> + safe_strcpy(opt, "1"); >>> + optval = opt; >>> + } >>> rctmp = param->par.func(optval); >>> break; >>> case OPT_IGNORE: >>> >>> to turn "option\0" into "option=1", no? >> Iirc extending this to the positive case was deliberately not done, for >> the risk of breaking custom handlers not expecting the standard boolean >> forms. We could likely go this route, but only after auditing all custom >> handlers, I'm afraid. > > Well - I've already audited them all once recently. What's once more... Of course if you did an audit (for this particular property) recently, that's definitely enough. Feel free to apply my earlier provided R-b also to this alternative change then. Jan > I'll have a go in due course; I'd definitely prefer to avoid special > casing the positive boolean form in individual handlers. > > ~Andrew
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c index 4fedc7c57012..501d4f0af94e 100644 --- a/xen/arch/x86/cpu/vpmu.c +++ b/xen/arch/x86/cpu/vpmu.c @@ -66,7 +66,9 @@ static int __init cf_check parse_vpmu_params(const char *s) if ( !ss ) ss = strchr(s, '\0'); - if ( (val = parse_bool(s, ss)) >= 0 ) + if ( s == ss ) + opt_vpmu_enabled = true; + else if ( (val = parse_bool(s, ss)) >= 0 ) { opt_vpmu_enabled = val; if ( !val )
This makes it behave slightly more like a regular boolean option. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> Slightly RFC, because there is no easy way of making the opposite "normal boolean" case work for no-vpmu. --- xen/arch/x86/cpu/vpmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)