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 03/33] x86/traps: add a system interrupt table for
 system interrupt dispatch
In-Reply-To: <20230410081438.1750-4-xin3.li@intel.com>
References: <20230410081438.1750-1-xin3.li@intel.com>
 <20230410081438.1750-4-xin3.li@intel.com>
Date:   Mon, 05 Jun 2023 10:34:58 +0200
Message-ID: <877csi8fyl.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
>  
>  /**
>   * DECLARE_IDTENTRY_IRQ - Declare functions for device interrupt IDT entry
> - *			  points (common/spurious)
> + *			  points (common/spurious) and their corresponding
> + *			  software based dispatch handlers in the non-noinstr
> + *			  text section
>   * @vector:	Vector number (ignored for C)
>   * @func:	Function name of the entry point
>   *
> - * Maps to DECLARE_IDTENTRY_ERRORCODE()
> + * Maps to DECLARE_IDTENTRY_ERRORCODE(), plus a dispatch function prototype
>   */
>  #define DECLARE_IDTENTRY_IRQ(vector, func)				\
> -	DECLARE_IDTENTRY_ERRORCODE(vector, func)
> +	DECLARE_IDTENTRY_ERRORCODE(vector, func);			\
> +	void dispatch_##func(struct pt_regs *regs, unsigned long
>  error_code)

Can these IDTENTRY changes please be separate from the actual table
implementation?
  
>  
> +#ifdef CONFIG_X86_64
> +
> +#ifndef CONFIG_X86_LOCAL_APIC

Seriously? You _cannot_ disable local APIC on x8664 builds.

> +/*
> + * Used when local APIC is not configured to build into the kernel, but
> + * dispatch_table_spurious_interrupt() needs
> dispatch_spurious_interrupt().

What? If you there is something which is not used in a certain
configuration then just exclude it via #ifdef in the table or provide a

#define foo   NULL

if you want to spare the #ifdeffery.

> + */
> +DEFINE_IDTENTRY_IRQ(spurious_interrupt)
> +{
> +	pr_info("Spurious interrupt (vector 0x%x) on CPU#%d, should never happen.\n",
> +		vector, smp_processor_id());
> +}

But mindlessly copying code which is even never compiled is a pretty
pointless exercise.

> +#endif
> +
> +static void dispatch_table_spurious_interrupt(struct pt_regs *regs)
> +{
> +	dispatch_spurious_interrupt(regs, regs->vector);
> +}
> +
> +#define SYSV(x,y) [(x) - FIRST_SYSTEM_VECTOR] = y
> +
> +static system_interrupt_handler system_interrupt_handlers[NR_SYSTEM_VECTORS] = {
> +	[0 ... NR_SYSTEM_VECTORS-1]		= dispatch_table_spurious_interrupt,
> +#ifdef CONFIG_SMP
> +	SYSV(RESCHEDULE_VECTOR,			dispatch_table_sysvec_reschedule_ipi),
> +	SYSV(CALL_FUNCTION_VECTOR,		dispatch_table_sysvec_call_function),
> +	SYSV(CALL_FUNCTION_SINGLE_VECTOR,	dispatch_table_sysvec_call_function_single),
> +	SYSV(REBOOT_VECTOR,			dispatch_table_sysvec_reboot),
> +#endif
> +
> +#ifdef CONFIG_X86_THERMAL_VECTOR
> +	SYSV(THERMAL_APIC_VECTOR,		dispatch_table_sysvec_thermal),
> +#endif
> +
> +#ifdef CONFIG_X86_MCE_THRESHOLD
> +	SYSV(THRESHOLD_APIC_VECTOR,		dispatch_table_sysvec_threshold),
> +#endif
> +
> +#ifdef CONFIG_X86_MCE_AMD
> +	SYSV(DEFERRED_ERROR_VECTOR,		dispatch_table_sysvec_deferred_error),
> +#endif
> +
> +#ifdef CONFIG_X86_LOCAL_APIC
> +	SYSV(LOCAL_TIMER_VECTOR,		dispatch_table_sysvec_apic_timer_interrupt),
> +	SYSV(X86_PLATFORM_IPI_VECTOR,		dispatch_table_sysvec_x86_platform_ipi),
> +# ifdef CONFIG_HAVE_KVM
> +	SYSV(POSTED_INTR_VECTOR,		dispatch_table_sysvec_kvm_posted_intr_ipi),
> +	SYSV(POSTED_INTR_WAKEUP_VECTOR,		dispatch_table_sysvec_kvm_posted_intr_wakeup_ipi),
> +	SYSV(POSTED_INTR_NESTED_VECTOR,		dispatch_table_sysvec_kvm_posted_intr_nested_ipi),
> +# endif
> +# ifdef CONFIG_IRQ_WORK
> +	SYSV(IRQ_WORK_VECTOR,			dispatch_table_sysvec_irq_work),
> +# endif
> +	SYSV(SPURIOUS_APIC_VECTOR,		dispatch_table_sysvec_spurious_apic_interrupt),

This is clearly in the #ifdef CONFIG_X86_LOCAL_APIC, so what is the
above hackery useful for?

> +	SYSV(ERROR_APIC_VECTOR,			dispatch_table_sysvec_error_interrupt),
> +#endif
> +};

Thanks,

        tglx