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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.08.15 at 16:47, <jonathan.creekmore@xxxxxxxxx> wrote:
> 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.

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.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -56,8 +56,8 @@ 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-$(HAS_KEXEC) += machine_kexec.o
> +obj-$(HAS_KEXEC) += crash.o
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += vm_event.o

If you already touch this, please move the two altered lines to their
proper (alphabetical) slot.

> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -64,7 +64,9 @@ static void noreturn do_nmi_crash(const struct 
> cpu_user_regs *regs)
>           */
>          set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
>  
> +#ifdef CONFIG_KEXEC
>          kexec_crash_save_cpu();
> +#endif

In cases like this code will look much better if you introduced a no-op
inline function for the !CONFIG_KEXEC case in the appropriate header.

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S

Similarly at least the changes here ...

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S

.. and here will want better abstraction, to not further uglify the code.

> --- a/xen/drivers/passthrough/vtd/dmar.h
> +++ b/xen/drivers/passthrough/vtd/dmar.h
> @@ -108,6 +108,7 @@ struct acpi_atsr_unit *acpi_find_matched_atsr_unit(const 
> struct pci_dev *);
>  
>  #define DMAR_OPERATION_TIMEOUT MILLISECS(1000)
>  
> +#ifdef CONFIG_KEXEC
>  #define IOMMU_WAIT_OP(iommu, offset, op, cond, sts) \
>  do {                                                \
>      s_time_t start_time = NOW();                    \
> @@ -125,6 +126,22 @@ do {                                                \
>          cpu_relax();                                            \
>      }                                                           \
>  } while (0)
> +#else
> +#define IOMMU_WAIT_OP(iommu, offset, op, cond, sts) \
> +do {                                                \
> +    s_time_t start_time = NOW();                    \
> +    while (1) {                                     \
> +        sts = op(iommu->reg, offset);               \
> +        if ( cond )                                 \
> +            break;                                  \
> +        if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT ) {    \
> +       panic("%s:%d:%s: DMAR hardware is malfunctional",     \
> +             __FILE__, __LINE__, __func__);                  \
> +        }                                                       \
> +        cpu_relax();                                            \
> +    }                                                           \
> +} while (0)
> +#endif

Along the same lines here - no way for such a macro to be duplicated
for a case likely no-one but you will ever build-test.

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -1,6 +1,6 @@
>  
> /****************************************************************************
> **
>   * config.h
> - * 
> + *
>   * A Linux-style configuration list.
>   */
>  

Please leave out this unrelated white space change.

Jan


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