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

[Andrew Cooper <amc96@xxxxxxxx>]

On 22/11/2021 09:04, Jan Beulich wrote:
> 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().

Why?

Debugging might be rare, but #BP gets used in non-error cases. 
Everything heading towards do_trap() really got 0 hits, which is
entirely expected because they're all fatal signals by default.

This list is in proper frequency order.

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

I firmly disagree.

do_unhandled_trap() is a fatal error path both here and for the non IST
case, and is absolutely the appropriate thing to call in the dangling
else from this chain.

It is only unreachable if 15 things line up correctly in a very fragile
piece of code, where both you and I have made errors in the past.

~Andrew