[PATCH 1/3] xen/x86: Drop the uses of invbool_param()

[PATCH 1/3] xen/x86: Drop the uses of invbool_param()

[Andrew Cooper]

There are only four users, and invbool_param() is an unnecessary cognitive
overhead to use.

Convert the four users to boolean_param(), and consistency use opt_* for the
variable name.

No change to the behaviour of the command line arguments.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpu/mcheck/mce.c       |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce_intel.c |  8 ++++----
 xen/arch/x86/cpu/mcheck/non-fatal.c |  2 +-
 xen/arch/x86/cpu/mcheck/x86_mca.h   |  2 +-
 xen/arch/x86/cpu/mwait-idle.c       |  6 +++---
 xen/arch/x86/setup.c                | 12 ++++++------
 6 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index d746d0e..cc446eb 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -30,8 +30,8 @@
 #include "util.h"
 #include "vmce.h"
 
-bool_t __read_mostly mce_disabled;
-invbool_param("mce", mce_disabled);
+bool_t __read_mostly opt_mce = 1;
+boolean_param("mce", opt_mce);
 bool_t __read_mostly mce_broadcast = 0;
 bool_t is_mc_panic;
 unsigned int __read_mostly nr_mce_banks;
@@ -627,7 +627,7 @@ static void set_poll_bankmask(struct cpuinfo_x86 *c)
     mb = per_cpu(poll_bankmask, cpu);
     BUG_ON(!mb);
 
-    if (cmci_support && !mce_disabled) {
+    if (cmci_support && opt_mce) {
         mb->num = per_cpu(no_cmci_banks, cpu)->num;
         bitmap_copy(mb->bank_map, per_cpu(no_cmci_banks, cpu)->bank_map,
                     nr_mce_banks);
@@ -734,7 +734,7 @@ void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp)
 {
     enum mcheck_type inited = mcheck_none;
 
-    if ( mce_disabled )
+    if ( !opt_mce )
     {
         if ( bsp )
             printk(XENLOG_INFO "MCE support disabled by bootparam\n");
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index 193366b..0b7fe53 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -587,7 +587,7 @@ static void cmci_discover(void)
 
 static void mce_set_owner(void)
 {
-    if (!cmci_support || mce_disabled == 1)
+    if (!cmci_support || !opt_mce)
         return;
 
     cmci_discover();
@@ -600,7 +600,7 @@ static void __cpu_mcheck_distribute_cmci(void *unused)
 
 static void cpu_mcheck_distribute_cmci(void)
 {
-    if (cmci_support && !mce_disabled)
+    if (cmci_support && opt_mce)
         on_each_cpu(__cpu_mcheck_distribute_cmci, NULL, 0);
 }
 
@@ -608,7 +608,7 @@ static void clear_cmci(void)
 {
     int i;
 
-    if (!cmci_support || mce_disabled == 1)
+    if (!cmci_support || !opt_mce)
         return;
 
     mce_printk(MCE_VERBOSE, "CMCI: clear_cmci support on CPU%d\n",
@@ -630,7 +630,7 @@ static void cpu_mcheck_disable(void)
 {
     clear_in_cr4(X86_CR4_MCE);
 
-    if (cmci_support && !mce_disabled)
+    if (cmci_support && opt_mce)
         clear_cmci();
 }
 
diff --git a/xen/arch/x86/cpu/mcheck/non-fatal.c b/xen/arch/x86/cpu/mcheck/non-fatal.c
index 8cd6635..da5cae9 100644
--- a/xen/arch/x86/cpu/mcheck/non-fatal.c
+++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
@@ -91,7 +91,7 @@ static int __init init_nonfatal_mce_checker(void)
 	struct cpuinfo_x86 *c = &boot_cpu_data;
 
 	/* Check for MCE support */
-	if (mce_disabled || !mce_available(c))
+	if (!opt_mce || !mce_available(c))
 		return -ENODEV;
 
 	if (__get_cpu_var(poll_bankmask) == NULL)
diff --git a/xen/arch/x86/cpu/mcheck/x86_mca.h b/xen/arch/x86/cpu/mcheck/x86_mca.h
index 76467d6..e25d619 100644
--- a/xen/arch/x86/cpu/mcheck/x86_mca.h
+++ b/xen/arch/x86/cpu/mcheck/x86_mca.h
@@ -153,6 +153,6 @@ struct mca_error_handler
 };
 
 /* Global variables */
-extern bool_t mce_disabled;
+extern bool_t opt_mce;
 
 #endif /* X86_MCA_H */
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 87f0e60..b86bdd7 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -70,8 +70,8 @@
 # define pr_debug(fmt...)
 #endif
 
-static __initdata bool_t no_mwait_idle;
-invbool_param("mwait-idle", no_mwait_idle);
+static __initdata bool_t opt_mwait_idle = 1;
+boolean_param("mwait-idle", opt_mwait_idle);
 
 static unsigned int mwait_substates;
 
@@ -832,7 +832,7 @@ static int __init mwait_idle_probe(void)
 	    !mwait_substates)
 		return -ENODEV;
 
-	if (!max_cstate || no_mwait_idle) {
+	if (!max_cstate || !opt_mwait_idle) {
 		pr_debug(PREFIX "disabled\n");
 		return -EPERM;
 	}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 76c7b0f..b8a28d7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -60,12 +60,12 @@ static unsigned int __initdata max_cpus;
 integer_param("maxcpus", max_cpus);
 
 /* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
-static bool_t __initdata disable_smep;
-invbool_param("smep", disable_smep);
+static bool_t __initdata opt_smep = 1;
+boolean_param("smep", opt_smep);
 
 /* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata disable_smap;
-invbool_param("smap", disable_smap);
+static bool_t __initdata opt_smap = 1;
+boolean_param("smap", opt_smap);
 
 /* Boot dom0 in pvh mode */
 static bool_t __initdata opt_dom0pvh;
@@ -1297,12 +1297,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     set_in_cr4(X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT);
 
-    if ( disable_smep )
+    if ( !opt_smep )
         setup_clear_cpu_cap(X86_FEATURE_SMEP);
     if ( cpu_has_smep )
         set_in_cr4(X86_CR4_SMEP);
 
-    if ( disable_smap )
+    if ( !opt_smap )
         setup_clear_cpu_cap(X86_FEATURE_SMAP);
     if ( cpu_has_smap )
         set_in_cr4(X86_CR4_SMAP);
-- 
2.1.4

[PATCH 2/3] xen/init: Drop invbool_param()

[Andrew Cooper]

There are now no users.  No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
---
 xen/common/kernel.c    | 3 +--
 xen/include/xen/init.h | 5 -----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 6a3196a..af6e563 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -121,12 +121,11 @@ void __init cmdline_parse(const char *cmdline)
                     simple_strtoll(optval, NULL, 0));
                 break;
             case OPT_BOOL:
-            case OPT_INVBOOL:
                 if ( !parse_bool(optval) )
                     bool_assert = !bool_assert;
                 assign_integer_param(
                     param,
-                    (param->type == OPT_BOOL) == bool_assert);
+                    bool_assert);
                 break;
             case OPT_SIZE:
                 assign_integer_param(
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 7072013..6081102 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -78,7 +78,6 @@ struct kernel_param {
         OPT_STR,
         OPT_UINT,
         OPT_BOOL,
-        OPT_INVBOOL,
         OPT_SIZE,
         OPT_CUSTOM
     } type;
@@ -98,10 +97,6 @@ extern struct kernel_param __setup_start, __setup_end;
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
         { __setup_str_##_var, OPT_BOOL, &_var, sizeof(_var) }
-#define invbool_param(_name, _var) \
-    __setup_str __setup_str_##_var[] = _name; \
-    __kparam __setup_##_var = \
-        { __setup_str_##_var, OPT_INVBOOL, &_var, sizeof(_var) }
 #define integer_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
     __kparam __setup_##_var = \
-- 
2.1.4

[PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Andrew Cooper]

There is no reason for any of it to be modified.  Additionally, link
.init.setup beside the other constant .init data.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 xen/arch/arm/xen.lds.S | 11 ++++++-----
 xen/arch/x86/xen.lds.S | 11 ++++++-----
 xen/include/xen/init.h |  4 ++--
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index f501a2f..0e2c582 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -115,6 +115,12 @@ SECTIONS
   .init.data : {
        *(.init.rodata)
        *(.init.rodata.str*)
+
+       . = ALIGN(32);
+       __setup_start = .;
+       *(.init.setup)
+       __setup_end = .;
+
        *(.init.data)
        *(.init.data.rel)
        *(.init.data.rel.*)
@@ -125,11 +131,6 @@ SECTIONS
        __ctors_end = .;
   } :text
   . = ALIGN(32);
-  .init.setup : {
-       __setup_start = .;
-       *(.init.setup)
-       __setup_end = .;
-  } :text
   .init.proc.info : {
        __proc_info_start = .;
        *(.init.proc.info)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c1ce027..86905c8 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -120,6 +120,12 @@ SECTIONS
   .init.data : {
        *(.init.rodata)
        *(.init.rodata.str*)
+
+       . = ALIGN(32);
+       __setup_start = .;
+       *(.init.setup)
+       __setup_end = .;
+
        *(.init.data)
        *(.init.data.rel)
        *(.init.data.rel.*)
@@ -146,11 +152,6 @@ SECTIONS
        __ctors_end = .;
   } :text
   . = ALIGN(32);
-  .init.setup : {
-       __setup_start = .;
-       *(.init.setup)
-       __setup_end = .;
-  } :text
   .initcall.init : {
        __initcall_start = .;
        *(.initcallpresmp.init)
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 6081102..142c5c8 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -87,8 +87,8 @@ struct kernel_param {
 
 extern struct kernel_param __setup_start, __setup_end;
 
-#define __setup_str static __initdata __attribute__((__aligned__(1))) char
-#define __kparam static __initsetup struct kernel_param
+#define __setup_str static const __initconst __attribute__((__aligned__(1))) char
+#define __kparam static const __initsetup struct kernel_param
 
 #define custom_param(_name, _var) \
     __setup_str __setup_str_##_var[] = _name; \
-- 
2.1.4

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Jan Beulich]

>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -120,6 +120,12 @@ SECTIONS
>    .init.data : {
>         *(.init.rodata)
>         *(.init.rodata.str*)
> +
> +       . = ALIGN(32);

Why 32?

> +       __setup_start = .;
> +       *(.init.setup)
> +       __setup_end = .;
> +
>         *(.init.data)
>         *(.init.data.rel)
>         *(.init.data.rel.*)
> @@ -146,11 +152,6 @@ SECTIONS
>         __ctors_end = .;
>    } :text
>    . = ALIGN(32);
> -  .init.setup : {
> -       __setup_start = .;
> -       *(.init.setup)
> -       __setup_end = .;
> -  } :text

If just because it was 32 here, I don't think that's a compelling
reason. With it (above, not necessarily here) reduced to 8 (which
of course could also be done while committing, if you agree and
there are no deeper reasons),
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 1/3] xen/x86: Drop the uses of invbool_param()

[Jan Beulich]

>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
> There are only four users, and invbool_param() is an unnecessary cognitive
> overhead to use.

While this isn't necessarily a bad change, I don't agree to either of
these arguments. So I'm going to make my ack here dependent on
seeing some kind of positive feedback on patch 2 by another
REST maintainer.

Jan

Re: [PATCH 2/3] xen/init: Drop invbool_param()

[Jan Beulich]

>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -121,12 +121,11 @@ void __init cmdline_parse(const char *cmdline)
>                      simple_strtoll(optval, NULL, 0));
>                  break;
>              case OPT_BOOL:
> -            case OPT_INVBOOL:
>                  if ( !parse_bool(optval) )
>                      bool_assert = !bool_assert;
>                  assign_integer_param(
>                      param,
> -                    (param->type == OPT_BOOL) == bool_assert);
> +                    bool_assert);

This function invocation should now really become a single line.

See patch 1 for my condition for providing an ack here.

Jan

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Andrew Cooper]

On 09/02/16 12:43, Jan Beulich wrote:
>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -120,6 +120,12 @@ SECTIONS
>>    .init.data : {
>>         *(.init.rodata)
>>         *(.init.rodata.str*)
>> +
>> +       . = ALIGN(32);
> Why 32?
>
>> +       __setup_start = .;
>> +       *(.init.setup)
>> +       __setup_end = .;
>> +
>>         *(.init.data)
>>         *(.init.data.rel)
>>         *(.init.data.rel.*)
>> @@ -146,11 +152,6 @@ SECTIONS
>>         __ctors_end = .;
>>    } :text
>>    . = ALIGN(32);
>> -  .init.setup : {
>> -       __setup_start = .;
>> -       *(.init.setup)
>> -       __setup_end = .;
>> -  } :text
> If just because it was 32 here, I don't think that's a compelling
> reason. With it (above, not necessarily here) reduced to 8 (which
> of course could also be done while committing, if you agree and
> there are no deeper reasons),
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

It was just because of the code here.  I still can't think of any
specific reason why 32 is needed, so the ALIGN() can just be dropped.

~Andrew

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Jan Beulich]

>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote:
> On 09/02/16 12:43, Jan Beulich wrote:
>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -120,6 +120,12 @@ SECTIONS
>>>    .init.data : {
>>>         *(.init.rodata)
>>>         *(.init.rodata.str*)
>>> +
>>> +       . = ALIGN(32);
>> Why 32?
>>
>>> +       __setup_start = .;
>>> +       *(.init.setup)
>>> +       __setup_end = .;
>>> +
>>>         *(.init.data)
>>>         *(.init.data.rel)
>>>         *(.init.data.rel.*)
>>> @@ -146,11 +152,6 @@ SECTIONS
>>>         __ctors_end = .;
>>>    } :text
>>>    . = ALIGN(32);
>>> -  .init.setup : {
>>> -       __setup_start = .;
>>> -       *(.init.setup)
>>> -       __setup_end = .;
>>> -  } :text
>> If just because it was 32 here, I don't think that's a compelling
>> reason. With it (above, not necessarily here) reduced to 8 (which
>> of course could also be done while committing, if you agree and
>> there are no deeper reasons),
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> It was just because of the code here.  I still can't think of any
> specific reason why 32 is needed, so the ALIGN() can just be dropped.

No, dropping ALIGN() altogether would make the placement of
__setup_start dependent upon the alignment of the previous
section (and since it's a strings section which precedes it,
problems would be quite likely). (This is, btw., why it would be
better if we used __startof__ and __alignof__ instead of linker
script generated symbols. I may give that a try ...)

Jan

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Andrew Cooper]

On 09/02/16 14:32, Jan Beulich wrote:
>>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote:
>> On 09/02/16 12:43, Jan Beulich wrote:
>>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/xen.lds.S
>>>> +++ b/xen/arch/x86/xen.lds.S
>>>> @@ -120,6 +120,12 @@ SECTIONS
>>>>    .init.data : {
>>>>         *(.init.rodata)
>>>>         *(.init.rodata.str*)
>>>> +
>>>> +       . = ALIGN(32);
>>> Why 32?
>>>
>>>> +       __setup_start = .;
>>>> +       *(.init.setup)
>>>> +       __setup_end = .;
>>>> +
>>>>         *(.init.data)
>>>>         *(.init.data.rel)
>>>>         *(.init.data.rel.*)
>>>> @@ -146,11 +152,6 @@ SECTIONS
>>>>         __ctors_end = .;
>>>>    } :text
>>>>    . = ALIGN(32);
>>>> -  .init.setup : {
>>>> -       __setup_start = .;
>>>> -       *(.init.setup)
>>>> -       __setup_end = .;
>>>> -  } :text
>>> If just because it was 32 here, I don't think that's a compelling
>>> reason. With it (above, not necessarily here) reduced to 8 (which
>>> of course could also be done while committing, if you agree and
>>> there are no deeper reasons),
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> It was just because of the code here.  I still can't think of any
>> specific reason why 32 is needed, so the ALIGN() can just be dropped.
> No, dropping ALIGN() altogether would make the placement of
> __setup_start dependent upon the alignment of the previous
> section (and since it's a strings section which precedes it,
> problems would be quite likely). (This is, btw., why it would be
> better if we used __startof__ and __alignof__ instead of linker
> script generated symbols. I may give that a try ...)

Actually, thinking about it,

andrewcoop@andrewcoop:/local/xen.git/xen$ pahole xen-syms -C kernel_param
struct kernel_param {
    const char  *              name;                 /*     0     8 */
    enum {
        OPT_STR = 0,
        OPT_UINT = 1,
        OPT_BOOL = 2,
        OPT_SIZE = 3,
        OPT_CUSTOM = 4,
    } type;                                          /*     8     4 */

    /* XXX 4 bytes hole, try to pack */

    void *                     var;                  /*    16     8 */
    unsigned int               len;                  /*    24     4 */

    /* size: 32, cachelines: 1, members: 4 */
    /* sum members: 24, holes: 1, sum holes: 4 */
    /* padding: 4 */
    /* last cacheline: 32 bytes */
};

This will be where the 32 byte alignment comes from, although dropping
the structure size to 24 bytes would be trivial.

~Andrew

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Jan Beulich]

>>> On 09.02.16 at 15:40, <andrew.cooper3@citrix.com> wrote:
> On 09/02/16 14:32, Jan Beulich wrote:
>>>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote:
>>> On 09/02/16 12:43, Jan Beulich wrote:
>>>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -120,6 +120,12 @@ SECTIONS
>>>>>    .init.data : {
>>>>>         *(.init.rodata)
>>>>>         *(.init.rodata.str*)
>>>>> +
>>>>> +       . = ALIGN(32);
>>>> Why 32?
>>>>
>>>>> +       __setup_start = .;
>>>>> +       *(.init.setup)
>>>>> +       __setup_end = .;
>>>>> +
>>>>>         *(.init.data)
>>>>>         *(.init.data.rel)
>>>>>         *(.init.data.rel.*)
>>>>> @@ -146,11 +152,6 @@ SECTIONS
>>>>>         __ctors_end = .;
>>>>>    } :text
>>>>>    . = ALIGN(32);
>>>>> -  .init.setup : {
>>>>> -       __setup_start = .;
>>>>> -       *(.init.setup)
>>>>> -       __setup_end = .;
>>>>> -  } :text
>>>> If just because it was 32 here, I don't think that's a compelling
>>>> reason. With it (above, not necessarily here) reduced to 8 (which
>>>> of course could also be done while committing, if you agree and
>>>> there are no deeper reasons),
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> It was just because of the code here.  I still can't think of any
>>> specific reason why 32 is needed, so the ALIGN() can just be dropped.
>> No, dropping ALIGN() altogether would make the placement of
>> __setup_start dependent upon the alignment of the previous
>> section (and since it's a strings section which precedes it,
>> problems would be quite likely). (This is, btw., why it would be
>> better if we used __startof__ and __alignof__ instead of linker
>> script generated symbols. I may give that a try ...)
> 
> Actually, thinking about it,
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ pahole xen-syms -C kernel_param
> struct kernel_param {
>     const char  *              name;                 /*     0     8 */
>     enum {
>         OPT_STR = 0,
>         OPT_UINT = 1,
>         OPT_BOOL = 2,
>         OPT_SIZE = 3,
>         OPT_CUSTOM = 4,
>     } type;                                          /*     8     4 */
> 
>     /* XXX 4 bytes hole, try to pack */
> 
>     void *                     var;                  /*    16     8 */
>     unsigned int               len;                  /*    24     4 */
> 
>     /* size: 32, cachelines: 1, members: 4 */
>     /* sum members: 24, holes: 1, sum holes: 4 */
>     /* padding: 4 */
>     /* last cacheline: 32 bytes */
> };
> 
> This will be where the 32 byte alignment comes from,

But that's the structure _size_, unrelated to any needed alignment.

> although dropping
> the structure size to 24 bytes would be trivial.

Patch already done (to be re-based on top of yours).

Jan

Re: [PATCH 1/3] xen/x86: Drop the uses of invbool_param()

[Ian Campbell]

On Tue, 2016-02-09 at 05:46 -0700, Jan Beulich wrote:
> > > > On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
> > There are only four users, and invbool_param() is an unnecessary
> > cognitive
> > overhead to use.
> 
> While this isn't necessarily a bad change, I don't agree to either of
> these arguments. So I'm going to make my ack here dependent on
> seeing some kind of positive feedback on patch 2 by another
> REST maintainer.

FWIW I do have to think a little harder about the double negative in
        bool_t foo; /* implicitly 0  init */
        invbool_param("foo", foo_disabled);
and what no-foo on the command line actually does in this case than I would
otherwise with:
        bool_t foo = true;
        boolean_param("foo", foo);

AFAICT the only real benefit of the inversion is that the variable can live
in .bss instead of .data, but it's a total of 4 bool_t's (so 4 bytes? Or
maybe 16 at the most) which hardly seems worth it to me.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Jan Beulich]

>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
> There is no reason for any of it to be modified.  Additionally, link
> .init.setup beside the other constant .init data.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Sadly I've noticed only after pushing that this breaks the build
with older gcc. 4.3.4 complains about a section type conflict in
mwait-idle.c. I've reverted the change for the time being.

Jan

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Andrew Cooper]

On 22/02/16 16:36, Jan Beulich wrote:
>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>> There is no reason for any of it to be modified.  Additionally, link
>> .init.setup beside the other constant .init data.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Sadly I've noticed only after pushing that this breaks the build
> with older gcc. 4.3.4 complains about a section type conflict in
> mwait-idle.c. I've reverted the change for the time being.

/sigh - I will investigate.

~Andrew

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]

On 2/22/16 10:36 AM, Jan Beulich wrote:
>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>> There is no reason for any of it to be modified.  Additionally, link
>> .init.setup beside the other constant .init data.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Sadly I've noticed only after pushing that this breaks the build
> with older gcc. 4.3.4 complains about a section type conflict in
> mwait-idle.c. I've reverted the change for the time being.
> 
> Jan
> 

Jan,

Can you tell me the distro (and any specifics you've got) which has this
environment? I'm setting up more build tests and I'll add this to there.

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 3/3] xen/init: Annotate all command line parameter infrastructure as const

[Jan Beulich]

>>> On 22.02.16 at 19:43, <cardoe@cardoe.com> wrote:
> On 2/22/16 10:36 AM, Jan Beulich wrote:
>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote:
>>> There is no reason for any of it to be modified.  Additionally, link
>>> .init.setup beside the other constant .init data.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> 
>> Sadly I've noticed only after pushing that this breaks the build
>> with older gcc. 4.3.4 complains about a section type conflict in
>> mwait-idle.c. I've reverted the change for the time being.
> 
> Can you tell me the distro (and any specifics you've got) which has this
> environment? I'm setting up more build tests and I'll add this to there.

SLE11; I don't think any other aspects matter much, nor do I think
the specific distro matters, as I don't expect we would have too
may patches on top of the respective upstream gcc.

Jan