[PATCH] x86: wrap kexec feature with CONFIG_KEXEC

[PATCH] 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>
---
 xen/arch/x86/Makefile              |  4 ++--
 xen/arch/x86/apic.c                |  3 ++-
 xen/arch/x86/crash.c               |  2 ++
 xen/arch/x86/setup.c               | 14 ++++++++++++++
 xen/arch/x86/x86_64/compat/entry.S |  9 +++++++++
 xen/arch/x86/x86_64/entry.S        |  8 ++++++++
 xen/drivers/passthrough/vtd/dmar.h | 17 +++++++++++++++++
 xen/include/asm-x86/config.h       |  2 +-
 8 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5f24951..5725a8d 100644
--- 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
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 2c9ae4e..729adf5 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -307,6 +307,7 @@ void disable_local_APIC(void)
                ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD));
     }
 
+#ifdef CONFIG_KEXEC
     if ( kexecing && (current_local_apic_mode() != apic_boot_mode) )
     {
         uint64_t msr_content;
@@ -334,7 +335,7 @@ void disable_local_APIC(void)
             break;
         }
     }
-
+#endif
 }
 
 /*
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 888a214..55f803a 100644
--- 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
         __stop_this_cpu();
 
         this_cpu(crash_save_done) = 1;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ff34670..14ff15a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -479,6 +479,8 @@ static void __init parse_video_info(void)
     }
 }
 
+#ifdef CONFIG_KEXEC
+
 static void __init kexec_reserve_area(struct e820map *e820)
 {
     unsigned long kdump_start = kexec_crash_area.start;
@@ -505,6 +507,8 @@ static void __init kexec_reserve_area(struct e820map *e820)
     }
 }
 
+#endif
+
 static void noinline init_done(void)
 {
     system_state = SYS_STATE_active;
@@ -617,9 +621,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
     cmdline_parse(cmdline);
 
+#ifdef CONFIG_KEXEC
     /* Must be after command line argument parsing and before
      * allocing any xenheap structures wanted in lower memory. */
     kexec_early_calculations();
+#endif
 
     parse_video_info();
 
@@ -782,6 +788,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     /* Create a temporary copy of the E820 map. */
     memcpy(&boot_e820, &e820, sizeof(e820));
 
+#ifdef CONFIG_KEXEC
     /* Early kexec reservation (explicit static start address). */
     nr_pages = 0;
     for ( i = 0; i < e820.nr_map; i++ )
@@ -789,6 +796,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             nr_pages += e820.map[i].size >> PAGE_SHIFT;
     set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
     kexec_reserve_area(&boot_e820);
+#endif
 
     initial_images = mod;
     nr_initial_images = mbi->mods_count;
@@ -973,6 +981,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             }
         }
 
+#ifdef CONFIG_KEXEC
         /* Don't overlap with modules. */
         e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size),
                              mod, mbi->mods_count, -1);
@@ -981,6 +990,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             e = (e - kexec_crash_area.size) & PAGE_MASK;
             kexec_crash_area.start = e;
         }
+#endif
     }
 
     if ( modules_headroom && !mod->reserved )
@@ -997,8 +1007,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : __pa(&_start),
                      __pa(&_end));
 
+#ifdef CONFIG_KEXEC
     /* Late kexec reservation (dynamic start address). */
     kexec_reserve_area(&boot_e820);
+#endif
 
     setup_max_pdx(raw_max_page);
     if ( highmem_start )
@@ -1125,6 +1137,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                          PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
     }
 
+#ifdef CONFIG_KEXEC
     if ( kexec_crash_area.size )
     {
         unsigned long s = PFN_DOWN(kexec_crash_area.start);
@@ -1135,6 +1148,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
                              s, e - s, PAGE_HYPERVISOR);
     }
+#endif
 
     xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) &
                    ~((1UL << L2_PAGETABLE_SHIFT) - 1);
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
         .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
         .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.
  */
 
-- 
2.1.4

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

[Jan Beulich]

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

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

[Andrew Cooper]

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@gmail.com>

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

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

[Jan Beulich]

>>> On 27.08.15 at 17:22, <andrew.cooper3@citrix.com> wrote:
> On 27/08/15 15:47, Jonathan Creekmore wrote:
>> @@ -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.

It's quite the other way around: Other than now (where two
registers get clobbered), no registers would be clobbered
anymore. Of course that's not to say the argument count
tables shouldn't be left alone.

Jan

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

[Andrew Cooper]

On 27/08/15 16:34, Jan Beulich wrote:
>>>> On 27.08.15 at 17:22, <andrew.cooper3@citrix.com> wrote:
>> On 27/08/15 15:47, Jonathan Creekmore wrote:
>>> @@ -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.
> It's quite the other way around:

Ah yes - of course.  Sorry for the noise.

~Andrew

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

[David Vrabel]

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.

What's your use case for this?

I think you should consider providing empty stub functions for !KEXEC
instead of all the #ifdefs.

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

This is particular might be best done by making kexecing a static const
variable equal to 0 in the !KEXEC case.

> --- 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.
>   */

Stray whitespace change.

David

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

[Jan Beulich]

>>> On 27.08.15 at 17:27, <david.vrabel@citrix.com> wrote:
> On 27/08/15 15:47, Jonathan Creekmore wrote:
>> @@ -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
> 
> This is particular might be best done by making kexecing a static const
> variable equal to 0 in the !KEXEC case.

Or even a #define (unless there's some use somewhere preventing
that).

Jan

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

[Jonathan Creekmore]

> On Aug 27, 2015, at 10:27 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> 
> 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.
> 
> What's your use case for this?
> 

The use case is for a slimmed down version of the hypervisor that can be used as a security hypervisor, exposing as little extra functionality as possible. When looking for features to trim out to reduce the attack surface, I saw the flag for KEXEC and wanted to disable that, then ran into compile problems.

> I think you should consider providing empty stub functions for !KEXEC
> instead of all the #ifdefs.
> 

Good points, all, who mentioned the empty stub functions. I am revising it now to use stubs instead of all of the #ifdefs.

> 
>> --- 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.
>>  */
> 
> Stray whitespace change.

I guess I really need to just turn off that feature (stripping trailing whitespace) in my editor. So useful when writing new code, not as helpful when trying to modify existing code.

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

[Ian Campbell]

On Thu, 2015-08-27 at 10:44 -0500, Jonathan Creekmore wrote:
> > 
> > On Aug 27, 2015, at 10:27 AM, David Vrabel <david.vrabel@citrix.com> 
> > wrote:
> > 
> > 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.
> > 
> > What's your use case for this?
> > 
> 
> The use case is for a slimmed down version of the hypervisor that can be 
> used as a security hypervisor, exposing as little extra functionality as 
> possible. When looking for features to trim out to reduce the attack 
> surface, I saw the flag for KEXEC and wanted to disable that, then ran 
> into compile problems.

Can this not be achieved at runtime with XSM?

In general (i.e. not 100% consistently, I think) we have tended to avoid
making things user-facing compile time options. Many of the existing
CONFIG_* and HAVE_* are really about things which are arch dependent, or
require specific porting to each arch etc. I think the KEXEC flag is one of
those.

This keeps the test matrix more reasonable (unlike e.g. Linux's Kconfig)
and also helps us by ensuring that users are mostly running one of a small
number of possible configs.

I slightly fear that after Kexec you are going to want to strip out more
and more stuff...

Ian.

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

[Andrew Cooper]

On 01/09/15 11:36, Ian Campbell wrote:
> On Thu, 2015-08-27 at 10:44 -0500, Jonathan Creekmore wrote:
>>> On Aug 27, 2015, at 10:27 AM, David Vrabel <david.vrabel@citrix.com> 
>>> wrote:
>>>
>>> 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.
>>> What's your use case for this?
>>>
>> The use case is for a slimmed down version of the hypervisor that can be 
>> used as a security hypervisor, exposing as little extra functionality as 
>> possible. When looking for features to trim out to reduce the attack 
>> surface, I saw the flag for KEXEC and wanted to disable that, then ran 
>> into compile problems.
> Can this not be achieved at runtime with XSM?

Doing so would be contrary to the intent of reducing the attack surface.

>
> In general (i.e. not 100% consistently, I think) we have tended to avoid
> making things user-facing compile time options. Many of the existing
> CONFIG_* and HAVE_* are really about things which are arch dependent, or
> require specific porting to each arch etc. I think the KEXEC flag is one of
> those.
>
> This keeps the test matrix more reasonable (unlike e.g. Linux's Kconfig)
> and also helps us by ensuring that users are mostly running one of a small
> number of possible configs.
>
> I slightly fear that after Kexec you are going to want to strip out more
> and more stuff...

I for one welcome a Kconfig style approach.  We will never be in the
same order of magnitude of options as Linux, and it will help to
properly modularise the code.

~Andrew

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

[Jan Beulich]

>>> On 01.09.15 at 12:44, <andrew.cooper3@citrix.com> wrote:
> On 01/09/15 11:36, Ian Campbell wrote:
>> In general (i.e. not 100% consistently, I think) we have tended to avoid
>> making things user-facing compile time options. Many of the existing
>> CONFIG_* and HAVE_* are really about things which are arch dependent, or
>> require specific porting to each arch etc. I think the KEXEC flag is one of
>> those.
>>
>> This keeps the test matrix more reasonable (unlike e.g. Linux's Kconfig)
>> and also helps us by ensuring that users are mostly running one of a small
>> number of possible configs.
>>
>> I slightly fear that after Kexec you are going to want to strip out more
>> and more stuff...
> 
> I for one welcome a Kconfig style approach.  We will never be in the
> same order of magnitude of options as Linux, and it will help to
> properly modularise the code.

Indeed the idea was brought up a few times already, and I would
also welcome such a step (accepting the downside of the larger
test matrix). Not the least considering the "no shadow mode" and
"big memory" build options that got introduced not so long ago.

Jan

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

[Andrew Cooper]

On 01/09/15 11:54, Jan Beulich wrote:
>>>> On 01.09.15 at 12:44, <andrew.cooper3@citrix.com> wrote:
>> On 01/09/15 11:36, Ian Campbell wrote:
>>> In general (i.e. not 100% consistently, I think) we have tended to avoid
>>> making things user-facing compile time options. Many of the existing
>>> CONFIG_* and HAVE_* are really about things which are arch dependent, or
>>> require specific porting to each arch etc. I think the KEXEC flag is one of
>>> those.
>>>
>>> This keeps the test matrix more reasonable (unlike e.g. Linux's Kconfig)
>>> and also helps us by ensuring that users are mostly running one of a small
>>> number of possible configs.
>>>
>>> I slightly fear that after Kexec you are going to want to strip out more
>>> and more stuff...
>> I for one welcome a Kconfig style approach.  We will never be in the
>> same order of magnitude of options as Linux, and it will help to
>> properly modularise the code.
> Indeed the idea was brought up a few times already, and I would
> also welcome such a step (accepting the downside of the larger
> test matrix). Not the least considering the "no shadow mode" and
> "big memory" build options that got introduced not so long ago.

>From a large attack surface point of view, support for 32bit ABI on
64bit Xen is a welcome candidate for more controlled environments.

>From a code volume point of view, having things like PV or support
configurable would be interesting.

~Andrew

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

[Jonathan Creekmore]

Andrew Cooper <andrew.cooper3@citrix.com> writes:

> On 01/09/15 11:54, Jan Beulich wrote: 
>>>>> On 01.09.15 at 12:44, <andrew.cooper3@citrix.com> wrote: 
>>> On 01/09/15 11:36, Ian Campbell wrote: 
>>>> In general (i.e. not 100% consistently, I think) we have 
>>>> tended to avoid making things user-facing compile time 
>>>> options. Many of the existing CONFIG_* and HAVE_* are really 
>>>> about things which are arch dependent, or require specific 
>>>> porting to each arch etc. I think the KEXEC flag is one of 
>>>> those. 
>>>> 
>>>> This keeps the test matrix more reasonable (unlike 
>>>> e.g. Linux's Kconfig) and also helps us by ensuring that 
>>>> users are mostly running one of a small number of possible 
>>>> configs. 
>>>> 
>>>> I slightly fear that after Kexec you are going to want to 
>>>> strip out more and more stuff... 
>>> I for one welcome a Kconfig style approach.  We will never be 
>>> in the same order of magnitude of options as Linux, and it 
>>> will help to properly modularise the code. 
>> Indeed the idea was brought up a few times already, and I would 
>> also welcome such a step (accepting the downside of the larger 
>> test matrix). Not the least considering the "no shadow mode" 
>> and "big memory" build options that got introduced not so long 
>> ago. 
> 
> From a large attack surface point of view, support for 32bit ABI 
> on 64bit Xen is a welcome candidate for more controlled 
> environments. 
> 
> From a code volume point of view, having things like PV or 
> support configurable would be interesting. 

I am not interested in unnecessarily stripping out more and more 
code. However, I do want to reduce the number of features and 
backwards-compatibility code-paths that are compiled into my 
build. Areas like the 32-bit ABI on 64-bit Xen like Andrew 
mentioned or the legacy Xenlinux ABI are paths that I am 
interested in pruning. One area I have a preliminary patch for is 
selectively compiling individual scheduling algorithms in, giving 
me the option to compile out the experimental algorithms from my 
build.

Obviously, I can carry my own patchqueues for these types of changes, but
I would prefer to upstream them where possible.

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

[Jan Beulich]

>>> On 01.09.15 at 16:29, <jonathan.creekmore@gmail.com> wrote:
> I am not interested in unnecessarily stripping out more and more 
> code. However, I do want to reduce the number of features and 
> backwards-compatibility code-paths that are compiled into my 
> build. Areas like the 32-bit ABI on 64-bit Xen like Andrew 
> mentioned or the legacy Xenlinux ABI are paths that I am 
> interested in pruning. One area I have a preliminary patch for is 
> selectively compiling individual scheduling algorithms in, giving 
> me the option to compile out the experimental algorithms from my 
> build.
> 
> Obviously, I can carry my own patchqueues for these types of changes, but
> I would prefer to upstream them where possible.

Which is both appreciated and understandable. I suppose you agree
though that if you were to follow the model used for the kexec part,
things would quickly become unwieldy. Hence I would strongly
suggest considering to introduce Linux'es or a Linux-like configure
mechanism before continuing to make code conditionally compilable.

Jan

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

[Jonathan Creekmore]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 01.09.15 at 16:29, <jonathan.creekmore@gmail.com> wrote: 
> 
> Which is both appreciated and understandable. I suppose you 
> agree though that if you were to follow the model used for the 
> kexec part, things would quickly become unwieldy. Hence I would 
> strongly suggest considering to introduce Linux'es or a 
> Linux-like configure mechanism before continuing to make code 
> conditionally compilable. 

Oh, I definitely agree.

What exactly are you looking for? Are you looking for just a KConfig
type of mechanism, or a full KBuild implementation. In a previous life,
I ported both to an embedded RTOS build. I think it might be simpler to
just start with a KConfig mechanism, though. If you want, I can take a
swing at starting down that path.

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

[Jan Beulich]

>>> On 01.09.15 at 19:55, <jonathan.creekmore@gmail.com> wrote:
> "Jan Beulich" <JBeulich@suse.com> writes:
> 
>>>>> On 01.09.15 at 16:29, <jonathan.creekmore@gmail.com> wrote: 
>> 
>> Which is both appreciated and understandable. I suppose you 
>> agree though that if you were to follow the model used for the 
>> kexec part, things would quickly become unwieldy. Hence I would 
>> strongly suggest considering to introduce Linux'es or a 
>> Linux-like configure mechanism before continuing to make code 
>> conditionally compilable. 
> 
> Oh, I definitely agree.
> 
> What exactly are you looking for? Are you looking for just a KConfig
> type of mechanism, or a full KBuild implementation. In a previous life,
> I ported both to an embedded RTOS build. I think it might be simpler to
> just start with a KConfig mechanism, though. If you want, I can take a
> swing at starting down that path.

Just Kconfig (I don't see any connection here to the need of a
clone of the Kbuild mechanism). But give it a little time for other
maintainers to voice an opinion (perhaps worthwhile spelling out
the intention in a separate mail thread).

Jan

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

[Andrew Cooper]

On 02/09/15 07:53, Jan Beulich wrote:
>>>> On 01.09.15 at 19:55, <jonathan.creekmore@gmail.com> wrote:
>> "Jan Beulich" <JBeulich@suse.com> writes:
>>
>>>>>> On 01.09.15 at 16:29, <jonathan.creekmore@gmail.com> wrote: 
>>> Which is both appreciated and understandable. I suppose you 
>>> agree though that if you were to follow the model used for the 
>>> kexec part, things would quickly become unwieldy. Hence I would 
>>> strongly suggest considering to introduce Linux'es or a 
>>> Linux-like configure mechanism before continuing to make code 
>>> conditionally compilable. 
>> Oh, I definitely agree.
>>
>> What exactly are you looking for? Are you looking for just a KConfig
>> type of mechanism, or a full KBuild implementation. In a previous life,
>> I ported both to an embedded RTOS build. I think it might be simpler to
>> just start with a KConfig mechanism, though. If you want, I can take a
>> swing at starting down that path.
> Just Kconfig (I don't see any connection here to the need of a
> clone of the Kbuild mechanism). But give it a little time for other
> maintainers to voice an opinion (perhaps worthwhile spelling out
> the intention in a separate mail thread).

For something like this, just Kconfig.  That will be a good improvement
on its own.

There are orthogonal reasons for looking to improve the build system
itself (In particular, being able to do out-of-tree builds), but lets
not merge the issues together.

~Andrew