[PATCH v2] x86: wrap kexec feature with CONFIG_KEXEC

[PATCH v2] x86: wrap kexec feature with CONFIG_KEXEC

[Jonathan Creekmore]

Add the appropriate #if checks around the kexec code in the x86 codebase
so that the feature can actually be turned off by the flag instead of
always required to be enabled on x86.

Signed-off-by: Jonathan Creekmore <jonathan.creekmore@gmail.com>

---
Changed since v1:

  * Reorder kexec files to be alphabetical in the makefile
  * Create macros for the kexec functions that are called when disabled
  * #define the kexec hypercalls to do_ni_hypercall when disabled
  * Remove unnecessary macro duplication
  * Remove unrelated whitespace changes
---
 xen/arch/x86/Makefile              |  4 ++--
 xen/arch/x86/setup.c               | 25 +++++++++++++------------
 xen/arch/x86/x86_64/compat/entry.S |  4 ++++
 xen/arch/x86/x86_64/entry.S        |  4 ++++
 xen/include/xen/kexec.h            | 11 +++++++++++
 5 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5f24951..2fc7f97 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -14,6 +14,7 @@ obj-bin-y += bzimage.init.o
 obj-bin-y += clear_page.o
 obj-bin-y += copy_page.o
 obj-y += compat.o
+obj-$(HAS_KEXEC) += crash.o
 obj-y += debug.o
 obj-y += delay.o
 obj-bin-y += dmi_scan.init.o
@@ -31,6 +32,7 @@ obj-y += io_apic.o
 obj-y += msi.o
 obj-y += ioport_emulate.o
 obj-y += irq.o
+obj-$(HAS_KEXEC) += machine_kexec.o
 obj-y += microcode_amd.o
 obj-y += microcode_intel.o
 # This must come after the vendor specific files.
@@ -56,8 +58,6 @@ obj-y += trace.o
 obj-y += traps.o
 obj-y += usercopy.o
 obj-y += x86_emulate.o
-obj-y += machine_kexec.o
-obj-y += crash.o
 obj-y += tboot.o
 obj-y += hpet.o
 obj-y += vm_event.o
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ff34670..333ba82 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -481,8 +481,8 @@ static void __init parse_video_info(void)
 
 static void __init kexec_reserve_area(struct e820map *e820)
 {
-    unsigned long kdump_start = kexec_crash_area.start;
-    unsigned long kdump_size  = kexec_crash_area.size;
+    unsigned long kdump_start = get_kexec_crash_area_start();
+    unsigned long kdump_size  = get_kexec_crash_area_size();
     static bool_t __initdata is_reserved = 0;
 
     kdump_size = (kdump_size + PAGE_SIZE - 1) & PAGE_MASK;
@@ -496,7 +496,8 @@ static void __init kexec_reserve_area(struct e820map *e820)
     {
         printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at %#lx)"
                "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
-        kexec_crash_area.start = kexec_crash_area.size = 0;
+        set_kexec_crash_area_start(0);
+        set_kexec_crash_area_size(0);
     }
     else
     {
@@ -974,12 +975,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         }
 
         /* Don't overlap with modules. */
-        e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
+        e = consider_modules(s, e, PAGE_ALIGN(get_kexec_crash_area_size()),
                              mod, mbi->mods_count, -1);
-        if ( !kexec_crash_area.start && (s < e) )
+        if ( !get_kexec_crash_area_start() && (s < e) )
         {
-            e = (e - kexec_crash_area.size) & PAGE_MASK;
-            kexec_crash_area.start = e;
+            e = (e - get_kexec_crash_area_size()) & PAGE_MASK;
+            set_kexec_crash_area_start(e);
         }
     }
 
@@ -1125,14 +1126,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                          PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
     }
 
-    if ( kexec_crash_area.size )
+    if ( get_kexec_crash_area_size() )
     {
-        unsigned long s = PFN_DOWN(kexec_crash_area.start);
-        unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
+        unsigned long s = PFN_DOWN(get_kexec_crash_area_start());
+        unsigned long e = min(s + PFN_UP(get_kexec_crash_area_size()),
                               PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
 
-        if ( e > s ) 
-            map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
+        if ( e > s )
+            map_pages_to_xen((unsigned long)__va(get_kexec_crash_area_start()),
                              s, e - s, PAGE_HYPERVISOR);
     }
 
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 1521779..fbe4885 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -12,6 +12,10 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+#ifndef CONFIG_KEXEC
+#define compat_kexec_op do_ni_hypercall
+#endif
+
 ENTRY(compat_hypercall)
         ASM_CLAC
         pushq $0
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 74677a2..a8815a7 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -13,6 +13,10 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+#ifndef CONFIG_KEXEC
+#define do_kexec_op do_ni_hypercall
+#endif
+
 /* %rbx: struct vcpu */
 ENTRY(switch_to_kernel)
         leaq  VCPU_trap_bounce(%rbx),%rdx
diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
index b7d121d..dface92 100644
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -18,6 +18,9 @@ extern xen_kexec_reserve_t kexec_crash_area;
 extern bool_t kexecing;
 
 void set_kexec_crash_area_size(u64 system_ram);
+#define get_kexec_crash_area_size() kexec_crash_area.size
+#define set_kexec_crash_area_start(x) kexec_crash_area.start = (x)
+#define get_kexec_crash_area_start() kexec_crash_area.start
 
 /* We have space for 4 images to support atomic update
  * of images. This is important for CRASH images since
@@ -79,6 +82,14 @@ void vmcoreinfo_append_str(const char *fmt, ...)
 #else /* !CONFIG_KEXEC */
 
 #define crashinfo_maxaddr_bits 0
+#define kexecing 0
+#define kexec_early_calculations()
+#define kexec_crash()
+#define kexec_crash_save_cpu()
+#define set_kexec_crash_area_size(system_ram)
+#define get_kexec_crash_area_size() 0
+#define set_kexec_crash_area_start(x)
+#define get_kexec_crash_area_start() 0
 
 #endif
 
-- 
2.1.4

Re: [PATCH v2] x86: wrap kexec feature with CONFIG_KEXEC

[Jan Beulich]

>>> On 28.08.15 at 17:02, <jonathan.creekmore@gmail.com> wrote:
> Changed since v1:
> 
>   * Reorder kexec files to be alphabetical in the makefile
>   * Create macros for the kexec functions that are called when disabled

Did you try using inline functions wherever possible, and that didn't
work _anywhere_?

>  * #define the kexec hypercalls to do_ni_hypercall when disabled
>  * Remove unnecessary macro duplication
>  * Remove unrelated whitespace changes

Please address all review comments on the previous version before
re-submitting (either verbally or by changing your patch). Quoting
a v1 comment I made:

"But you realize that these HAVE_* variables aren't meant to be used
 for disabling features? If you really wanted something like that, you'd
 need to introduce "kexec=y" in the top level Makefile, overridable by
 "kexec=n" on the make command line."

While I realize I mistakenly wrote HAVE_* when I meant HAS_*, but
the issue remains: There's no top level Makefile control to disable
kexec (see the top of xen/Rules.mk for examples).

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -481,8 +481,8 @@ static void __init parse_video_info(void)
>  
>  static void __init kexec_reserve_area(struct e820map *e820)
>  {
> -    unsigned long kdump_start = kexec_crash_area.start;
> -    unsigned long kdump_size  = kexec_crash_area.size;
> +    unsigned long kdump_start = get_kexec_crash_area_start();
> +    unsigned long kdump_size  = get_kexec_crash_area_size();
>      static bool_t __initdata is_reserved = 0;
>  
>      kdump_size = (kdump_size + PAGE_SIZE - 1) & PAGE_MASK;
> @@ -496,7 +496,8 @@ static void __init kexec_reserve_area(struct e820map *e820)
>      {
>          printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at %#lx)"
>                 "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
> -        kexec_crash_area.start = kexec_crash_area.size = 0;
> +        set_kexec_crash_area_start(0);
> +        set_kexec_crash_area_size(0);
>      }
>      else
>      {

I'm sorry, but I think in cases like this simply making the entire
body of the function conditional with #ifdef is better than fiddling
with the actual code.

> @@ -974,12 +975,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          }
>  
>          /* Don't overlap with modules. */
> -        e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
> +        e = consider_modules(s, e, PAGE_ALIGN(get_kexec_crash_area_size()),
>                               mod, mbi->mods_count, -1);
> -        if ( !kexec_crash_area.start && (s < e) )
> +        if ( !get_kexec_crash_area_start() && (s < e) )
>          {
> -            e = (e - kexec_crash_area.size) & PAGE_MASK;
> -            kexec_crash_area.start = e;
> +            e = (e - get_kexec_crash_area_size()) & PAGE_MASK;
> +            set_kexec_crash_area_start(e);
>          }
>      }
>  
> @@ -1125,14 +1126,14 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                           PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
>      }
>  
> -    if ( kexec_crash_area.size )
> +    if ( get_kexec_crash_area_size() )
>      {
> -        unsigned long s = PFN_DOWN(kexec_crash_area.start);
> -        unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
> +        unsigned long s = PFN_DOWN(get_kexec_crash_area_start());
> +        unsigned long e = min(s + PFN_UP(get_kexec_crash_area_size()),
>                                PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
>  
> -        if ( e > s ) 
> -            map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
> +        if ( e > s )
> +            map_pages_to_xen((unsigned long)__va(get_kexec_crash_area_start()),
>                               s, e - s, PAGE_HYPERVISOR);

While less clear for these, I still think the same as above applies
here too. I.e. often neither the "use #ifdef everywhere" nor the
"don't use #ifdef outside of header files" end up being the best
approach. As a rule of thumb, the change having the least impact
on existing code (which commonly would be the smallest possible
patch) is the most desirable one for adjustments like the one you
propose here.

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -12,6 +12,10 @@
>  #include <public/xen.h>
>  #include <irq_vectors.h>
>  
> +#ifndef CONFIG_KEXEC
> +#define compat_kexec_op do_ni_hypercall
> +#endif

I'd prefer this and ...

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -13,6 +13,10 @@
>  #include <public/xen.h>
>  #include <irq_vectors.h>
>  
> +#ifndef CONFIG_KEXEC
> +#define do_kexec_op do_ni_hypercall
> +#endif

... this to be moved closer to the table where the symbols actually
get used.

Jan