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
