Re: [PATCH 2/3] x86/entry: Rework the exception entrypoints

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2023 12:59, Andrew Cooper wrote:
> This fixes two issues preventing livepatching.  First, that #PF and NMI fall
> through into other functions,

I thought this was deliberate, aiming at avoiding the unconditional branch
for the most commonly taken path each. I'm not really opposed to the change,
but I think it wants saying a little more as to how little (or big) of an
effect this has (or at least is expected to have).

> and second to add ELF metadata.
> 
> Use a macro to generate the entrypoints programatically, rather than
> opencoding them all.  Switch to using the architectural short names.
> 
> Remove the DECLARE_TRAP_HANDLER{,_CONST}() infrastructure.  Only NMI/#MC are
> referenced externally (and NMI will cease to be soon, as part of adding FRED
> support).  Move the entrypoint declarations into the respective traps.c where
> they're used, rather than keeping them visible across ~all of Xen.

What about Misra? Won't they be unhappy about global functions with no
declaration in any header?

> Drop the long-stale comment at the top of init_idt_traps().  It's mostly
> discussing a 32bit Xen.

The use of interrupt gates isn't 32-bit only, and justifying why trap gates
aren't used looks to me like quite reasonable a purpose of that comment.

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -142,6 +142,50 @@ process_trap:
>  
>          .section .text.entry, "ax", @progbits
>  
> +.macro IDT_ENTRY vec label handler

As said in reply to another recent patch: Could we agree on separating
macro parameters by commas? I also wonder whether they shouldn't all have
":req" tagged onto them, as none of them is optional.

> +ENTRY(\label)
> +        ENDBR64
> +     .if ((1 << \vec) & X86_EXC_HAVE_EC) == 0

Nit: Hard tab slipped in here.

> +            push $0
> +        .endif
> +        movl  $\vec, 4(%rsp)
> +        jmp   \handler
> +
> +        .type \label, @function
> +        .size \label, . - \label
> +.endm
> +
> +.macro ENTRY vec label
> +        IDT_ENTRY \vec \label handle_exception
> +.endm
> +.macro ENTRY_IST vec label
> +        IDT_ENTRY \vec \label handle_ist_exception
> +.endm
> +
> +
> +ENTRY     X86_EXC_DE  entry_DE  /* 00 Divide Error */
> +ENTRY_IST X86_EXC_DB  entry_DB  /* 01 Debug Exception */
> +ENTRY_IST X86_EXC_NMI entry_NMI /* 02 Non-Maskable Interrupt */
> +ENTRY     X86_EXC_BP  entry_BP  /* 03 Breakpoint (int3) */
> +ENTRY     X86_EXC_OF  entry_OF  /* 04 Overflow (into) */
> +ENTRY     X86_EXC_BR  entry_BR  /* 05 Bound Range */
> +ENTRY     X86_EXC_UD  entry_UD  /* 06 Undefined Opcode */
> +ENTRY     X86_EXC_NM  entry_NM  /* 07 No Maths (Device Not Present) */
> +/*   _IST X86_EXC_DF  entry_DF     08 Double Fault - Handled specially */
> +/*        X86_EXC_CSO entry_CSO    09 Coprocessor Segment Override - Autogen 
> */
> +ENTRY     X86_EXC_TS  entry_TS  /* 10 Invalid TSS */
> +ENTRY     X86_EXC_NP  entry_NP  /* 11 Segment Not Present */
> +ENTRY     X86_EXC_SS  entry_SS  /* 12 Stack Segment Fault */
> +ENTRY     X86_EXC_GP  entry_GP  /* 13 General Protection Fault */
> +ENTRY     X86_EXC_PF  entry_PF  /* 14 Page Fault */
> +/*        X86_EXC_SPV entry_SPV    15 PIC Spurious Interrupt Vector - 
> Autogen */
> +ENTRY     X86_EXC_MF  entry_MF  /* 16 Maths Fault (x87 FPU) */
> +ENTRY     X86_EXC_AC  entry_AC  /* 17 Alignment Check */
> +ENTRY_IST X86_EXC_MC  entry_MC  /* 18 Machine Check */
> +ENTRY     X86_EXC_XM  entry_XM  /* 19 SIMD Maths Fault */
> +/*        X86_EXC_VE  entry_VE     20 Virtualisation Exception - Not 
> implemented */
> +ENTRY     X86_EXC_CP  entry_CP  /* 21 Control-flow Protection */

I expect you won't like the request, but still: There's a lot of redundancy
here. The first two arguments could all be folded, having the macro attach
the X86_EXC_ and entry_ prefixes. Or wait - only quite, as long as X86_EXC_*
are C macros rather than assembler equates.

Jan