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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.14 at 11:48, <andrew.cooper3@xxxxxxxxxx> 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

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