Re: [PATCH 4/5] x86/traps: Drop exception_table[] and use if/else dispatching

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2021 19:21, Andrew Cooper wrote:
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -773,14 +773,48 @@ handle_exception_saved:
>          sti
>  1:      movq  %rsp,%rdi
>          movzbl UREGS_entry_vector(%rsp),%eax
> -        leaq  exception_table(%rip),%rdx
>  #ifdef CONFIG_PERF_COUNTERS
>          lea   per_cpu__perfcounters(%rip), %rcx
>          add   STACK_CPUINFO_FIELD(per_cpu_offset)(%r14), %rcx
>          incl  ASM_PERFC_exceptions * 4(%rcx, %rax, 4)
>  #endif
> -        mov   (%rdx, %rax, 8), %rdx
> -        INDIRECT_CALL %rdx
> +
> +        /*
> +         * Dispatch to appropriate C handlers.
> +         *
> +         * The logic is implemented as an if/else chain.  DISPATCH() calls
> +         * need be in frequency order for best performance.
> +         */
> +#define DISPATCH(vec, handler)         \
> +        cmp   $vec, %al;               \
> +        jne   .L_ ## vec ## _done;     \
> +        call  handler;                 \
> +        jmp   .L_exn_dispatch_done;    \
> +.L_ ## vec ## _done:
> +
> +        DISPATCH(X86_EXC_PF, do_page_fault)
> +        DISPATCH(X86_EXC_GP, do_general_protection)
> +        DISPATCH(X86_EXC_UD, do_invalid_op)
> +        DISPATCH(X86_EXC_NM, do_device_not_available)
> +        DISPATCH(X86_EXC_BP, do_int3)
> +
> +        /* Logically "if ( (1 << vec) & MASK ) { do_trap(); }" */
> +        mov   $(1 << X86_EXC_DE) | (1 << X86_EXC_OF) | (1 << X86_EXC_BR) |\
> +               (1 << X86_EXC_NP) | (1 << X86_EXC_SS) | (1 << X86_EXC_MF) |\
> +               (1 << X86_EXC_AC) | (1 << X86_EXC_XM), %edx
> +        bt    %eax, %edx
> +        jnc   .L_do_trap_done
> +        call  do_trap
> +        jmp   .L_exn_dispatch_done
> +.L_do_trap_done:
> +
> +        DISPATCH(X86_EXC_CP, do_entry_CP)
> +#undef DISPATCH

The basis for the choice of ordering imo wants spelling out here. For example,
despite the data provided in the description I'm not really convinced #BP
wants handling ahead of everything going to do_trap().

> @@ -1012,9 +1046,28 @@ handle_ist_exception:
>          incl  ASM_PERFC_exceptions * 4(%rcx, %rax, 4)
>  #endif
>  
> -        leaq  exception_table(%rip),%rdx
> -        mov   (%rdx, %rax, 8), %rdx
> -        INDIRECT_CALL %rdx
> +        /*
> +         * Dispatch to appropriate C handlers.
> +         *
> +         * The logic is implemented as an if/else chain.  DISPATCH() calls
> +         * need be in frequency order for best performance.
> +         */
> +#define DISPATCH(vec, handler)         \
> +        cmp   $vec, %al;               \
> +        jne   .L_ ## vec ## _done;     \
> +        call  handler;                 \
> +        jmp   .L_ist_dispatch_done;    \
> +.L_ ## vec ## _done:
> +
> +        DISPATCH(X86_EXC_NMI, do_nmi)
> +        DISPATCH(X86_EXC_DB,  do_debug)
> +        DISPATCH(X86_EXC_MC,  do_machine_check)
> +#undef DISPATCH
> +
> +        call  do_unhandled_trap
> +        BUG   /* do_unhandled_trap() shouldn't return. */

While I agree with putting BUG here, I don't see the need for the CALL.
Unlike in handle_exception there's no vector left unhandled by the
DISPATCH() invocations. The CALL being there give the (wrong) impression
there would / might be.

Jan