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

Commit Message

Andrew Cooper Feb. 8, 2016, 5:07 p.m. UTC

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 <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
---
 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(-)

Comments

Jan Beulich Feb. 9, 2016, 12:46 p.m. UTC | #1

>>> On 08.02.16 at 18:07, <[e-mail]> 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

Ian Campbell Feb. 11, 2016, 9:58 a.m. UTC | #2

On Tue, 2016-02-09 at 05:46 -0700, Jan Beulich wrote:
> > > > On 08.02.16 at 18:07, <[e-mail]> 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.

Patch

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);