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

[Jan Beulich <jbeulich@suse.com>]

On 22.11.2021 17:16, Andrew Cooper wrote:
> 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.

#MF and #XM certainly aren't. For some of the others I agree with
"by default", but without extending this to "in general".

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

The 15 things are: There may not be a new exception type vectored here
without updating the dispatch code. Failing to do so is quite fine to
be caught by the BUG alone. And adding new IST handlers requires care
in a number of other places anyway, so anyone doing so will surely
grep for all occurrences of the types presently handled here. That'll
make them spot the dispatch which needs updating alongside the several
other places.

Jan