| Message ID | 20200226124705.29212-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add hypervisor sysfs-like support | expand |
On 26.02.2020 13:46, Juergen Gross wrote: > Support of other variable sizes than that of normal bool ones for > boolean_parameter() don't make sense, so catch any other sized > variables at build time. Nit: boolean_param() > @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; > __kparam __setup_##_var = \ > { .name = __setup_str_##_var, \ > .type = OPT_BOOL, \ > - .len = sizeof(_var), \ > + .len = sizeof(_var) + \ > + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ I was first going to suggest to see about tightening this to do an actual type check, but I think we have a number of cases where boolean_param() actually involves int8_t variables, to allow us to detect whether a command line option was used. Hence Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Juergen, On 26/02/2020 12:46, Juergen Gross wrote: > Support of other variable sizes than that of normal bool ones for > boolean_parameter() don't make sense, so catch any other sized > variables at build time. > > Fix the one parameter using a plain int instead of bool. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V6: > - new patch > --- > xen/arch/x86/hvm/asid.c | 2 +- > xen/include/xen/param.h | 8 ++++++-- > 2 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/hvm/asid.c b/xen/arch/x86/hvm/asid.c > index 8e00a28443..8b5bb86dfd 100644 > --- a/xen/arch/x86/hvm/asid.c > +++ b/xen/arch/x86/hvm/asid.c > @@ -25,7 +25,7 @@ > #include <asm/hvm/asid.h> > > /* Xen command-line option to enable ASIDs */ > -static int opt_asid_enabled = 1; > +static bool opt_asid_enabled = true; > boolean_param("asid", opt_asid_enabled); > > /* > diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h > index 75471eb4ad..d4578cd27f 100644 > --- a/xen/include/xen/param.h > +++ b/xen/include/xen/param.h > @@ -2,6 +2,8 @@ > #define _XEN_PARAM_H > > #include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/stdbool.h> > > /* > * Used for kernel command line parameter setup > @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; > __kparam __setup_##_var = \ > { .name = __setup_str_##_var, \ > .type = OPT_BOOL, \ > - .len = sizeof(_var), \ > + .len = sizeof(_var) + \ > + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ From my understanding, sizeof(bool) is not necessarily 1 (it can be greater). While this is fine to use it in Xen, I think we want it to always be one when exposed in the hypfs. > .par.var = &_var } > #define integer_param(_name, _var) \ > __setup_str __setup_str_##_var[] = _name; \ > @@ -86,7 +89,8 @@ extern const struct kernel_param __param_start[], __param_end[]; > __rtparam __rtpar_##_var = \ > { .name = _name, \ > .type = OPT_BOOL, \ > - .len = sizeof(_var), \ > + .len = sizeof(_var) + \ > + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ > .par.var = &_var } > #define integer_runtime_only_param(_name, _var) \ > __rtparam __rtpar_##_var = \ > Cheers,
On 09.03.2020 12:43, Julien Grall wrote: > On 26/02/2020 12:46, Juergen Gross wrote: >> @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; >> __kparam __setup_##_var = \ >> { .name = __setup_str_##_var, \ >> .type = OPT_BOOL, \ >> - .len = sizeof(_var), \ >> + .len = sizeof(_var) + \ >> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ > > From my understanding, sizeof(bool) is not necessarily 1 (it can be > greater). While this is fine to use it in Xen, I think we want it to > always be one when exposed in the hypfs. I don't think so: We want variable of type 'bool' to be updated consistently (i.e. by a write to the full variable). Hence I think sizeof(bool) is correct here. I can see though that the hypercall interface then gains a dependency on the hypervisor's representation of 'bool', but I think such ought to be taken care of in the function carrying out the write, not in the macro here. Jan
On 09.03.20 12:55, Jan Beulich wrote: > On 09.03.2020 12:43, Julien Grall wrote: >> On 26/02/2020 12:46, Juergen Gross wrote: >>> @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; >>> __kparam __setup_##_var = \ >>> { .name = __setup_str_##_var, \ >>> .type = OPT_BOOL, \ >>> - .len = sizeof(_var), \ >>> + .len = sizeof(_var) + \ >>> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ >> >> From my understanding, sizeof(bool) is not necessarily 1 (it can be >> greater). While this is fine to use it in Xen, I think we want it to >> always be one when exposed in the hypfs. > > I don't think so: We want variable of type 'bool' to be updated > consistently (i.e. by a write to the full variable). Hence I > think sizeof(bool) is correct here. I can see though that the > hypercall interface then gains a dependency on the hypervisor's > representation of 'bool', but I think such ought to be taken > care of in the function carrying out the write, not in the > macro here. So you think I should special case bool entries when returning the size information? Or do you think its fine to have the hypervisor's size reported and let the lib do the size handling correctly? Juergen
On 09.03.2020 14:01, Jürgen Groß wrote: > On 09.03.20 12:55, Jan Beulich wrote: >> On 09.03.2020 12:43, Julien Grall wrote: >>> On 26/02/2020 12:46, Juergen Gross wrote: >>>> @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; >>>> __kparam __setup_##_var = \ >>>> { .name = __setup_str_##_var, \ >>>> .type = OPT_BOOL, \ >>>> - .len = sizeof(_var), \ >>>> + .len = sizeof(_var) + \ >>>> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ >>> >>> From my understanding, sizeof(bool) is not necessarily 1 (it can be >>> greater). While this is fine to use it in Xen, I think we want it to >>> always be one when exposed in the hypfs. >> >> I don't think so: We want variable of type 'bool' to be updated >> consistently (i.e. by a write to the full variable). Hence I >> think sizeof(bool) is correct here. I can see though that the >> hypercall interface then gains a dependency on the hypervisor's >> representation of 'bool', but I think such ought to be taken >> care of in the function carrying out the write, not in the >> macro here. > > So you think I should special case bool entries when returning the > size information? Or do you think its fine to have the hypervisor's > size reported and let the lib do the size handling correctly? Either way would be fine by me, but I think not having callers have a (required) way to know the hypervisor's sizeof(bool) would be a more clean interface overall. Jan
On 09.03.20 14:06, Jan Beulich wrote: > On 09.03.2020 14:01, Jürgen Groß wrote: >> On 09.03.20 12:55, Jan Beulich wrote: >>> On 09.03.2020 12:43, Julien Grall wrote: >>>> On 26/02/2020 12:46, Juergen Gross wrote: >>>>> @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; >>>>> __kparam __setup_##_var = \ >>>>> { .name = __setup_str_##_var, \ >>>>> .type = OPT_BOOL, \ >>>>> - .len = sizeof(_var), \ >>>>> + .len = sizeof(_var) + \ >>>>> + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ >>>> >>>> From my understanding, sizeof(bool) is not necessarily 1 (it can be >>>> greater). While this is fine to use it in Xen, I think we want it to >>>> always be one when exposed in the hypfs. >>> >>> I don't think so: We want variable of type 'bool' to be updated >>> consistently (i.e. by a write to the full variable). Hence I >>> think sizeof(bool) is correct here. I can see though that the >>> hypercall interface then gains a dependency on the hypervisor's >>> representation of 'bool', but I think such ought to be taken >>> care of in the function carrying out the write, not in the >>> macro here. >> >> So you think I should special case bool entries when returning the >> size information? Or do you think its fine to have the hypervisor's >> size reported and let the lib do the size handling correctly? > > Either way would be fine by me, but I think not having callers have > a (required) way to know the hypervisor's sizeof(bool) would be a > more clean interface overall. The size is reported via the dirent, so this is fine. And when you have a look into patch 5 you'll see that the writing of the bool is merged with the uint writing in the lib code. I should remove the safety check regarding sizeof(bool) matching the size reported in the dirent, however (this is a leftover I forgot to remove). Juergen
diff --git a/xen/arch/x86/hvm/asid.c b/xen/arch/x86/hvm/asid.c index 8e00a28443..8b5bb86dfd 100644 --- a/xen/arch/x86/hvm/asid.c +++ b/xen/arch/x86/hvm/asid.c @@ -25,7 +25,7 @@ #include <asm/hvm/asid.h> /* Xen command-line option to enable ASIDs */ -static int opt_asid_enabled = 1; +static bool opt_asid_enabled = true; boolean_param("asid", opt_asid_enabled); /* diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h index 75471eb4ad..d4578cd27f 100644 --- a/xen/include/xen/param.h +++ b/xen/include/xen/param.h @@ -2,6 +2,8 @@ #define _XEN_PARAM_H #include <xen/init.h> +#include <xen/lib.h> +#include <xen/stdbool.h> /* * Used for kernel command line parameter setup @@ -46,7 +48,8 @@ extern const struct kernel_param __param_start[], __param_end[]; __kparam __setup_##_var = \ { .name = __setup_str_##_var, \ .type = OPT_BOOL, \ - .len = sizeof(_var), \ + .len = sizeof(_var) + \ + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ .par.var = &_var } #define integer_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ @@ -86,7 +89,8 @@ extern const struct kernel_param __param_start[], __param_end[]; __rtparam __rtpar_##_var = \ { .name = _name, \ .type = OPT_BOOL, \ - .len = sizeof(_var), \ + .len = sizeof(_var) + \ + BUILD_BUG_ON_ZERO(sizeof(_var) != sizeof(bool)), \ .par.var = &_var } #define integer_runtime_only_param(_name, _var) \ __rtparam __rtpar_##_var = \
Support of other variable sizes than that of normal bool ones for boolean_parameter() don't make sense, so catch any other sized variables at build time. Fix the one parameter using a plain int instead of bool. Signed-off-by: Juergen Gross <jgross@suse.com> --- V6: - new patch --- xen/arch/x86/hvm/asid.c | 2 +- xen/include/xen/param.h | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-)