[PATCH RFC 0/9] x86: Improvements to trap handling

[PATCH RFC 0/9] x86: Improvements to trap handling

[Andrew Cooper]

This is a large change to trap handling.  Its underlying purpose is to avoid
the current situation where ignore_int() in the .init section remains patched
into each IDT in the reserved exception vectors.

As a side effect, Xen gains full bugframe and exception_table support from the
beginning of __start_xen.

This is far from comprehensivly tested, but has been tested with bugframes and
extable redirects from right after setting up the console (so printing works),
along with the panic() and reboot paths.

I notice that Feng Wu has submitted a patch moving the irq-stub generation,
and it is completely by chance that I have functionally similar patch in this
series.

Andrew Cooper (9):
  x86/traps: Names for system descriptor types
  x86/traps: Make panic and reboot paths safe during early boot
  x86/traps: Make the main trap handlers safe for use early during Xen boot
  x86/misc: Early cleanup
  x86/traps: Functional prep work
  x86/boot: Install trap handlers much earlier on boot
  x86/boot: Drop pre-C IDT patching
  x86/irqs: Move interrupt-stub generation out of C
  x86/misc: Post cleanup

 xen/arch/x86/boot/x86_64.S      |   56 +-----------------
 xen/arch/x86/cpu/common.c       |   70 +++++++++++++++++-----
 xen/arch/x86/cpu/mcheck/mce.c   |    5 +-
 xen/arch/x86/crash.c            |    2 +-
 xen/arch/x86/i8259.c            |   69 +---------------------
 xen/arch/x86/mm.c               |   10 ++--
 xen/arch/x86/setup.c            |   42 ++++++-------
 xen/arch/x86/shutdown.c         |   42 +++++++------
 xen/arch/x86/smpboot.c          |   21 ++-----
 xen/arch/x86/traps.c            |  124 ++++++++++++++++++++++++---------------
 xen/arch/x86/x86_64/Makefile    |    1 +
 xen/arch/x86/x86_64/entry.S     |   22 +++----
 xen/arch/x86/x86_64/irqgen.S    |   72 +++++++++++++++++++++++
 xen/arch/x86/x86_64/traps.c     |   31 +---------
 xen/common/symbols.c            |    2 +-
 xen/drivers/char/console.c      |    5 --
 xen/include/asm-x86/asm_defns.h |    5 --
 xen/include/asm-x86/config.h    |    1 +
 xen/include/asm-x86/desc.h      |    9 ++-
 xen/include/asm-x86/ldt.h       |    2 +-
 xen/include/asm-x86/processor.h |    6 +-
 xen/include/asm-x86/setup.h     |    1 -
 xen/include/asm-x86/system.h    |    1 +
 xen/include/xen/kernel.h        |    1 +
 xen/include/xen/sched.h         |    1 +
 25 files changed, 295 insertions(+), 306 deletions(-)
 create mode 100644 xen/arch/x86/x86_64/irqgen.S

[PATCH RFC 1/9] x86/traps: Names for system descriptor types

[Andrew Cooper]

Avoids some particularly obscure magic numbers

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/crash.c        |    2 +-
 xen/arch/x86/traps.c        |    8 ++++----
 xen/arch/x86/x86_64/traps.c |    4 ++--
 xen/include/asm-x86/desc.h  |    8 ++++++++
 xen/include/asm-x86/ldt.h   |    2 +-
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index aed3b3e..97adf02 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -149,7 +149,7 @@ static void nmi_shootdown_cpus(void)
              * This update is safe from a security point of view, as this pcpu 
              * is never going to try to sysret back to a PV vcpu.
              */
-            _set_gate_lower(&idt_tables[i][TRAP_nmi], 14, 0, &trap_nop);
+            _set_gate_lower(&idt_tables[i][TRAP_nmi], DESC_TYPE_irq_gate, 0, &trap_nop);
             set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE);
         }
         else
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 40366f1..1de70af 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3431,8 +3431,8 @@ static void __set_intr_gate(unsigned int n, uint32_t dpl, void *addr)
     /* Keep secondary tables in sync with IRQ updates. */
     for ( i = 1; i < nr_cpu_ids; i++ )
         if ( idt_tables[i] != NULL )
-            _set_gate(&idt_tables[i][n], 14, dpl, addr);
-    _set_gate(&idt_table[n], 14, dpl, addr);
+            _set_gate(&idt_tables[i][n], DESC_TYPE_irq_gate, dpl, addr);
+    _set_gate(&idt_table[n], DESC_TYPE_irq_gate, dpl, addr);
 }
 
 static void set_swint_gate(unsigned int n, void *addr)
@@ -3457,12 +3457,12 @@ void load_TR(void)
         this_cpu(gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
         offsetof(struct tss_struct, __cacheline_filler) - 1,
-        9);
+        DESC_TYPE_tss_avail);
     _set_tssldt_desc(
         this_cpu(compat_gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
         offsetof(struct tss_struct, __cacheline_filler) - 1,
-        11);
+        DESC_TYPE_tss_busy);
 
     /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
     asm volatile (
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 3a48478..cdaf1e6 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -394,10 +394,10 @@ void __devinit subarch_percpu_traps_init(void)
          * The 32-on-64 hypercall entry vector is only accessible from ring 1.
          * Also note that this is a trap gate, not an interrupt gate.
          */
-        _set_gate(idt_table+HYPERCALL_VECTOR, 15, 1, &compat_hypercall);
+        _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
 
         /* Fast trap for int80 (faster than taking the #GP-fixup path). */
-        _set_gate(idt_table+0x80, 15, 3, &int80_direct_trap);
+        _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
     }
 
     stack_bottom = (char *)get_stack_bottom();
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 4edb834..6b2094f 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -98,6 +98,14 @@
 
 #ifndef __ASSEMBLY__
 
+/* System Descriptor types in the GDT */
+#define DESC_TYPE_ldt       (2)
+#define DESC_TYPE_tss_avail (9)
+#define DESC_TYPE_tss_busy  (11)
+#define DESC_TYPE_call_gate (12)
+#define DESC_TYPE_irq_gate  (14)
+#define DESC_TYPE_trap_gate (15)
+
 struct desc_struct {
     u32 a, b;
 };
diff --git a/xen/include/asm-x86/ldt.h b/xen/include/asm-x86/ldt.h
index b6f7beb..4ba6d32 100644
--- a/xen/include/asm-x86/ldt.h
+++ b/xen/include/asm-x86/ldt.h
@@ -18,7 +18,7 @@ static inline void load_LDT(struct vcpu *v)
         desc = (!is_pv_32on64_vcpu(v)
                 ? this_cpu(gdt_table) : this_cpu(compat_gdt_table))
                + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
-        _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, 2);
+        _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, DESC_TYPE_ldt);
         __asm__ __volatile__ ( "lldt %%ax" : : "a" (LDT_ENTRY << 3) );
     }
 }
-- 
1.7.10.4

Re: [PATCH RFC 1/9] x86/traps: Names for system descriptor types

[Andrew Cooper]

On 15/05/14 10:48, Andrew Cooper wrote:
> Avoids some particularly obscure magic numbers
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---

Something I missed from the --- section.

I was toying with the naming scheme SYS_DESC_xxx instead as perhaps
being slightly more informative in context.

~Andrew

>  xen/arch/x86/crash.c        |    2 +-
>  xen/arch/x86/traps.c        |    8 ++++----
>  xen/arch/x86/x86_64/traps.c |    4 ++--
>  xen/include/asm-x86/desc.h  |    8 ++++++++
>  xen/include/asm-x86/ldt.h   |    2 +-
>  5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
> index aed3b3e..97adf02 100644
> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -149,7 +149,7 @@ static void nmi_shootdown_cpus(void)
>               * This update is safe from a security point of view, as this pcpu 
>               * is never going to try to sysret back to a PV vcpu.
>               */
> -            _set_gate_lower(&idt_tables[i][TRAP_nmi], 14, 0, &trap_nop);
> +            _set_gate_lower(&idt_tables[i][TRAP_nmi], DESC_TYPE_irq_gate, 0, &trap_nop);
>              set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE);
>          }
>          else
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 40366f1..1de70af 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3431,8 +3431,8 @@ static void __set_intr_gate(unsigned int n, uint32_t dpl, void *addr)
>      /* Keep secondary tables in sync with IRQ updates. */
>      for ( i = 1; i < nr_cpu_ids; i++ )
>          if ( idt_tables[i] != NULL )
> -            _set_gate(&idt_tables[i][n], 14, dpl, addr);
> -    _set_gate(&idt_table[n], 14, dpl, addr);
> +            _set_gate(&idt_tables[i][n], DESC_TYPE_irq_gate, dpl, addr);
> +    _set_gate(&idt_table[n], DESC_TYPE_irq_gate, dpl, addr);
>  }
>  
>  static void set_swint_gate(unsigned int n, void *addr)
> @@ -3457,12 +3457,12 @@ void load_TR(void)
>          this_cpu(gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
>          (unsigned long)tss,
>          offsetof(struct tss_struct, __cacheline_filler) - 1,
> -        9);
> +        DESC_TYPE_tss_avail);
>      _set_tssldt_desc(
>          this_cpu(compat_gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
>          (unsigned long)tss,
>          offsetof(struct tss_struct, __cacheline_filler) - 1,
> -        11);
> +        DESC_TYPE_tss_busy);
>  
>      /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
>      asm volatile (
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 3a48478..cdaf1e6 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -394,10 +394,10 @@ void __devinit subarch_percpu_traps_init(void)
>           * The 32-on-64 hypercall entry vector is only accessible from ring 1.
>           * Also note that this is a trap gate, not an interrupt gate.
>           */
> -        _set_gate(idt_table+HYPERCALL_VECTOR, 15, 1, &compat_hypercall);
> +        _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
>  
>          /* Fast trap for int80 (faster than taking the #GP-fixup path). */
> -        _set_gate(idt_table+0x80, 15, 3, &int80_direct_trap);
> +        _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
>      }
>  
>      stack_bottom = (char *)get_stack_bottom();
> diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
> index 4edb834..6b2094f 100644
> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -98,6 +98,14 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +/* System Descriptor types in the GDT */
> +#define DESC_TYPE_ldt       (2)
> +#define DESC_TYPE_tss_avail (9)
> +#define DESC_TYPE_tss_busy  (11)
> +#define DESC_TYPE_call_gate (12)
> +#define DESC_TYPE_irq_gate  (14)
> +#define DESC_TYPE_trap_gate (15)
> +
>  struct desc_struct {
>      u32 a, b;
>  };
> diff --git a/xen/include/asm-x86/ldt.h b/xen/include/asm-x86/ldt.h
> index b6f7beb..4ba6d32 100644
> --- a/xen/include/asm-x86/ldt.h
> +++ b/xen/include/asm-x86/ldt.h
> @@ -18,7 +18,7 @@ static inline void load_LDT(struct vcpu *v)
>          desc = (!is_pv_32on64_vcpu(v)
>                  ? this_cpu(gdt_table) : this_cpu(compat_gdt_table))
>                 + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
> -        _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, 2);
> +        _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, DESC_TYPE_ldt);
>          __asm__ __volatile__ ( "lldt %%ax" : : "a" (LDT_ENTRY << 3) );
>      }
>  }

Re: [PATCH RFC 1/9] x86/traps: Names for system descriptor types

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> Avoids some particularly obscure magic numbers

Very desirable. A small comment below.

> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -98,6 +98,14 @@
>  
>  #ifndef __ASSEMBLY__
>  
> +/* System Descriptor types in the GDT */
> +#define DESC_TYPE_ldt       (2)
> +#define DESC_TYPE_tss_avail (9)
> +#define DESC_TYPE_tss_busy  (11)
> +#define DESC_TYPE_call_gate (12)
> +#define DESC_TYPE_irq_gate  (14)
> +#define DESC_TYPE_trap_gate (15)

Either you say GDT/IDT in the comment, or you separate the GDT
ones from the IDT ones.

Plus the comment is missing a stop.

And finally I don't think the parentheses are warranted here.

Jan

Re: [PATCH RFC 1/9] x86/traps: Names for system descriptor types

[Andrew Cooper]

On 15/05/14 11:08, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> Avoids some particularly obscure magic numbers
> Very desirable. A small comment below.
>
>> --- a/xen/include/asm-x86/desc.h
>> +++ b/xen/include/asm-x86/desc.h
>> @@ -98,6 +98,14 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  
>> +/* System Descriptor types in the GDT */
>> +#define DESC_TYPE_ldt       (2)
>> +#define DESC_TYPE_tss_avail (9)
>> +#define DESC_TYPE_tss_busy  (11)
>> +#define DESC_TYPE_call_gate (12)
>> +#define DESC_TYPE_irq_gate  (14)
>> +#define DESC_TYPE_trap_gate (15)
> Either you say GDT/IDT in the comment, or you separate the GDT
> ones from the IDT ones.

Will say GDT/IDT.  These type numbers generic across both tables.

>
> Plus the comment is missing a stop.
>
> And finally I don't think the parentheses are warranted here.
>
> Jan
>

Ok.  Any views on SYS_DESC_xxx ?  I think I prefer it slightly, and
would allow removal of the _gate suffix, which would not be useful in
context.

~Andrew

Re: [PATCH RFC 1/9] x86/traps: Names for system descriptor types

[Jan Beulich]

>>> On 15.05.14 at 12:26, <andrew.cooper3@citrix.com> wrote:
> Ok.  Any views on SYS_DESC_xxx ?  I think I prefer it slightly, and
> would allow removal of the _gate suffix, which would not be useful in
> context.

I actually don't think SYS_DESC_ vs DESC_TYPE_ matters that much;
perhaps the former is slightly more precise. I wouldn't, however, want
the _gate suffixes be dropped, irrespective that there's no task gate
in x86-64 that could be confused with the TSS one - they are gate
descriptors, and hence they should be named that way.

Jan

[PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Andrew Cooper]

Reverse two conditions in show_registers().  For an early crash, it is not
safe to dereference current for its HVM status before knowing that it is a
guest vcpu.

Introduce SYS_STATE_smp_boot to help distinguish whether APs need to be
considered during boot.  This involves tweaking quite a few system_state
checks so their semantics remain identical.

Make use of SYS_STATE_smp_boot to help machine_{halt,restart}() know if/when
it is safe to enable interrupts and access the local apic to send IPIs.
Before system_state == SYS_STATE_smp_boot, we can be certain that only the BSP
is running.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm.c           |   10 +++++-----
 xen/arch/x86/setup.c        |    2 ++
 xen/arch/x86/shutdown.c     |   38 +++++++++++++++++++++++---------------
 xen/arch/x86/x86_64/traps.c |    2 +-
 xen/common/symbols.c        |    2 +-
 xen/include/xen/kernel.h    |    1 +
 6 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1a8a5e0..bd67cde 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5246,7 +5246,7 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
     pl4e = &idle_pg_table[l4_table_offset(v)];
     if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
     {
-        bool_t locking = system_state > SYS_STATE_boot;
+        bool_t locking = system_state >= SYS_STATE_active;
         l3_pgentry_t *pl3e = alloc_xen_pagetable();
 
         if ( !pl3e )
@@ -5278,7 +5278,7 @@ static l2_pgentry_t *virt_to_xen_l2e(unsigned long v)
 
     if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
     {
-        bool_t locking = system_state > SYS_STATE_boot;
+        bool_t locking = system_state >= SYS_STATE_active;
         l2_pgentry_t *pl2e = alloc_xen_pagetable();
 
         if ( !pl2e )
@@ -5311,7 +5311,7 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
 
     if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
     {
-        bool_t locking = system_state > SYS_STATE_boot;
+        bool_t locking = system_state >= SYS_STATE_active;
         l1_pgentry_t *pl1e = alloc_xen_pagetable();
 
         if ( !pl1e )
@@ -5353,7 +5353,7 @@ int map_pages_to_xen(
     unsigned long nr_mfns,
     unsigned int flags)
 {
-    bool_t locking = system_state > SYS_STATE_boot;
+    bool_t locking = system_state >= SYS_STATE_active;
     l2_pgentry_t *pl2e, ol2e;
     l1_pgentry_t *pl1e, ol1e;
     unsigned int  i;
@@ -5670,7 +5670,7 @@ int map_pages_to_xen(
 
 void destroy_xen_mappings(unsigned long s, unsigned long e)
 {
-    bool_t locking = system_state > SYS_STATE_boot;
+    bool_t locking = system_state >= SYS_STATE_active;
     l2_pgentry_t *pl2e;
     l1_pgentry_t *pl1e;
     unsigned int  i;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d8598a3..a864b9f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1319,6 +1319,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     console_init_postirq();
 
+    system_state = SYS_STATE_smp_boot;
+
     do_presmp_initcalls();
 
     for_each_present_cpu ( i )
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 827515d..44bcd7f 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -96,8 +96,13 @@ void machine_halt(void)
 {
     watchdog_disable();
     console_start_sync();
-    local_irq_enable();
-    smp_call_function(__machine_halt, NULL, 0);
+
+    if ( system_state >= SYS_STATE_smp_boot )
+    {
+        local_irq_enable();
+        smp_call_function(__machine_halt, NULL, 0);
+    }
+
     __machine_halt(NULL);
 }
 
@@ -466,18 +471,6 @@ void machine_restart(unsigned int delay_millisecs)
     console_start_sync();
     spin_debug_disable();
 
-    local_irq_enable();
-
-    /* Ensure we are the boot CPU. */
-    if ( get_apic_id() != boot_cpu_physical_apicid )
-    {
-        /* Send IPI to the boot CPU (logical cpu 0). */
-        on_selected_cpus(cpumask_of(0), __machine_restart,
-                         &delay_millisecs, 0);
-        for ( ; ; )
-            halt();
-    }
-
     /*
      * We may be called from an interrupt context, and various functions we
      * may need to call (alloc_domheap_pages, map_domain_page, ...) assert that
@@ -485,7 +478,22 @@ void machine_restart(unsigned int delay_millisecs)
      */
     local_irq_count(0) = 0;
 
-    smp_send_stop();
+    if ( system_state >= SYS_STATE_smp_boot )
+    {
+        local_irq_enable();
+
+        /* Ensure we are the boot CPU. */
+        if ( get_apic_id() != boot_cpu_physical_apicid )
+        {
+            /* Send IPI to the boot CPU (logical cpu 0). */
+            on_selected_cpus(cpumask_of(0), __machine_restart,
+                             &delay_millisecs, 0);
+            for ( ; ; )
+                halt();
+        }
+
+        smp_send_stop();
+    }
 
     mdelay(delay_millisecs);
 
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index cdaf1e6..e1c7b3b 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -86,7 +86,7 @@ void show_registers(struct cpu_user_regs *regs)
     enum context context;
     struct vcpu *v = current;
 
-    if ( has_hvm_container_vcpu(v) && guest_mode(regs) )
+    if ( guest_mode(regs) && has_hvm_container_vcpu(v) )
     {
         struct segment_register sreg;
         context = CTXT_hvm_guest;
diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index 45941e1..bc2fde6 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -96,7 +96,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
 bool_t is_active_kernel_text(unsigned long addr)
 {
     return (is_kernel_text(addr) ||
-            (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
+            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
 }
 
 const char *symbols_lookup(unsigned long addr,
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 54e88dd..2c6d448 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -92,6 +92,7 @@ extern char _sinittext[], _einittext[];
 extern enum system_state {
     SYS_STATE_early_boot,
     SYS_STATE_boot,
+    SYS_STATE_smp_boot,
     SYS_STATE_active,
     SYS_STATE_suspend,
     SYS_STATE_resume
-- 
1.7.10.4

Re: [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> Make use of SYS_STATE_smp_boot to help machine_{halt,restart}() know if/when
> it is safe to enable interrupts and access the local apic to send IPIs.
> Before system_state == SYS_STATE_smp_boot, we can be certain that only the BSP
> is running.

Hmm, tying SMP boot and IRQ enabling together seems a little
problematic, even if on x86 the former happens soon after the latter
right now. Perhaps these ought to be distinct states?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5246,7 +5246,7 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>      pl4e = &idle_pg_table[l4_table_offset(v)];
>      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>      {
> -        bool_t locking = system_state > SYS_STATE_boot;
> +        bool_t locking = system_state >= SYS_STATE_active;

Did you just mechanically adjust occurrences like this one, to (as the
description says) have their semantics remain identical? I ask because
it would seem to me that here you'd likely better change the semantics
by keeping the code unchanged.

> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -96,7 +96,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
>  bool_t is_active_kernel_text(unsigned long addr)
>  {
>      return (is_kernel_text(addr) ||
> -            (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
> +            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));

And here, contrary to the description, you actually do a semantic
(but correct!) change.

Jan

Re: [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Andrew Cooper]

On 15/05/14 11:19, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> Make use of SYS_STATE_smp_boot to help machine_{halt,restart}() know if/when
>> it is safe to enable interrupts and access the local apic to send IPIs.
>> Before system_state == SYS_STATE_smp_boot, we can be certain that only the BSP
>> is running.
> Hmm, tying SMP boot and IRQ enabling together seems a little
> problematic, even if on x86 the former happens soon after the latter
> right now. Perhaps these ought to be distinct states?

Which states would you suggest then?

The key problems I encountered were pagefaults before the LAPIC has its
mmio region mapped, and x2apic_enabled isn't correct until apic_init(),
risking a pagefault for the MMIO region and protection fault from the MSRs.

>
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -5246,7 +5246,7 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>>      pl4e = &idle_pg_table[l4_table_offset(v)];
>>      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>>      {
>> -        bool_t locking = system_state > SYS_STATE_boot;
>> +        bool_t locking = system_state >= SYS_STATE_active;
> Did you just mechanically adjust occurrences like this one, to (as the
> description says) have their semantics remain identical? I ask because
> it would seem to me that here you'd likely better change the semantics
> by keeping the code unchanged.
>
>> --- a/xen/common/symbols.c
>> +++ b/xen/common/symbols.c
>> @@ -96,7 +96,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
>>  bool_t is_active_kernel_text(unsigned long addr)
>>  {
>>      return (is_kernel_text(addr) ||
>> -            (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
>> +            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
> And here, contrary to the description, you actually do a semantic
> (but correct!) change.
>
> Jan
>

I attempted to change each of them such that SYS_STATE_boot and
SYS_STATE_smp_boot acted the same, and that further insertions of new
states wouldn't require changes quite this wide.

~Andrew

Re: [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Jan Beulich]

>>> On 15.05.14 at 12:53, <andrew.cooper3@citrix.com> wrote:
> On 15/05/14 11:19, Jan Beulich wrote:
>>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>>> Make use of SYS_STATE_smp_boot to help machine_{halt,restart}() know if/when
>>> it is safe to enable interrupts and access the local apic to send IPIs.
>>> Before system_state == SYS_STATE_smp_boot, we can be certain that only the 
> BSP
>>> is running.
>> Hmm, tying SMP boot and IRQ enabling together seems a little
>> problematic, even if on x86 the former happens soon after the latter
>> right now. Perhaps these ought to be distinct states?
> 
> Which states would you suggest then?

Perhaps "IRQs enabled" and "SMP boot"?

>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5246,7 +5246,7 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>>>      pl4e = &idle_pg_table[l4_table_offset(v)];
>>>      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>>>      {
>>> -        bool_t locking = system_state > SYS_STATE_boot;
>>> +        bool_t locking = system_state >= SYS_STATE_active;
>> Did you just mechanically adjust occurrences like this one, to (as the
>> description says) have their semantics remain identical? I ask because
>> it would seem to me that here you'd likely better change the semantics
>> by keeping the code unchanged.
>>
>>> --- a/xen/common/symbols.c
>>> +++ b/xen/common/symbols.c
>>> @@ -96,7 +96,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
>>>  bool_t is_active_kernel_text(unsigned long addr)
>>>  {
>>>      return (is_kernel_text(addr) ||
>>> -            (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
>>> +            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));
>> And here, contrary to the description, you actually do a semantic
>> (but correct!) change.
> 
> I attempted to change each of them such that SYS_STATE_boot and
> SYS_STATE_smp_boot acted the same, and that further insertions of new
> states wouldn't require changes quite this wide.

For the former, I think if the locking is okay at that point (which I
think it is) you should drop the change and just mention the semantic
change. For the latter, all I was after is that you make the patch
description match it implementation.

Jan

Re: [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Andrew Cooper]

On 15/05/14 13:12, Jan Beulich wrote:
>> On 15/05/14 11:19, Jan Beulich wrote:
>>> Hmm, tying SMP boot and IRQ enabling together seems a little
>>> problematic, even if on x86 the former happens soon after the latter
>>> right now. Perhaps these ought to be distinct states?
>> Which states would you suggest then?
> Perhaps "IRQs enabled" and "SMP boot"?

I don’t see how these would help in this case.

For both machine_{halt,reboot}(), the local_irq_enable() is purely to
prevent tripping the assertion in
on_selected_cpus()/smp_call_function(), which is an smp thing rather
than an irq thing.

I think SYS_STATE_smp_boot is sufficient here. Perhaps I could reword
the commit message to put less emphasis on "enabling irqs" and more on
the smp side?

~Andrew

Re: [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

[Jan Beulich]

>>> On 15.05.14 at 17:46, <andrew.cooper3@citrix.com> wrote:
> On 15/05/14 13:12, Jan Beulich wrote:
>>> On 15/05/14 11:19, Jan Beulich wrote:
>>>> Hmm, tying SMP boot and IRQ enabling together seems a little
>>>> problematic, even if on x86 the former happens soon after the latter
>>>> right now. Perhaps these ought to be distinct states?
>>> Which states would you suggest then?
>> Perhaps "IRQs enabled" and "SMP boot"?
> 
> I don’t see how these would help in this case.

I didn't say it would make a difference here, all I said was that it's
not very consistent.

> For both machine_{halt,reboot}(), the local_irq_enable() is purely to
> prevent tripping the assertion in
> on_selected_cpus()/smp_call_function(), which is an smp thing rather
> than an irq thing.
> 
> I think SYS_STATE_smp_boot is sufficient here. Perhaps I could reword
> the commit message to put less emphasis on "enabling irqs" and more on
> the smp side?

That might help.

Jan

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

[PATCH RFC 3/9] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Andrew Cooper]

Most of this patch is an analysis of the safety of the trap handlers.

Traps 0, 4, 5, 9-12, 16, 17 and 19 all end up in do_trap().  do_trap() is
mostly safe, performing an exception table search and possibly panic()s.

There is one complication with traps 16 and 19 which will see about calling
the fpu_exception_callback.  This involves following current which is not
valid early on boot.  The has_hvm_container_vcpu(curr) check is preceeded with
a system_state check, so in the exceedingly unlikely case that Xen takes an
x87/SIMP trap while booting, it will panic() instead of following a bogus
current vcpu.

Traps 1, 3, 6-8, 13 and 15 are completely safe with respect to running during
early boot.  They all have well formed and obvious differences between faults
in Xen and faults in guests, with the Xen faults doing little more than
exception table walks or panic()s.

Trap 2 is a complicted codepath, but appears safe.  For the possible injection
of NMIs into dom0 there is a NULL domian pointer check.  The possible softirq
raised for PCI SERR will be devered until we start the idle vcpu, but is safe.

Trap 14 is very complicated.  The code is certainly unsafe for boot as
fixup_page_fault() will dereference current to find the running domain.  There
exists an explicit do_early_page_fault() handler which shall continue to be
used.

Trap 18 has a default handler before the MCE infrastructure is set up, which
has always been unsafe and liable to deadlock itself with the console lock.
As it is expected never to trigger, and if it did we would be in serious
problems, the simple printk() is replaced with a fatal error path.

Trap 20 (Virtualisation Exception) is currently not implemented.  It is fatal
one way or another, and will become more explicitly so with later changes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/cpu/mcheck/mce.c |    5 +++--
 xen/arch/x86/traps.c          |    3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index a81952c..5488411 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -72,8 +72,9 @@ custom_param("mce_verbosity", mce_set_verbosity);
 /* Handle unconfigured int18 (should never happen) */
 static void unexpected_machine_check(struct cpu_user_regs *regs, long error_code)
 {
-    printk(XENLOG_ERR "CPU#%d: Unexpected int18 (Machine Check).\n",
-           smp_processor_id());
+    console_force_unlock();
+    printk("Unexpected Machine Check Exception");
+    fatal_trap(TRAP_machine_check, regs);
 }
 
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1de70af..65e4609 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -561,7 +561,8 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code)
     }
 
     if ( ((trapnr == TRAP_copro_error) || (trapnr == TRAP_simd_error)) &&
-         has_hvm_container_vcpu(curr) && curr->arch.hvm_vcpu.fpu_exception_callback )
+         system_state >= SYS_STATE_active && has_hvm_container_vcpu(curr) &&
+         curr->arch.hvm_vcpu.fpu_exception_callback )
     {
         curr->arch.hvm_vcpu.fpu_exception_callback(
             curr->arch.hvm_vcpu.fpu_exception_callback_arg, regs);
-- 
1.7.10.4

Re: [PATCH RFC 3/9] x86/traps: Make the main trap handlers safe for use early during Xen boot

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> Most of this patch is an analysis of the safety of the trap handlers.
> 
> Traps 0, 4, 5, 9-12, 16, 17 and 19 all end up in do_trap().  do_trap() is
> mostly safe, performing an exception table search and possibly panic()s.
> 
> There is one complication with traps 16 and 19 which will see about calling
> the fpu_exception_callback.  This involves following current which is not
> valid early on boot.  The has_hvm_container_vcpu(curr) check is preceeded 
> with
> a system_state check, so in the exceedingly unlikely case that Xen takes an
> x87/SIMP trap while booting, it will panic() instead of following a bogus
> current vcpu.
> 
> Traps 1, 3, 6-8, 13 and 15 are completely safe with respect to running during
> early boot.  They all have well formed and obvious differences between 
> faults
> in Xen and faults in guests, with the Xen faults doing little more than
> exception table walks or panic()s.
> 
> Trap 2 is a complicted codepath, but appears safe.  For the possible 
> injection
> of NMIs into dom0 there is a NULL domian pointer check.  The possible 
> softirq
> raised for PCI SERR will be devered until we start the idle vcpu, but is 
> safe.
> 
> Trap 14 is very complicated.  The code is certainly unsafe for boot as
> fixup_page_fault() will dereference current to find the running domain.  
> There
> exists an explicit do_early_page_fault() handler which shall continue to be
> used.
> 
> Trap 18 has a default handler before the MCE infrastructure is set up, which
> has always been unsafe and liable to deadlock itself with the console lock.
> As it is expected never to trigger, and if it did we would be in serious
> problems, the simple printk() is replaced with a fatal error path.
> 
> Trap 20 (Virtualisation Exception) is currently not implemented.  It is 
> fatal
> one way or another, and will become more explicitly so with later changes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH RFC 4/9] x86/misc: Early cleanup

[Andrew Cooper]

Various bits of cleanup without functional impact as far as the series goes,
but make subsequent patches cleaner.

* WARN_ON(1) is just WARN().
* Replace hand-crafted rolled stack printing with fatal_trap().
* 16 bss bytes is overkill for an empty idtr to triple fault with.  Construct
  it on the stack using an appropriate struct, and correct the asm memory
  constraint.
* machine_restart() disables the watchdog itself, covering itself from other
  callers.  Drop superfluous braces and watchdog_disable() from panic().
* unsigned and const correctness for trapstr(), along with whitespace cleanup.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/shutdown.c    |    4 ++--
 xen/arch/x86/traps.c       |   34 +++++++++++++---------------------
 xen/drivers/char/console.c |    5 -----
 3 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 44bcd7f..e344503 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -34,7 +34,6 @@ enum reboot_type {
         BOOT_CF9 = 'p',
 };
 
-static long no_idt[2];
 static int reboot_mode;
 
 /*
@@ -466,6 +465,7 @@ void machine_restart(unsigned int delay_millisecs)
 {
     unsigned int i, attempt;
     enum reboot_type orig_reboot_type = reboot_type;
+    struct desc_ptr no_idt = { 0 };
 
     watchdog_disable();
     console_start_sync();
@@ -532,7 +532,7 @@ void machine_restart(unsigned int delay_millisecs)
                            ? BOOT_ACPI : BOOT_TRIPLE);
             break;
         case BOOT_TRIPLE:
-            asm volatile ( "lidt %0 ; int3" : "=m" (no_idt) );
+            asm volatile ("lidt %0; int3" : : "m" (no_idt));
             reboot_type = BOOT_KBD;
             break;
         case BOOT_ACPI:
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 65e4609..6f8acee 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -375,21 +375,18 @@ void vcpu_show_execution_state(struct vcpu *v)
     vcpu_unpause(v);
 }
 
-static char *trapstr(int trapnr)
-{
-    static char *strings[] = { 
-        "divide error", "debug", "nmi", "bkpt", "overflow", "bounds", 
-        "invalid opcode", "device not available", "double fault", 
-        "coprocessor segment", "invalid tss", "segment not found", 
-        "stack error", "general protection fault", "page fault", 
-        "spurious interrupt", "coprocessor error", "alignment check", 
+static const char *trapstr(unsigned int trapnr)
+{
+    static char *strings[] = {
+        "divide error", "debug", "nmi", "bkpt", "overflow", "bounds",
+        "invalid opcode", "device not available", "double fault",
+        "coprocessor segment", "invalid tss", "segment not found",
+        "stack error", "general protection fault", "page fault",
+        "spurious interrupt", "coprocessor error", "alignment check",
         "machine check", "simd error"
     };
 
-    if ( (trapnr < 0) || (trapnr >= ARRAY_SIZE(strings)) )
-        return "???";
-
-    return strings[trapnr];
+    return trapnr < ARRAY_SIZE(strings) ? strings[trapnr] : "???";
 }
 
 /*
@@ -1485,15 +1482,10 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
 
     if ( stuck++ == 1000 )
     {
-        unsigned long *stk = (unsigned long *)regs;
-        printk("Early fatal page fault at %04x:%p (cr2=%p, ec=%04x)\n", 
+        console_start_sync();
+        printk("Early fatal page fault at %04x:%p (cr2=%p, ec=%04x)\n",
                regs->cs, _p(regs->eip), _p(cr2), regs->error_code);
-        show_page_walk(cr2);
-        printk("Stack dump: ");
-        while ( ((long)stk & ((PAGE_SIZE - 1) & ~(BYTES_PER_LONG - 1))) != 0 )
-            printk("%p ", _p(*stk++));
-        for ( ; ; )
-            halt();
+        fatal_trap(TRAP_page_fault, regs);
     }
 }
 
@@ -3393,7 +3385,7 @@ void do_debug(struct cpu_user_regs *regs)
             }
             if ( !debugger_trap_fatal(TRAP_debug, regs) )
             {
-                WARN_ON(1);
+                WARN();
                 regs->eflags &= ~X86_EFLAGS_TF;
             }
         }
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 50b4415..2f6c090 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1124,14 +1124,9 @@ void panic(const char *fmt, ...)
 #endif
 
     if ( opt_noreboot )
-    {
         machine_halt();
-    }
     else
-    {
-        watchdog_disable();
         machine_restart(5000);
-    }
 }
 
 void __bug(char *file, int line)
-- 
1.7.10.4

Re: [PATCH RFC 4/9] x86/misc: Early cleanup

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> * machine_restart() disables the watchdog itself, covering itself from other
>   callers.  Drop superfluous braces and watchdog_disable() from panic().

Hmm, panic() is common code, and machine_restart()'s property of
calling watchdog_disable() is x86-specific. I realize that ARM just
doesn't have a watchdog right now, but do we really want to make
it a requirement that each watchdog-capable arch does that in
machine_restart()? Otoh an arch that can tolerate its watchdog
being disabled over the restart attempt would then not get this
enforced onto it (in which case only the description above would need
a little tweaking).

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -375,21 +375,18 @@ void vcpu_show_execution_state(struct vcpu *v)
>      vcpu_unpause(v);
>  }
>  
> -static char *trapstr(int trapnr)
> -{
> -    static char *strings[] = { 
> -        "divide error", "debug", "nmi", "bkpt", "overflow", "bounds", 
> -        "invalid opcode", "device not available", "double fault", 
> -        "coprocessor segment", "invalid tss", "segment not found", 
> -        "stack error", "general protection fault", "page fault", 
> -        "spurious interrupt", "coprocessor error", "alignment check", 
> +static const char *trapstr(unsigned int trapnr)
> +{
> +    static char *strings[] = {

If you already touch this, and if you already make the function
return const char *, then this also wants to become const char *const.

Jan

Re: [PATCH RFC 4/9] x86/misc: Early cleanup

[Andrew Cooper]

On 15/05/14 11:32, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> * machine_restart() disables the watchdog itself, covering itself from other
>>   callers.  Drop superfluous braces and watchdog_disable() from panic().
> Hmm, panic() is common code, and machine_restart()'s property of
> calling watchdog_disable() is x86-specific. I realize that ARM just
> doesn't have a watchdog right now, but do we really want to make
> it a requirement that each watchdog-capable arch does that in
> machine_restart()? Otoh an arch that can tolerate its watchdog
> being disabled over the restart attempt would then not get this
> enforced onto it (in which case only the description above would need
> a little tweaking).

machine_halt() needs just as much watchdog intervention as
machine_restart(), yet that was asymmetric.

I would argue that it is up to the arch to know whether playing with the
watchdog is needed for these things.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -375,21 +375,18 @@ void vcpu_show_execution_state(struct vcpu *v)
>>      vcpu_unpause(v);
>>  }
>>  
>> -static char *trapstr(int trapnr)
>> -{
>> -    static char *strings[] = { 
>> -        "divide error", "debug", "nmi", "bkpt", "overflow", "bounds", 
>> -        "invalid opcode", "device not available", "double fault", 
>> -        "coprocessor segment", "invalid tss", "segment not found", 
>> -        "stack error", "general protection fault", "page fault", 
>> -        "spurious interrupt", "coprocessor error", "alignment check", 
>> +static const char *trapstr(unsigned int trapnr)
>> +{
>> +    static char *strings[] = {
> If you already touch this, and if you already make the function
> return const char *, then this also wants to become const char *const.
>
> Jan
>

Will do.

~Andrew

[PATCH RFC 5/9] x86/traps: Functional prep work

[Andrew Cooper]

* Promote certain actions to earlier in __start_xen().
* Declare double_fault and early_page_fault as standard trap handlers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

It would be nice if the linker could sort the exception tables, and the Xen
check could be one for sanity.
---
 xen/arch/x86/setup.c            |   11 ++++++-----
 xen/arch/x86/x86_64/traps.c     |    1 -
 xen/include/asm-x86/processor.h |    2 ++
 xen/include/asm-x86/setup.h     |    1 -
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a864b9f..a69e25e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .stop_bits = 1
     };
 
+    set_processor_id(0);
+    set_current((struct vcpu *)0xfffff000); /* debug sanity */
+    this_cpu(curr_vcpu) = idle_vcpu[0] = current;
+
+    sort_exception_tables();
+
     percpu_init_areas();
 
     set_intr_gate(TRAP_page_fault, &early_page_fault);
@@ -588,9 +594,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     parse_video_info();
 
-    set_current((struct vcpu *)0xfffff000); /* debug sanity */
-    idle_vcpu[0] = current;
-    set_processor_id(0); /* needed early, for smp_processor_id() */
     if ( cpu_has_efer )
         rdmsrl(MSR_EFER, this_cpu(efer));
     asm volatile ( "mov %%cr4,%0" : "=r" (this_cpu(cr4)) );
@@ -1212,8 +1215,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( opt_watchdog ) 
         nmi_watchdog = NMI_LOCAL_APIC;
 
-    sort_exception_tables();
-
     find_smp_config();
 
     dmi_scan_machine();
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index e1c7b3b..e6c66a0 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -223,7 +223,6 @@ void show_page_walk(unsigned long addr)
            l1_table_offset(addr), l1e_get_intpte(l1e), pfn);
 }
 
-void double_fault(void);
 void do_double_fault(struct cpu_user_regs *regs)
 {
     unsigned int cpu;
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 35b2433..c9051be 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -500,12 +500,14 @@ DECLARE_TRAP_HANDLER(overflow);
 DECLARE_TRAP_HANDLER(bounds);
 DECLARE_TRAP_HANDLER(invalid_op);
 DECLARE_TRAP_HANDLER(device_not_available);
+DECLARE_TRAP_HANDLER(double_fault);
 DECLARE_TRAP_HANDLER(coprocessor_segment_overrun);
 DECLARE_TRAP_HANDLER(invalid_TSS);
 DECLARE_TRAP_HANDLER(segment_not_present);
 DECLARE_TRAP_HANDLER(stack_segment);
 DECLARE_TRAP_HANDLER(general_protection);
 DECLARE_TRAP_HANDLER(page_fault);
+DECLARE_TRAP_HANDLER(early_page_fault);
 DECLARE_TRAP_HANDLER(coprocessor_error);
 DECLARE_TRAP_HANDLER(simd_coprocessor_error);
 DECLARE_TRAP_HANDLER(machine_check);
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index 3039e1b..8f8c6f3 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -7,7 +7,6 @@ extern unsigned long xenheap_initial_phys_start;
 
 void early_cpu_init(void);
 void early_time_init(void);
-void early_page_fault(void);
 
 int intel_cpu_init(void);
 int amd_init_cpu(void);
-- 
1.7.10.4

Re: [PATCH RFC 5/9] x86/traps: Functional prep work

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          .stop_bits = 1
>      };
>  
> +    set_processor_id(0);
> +    set_current((struct vcpu *)0xfffff000); /* debug sanity */
> +    this_cpu(curr_vcpu) = idle_vcpu[0] = current;

The this_cpu() part wasn't there in the original code - is that really
needed, and ...

> +
> +    sort_exception_tables();
> +
>      percpu_init_areas();

... is that really safe/meaningful before this function got called?

Jan

Re: [PATCH RFC 5/9] x86/traps: Functional prep work

[Andrew Cooper]

On 15/05/14 11:36, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>          .stop_bits = 1
>>      };
>>  
>> +    set_processor_id(0);
>> +    set_current((struct vcpu *)0xfffff000); /* debug sanity */
>> +    this_cpu(curr_vcpu) = idle_vcpu[0] = current;
> The this_cpu() part wasn't there in the original code - is that really
> needed, and ...

I was attempting to go for similarity between __start_xen and
start_secondary, which reminds me I need a further fix regarding cr4,
which still loads CR4.MCE on APs before having a TRAP_machine_check
handler available.

>
>> +
>> +    sort_exception_tables();
>> +
>>      percpu_init_areas();
> ... is that really safe/meaningful before this function got called?
>
> Jan
>

There is no specific relationship between sort_exception_tables() and
percpu_init_areas(), both of which are tweaking well defined state
inside the .data section.

sort_excetpion_tables() is a prerequisite for getting extable fixups to
work in the trap handlers, but as indicated, it would be nice to turn it
into something more like "assert exception tables are sorted" and making
the linker do the work.

~Andrew

Re: [PATCH RFC 5/9] x86/traps: Functional prep work

[Jan Beulich]

>>> On 15.05.14 at 12:45, <andrew.cooper3@citrix.com> wrote:
> On 15/05/14 11:36, Jan Beulich wrote:
>>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>          .stop_bits = 1
>>>      };
>>>  
>>> +    set_processor_id(0);
>>> +    set_current((struct vcpu *)0xfffff000); /* debug sanity */
>>> +    this_cpu(curr_vcpu) = idle_vcpu[0] = current;
>> The this_cpu() part wasn't there in the original code - is that really
>> needed, and ...
> 
> I was attempting to go for similarity between __start_xen and
> start_secondary, which reminds me I need a further fix regarding cr4,
> which still loads CR4.MCE on APs before having a TRAP_machine_check
> handler available.
> 
>>
>>> +
>>> +    sort_exception_tables();
>>> +
>>>      percpu_init_areas();
>> ... is that really safe/meaningful before this function got called?
> 
> There is no specific relationship between sort_exception_tables() and
> percpu_init_areas(), both of which are tweaking well defined state
> inside the .data section.
> 
> sort_excetpion_tables() is a prerequisite for getting extable fixups to
> work in the trap handlers, but as indicated, it would be nice to turn it
> into something more like "assert exception tables are sorted" and making
> the linker do the work.

The comment wasn't about sort_exception_tables(), but about the
(at least apparent) conflict of this_cpu() getting used before
percpu_init_areas().

Jan

Re: [PATCH RFC 5/9] x86/traps: Functional prep work

[Andrew Cooper]

On 15/05/14 13:15, Jan Beulich wrote:
>>>> On 15.05.14 at 12:45, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/14 11:36, Jan Beulich wrote:
>>>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>          .stop_bits = 1
>>>>      };
>>>>  
>>>> +    set_processor_id(0);
>>>> +    set_current((struct vcpu *)0xfffff000); /* debug sanity */
>>>> +    this_cpu(curr_vcpu) = idle_vcpu[0] = current;
>>> The this_cpu() part wasn't there in the original code - is that really
>>> needed, and ...
>> I was attempting to go for similarity between __start_xen and
>> start_secondary, which reminds me I need a further fix regarding cr4,
>> which still loads CR4.MCE on APs before having a TRAP_machine_check
>> handler available.
>>
>>>> +
>>>> +    sort_exception_tables();
>>>> +
>>>>      percpu_init_areas();
>>> ... is that really safe/meaningful before this function got called?
>> There is no specific relationship between sort_exception_tables() and
>> percpu_init_areas(), both of which are tweaking well defined state
>> inside the .data section.
>>
>> sort_excetpion_tables() is a prerequisite for getting extable fixups to
>> work in the trap handlers, but as indicated, it would be nice to turn it
>> into something more like "assert exception tables are sorted" and making
>> the linker do the work.
> The comment wasn't about sort_exception_tables(), but about the
> (at least apparent) conflict of this_cpu() getting used before
> percpu_init_areas().
>
> Jan
>

Ah - I see what you mean.

The BSP per_cpu_offset is 0, so the code as patched does work correctly.

It would however become a latent bug if the implementation of per_cpu
variables changed such that the BSP didn't use the copy of the per_cpu
data in the .data section.

I shall just drop the this_cpu() bit.  Consistency with start_secondary
is not worth this latent bug.

~Andrew

[PATCH RFC 6/9] x86/boot: Install trap handlers much earlier on boot

[Andrew Cooper]

Patch the trap handlers into the master idt very early on boot, and setup &
load the GDT, LDT, TR and IDT such that once the IDT is loaded, all state is
safe for the traps to function correctly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/cpu/common.c    |   70 +++++++++++++++++++++++++++++++++---------
 xen/arch/x86/setup.c         |    9 ++++--
 xen/arch/x86/smpboot.c       |   21 ++++---------
 xen/arch/x86/traps.c         |   31 ++++++++++++++-----
 xen/arch/x86/x86_64/traps.c  |   28 -----------------
 xen/include/asm-x86/system.h |    1 +
 xen/include/xen/sched.h      |    1 +
 7 files changed, 94 insertions(+), 67 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4122684..bd8f66c 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -515,6 +515,61 @@ void __init early_cpu_init(void)
 	centaur_init_cpu();
 	early_cpu_detect();
 }
+
+/*
+ * Sets up system tables and descriptors.
+ *
+ * - Sets up TSS with stack pointers, including ISTs
+ * - Inserts TSS selector into regular and compat GDTs
+ * - Loads GDT, Null LDT, TR and IDT
+ */
+void __cpuinit load_system_tables(void)
+{
+	unsigned int cpu = smp_processor_id();
+	unsigned long stack_bottom = get_stack_bottom(),
+		stack_top = stack_bottom & ~(STACK_SIZE - 1);
+
+	struct tss_struct *tss = &this_cpu(init_tss);
+	struct desc_struct *gdt =
+		this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
+	struct desc_struct *compat_gdt =
+		this_cpu(compat_gdt_table)- FIRST_RESERVED_GDT_ENTRY;
+
+	struct desc_ptr gdtr = {
+		.base = (unsigned long)gdt,
+		.limit = LAST_RESERVED_GDT_BYTE,
+	};
+	struct desc_ptr idtr = {
+		.base = (unsigned long)idt_tables[cpu],
+		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
+	};
+
+	/* Main stack for interrupts/exceptions */
+	tss->rsp0 = stack_bottom;
+	tss->bitmap = IOBMP_INVALID_OFFSET;
+
+	/* MCE, NMI and Double Fault handlers get their own stacks */
+	tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
+	tss->ist[IST_DF	 - 1] = stack_top + IST_DF  * PAGE_SIZE;
+	tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
+
+	_set_tssldt_desc(
+		gdt + TSS_ENTRY,
+		(unsigned long)tss,
+		offsetof(struct tss_struct, __cacheline_filler) - 1,
+		DESC_TYPE_tss_avail);
+	_set_tssldt_desc(
+		compat_gdt + TSS_ENTRY,
+		(unsigned long)tss,
+		offsetof(struct tss_struct, __cacheline_filler) - 1,
+		DESC_TYPE_tss_busy);
+
+	asm volatile ( "lgdt %0"   : : "m"  (gdtr) );
+	asm volatile ( "lldt %w0"  : : "rm" (0) );
+	asm volatile ( "ltr  %w0"  : : "rm" (TSS_ENTRY << 3) );
+	asm volatile ( "lidt %0"   : : "m"  (idtr) );
+}
+
 /*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
@@ -524,11 +579,6 @@ void __init early_cpu_init(void)
 void __cpuinit cpu_init(void)
 {
 	int cpu = smp_processor_id();
-	struct tss_struct *t = &this_cpu(init_tss);
-	struct desc_ptr gdt_desc = {
-		.base = (unsigned long)(this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY),
-		.limit = LAST_RESERVED_GDT_BYTE
-	};
 
 	if (cpumask_test_and_set_cpu(cpu, &cpu_initialized)) {
 		printk(KERN_WARNING "CPU#%d already initialized!\n", cpu);
@@ -543,22 +593,12 @@ void __cpuinit cpu_init(void)
 	/* Install correct page table. */
 	write_ptbase(current);
 
-	asm volatile ( "lgdt %0" : : "m" (gdt_desc) );
-
 	/* No nested task. */
 	asm volatile ("pushf ; andw $0xbfff,(%"__OP"sp) ; popf" );
 
 	/* Ensure FPU gets initialised for each domain. */
 	stts();
 
-	/* Set up and load the per-CPU TSS and LDT. */
-	t->bitmap = IOBMP_INVALID_OFFSET;
-	/* Bottom-of-stack must be 16-byte aligned! */
-	BUG_ON((get_stack_bottom() & 15) != 0);
-	t->rsp0 = get_stack_bottom();
-	load_TR();
-	asm volatile ( "lldt %%ax" : : "a" (0) );
-
 	/* Clear all 6 debug registers: */
 #define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
 	CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a69e25e..ef5863f 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -558,15 +558,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         .stop_bits = 1
     };
 
+    /* Critical region without IDT or TSS.  Any fault is deadly! */
+
     set_processor_id(0);
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
     this_cpu(curr_vcpu) = idle_vcpu[0] = current;
 
+    init_idt_traps();
+    load_system_tables();
+
     sort_exception_tables();
 
-    percpu_init_areas();
+    /* Full trap support from here on in */
 
-    set_intr_gate(TRAP_page_fault, &early_page_fault);
+    percpu_init_areas();
 
     loader = (mbi->flags & MBI_LOADERNAME)
         ? (char *)__va(mbi->boot_loader_name) : "unknown";
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 5014397..7ecf465 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -303,15 +303,6 @@ static void set_cpu_sibling_map(int cpu)
     }
 }
 
-static void construct_percpu_idt(unsigned int cpu)
-{
-    unsigned char idt_load[10];
-
-    *(unsigned short *)(&idt_load[0]) = (IDT_ENTRIES*sizeof(idt_entry_t))-1;
-    *(unsigned long  *)(&idt_load[2]) = (unsigned long)idt_tables[cpu];
-    __asm__ __volatile__ ( "lidt %0" : "=m" (idt_load) );
-}
-
 void start_secondary(void *unused)
 {
     /*
@@ -320,6 +311,8 @@ void start_secondary(void *unused)
      */
     unsigned int cpu = booting_cpu;
 
+    /* Critical region without IDT or TSS.  Any fault is deadly! */
+
     set_processor_id(cpu);
     set_current(idle_vcpu[cpu]);
     this_cpu(curr_vcpu) = idle_vcpu[cpu];
@@ -345,6 +338,10 @@ void start_secondary(void *unused)
      */
     spin_debug_disable();
 
+    load_system_tables();
+
+    /* Full trap support from here on in */
+
     percpu_traps_init();
 
     init_percpu_time();
@@ -353,12 +350,6 @@ void start_secondary(void *unused)
 
     smp_callin();
 
-    /*
-     * At this point, boot CPU has fully initialised the IDT. It is
-     * now safe to make ourselves a private copy.
-     */
-    construct_percpu_idt(cpu);
-
     setup_secondary_APIC_clock();
 
     /*
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6f8acee..41a9d77 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3497,14 +3497,14 @@ void __devinit percpu_traps_init(void)
     ler_enable();
 }
 
-void __init trap_init(void)
+void __init init_idt_traps(void)
 {
     /*
-     * Note that interrupt gates are always used, rather than trap gates. We 
-     * must have interrupts disabled until DS/ES/FS/GS are saved because the 
-     * first activation must have the "bad" value(s) for these registers and 
-     * we may lose them if another activation is installed before they are 
-     * saved. The page-fault handler also needs interrupts disabled until %cr2 
+     * Note that interrupt gates are always used, rather than trap gates. We
+     * must have interrupts disabled until DS/ES/FS/GS are saved because the
+     * first activation must have the "bad" value(s) for these registers and
+     * we may lose them if another activation is installed before they are
+     * saved. The page-fault handler also needs interrupts disabled until %cr2
      * has been read and saved on the stack.
      */
     set_intr_gate(TRAP_divide_error,&divide_error);
@@ -3515,23 +3515,40 @@ void __init trap_init(void)
     set_intr_gate(TRAP_bounds,&bounds);
     set_intr_gate(TRAP_invalid_op,&invalid_op);
     set_intr_gate(TRAP_no_device,&device_not_available);
+    set_intr_gate(TRAP_double_fault, &double_fault);
     set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
     set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
     set_intr_gate(TRAP_no_segment,&segment_not_present);
     set_intr_gate(TRAP_stack_error,&stack_segment);
     set_intr_gate(TRAP_gp_fault,&general_protection);
-    set_intr_gate(TRAP_page_fault,&page_fault);
+    set_intr_gate(TRAP_page_fault,&early_page_fault);
     set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
     set_intr_gate(TRAP_copro_error,&coprocessor_error);
     set_intr_gate(TRAP_alignment_check,&alignment_check);
     set_intr_gate(TRAP_machine_check,&machine_check);
     set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
 
+    /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
+    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
+    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
+    set_ist(&idt_table[TRAP_machine_check], IST_MCE);
+
     /* CPU0 uses the master IDT. */
     idt_tables[0] = idt_table;
 
     this_cpu(gdt_table) = boot_cpu_gdt_table;
     this_cpu(compat_gdt_table) = boot_cpu_compat_gdt_table;
+}
+
+void __init trap_init(void)
+{
+    set_intr_gate(TRAP_page_fault, &page_fault);
+
+    /* The 32-on-64 hypercall entry vector is only accessible from ring 1. */
+    _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
+
+    /* Fast trap for int80 (faster than taking the #GP-fixup path). */
+    _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
 
     percpu_traps_init();
 
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index e6c66a0..d09b6b6 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -379,25 +379,6 @@ static int write_stack_trampoline(
 void __devinit subarch_percpu_traps_init(void)
 {
     char *stack_bottom, *stack;
-    int   cpu = smp_processor_id();
-
-    if ( cpu == 0 )
-    {
-        /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
-        set_intr_gate(TRAP_double_fault, &double_fault);
-        set_ist(&idt_table[TRAP_double_fault],  IST_DF);
-        set_ist(&idt_table[TRAP_nmi],           IST_NMI);
-        set_ist(&idt_table[TRAP_machine_check], IST_MCE);
-
-        /*
-         * The 32-on-64 hypercall entry vector is only accessible from ring 1.
-         * Also note that this is a trap gate, not an interrupt gate.
-         */
-        _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
-
-        /* Fast trap for int80 (faster than taking the #GP-fixup path). */
-        _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
-    }
 
     stack_bottom = (char *)get_stack_bottom();
     stack        = (char *)((unsigned long)stack_bottom & ~(STACK_SIZE - 1));
@@ -405,15 +386,6 @@ void __devinit subarch_percpu_traps_init(void)
     /* IST_MAX IST pages + 1 syscall page + 1 guard page + primary stack. */
     BUILD_BUG_ON((IST_MAX + 2) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
 
-    /* Machine Check handler has its own per-CPU 4kB stack. */
-    this_cpu(init_tss).ist[IST_MCE-1] = (unsigned long)&stack[IST_MCE * PAGE_SIZE];
-
-    /* Double-fault handler has its own per-CPU 4kB stack. */
-    this_cpu(init_tss).ist[IST_DF-1] = (unsigned long)&stack[IST_DF * PAGE_SIZE];
-
-    /* NMI handler has its own per-CPU 4kB stack. */
-    this_cpu(init_tss).ist[IST_NMI-1] = (unsigned long)&stack[IST_NMI * PAGE_SIZE];
-
     /* Trampoline for SYSCALL entry from long mode. */
     stack = &stack[IST_MAX * PAGE_SIZE]; /* Skip the IST stacks. */
     wrmsrl(MSR_LSTAR, (unsigned long)stack);
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index e9602aa..c5e482a 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -179,6 +179,7 @@ static inline int local_irq_is_enabled(void)
 #define BROKEN_INIT_AFTER_S1    0x0002
 
 void trap_init(void);
+void init_idt_traps(void);
 void percpu_traps_init(void);
 void subarch_percpu_traps_init(void);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 44851ae..acbe117 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -770,6 +770,7 @@ void domain_unpause(struct domain *d);
 void domain_pause_by_systemcontroller(struct domain *d);
 void domain_unpause_by_systemcontroller(struct domain *d);
 void cpu_init(void);
+void load_system_tables(void);
 
 struct scheduler;
 
-- 
1.7.10.4

Re: [PATCH RFC 6/9] x86/boot: Install trap handlers much earlier on boot

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> +void __cpuinit load_system_tables(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	unsigned long stack_bottom = get_stack_bottom(),
> +		stack_top = stack_bottom & ~(STACK_SIZE - 1);
> +
> +	struct tss_struct *tss = &this_cpu(init_tss);
> +	struct desc_struct *gdt =
> +		this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
> +	struct desc_struct *compat_gdt =
> +		this_cpu(compat_gdt_table)- FIRST_RESERVED_GDT_ENTRY;
> +
> +	struct desc_ptr gdtr = {
> +		.base = (unsigned long)gdt,
> +		.limit = LAST_RESERVED_GDT_BYTE,
> +	};
> +	struct desc_ptr idtr = {
> +		.base = (unsigned long)idt_tables[cpu],
> +		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
> +	};
> +
> +	/* Main stack for interrupts/exceptions */
> +	tss->rsp0 = stack_bottom;
> +	tss->bitmap = IOBMP_INVALID_OFFSET;
> +
> +	/* MCE, NMI and Double Fault handlers get their own stacks */
> +	tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
> +	tss->ist[IST_DF	 - 1] = stack_top + IST_DF  * PAGE_SIZE;

Hard tab.

> +	tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
> +
> +	_set_tssldt_desc(
> +		gdt + TSS_ENTRY,
> +		(unsigned long)tss,
> +		offsetof(struct tss_struct, __cacheline_filler) - 1,
> +		DESC_TYPE_tss_avail);
> +	_set_tssldt_desc(
> +		compat_gdt + TSS_ENTRY,
> +		(unsigned long)tss,
> +		offsetof(struct tss_struct, __cacheline_filler) - 1,
> +		DESC_TYPE_tss_busy);
> +
> +	asm volatile ( "lgdt %0"   : : "m"  (gdtr) );
> +	asm volatile ( "lldt %w0"  : : "rm" (0) );
> +	asm volatile ( "ltr  %w0"  : : "rm" (TSS_ENTRY << 3) );
> +	asm volatile ( "lidt %0"   : : "m"  (idtr) );

I think you ought to load the IDT right after the GDT, so that eventual
faults on the LLDT and LTR have less chance of ending in a triple fault.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -558,15 +558,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          .stop_bits = 1
>      };
>  
> +    /* Critical region without IDT or TSS.  Any fault is deadly! */

Which raises the question whether the next patch shouldn't be
dropped.

>      set_processor_id(0);
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>      this_cpu(curr_vcpu) = idle_vcpu[0] = current;
>  
> +    init_idt_traps();
> +    load_system_tables();
> +
>      sort_exception_tables();
>  
> -    percpu_init_areas();
> +    /* Full trap support from here on in */

Stop missing (also elsewhere).

> @@ -345,6 +338,10 @@ void start_secondary(void *unused)
>       */
>      spin_debug_disable();
>  
> +    load_system_tables();
> +
> +    /* Full trap support from here on in */
> +
>      percpu_traps_init();

I guess this ends up being confusing, even if it's just because the
name of the function perhaps no longer reflects its purpose.

> @@ -3515,23 +3515,40 @@ void __init trap_init(void)
>      set_intr_gate(TRAP_bounds,&bounds);
>      set_intr_gate(TRAP_invalid_op,&invalid_op);
>      set_intr_gate(TRAP_no_device,&device_not_available);
> +    set_intr_gate(TRAP_double_fault, &double_fault);
>      set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
>      set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
>      set_intr_gate(TRAP_no_segment,&segment_not_present);
>      set_intr_gate(TRAP_stack_error,&stack_segment);
>      set_intr_gate(TRAP_gp_fault,&general_protection);
> -    set_intr_gate(TRAP_page_fault,&page_fault);
> +    set_intr_gate(TRAP_page_fault,&early_page_fault);
>      set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
>      set_intr_gate(TRAP_copro_error,&coprocessor_error);
>      set_intr_gate(TRAP_alignment_check,&alignment_check);
>      set_intr_gate(TRAP_machine_check,&machine_check);
>      set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
>  
> +    /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
> +    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
> +    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
> +    set_ist(&idt_table[TRAP_machine_check], IST_MCE);

Mind putting them right aside the respective set_intr_gate() above?

> +void __init trap_init(void)
> +{
> +    set_intr_gate(TRAP_page_fault, &page_fault);
> +
> +    /* The 32-on-64 hypercall entry vector is only accessible from ring 1. */
> +    _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
> +
> +    /* Fast trap for int80 (faster than taking the #GP-fixup path). */
> +    _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);

Long lines?

Jan

Re: [PATCH RFC 6/9] x86/boot: Install trap handlers much earlier on boot

[Andrew Cooper]

On 15/05/14 11:53, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> +void __cpuinit load_system_tables(void)
>> +{
>> +	unsigned int cpu = smp_processor_id();
>> +	unsigned long stack_bottom = get_stack_bottom(),
>> +		stack_top = stack_bottom & ~(STACK_SIZE - 1);
>> +
>> +	struct tss_struct *tss = &this_cpu(init_tss);
>> +	struct desc_struct *gdt =
>> +		this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
>> +	struct desc_struct *compat_gdt =
>> +		this_cpu(compat_gdt_table)- FIRST_RESERVED_GDT_ENTRY;
>> +
>> +	struct desc_ptr gdtr = {
>> +		.base = (unsigned long)gdt,
>> +		.limit = LAST_RESERVED_GDT_BYTE,
>> +	};
>> +	struct desc_ptr idtr = {
>> +		.base = (unsigned long)idt_tables[cpu],
>> +		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>> +	};
>> +
>> +	/* Main stack for interrupts/exceptions */
>> +	tss->rsp0 = stack_bottom;
>> +	tss->bitmap = IOBMP_INVALID_OFFSET;
>> +
>> +	/* MCE, NMI and Double Fault handlers get their own stacks */
>> +	tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
>> +	tss->ist[IST_DF	 - 1] = stack_top + IST_DF  * PAGE_SIZE;
> Hard tab.

These should all be hard tabs, following the file's style.

>
>> +	tss->ist[IST_NMI - 1] = stack_top + IST_NMI * PAGE_SIZE;
>> +
>> +	_set_tssldt_desc(
>> +		gdt + TSS_ENTRY,
>> +		(unsigned long)tss,
>> +		offsetof(struct tss_struct, __cacheline_filler) - 1,
>> +		DESC_TYPE_tss_avail);
>> +	_set_tssldt_desc(
>> +		compat_gdt + TSS_ENTRY,
>> +		(unsigned long)tss,
>> +		offsetof(struct tss_struct, __cacheline_filler) - 1,
>> +		DESC_TYPE_tss_busy);
>> +
>> +	asm volatile ( "lgdt %0"   : : "m"  (gdtr) );
>> +	asm volatile ( "lldt %w0"  : : "rm" (0) );
>> +	asm volatile ( "ltr  %w0"  : : "rm" (TSS_ENTRY << 3) );
>> +	asm volatile ( "lidt %0"   : : "m"  (idtr) );
> I think you ought to load the IDT right after the GDT, so that eventual
> faults on the LLDT and LTR have less chance of ending in a triple fault.

That particular lldt can't possibly fault.  I wanted to avoid having IST
information in the IDT without a loaded TSS, but given the proximity of
the loads, catching an invalid_tss from the 'ltr' might be useful.

>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -558,15 +558,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>          .stop_bits = 1
>>      };
>>  
>> +    /* Critical region without IDT or TSS.  Any fault is deadly! */
> Which raises the question whether the next patch shouldn't be
> dropped.

With this patch, the BSP code looks like:

* patch ingnore_int everywhere into idt_table
* call into C
* patch real trap handlers into idt_table
* load system tables such that faults all work.

Therefore, the ignore_int() patching is almost completely pointless,
which is why it is completely removed in the subsequent patch.

>
>>      set_processor_id(0);
>>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>>      this_cpu(curr_vcpu) = idle_vcpu[0] = current;
>>  
>> +    init_idt_traps();
>> +    load_system_tables();
>> +
>>      sort_exception_tables();
>>  
>> -    percpu_init_areas();
>> +    /* Full trap support from here on in */
> Stop missing (also elsewhere).
>
>> @@ -345,6 +338,10 @@ void start_secondary(void *unused)
>>       */
>>      spin_debug_disable();
>>  
>> +    load_system_tables();
>> +
>> +    /* Full trap support from here on in */
>> +
>>      percpu_traps_init();
> I guess this ends up being confusing, even if it's just because the
> name of the function perhaps no longer reflects its purpose.

Yes - there is a little more poor resultant naming than I would like.  I
am also not completely happy with the names "load_system_tables()" and
"init_idt_traps()", but can't think of more appropriate names offhand.

>
>> @@ -3515,23 +3515,40 @@ void __init trap_init(void)
>>      set_intr_gate(TRAP_bounds,&bounds);
>>      set_intr_gate(TRAP_invalid_op,&invalid_op);
>>      set_intr_gate(TRAP_no_device,&device_not_available);
>> +    set_intr_gate(TRAP_double_fault, &double_fault);
>>      set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
>>      set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
>>      set_intr_gate(TRAP_no_segment,&segment_not_present);
>>      set_intr_gate(TRAP_stack_error,&stack_segment);
>>      set_intr_gate(TRAP_gp_fault,&general_protection);
>> -    set_intr_gate(TRAP_page_fault,&page_fault);
>> +    set_intr_gate(TRAP_page_fault,&early_page_fault);
>>      set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
>>      set_intr_gate(TRAP_copro_error,&coprocessor_error);
>>      set_intr_gate(TRAP_alignment_check,&alignment_check);
>>      set_intr_gate(TRAP_machine_check,&machine_check);
>>      set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
>>  
>> +    /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
>> +    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
>> +    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
>> +    set_ist(&idt_table[TRAP_machine_check], IST_MCE);
> Mind putting them right aside the respective set_intr_gate() above?

Sorted in a later patch.  For now I think it is clearer as obvious code
motion out of cpu_init().

>
>> +void __init trap_init(void)
>> +{
>> +    set_intr_gate(TRAP_page_fault, &page_fault);
>> +
>> +    /* The 32-on-64 hypercall entry vector is only accessible from ring 1. */
>> +    _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
>> +
>> +    /* Fast trap for int80 (faster than taking the #GP-fixup path). */
>> +    _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
> Long lines?
>
> Jan

Again, code motion, although this long line was because of
"DESC_TYPE_trap_gate".  "SYS_DESC_trap" in the first patch would fix the
issue.

~Andrew

Re: [PATCH RFC 6/9] x86/boot: Install trap handlers much earlier on boot

[Jan Beulich]

>>> On 15.05.14 at 13:05, <andrew.cooper3@citrix.com> wrote:
> On 15/05/14 11:53, Jan Beulich wrote:
>>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>>> +void __cpuinit load_system_tables(void)
>>> +{
>>> +	unsigned int cpu = smp_processor_id();
>>> +	unsigned long stack_bottom = get_stack_bottom(),
>>> +		stack_top = stack_bottom & ~(STACK_SIZE - 1);
>>> +
>>> +	struct tss_struct *tss = &this_cpu(init_tss);
>>> +	struct desc_struct *gdt =
>>> +		this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
>>> +	struct desc_struct *compat_gdt =
>>> +		this_cpu(compat_gdt_table)- FIRST_RESERVED_GDT_ENTRY;
>>> +
>>> +	struct desc_ptr gdtr = {
>>> +		.base = (unsigned long)gdt,
>>> +		.limit = LAST_RESERVED_GDT_BYTE,
>>> +	};
>>> +	struct desc_ptr idtr = {
>>> +		.base = (unsigned long)idt_tables[cpu],
>>> +		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
>>> +	};
>>> +
>>> +	/* Main stack for interrupts/exceptions */
>>> +	tss->rsp0 = stack_bottom;
>>> +	tss->bitmap = IOBMP_INVALID_OFFSET;
>>> +
>>> +	/* MCE, NMI and Double Fault handlers get their own stacks */
>>> +	tss->ist[IST_MCE - 1] = stack_top + IST_MCE * PAGE_SIZE;
>>> +	tss->ist[IST_DF	 - 1] = stack_top + IST_DF  * PAGE_SIZE;
>> Hard tab.
> 
> These should all be hard tabs, following the file's style.

At the line start yes, but not in the middle of a line please.

>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -558,15 +558,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>          .stop_bits = 1
>>>      };
>>>  
>>> +    /* Critical region without IDT or TSS.  Any fault is deadly! */
>> Which raises the question whether the next patch shouldn't be
>> dropped.
> 
> With this patch, the BSP code looks like:
> 
> * patch ingnore_int everywhere into idt_table
> * call into C
> * patch real trap handlers into idt_table
> * load system tables such that faults all work.
> 
> Therefore, the ignore_int() patching is almost completely pointless,
> which is why it is completely removed in the subsequent patch.

Oh, right, this is indeed the last action before calling __start_xen.

>>> @@ -345,6 +338,10 @@ void start_secondary(void *unused)
>>>       */
>>>      spin_debug_disable();
>>>  
>>> +    load_system_tables();
>>> +
>>> +    /* Full trap support from here on in */
>>> +
>>>      percpu_traps_init();
>> I guess this ends up being confusing, even if it's just because the
>> name of the function perhaps no longer reflects its purpose.
> 
> Yes - there is a little more poor resultant naming than I would like.  I
> am also not completely happy with the names "load_system_tables()" and
> "init_idt_traps()", but can't think of more appropriate names offhand.

I think those latter names are okay. I may just be desirable to
rename percpu_traps_init() along with all the traps related stuff
getting removed from it.

>>> @@ -3515,23 +3515,40 @@ void __init trap_init(void)
>>>      set_intr_gate(TRAP_bounds,&bounds);
>>>      set_intr_gate(TRAP_invalid_op,&invalid_op);
>>>      set_intr_gate(TRAP_no_device,&device_not_available);
>>> +    set_intr_gate(TRAP_double_fault, &double_fault);
>>>      set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
>>>      set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
>>>      set_intr_gate(TRAP_no_segment,&segment_not_present);
>>>      set_intr_gate(TRAP_stack_error,&stack_segment);
>>>      set_intr_gate(TRAP_gp_fault,&general_protection);
>>> -    set_intr_gate(TRAP_page_fault,&page_fault);
>>> +    set_intr_gate(TRAP_page_fault,&early_page_fault);
>>>      set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
>>>      set_intr_gate(TRAP_copro_error,&coprocessor_error);
>>>      set_intr_gate(TRAP_alignment_check,&alignment_check);
>>>      set_intr_gate(TRAP_machine_check,&machine_check);
>>>      set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
>>>  
>>> +    /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
>>> +    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
>>> +    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
>>> +    set_ist(&idt_table[TRAP_machine_check], IST_MCE);
>> Mind putting them right aside the respective set_intr_gate() above?
> 
> Sorted in a later patch.  For now I think it is clearer as obvious code
> motion out of cpu_init().

Okay, if a subsequent patch deals with that, I guess it's fine then
this way.

>>> +void __init trap_init(void)
>>> +{
>>> +    set_intr_gate(TRAP_page_fault, &page_fault);
>>> +
>>> +    /* The 32-on-64 hypercall entry vector is only accessible from ring 1. */
>>> +    _set_gate(idt_table+HYPERCALL_VECTOR, DESC_TYPE_trap_gate, 1, &compat_hypercall);
>>> +
>>> +    /* Fast trap for int80 (faster than taking the #GP-fixup path). */
>>> +    _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
>> Long lines?
> 
> Again, code motion, although this long line was because of
> "DESC_TYPE_trap_gate".  "SYS_DESC_trap" in the first patch would fix the
> issue.

Adjusting formatting while moving code is not a problem I would say,
and should be done if the code was violating coding style.

Jan

[PATCH RFC 7/9] x86/boot: Drop pre-C IDT patching

[Andrew Cooper]

It is not needed now that __start_xen sets itself up with complete trap
handlers as its first action.  This fixes a potential issue introduced in

  c/s 7e510a7b874
  "x86/boot: move some __high_start code and data into init sections"

which would leave ignore_int (in the .init section) patched into the reserved
exceptions in all IDTs.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/boot/x86_64.S |   56 +-------------------------------------------
 1 file changed, 1 insertion(+), 55 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 67dfef9..943f409 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -25,64 +25,15 @@
         leaq    1f(%rip),%rax
         pushq   %rax
         lretq
-1:      lidt    idt_descr(%rip)
-
+1:
         test    %ebx,%ebx
         jnz     start_secondary
-        jmp     start_bsp
-
-        .section .init.text, "ax", @progbits
-
-start_bsp:
-        /* Initialise IDT with simple error defaults. */
-        leaq    ignore_int(%rip),%rcx
-        movl    %ecx,%eax
-        andl    $0xFFFF0000,%eax
-        orl     $0x00008E00,%eax
-        shlq    $32,%rax
-        movl    %ecx,%edx
-        andl    $0x0000FFFF,%edx
-        orl     $(__HYPERVISOR_CS64<<16),%edx
-        orq     %rdx,%rax
-        shrq    $32,%rcx
-        movl    %ecx,%edx
-        leaq    idt_table(%rip),%rdi
-        movl    $256,%ecx
-1:      movq    %rax,(%rdi)
-        movq    %rdx,8(%rdi)
-        addq    $16,%rdi
-        loop    1b
 
         /* Pass off the Multiboot info structure to C land. */
         mov     multiboot_ptr(%rip),%edi
         call    __start_xen
         ud2     /* Force a panic (invalid opcode). */
 
-/* This is the default interrupt handler. */
-ignore_int:
-        SAVE_ALL CLAC
-        movq    %cr2,%rsi
-        leaq    int_msg(%rip),%rdi
-        xorl    %eax,%eax
-        call    printk
-        movq    %rsp,%rbp
-0:      movq    (%rbp),%rsi
-        addq    $8,%rbp
-        leaq    hex_msg(%rip),%rdi
-        xorl    %eax,%eax
-        call    printk
-        testq   $0xff8,%rbp
-        jnz     0b
-1:      hlt
-        jmp     1b
-
-        .section .init.rodata, "a", @progbits
-
-int_msg:
-        .asciz "Unknown interrupt (cr2=%016lx)\n"
-hex_msg:
-        .asciz "    %016lx"
-
 /*** DESCRIPTOR TABLES ***/
 
         .data
@@ -95,11 +46,6 @@ GLOBAL(gdt_descr)
         .word   LAST_RESERVED_GDT_BYTE
         .quad   boot_cpu_gdt_table - FIRST_RESERVED_GDT_BYTE
 
-        .word   0,0,0
-GLOBAL(idt_descr)
-        .word   256*16-1
-        .quad   idt_table
-
 GLOBAL(stack_start)
         .quad   cpu0_stack
 
-- 
1.7.10.4

[PATCH RFC 8/9] x86/irqs: Move interrupt-stub generation out of C

[Andrew Cooper]

Also generate stubs for reserved exceptions, which find their way to
do_reserved_exception() along with their entry vector for informative
purposes.

 * Move all automatic stub generation out of i8259.c and into irqgen.S.
 * Move patching of the master IDT into trap_init(), and provide ASSERT()s to
   ensure we have fully populated the IDT, and unexpectedly clobbered any
   traps.
 * Demote TRAP_copro_seg and TRAP_spurious_int to being reserved exceptions,
   and remove their custom entry points.
 * Introduce BAD_VIRT_ADDR as a virtual address guaranteed to cause a #GP
   fault if followed, and use it in preference to NULL pointers in the
   exception table.
 * Acquaint Xen with #VE but leave it reserved.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---
The .macro hackary in irqgen.S is very ugly, and I dislike it.  This is the
only way I can find of getting a literal number out of 'vec'.  If anyone knows
how to beat GAS into submission, I will very happily accept improvements.
---
 xen/arch/x86/i8259.c            |   69 +------------------------------------
 xen/arch/x86/traps.c            |   39 ++++++++++++++++-----
 xen/arch/x86/x86_64/Makefile    |    1 +
 xen/arch/x86/x86_64/entry.S     |   22 +++++-------
 xen/arch/x86/x86_64/irqgen.S    |   72 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/asm_defns.h |    5 ---
 xen/include/asm-x86/config.h    |    1 +
 xen/include/asm-x86/processor.h |    4 +--
 8 files changed, 117 insertions(+), 96 deletions(-)
 create mode 100644 xen/arch/x86/x86_64/irqgen.S

diff --git a/xen/arch/x86/i8259.c b/xen/arch/x86/i8259.c
index 9fec490..9f99995 100644
--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -23,65 +23,6 @@
 #include <io_ports.h>
 
 /*
- * Common place to define all x86 IRQ vectors
- *
- * This builds up the IRQ handler stubs using some ugly macros in irq.h
- *
- * These macros create the low-level assembly IRQ routines that save
- * register context and call do_IRQ(). do_IRQ() then does all the
- * operations that are needed to keep the AT (or SMP IOAPIC)
- * interrupt-controller happy.
- */
-
-__asm__(".section .text");
-
-#define IRQ_NAME(nr) VEC##nr##_interrupt
-
-#define BI(nr)                                           \
-void IRQ_NAME(nr)(void);                                 \
-__asm__(                                                 \
-".if " STR(0x##nr) " >= " STR(FIRST_DYNAMIC_VECTOR) "\n" \
-__ALIGN_STR "\n"                                         \
-STR(IRQ_NAME(nr)) ":\n\t"                                \
-BUILD_IRQ(0x##nr) "\n"                                   \
-".else\n"                                                \
-".equ " STR(IRQ_NAME(nr)) ", 0\n"                        \
-".endif\n")
-
-#define BUILD_16_IRQS(x) \
-    BI(x##0); BI(x##1); BI(x##2); BI(x##3); \
-    BI(x##4); BI(x##5); BI(x##6); BI(x##7); \
-    BI(x##8); BI(x##9); BI(x##a); BI(x##b); \
-    BI(x##c); BI(x##d); BI(x##e); BI(x##f)
-
-BUILD_16_IRQS(0); BUILD_16_IRQS(1); BUILD_16_IRQS(2); BUILD_16_IRQS(3);
-BUILD_16_IRQS(4); BUILD_16_IRQS(5); BUILD_16_IRQS(6); BUILD_16_IRQS(7);
-BUILD_16_IRQS(8); BUILD_16_IRQS(9); BUILD_16_IRQS(a); BUILD_16_IRQS(b);
-BUILD_16_IRQS(c); BUILD_16_IRQS(d); BUILD_16_IRQS(e); BUILD_16_IRQS(f);
-
-#undef BUILD_16_IRQS
-#undef BI
-
-
-#define IRQ(x,y) IRQ_NAME(x##y)
-
-#define IRQLIST_16(x) \
-    IRQ(x,0), IRQ(x,1), IRQ(x,2), IRQ(x,3), \
-    IRQ(x,4), IRQ(x,5), IRQ(x,6), IRQ(x,7), \
-    IRQ(x,8), IRQ(x,9), IRQ(x,a), IRQ(x,b), \
-    IRQ(x,c), IRQ(x,d), IRQ(x,e), IRQ(x,f)
-
-static void (*__initdata interrupt[NR_VECTORS])(void) = {
-    IRQLIST_16(0), IRQLIST_16(1), IRQLIST_16(2), IRQLIST_16(3),
-    IRQLIST_16(4), IRQLIST_16(5), IRQLIST_16(6), IRQLIST_16(7),
-    IRQLIST_16(8), IRQLIST_16(9), IRQLIST_16(a), IRQLIST_16(b),
-    IRQLIST_16(c), IRQLIST_16(d), IRQLIST_16(e), IRQLIST_16(f)
-};
-
-#undef IRQ
-#undef IRQLIST_16
-
-/*
  * This is the 'legacy' 8259A Programmable Interrupt Controller,
  * present in the majority of PC/AT boxes.
  * plus some generic x86 specific things if generic specifics makes
@@ -395,7 +336,7 @@ static struct irqaction __read_mostly cascade = { no_action, "cascade", NULL};
 
 void __init init_IRQ(void)
 {
-    int vector, irq, cpu = smp_processor_id();
+    int irq, cpu = smp_processor_id();
 
     init_bsp_APIC();
 
@@ -403,14 +344,6 @@ void __init init_IRQ(void)
 
     BUG_ON(init_irq_data() < 0);
 
-    for ( vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++ )
-    {
-        if (vector == HYPERCALL_VECTOR || vector == LEGACY_SYSCALL_VECTOR)
-            continue;
-        BUG_ON(!interrupt[vector]);
-        set_intr_gate(vector, interrupt[vector]);
-    }
-
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 41a9d77..4dfcf4f 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -383,7 +383,7 @@ static const char *trapstr(unsigned int trapnr)
         "coprocessor segment", "invalid tss", "segment not found",
         "stack error", "general protection fault", "page fault",
         "spurious interrupt", "coprocessor error", "alignment check",
-        "machine check", "simd error"
+        "machine check", "simd error", "virtualisation error"
     };
 
     return trapnr < ARRAY_SIZE(strings) ? strings[trapnr] : "???";
@@ -534,6 +534,15 @@ int set_guest_nmi_trapbounce(void)
     return !null_trap_bounce(v, tb);
 }
 
+void do_reserved_exception(struct cpu_user_regs *regs)
+{
+    unsigned int trapnr = regs->entry_vector;
+
+    DEBUGGER_trap_fatal(trapnr, regs);
+    show_execution_state(regs);
+    panic("FATAL RESERVED TRAP: vector = %d (%s)", trapnr, trapstr(trapnr));
+}
+
 static void do_trap(struct cpu_user_regs *regs, int use_error_code)
 {
     struct vcpu *curr = current;
@@ -589,7 +598,6 @@ void do_##name(struct cpu_user_regs *regs)              \
 DO_ERROR_NOCODE(divide_error)
 DO_ERROR_NOCODE(overflow)
 DO_ERROR_NOCODE(bounds)
-DO_ERROR_NOCODE(coprocessor_segment_overrun)
 DO_ERROR(       invalid_TSS)
 DO_ERROR(       segment_not_present)
 DO_ERROR(       stack_segment)
@@ -3414,10 +3422,6 @@ void do_debug(struct cpu_user_regs *regs)
     return;
 }
 
-void do_spurious_interrupt_bug(struct cpu_user_regs *regs)
-{
-}
-
 static void __set_intr_gate(unsigned int n, uint32_t dpl, void *addr)
 {
     int i;
@@ -3516,13 +3520,11 @@ void __init init_idt_traps(void)
     set_intr_gate(TRAP_invalid_op,&invalid_op);
     set_intr_gate(TRAP_no_device,&device_not_available);
     set_intr_gate(TRAP_double_fault, &double_fault);
-    set_intr_gate(TRAP_copro_seg,&coprocessor_segment_overrun);
     set_intr_gate(TRAP_invalid_tss,&invalid_TSS);
     set_intr_gate(TRAP_no_segment,&segment_not_present);
     set_intr_gate(TRAP_stack_error,&stack_segment);
     set_intr_gate(TRAP_gp_fault,&general_protection);
     set_intr_gate(TRAP_page_fault,&early_page_fault);
-    set_intr_gate(TRAP_spurious_int,&spurious_interrupt_bug);
     set_intr_gate(TRAP_copro_error,&coprocessor_error);
     set_intr_gate(TRAP_alignment_check,&alignment_check);
     set_intr_gate(TRAP_machine_check,&machine_check);
@@ -3540,8 +3542,12 @@ void __init init_idt_traps(void)
     this_cpu(compat_gdt_table) = boot_cpu_compat_gdt_table;
 }
 
+extern void (*__initconst autogen_entrypoints[NR_VECTORS])(void);
 void __init trap_init(void)
 {
+    unsigned int vector;
+
+    /* Replace early pagefault with real pagefault handler */
     set_intr_gate(TRAP_page_fault, &page_fault);
 
     /* The 32-on-64 hypercall entry vector is only accessible from ring 1. */
@@ -3550,6 +3556,23 @@ void __init trap_init(void)
     /* Fast trap for int80 (faster than taking the #GP-fixup path). */
     _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
 
+    for ( vector = 0; vector < NR_VECTORS; vector++ )
+    {
+        if ( autogen_entrypoints[vector] )
+        {
+            /* Found autogenerated entry point - confirm we won't clobber an
+             * existing trap. */
+            ASSERT(idt_table[vector].b == 0);
+            set_intr_gate(vector, autogen_entrypoints[vector]);
+        }
+        else
+        {
+            /* No autogenerated entry point - confirm we have an existing
+             * trap in place. */
+            ASSERT(idt_table[vector].b != 0);
+        }
+    }
+
     percpu_traps_init();
 
     cpu_init();
diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
index 7f8fb3d..4b23d89 100644
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -1,6 +1,7 @@
 subdir-y += compat
 
 obj-bin-y += entry.o
+obj-bin-y += irqgen.o
 obj-bin-y += gpr_switch.o
 obj-y += mm.o
 obj-y += traps.o
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 4796e65..1bd4ee4 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -475,6 +475,12 @@ ENTRY(common_interrupt)
         callq do_IRQ
         jmp ret_from_intr
 
+ENTRY(reserved_exception)
+        SAVE_ALL CLAC
+        movq %rsp,%rdi
+        callq do_reserved_exception
+        jmp ret_from_intr
+
 /* No special register assumptions. */
 ENTRY(ret_from_intr)
         GET_CURRENT(%rbx)
@@ -586,11 +592,6 @@ ENTRY(invalid_op)
         movl  $TRAP_invalid_op,4(%rsp)
         jmp   handle_exception
 
-ENTRY(coprocessor_segment_overrun)
-        pushq $0
-        movl  $TRAP_copro_seg,4(%rsp)
-        jmp   handle_exception
-
 ENTRY(invalid_TSS)
         movl  $TRAP_invalid_tss,4(%rsp)
         jmp   handle_exception
@@ -611,11 +612,6 @@ ENTRY(alignment_check)
         movl  $TRAP_alignment_check,4(%rsp)
         jmp   handle_exception
 
-ENTRY(spurious_interrupt_bug)
-        pushq $0
-        movl  $TRAP_spurious_int,4(%rsp)
-        jmp   handle_exception
-
 ENTRY(double_fault)
         movl  $TRAP_double_fault,4(%rsp)
         /* Set AC to reduce chance of further SMAP faults */
@@ -716,14 +712,14 @@ ENTRY(exception_table)
         .quad do_bounds
         .quad do_invalid_op
         .quad do_device_not_available
-        .quad 0 # double_fault
-        .quad do_coprocessor_segment_overrun
+        .quad BAD_VIRT_ADDR # double_fault
+        .quad BAD_VIRT_ADDR # coproc_seg_overrun, reserved
         .quad do_invalid_TSS
         .quad do_segment_not_present
         .quad do_stack_segment
         .quad do_general_protection
         .quad do_page_fault
-        .quad do_spurious_interrupt_bug
+        .quad BAD_VIRT_ADDR # IRQ7 spurious, reserved
         .quad do_coprocessor_error
         .quad do_alignment_check
         .quad do_machine_check
diff --git a/xen/arch/x86/x86_64/irqgen.S b/xen/arch/x86/x86_64/irqgen.S
new file mode 100644
index 0000000..6389b4e
--- /dev/null
+++ b/xen/arch/x86/x86_64/irqgen.S
@@ -0,0 +1,72 @@
+/* Automatically generated interrupt entry points */
+
+#include <xen/config.h>
+#include <asm/asm_defns.h>
+#include <irq_vectors.h>
+
+.altmacro
+
+/* Make a symbol by evaluating num for a real number */
+.macro mksym name, num
+        __mksym \name, %num
+.endm
+.macro __mksym, name, num
+        \name\num:
+.endm
+
+/* Equate a symbol with 0 by evaluating num for a real number */
+.macro nullsym name, num
+        __nullsym \name, %num
+.endm
+.macro __nullsym, name, num
+        .equ \name\num\(), 0
+.endm
+
+/* Create a .quad of a symbol evaluating num for a real number */
+.macro mkquad name, num
+        __mkquad \name, %num
+.endm
+.macro __mkquad name, num
+        .quad \name\num
+.endm
+
+/* Automatically generate stub entry points */
+vec = 0
+.rept NR_VECTORS
+    ALIGN
+
+    /* Common interrupts, heading towards do_IRQ() */
+    .if vec >= FIRST_DYNAMIC_VECTOR && vec != HYPERCALL_VECTOR && vec != LEGACY_SYSCALL_VECTOR
+        mksym autogen_vec_, vec
+        pushq $0
+        movb  $(vec),4(%rsp)
+        jmp common_interrupt
+
+    /* Reserved exceptions, heading towards do_reserved_exception() */
+    .elseif vec == TRAP_copro_seg || vec == TRAP_spurious_int || (vec >  TRAP_simd_error && vec <= TRAP_last_reserved)
+        mksym autogen_vec_, vec
+        pushq $0
+        movb  $(vec),4(%rsp)
+        jmp reserved_exception
+
+    /* Hand generated in entry.S */
+    .else
+        nullsym autogen_vec_, vec
+    .endif
+
+vec = vec + 1
+.endr
+
+
+.section ".init.rodata", "a", @progbits
+
+/* Entry points of automatically generated subs from above */
+GLOBAL(autogen_entrypoints)
+        vec = 0
+
+        .rept NR_VECTORS
+        mkquad autogen_vec_, vec
+        vec = vec + 1
+        .endr
+
+        .size autogen_entrypoints, . - autogen_entrypoints
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index df4873b..87a462f 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -357,9 +357,4 @@ static inline void stac(void)
 #define REX64_PREFIX "rex64/"
 #endif
 
-#define BUILD_IRQ(nr)                           \
-    "pushq $0\n\t"                              \
-    "movl $"#nr",4(%rsp)\n\t"                   \
-    "jmp common_interrupt"
-
 #endif /* __X86_ASM_DEFNS_H__ */
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 02ab1fc..5df2fe2 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -106,6 +106,7 @@
 
 /* Return value for zero-size _xmalloc(), distinguished from NULL. */
 #define ZERO_BLOCK_PTR ((void *)0xBAD0BAD0BAD0BAD0UL)
+#define BAD_VIRT_ADDR  _AC(0x8000000000000000,UL)
 
 #ifndef __ASSEMBLY__
 extern unsigned long trampoline_phys;
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index c9051be..13ee755 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -113,6 +113,7 @@
 #define TRAP_alignment_check  17
 #define TRAP_machine_check    18
 #define TRAP_simd_error       19
+#define TRAP_virt_error       20
 #define TRAP_last_reserved    31
 
 /* Set for entry via SYSCALL. Informs return code to use SYSRETQ not IRETQ. */
@@ -501,7 +502,6 @@ DECLARE_TRAP_HANDLER(bounds);
 DECLARE_TRAP_HANDLER(invalid_op);
 DECLARE_TRAP_HANDLER(device_not_available);
 DECLARE_TRAP_HANDLER(double_fault);
-DECLARE_TRAP_HANDLER(coprocessor_segment_overrun);
 DECLARE_TRAP_HANDLER(invalid_TSS);
 DECLARE_TRAP_HANDLER(segment_not_present);
 DECLARE_TRAP_HANDLER(stack_segment);
@@ -512,12 +512,12 @@ DECLARE_TRAP_HANDLER(coprocessor_error);
 DECLARE_TRAP_HANDLER(simd_coprocessor_error);
 DECLARE_TRAP_HANDLER(machine_check);
 DECLARE_TRAP_HANDLER(alignment_check);
-DECLARE_TRAP_HANDLER(spurious_interrupt_bug);
 #undef DECLARE_TRAP_HANDLER
 
 void trap_nop(void);
 void enable_nmis(void);
 void noreturn do_nmi_crash(struct cpu_user_regs *regs);
+void do_reserved_exception(struct cpu_user_regs *regs);
 
 void syscall_enter(void);
 void sysenter_entry(void);
-- 
1.7.10.4

Re: [PATCH RFC 8/9] x86/irqs: Move interrupt-stub generation out of C

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -383,7 +383,7 @@ static const char *trapstr(unsigned int trapnr)
>          "coprocessor segment", "invalid tss", "segment not found",
>          "stack error", "general protection fault", "page fault",
>          "spurious interrupt", "coprocessor error", "alignment check",
> -        "machine check", "simd error"
> +        "machine check", "simd error", "virtualisation error"

"virtualisation exception" please.

> @@ -534,6 +534,15 @@ int set_guest_nmi_trapbounce(void)
>      return !null_trap_bounce(v, tb);
>  }
>  
> +void do_reserved_exception(struct cpu_user_regs *regs)
> +{
> +    unsigned int trapnr = regs->entry_vector;
> +
> +    DEBUGGER_trap_fatal(trapnr, regs);
> +    show_execution_state(regs);
> +    panic("FATAL RESERVED TRAP: vector = %d (%s)", trapnr, trapstr(trapnr));

I think I would much prefer vectors always getting printed in hex. If
you want to keep this decimal, please make it e.g.

"FATAL RESERVED TRAP #%d: %s"

> @@ -3550,6 +3556,23 @@ void __init trap_init(void)
>      /* Fast trap for int80 (faster than taking the #GP-fixup path). */
>      _set_gate(idt_table+0x80, DESC_TYPE_trap_gate, 3, &int80_direct_trap);
>  
> +    for ( vector = 0; vector < NR_VECTORS; vector++ )
> +    {
> +        if ( autogen_entrypoints[vector] )
> +        {
> +            /* Found autogenerated entry point - confirm we won't clobber an
> +             * existing trap. */
> +            ASSERT(idt_table[vector].b == 0);
> +            set_intr_gate(vector, autogen_entrypoints[vector]);
> +        }
> +        else
> +        {
> +            /* No autogenerated entry point - confirm we have an existing
> +             * trap in place. */
> +            ASSERT(idt_table[vector].b != 0);
> +        }
> +    }

Interesting approach. The only downside is that adding proper support
for a vector will then also need to touch the auto-generation logic. But
since that ought to be rare, I suppose that's fine.

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -475,6 +475,12 @@ ENTRY(common_interrupt)
>          callq do_IRQ
>          jmp ret_from_intr
>  
> +ENTRY(reserved_exception)
> +        SAVE_ALL CLAC
> +        movq %rsp,%rdi
> +        callq do_reserved_exception
> +        jmp ret_from_intr
> +

Shouldn't you account for an error code having got pushed by
hardware?

> @@ -716,14 +712,14 @@ ENTRY(exception_table)
>          .quad do_bounds
>          .quad do_invalid_op
>          .quad do_device_not_available
> -        .quad 0 # double_fault
> -        .quad do_coprocessor_segment_overrun
> +        .quad BAD_VIRT_ADDR # double_fault
> +        .quad BAD_VIRT_ADDR # coproc_seg_overrun, reserved
>          .quad do_invalid_TSS
>          .quad do_segment_not_present
>          .quad do_stack_segment
>          .quad do_general_protection
>          .quad do_page_fault
> -        .quad do_spurious_interrupt_bug
> +        .quad BAD_VIRT_ADDR # IRQ7 spurious, reserved

IRQ 7 alone is rather confusing, as we don't put IRQ 7 here. Either
say "PIC default", or name it vector 15.

> --- /dev/null
> +++ b/xen/arch/x86/x86_64/irqgen.S
> @@ -0,0 +1,72 @@
> +/* Automatically generated interrupt entry points */
> +
> +#include <xen/config.h>
> +#include <asm/asm_defns.h>
> +#include <irq_vectors.h>
> +
> +.altmacro
> +
> +/* Make a symbol by evaluating num for a real number */
> +.macro mksym name, num
> +        __mksym \name, %num
> +.endm
> +.macro __mksym, name, num
> +        \name\num:
> +.endm
> +
> +/* Equate a symbol with 0 by evaluating num for a real number */
> +.macro nullsym name, num
> +        __nullsym \name, %num
> +.endm
> +.macro __nullsym, name, num
> +        .equ \name\num\(), 0
> +.endm
> +
> +/* Create a .quad of a symbol evaluating num for a real number */
> +.macro mkquad name, num
> +        __mkquad \name, %num
> +.endm
> +.macro __mkquad name, num
> +        .quad \name\num
> +.endm
> +
> +/* Automatically generate stub entry points */
> +vec = 0
> +.rept NR_VECTORS
> +    ALIGN
> +
> +    /* Common interrupts, heading towards do_IRQ() */
> +    .if vec >= FIRST_DYNAMIC_VECTOR && vec != HYPERCALL_VECTOR && vec != LEGACY_SYSCALL_VECTOR
> +        mksym autogen_vec_, vec
> +        pushq $0
> +        movb  $(vec),4(%rsp)
> +        jmp common_interrupt
> +
> +    /* Reserved exceptions, heading towards do_reserved_exception() */
> +    .elseif vec == TRAP_copro_seg || vec == TRAP_spurious_int || (vec >  TRAP_simd_error && vec <= TRAP_last_reserved)

Double blank.

> +        mksym autogen_vec_, vec
> +        pushq $0
> +        movb  $(vec),4(%rsp)
> +        jmp reserved_exception
> +
> +    /* Hand generated in entry.S */
> +    .else
> +        nullsym autogen_vec_, vec
> +    .endif
> +
> +vec = vec + 1
> +.endr
> +
> +
> +.section ".init.rodata", "a", @progbits
> +
> +/* Entry points of automatically generated subs from above */
> +GLOBAL(autogen_entrypoints)
> +        vec = 0
> +
> +        .rept NR_VECTORS
> +        mkquad autogen_vec_, vec
> +        vec = vec + 1
> +        .endr
> +
> +        .size autogen_entrypoints, . - autogen_entrypoints

Please settle on consistent indentation of directives.

And no, I can't see ways around the % and extra macro level.

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -113,6 +113,7 @@
>  #define TRAP_alignment_check  17
>  #define TRAP_machine_check    18
>  #define TRAP_simd_error       19
> +#define TRAP_virt_error       20

And this could perhaps be TRAP_virtualisation.

Jan

[PATCH RFC 9/9] x86/misc: Post cleanup

[Andrew Cooper]

* panic() now works on early boot.  Replace EARLY_FAIL()
* idt_table is 4096 bytes.  Page align it.
* Cleanup __set_intr_gate() & friends.  The master IDT is fully constructed on
  early boot, and only subsequently altered on the crash path.  Make them
  private to traps.c, move them into .init, and remove the loop over all idts,
  as __set_intr_gate() will never find an AP to patch. (For some reason,
  leaving out the noinline causes ~1.5k of code bloat from GCC inlining
  everything)

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/setup.c       |   20 ++++++--------------
 xen/arch/x86/traps.c       |   13 ++++---------
 xen/include/asm-x86/desc.h |    1 -
 3 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ef5863f..1c0d1c7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -134,11 +134,6 @@ static void __init parse_acpi_param(char *s)
     }
 }
 
-#define EARLY_FAIL(f, a...) do {                \
-    printk( f , ## a );                         \
-    for ( ; ; ) halt();                         \
-} while (0)
-
 static const module_t *__initdata initial_images;
 static unsigned int __initdata nr_initial_images;
 
@@ -671,11 +666,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     /* Check that we have at least one Multiboot module. */
     if ( !(mbi->flags & MBI_MODULES) || (mbi->mods_count == 0) )
-        EARLY_FAIL("dom0 kernel not specified. "
-                   "Check bootloader configuration.\n");
+        panic("dom0 kernel not specified. Check bootloader configuration.");
 
     if ( ((unsigned long)cpu0_stack & (STACK_SIZE-1)) != 0 )
-        EARLY_FAIL("Misaligned CPU0 stack.\n");
+        panic("Misaligned CPU0 stack.");
 
     if ( efi_enabled )
     {
@@ -756,9 +750,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         e820_raw_nr = 2;
     }
     else
-    {
-        EARLY_FAIL("Bootloader provided no memory information.\n");
-    }
+        panic("Bootloader provided no memory information.");
 
     /* Sanitise the raw E820 map to produce a final clean version. */
     max_page = raw_max_page = init_e820(memmap_type, e820_raw, &e820_raw_nr);
@@ -793,7 +785,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     for ( i = 0; !efi_enabled && i < mbi->mods_count; i++ )
     {
         if ( mod[i].mod_start & (PAGE_SIZE - 1) )
-            EARLY_FAIL("Bootloader didn't honor module alignment request.\n");
+            panic("Bootloader didn't honor module alignment request.");
         mod[i].mod_end -= mod[i].mod_start;
         mod[i].mod_start >>= PAGE_SHIFT;
         mod[i].reserved = 0;
@@ -966,7 +958,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
 
     if ( modules_headroom && !mod->reserved )
-        EARLY_FAIL("Not enough memory to relocate the dom0 kernel image.\n");
+        panic("Not enough memory to relocate the dom0 kernel image.");
     for ( i = 0; i < mbi->mods_count; ++i )
     {
         uint64_t s = (uint64_t)mod[i].mod_start << PAGE_SHIFT;
@@ -975,7 +967,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
 
     if ( !xen_phys_start )
-        EARLY_FAIL("Not enough memory to relocate Xen.\n");
+        panic("Not enough memory to relocate Xen.");
     reserve_e820_ram(&boot_e820, efi_enabled ? mbi->mem_upper : __pa(&_start),
                      __pa(&_end));
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4dfcf4f..d16d3e7 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -97,7 +97,7 @@ DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, gdt_table);
 DEFINE_PER_CPU_READ_MOSTLY(struct desc_struct *, compat_gdt_table);
 
 /* Master table, used by CPU0. */
-idt_entry_t idt_table[IDT_ENTRIES];
+idt_entry_t idt_table[IDT_ENTRIES] __attribute__ ((__section__(".bss.page_aligned")));
 
 /* Pointer to the IDT of every CPU. */
 idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
@@ -3422,22 +3422,17 @@ void do_debug(struct cpu_user_regs *regs)
     return;
 }
 
-static void __set_intr_gate(unsigned int n, uint32_t dpl, void *addr)
+static void __init noinline __set_intr_gate(unsigned int n, uint32_t dpl, void *addr)
 {
-    int i;
-    /* Keep secondary tables in sync with IRQ updates. */
-    for ( i = 1; i < nr_cpu_ids; i++ )
-        if ( idt_tables[i] != NULL )
-            _set_gate(&idt_tables[i][n], DESC_TYPE_irq_gate, dpl, addr);
     _set_gate(&idt_table[n], DESC_TYPE_irq_gate, dpl, addr);
 }
 
-static void set_swint_gate(unsigned int n, void *addr)
+static void __init set_swint_gate(unsigned int n, void *addr)
 {
     __set_intr_gate(n, 3, addr);
 }
 
-void set_intr_gate(unsigned int n, void *addr)
+static void __init set_intr_gate(unsigned int n, void *addr)
 {
     __set_intr_gate(n, 0, addr);
 }
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 6b2094f..f11f920 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -199,7 +199,6 @@ DECLARE_PER_CPU(struct desc_struct *, gdt_table);
 extern struct desc_struct boot_cpu_compat_gdt_table[];
 DECLARE_PER_CPU(struct desc_struct *, compat_gdt_table);
 
-extern void set_intr_gate(unsigned int irq, void * addr);
 extern void load_TR(void);
 
 #endif /* !__ASSEMBLY__ */
-- 
1.7.10.4

Re: [PATCH RFC 9/9] x86/misc: Post cleanup

[Jan Beulich]

>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
> * panic() now works on early boot.  Replace EARLY_FAIL()

Very good.

> * idt_table is 4096 bytes.  Page align it.

Size and alignment have nothing to do with one another here; I see no
reason for aligning the table on more than a 16-byte boundary.

Jan

Re: [PATCH RFC 9/9] x86/misc: Post cleanup

[Andrew Cooper]

On 15/05/14 14:14, Jan Beulich wrote:
>>>> On 15.05.14 at 11:48, <andrew.cooper3@citrix.com> wrote:
>> * panic() now works on early boot.  Replace EARLY_FAIL()
> Very good.
>
>> * idt_table is 4096 bytes.  Page align it.
> Size and alignment have nothing to do with one another here; I see no
> reason for aligning the table on more than a 16-byte boundary.
>
> Jan
>

I was considering getting it in a single TLB entry, but on second
thoughts this is currently in the bss, so will be covered by the Xen
mappings.  I will drop it.

~Andrew

Re: [PATCH RFC 0/9] x86: Improvements to trap handling

[Wu, Feng]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: Thursday, May 15, 2014 5:48 PM
> To: Xen-devel
> Cc: Andrew Cooper; Wu, Feng; Keir Fraser; Jan Beulich; Tim Deegan
> Subject: [Xen-devel] [PATCH RFC 0/9] x86: Improvements to trap handling
> 
> This is a large change to trap handling.  Its underlying purpose is to avoid
> the current situation where ignore_int() in the .init section remains patched
> into each IDT in the reserved exception vectors.
> 
> As a side effect, Xen gains full bugframe and exception_table support from the
> beginning of __start_xen.
> 
> This is far from comprehensivly tested, but has been tested with bugframes
> and
> extable redirects from right after setting up the console (so printing works),
> along with the panic() and reboot paths.
> 
> I notice that Feng Wu has submitted a patch moving the irq-stub generation,
> and it is completely by chance that I have functionally similar patch in this
> series.

Andrew, I just notice your patch, just go ahead with yours, I will go with
other tasks. :) 

Thanks,
Feng

> 
> Andrew Cooper (9):
>   x86/traps: Names for system descriptor types
>   x86/traps: Make panic and reboot paths safe during early boot
>   x86/traps: Make the main trap handlers safe for use early during Xen boot
>   x86/misc: Early cleanup
>   x86/traps: Functional prep work
>   x86/boot: Install trap handlers much earlier on boot
>   x86/boot: Drop pre-C IDT patching
>   x86/irqs: Move interrupt-stub generation out of C
>   x86/misc: Post cleanup
> 
>  xen/arch/x86/boot/x86_64.S      |   56 +-----------------
>  xen/arch/x86/cpu/common.c       |   70 +++++++++++++++++-----
>  xen/arch/x86/cpu/mcheck/mce.c   |    5 +-
>  xen/arch/x86/crash.c            |    2 +-
>  xen/arch/x86/i8259.c            |   69 +---------------------
>  xen/arch/x86/mm.c               |   10 ++--
>  xen/arch/x86/setup.c            |   42 ++++++-------
>  xen/arch/x86/shutdown.c         |   42 +++++++------
>  xen/arch/x86/smpboot.c          |   21 ++-----
>  xen/arch/x86/traps.c            |  124
> ++++++++++++++++++++++++---------------
>  xen/arch/x86/x86_64/Makefile    |    1 +
>  xen/arch/x86/x86_64/entry.S     |   22 +++----
>  xen/arch/x86/x86_64/irqgen.S    |   72 +++++++++++++++++++++++
>  xen/arch/x86/x86_64/traps.c     |   31 +---------
>  xen/common/symbols.c            |    2 +-
>  xen/drivers/char/console.c      |    5 --
>  xen/include/asm-x86/asm_defns.h |    5 --
>  xen/include/asm-x86/config.h    |    1 +
>  xen/include/asm-x86/desc.h      |    9 ++-
>  xen/include/asm-x86/ldt.h       |    2 +-
>  xen/include/asm-x86/processor.h |    6 +-
>  xen/include/asm-x86/setup.h     |    1 -
>  xen/include/asm-x86/system.h    |    1 +
>  xen/include/xen/kernel.h        |    1 +
>  xen/include/xen/sched.h         |    1 +
>  25 files changed, 295 insertions(+), 306 deletions(-)
>  create mode 100644 xen/arch/x86/x86_64/irqgen.S
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel