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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/08/15 15:47, Jonathan Creekmore 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.
>
> Signed-off-by: Jonathan Creekmore <jonathan.creekmore@xxxxxxxxx>

In principle, this is a good change, but is definitely aimed at 4.7 now.

> diff --git a/xen/arch/x86/x86_64/compat/entry.S 
> b/xen/arch/x86/x86_64/compat/entry.S
> index 1521779..e2fe49c 100644
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -428,7 +428,11 @@ ENTRY(compat_hypercall_table)
>          .quad do_hvm_op
>          .quad do_sysctl                 /* 35 */
>          .quad do_domctl
> +#if CONFIG_KEXEC
>          .quad compat_kexec_op
> +#else
> +        .quad do_ni_hypercall
> +#endif
>          .quad do_tmem_op
>          .quad do_ni_hypercall           /* reserved for XenClient */
>          .quad do_xenpmu_op              /* 40 */
> @@ -479,6 +483,11 @@ ENTRY(compat_hypercall_args_table)
>          .byte 2 /* do_hvm_op                */
>          .byte 1 /* do_sysctl                */  /* 35 */
>          .byte 1 /* do_domctl                */
> +#if CONFIG_KEXEC
> +        .byte 2 /* compat_kexec_op          */
> +#else
> +     .byte 0 /* do_ni_hypercall          */
> +#endif

You have a hard tab here.

>          .byte 2 /* compat_kexec_op          */
>          .byte 1 /* do_tmem_op               */
>          .byte 0 /* reserved for XenClient   */
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 74677a2..357f3d5 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -761,7 +761,11 @@ ENTRY(hypercall_table)
>          .quad do_hvm_op
>          .quad do_sysctl             /* 35 */
>          .quad do_domctl
> +#ifdef CONFIG_KEXEC
>          .quad do_kexec_op
> +#else
> +     .quad do_ni_hypercall
> +#endif
>          .quad do_tmem_op
>          .quad do_ni_hypercall       /* reserved for XenClient */
>          .quad do_xenpmu_op          /* 40 */
> @@ -812,7 +816,11 @@ ENTRY(hypercall_args_table)
>          .byte 2 /* do_hvm_op            */
>          .byte 1 /* do_sysctl            */  /* 35 */
>          .byte 1 /* do_domctl            */
> +#ifdef CONFIG_KEXEC
>          .byte 2 /* do_kexec             */
> +#else
> +        .byte 0 /* do_ni_hypercall      */
> +#endif

These changes will corrupt guest registers in debug builds.

Don't alter the args table at all, and use

#ifndef CONFIG_KEXEC
#define do_kexec_op do_ni_hypercall
#define compat_kexec_op do_ni_hypercall
#endif

rather than patching the hypercall table itself.

~Andrew

>          .byte 1 /* do_tmem_op           */
>          .byte 0 /* reserved for XenClient */
>          .byte 2 /* do_xenpmu_op         */  /* 40 */
> diff --git a/xen/drivers/passthrough/vtd/dmar.h 
> b/xen/drivers/passthrough/vtd/dmar.h
> index 729b603..697e5e5 100644
> --- 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
>  
>  int vtd_hw_check(void);
>  void disable_pmr(struct iommu *iommu);
> diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
> index 3e9be83..e84e489 100644
> --- 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.
>   */
>  

This is an unrelated hunk and should be dropped.

~Andrew

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