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)), \
           .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 = \
-- 
2.16.4


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

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

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

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,

-- 
Julien Grall

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

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

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

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

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

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

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

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

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