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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.08.15 at 17:02, <jonathan.creekmore@xxxxxxxxx> 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

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