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
