Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Xin Li <xin3.li@intel.com>, linux-kernel@vger.kernel.org,
        x86@kernel.org, kvm@vger.kernel.org
Cc:     mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, peterz@infradead.org, andrew.cooper3@citrix.com,
        seanjc@google.com, pbonzini@redhat.com, ravi.v.shankar@intel.com,
        jiangshanlai@gmail.com, shan.kang@intel.com
Subject: Re: [PATCH v8 04/33] x86/traps: add install_system_interrupt_handler()
In-Reply-To: <20230410081438.1750-5-xin3.li@intel.com>
References: <20230410081438.1750-1-xin3.li@intel.com>
 <20230410081438.1750-5-xin3.li@intel.com>
Date:   Mon, 05 Jun 2023 10:57:37 +0200
Message-ID: <87y1ky70ce.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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