Re: [PATCH v8 04/33] x86/traps: add install_system_interrupt_handler()

[Thomas Gleixner]

On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> Some kernel components install system interrupt handlers into the IDT,
> and we need to do the same for system_interrupt_handlers.

We need to the same? This sentence does not make any sense at all.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

> +void install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr);

Why is this void *?

>  
> +void __init install_system_interrupt_handler(unsigned int n, const void *asm_addr, const void *addr)
> +{
> +	BUG_ON(n < FIRST_SYSTEM_VECTOR);
> +
> +#ifdef CONFIG_X86_64
> +	system_interrupt_handlers[n - FIRST_SYSTEM_VECTOR] = (system_interrupt_handler)addr;

Just that you can add a silly typecast here, right?

Oh well.

> -	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, asm_sysvec_xen_hvm_callback);
> +	install_system_interrupt_handler(HYPERVISOR_CALLBACK_VECTOR,
> +					 asm_sysvec_xen_hvm_callback,
> +					 sysvec_xen_hvm_callback);

Can we please make this less convoluted?

#ifdef CONFIG_X86_64
static inline void sysvec_setup_fred(unsigned int vector, void (*func)(struct pt_regs*))
{
        ...
}
#else
static inline void sysvec_setup_fred(unsigned int vector, void (*func)(struct pt_regs*))
{
}
#endif

#define sysvec_install(vector, func) {                                  \
        sysvec_setup_fred(vector, func);                                \
        alloc_intr_gate(vector, asm_##func);                            \
}

-	alloc_intr_gate(HYPERVISOR_CALLBACK_VECTOR, asm_sysvec_xen_hvm_callback);
+	sysvec_install(HYPERVISOR_CALLBACK_VECTOR, sysvec_xen_hvm_callback);

No?

Thanks,

        tglx