MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.11.1603122104360.3657@nanos>
References: <cover.1457790776.git.nasa4836@gmail.com> <fa064e4e2fd688a42480f686825f485ade4e30ca.1457790777.git.nasa4836@gmail.com>
 <alpine.DEB.2.11.1603122104360.3657@nanos>
From: Jianyu Zhan <nasa4836@gmail.com>
Date: Sun, 13 Mar 2016 09:28:15 +0800
Message-ID: <CAHz2CGXs3cweb9m+m5fS2ZqWkqSbp18eZtOsSAK3KQC+N7f_4A@mail.gmail.com>
Subject: Re: [PATCH 3/3] x86/irq: update first_system_vector only when
 X86_LOCAL_PIC is on
To: Thomas Gleixner <tglx@linutronix.de>
Cc: mingo@redhat.com, "H. Peter Anvin" <hpa@zytor.com>,
        Aravind.Gopalakrishnan@amd.com, brgerst@gmail.com, bp@suse.de,
        feng.wu@intel.com, jiang.liu@linux.intel.com,
        Tejun Heo <tj@kernel.org>, dvlasenk@redhat.com, penberg@cs.helsinki.fi,
        Yinghai Lu <yhlu.kernel@gmail.com>, andi@firstfloor.org,
        Andy Lutomirski <luto@kernel.org>, ajm@sgi.com,
        Yinghai Lu <yinghai@kernel.org>, Akinobu Mita <akinobu.mita@gmail.com>,
        x86@kernel.org, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2687
Lines: 88

On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> This is pointless, because it's only called when local apic is enabled as all
> call sites of alloc_intr_gate() depend on CONFIG_X86_LOCAL_APIC ....

Not exactly,  currently at least  smp_intr_init()  DOES NOT depend on
CONFIG_X86_LOCAL_APIC:

static void __init smp_intr_init(void)
{
#ifdef CONFIG_SMP
       /*
        * The reschedule interrupt is a CPU-to-CPU reschedule-helper
        * IPI, driven by wakeup.
        */
       alloc_intr_gate(RESCHEDULE_VECTOR, reschedule_interrupt);

       /* IPI for generic function call */
       alloc_intr_gate(CALL_FUNCTION_VECTOR, call_function_interrupt);

...
}

So alloc_intr_gate will be called, and first_system_vector will be updated !

I know this is weird, because modern SMP machines implies Local APIC.
But currently we have CONFIG_SMP detangle from CONFIG_X86_LOCAL_APIC,
which I think is fine.

Another place which is weird is CONFIG_IRQ_WORK.  Technically,  it
does not depend
on SMP,   nor even necessary Local APIC.  Actually, it is just a base
configuration selected
by others.  But currently we have the

#ifdef CONFIG_IRQ_WORK
       alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);
#endif

block surrounded by CONFIG_X86_LOCAL_APIC.

In new scheme, I just move it out,  see [2/3] patch.


>
>>       } else {
>>               BUG();
>>       }
>> diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
>> index 0e9fa7c..e999b38 100644
>> --- a/arch/x86/kernel/irqinit.c
>> +++ b/arch/x86/kernel/irqinit.c
>> @@ -188,9 +188,6 @@ void __init native_init_IRQ(void)
>>        * 'special' SMP interrupts)
>>        */
>>       i = FIRST_EXTERNAL_VECTOR;
>> -#ifndef CONFIG_X86_LOCAL_APIC
>> -#define first_system_vector NR_VECTORS
>> -#endif
>>       for_each_clear_bit_from(i, used_vectors, first_system_vector) {
>
> And how exactly is this here supposed to compile when CONFIG_X86_LOCAL_APIC=n?

Dunno.  I guess this code on !CONFIG_X86_LOCAL_APIC case hasn't been
tested yet ?

 first_system_vector is a global variable, and is initially assigned
to FIRST_SYSTEM_VECTOR:

int first_system_vector = FIRST_SYSTEM_VECTOR;

#ifdef CONFIG_X86_LOCAL_APIC
#define FIRST_SYSTEM_VECTOR             LOCAL_TIMER_VECTOR
#else
#define FIRST_SYSTEM_VECTOR             NR_VECTORS
#endif

For CONFIG_X86_LOCAL_APIC case,  the define makes sense.
But for ! CONFIG_X86_LOCAL_APIC case,  why we confine it to NR_VECTORS
is a mystery
to me.  Have digged into git history, but found no proof.

So to maintain consistency, this patch just retain what it is,  but we
do not bother update it for
!CONFIG_X86_LOCAL_APIC case.

Regards,
Jianyu Zhan