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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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