Re: [PATCH 3/3] x86/irq: update first_system_vector only when X86_LOCAL_PIC is on

[Jianyu Zhan]

On Sun, Mar 13, 2016 at 2:35 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> On Sun, Mar 13, 2016 at 4:08 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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
>
> It does, because CONFIG_SMP enables CONFIG_X86_LOCAL_APIC
>

It is.  Once CONFIG_SMP is on, CONFIG_X86_LOCAL_APIC will be turned on.

So the situation should be described as:   first_system_vector will be updated
on system vector init,  on SMP case(which indirectly implies
CONFIG_X86_LOCAL_APIC)
and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC).

So initial code:

#ifndef CONFIG_X86_LOCAL_APIC
#define first_system_vector NR_VECTORS
#endif

is actually not needed,  becaused it won't ever change on !
CONFIG_X86_LOCAL_APIC
case.

I will clarify this in v2.

>> 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.
>
> Do you actually understand how all that works together?
>

So the dependency should be reversed, and it should be like this:

Currently we have CONFIG_X86_LOCAL_APIC detangle from  CONFIG_SMP
(we can enable CONFIG_X86_LOCAL_APIC on 32-bit uniprocessor).


>> 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
>
> Have you tried to enable it independent from CONFIG_X86_LOCAL_APIC?
>

I did.  I can turn off SMP and  did not use UP_APIC.  But IRQ_WORK is still
able be turn on(by selected by other subsystem).

My purpose of posting this patch set is trying to make the system vector layout
reveal this fact.

we have SMP system vector defined first( these may rely on or not rely
on CONFIG_X86_LOCAL_APIC),

then comes the CONFIG_X86_LOCAL_APIC dependent vector definition.

then comes the rest vector definition that not only depends on
CONFIG_X86_LOCAL_APIC, but
also others(like MCE, virtualization).

Among these stands out IRQ_WORK,   which neither depends on SMP nor
CONFIG_X86_LOCAL_APIC.

So the new init funciton is look like(from [2/3]):

static void __init system_intr_init(void)
{
       smp_intr_init();

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

       apic_intr_init();
}


>> >>       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 ?
>
> It's your job to at least compile test your patches not the job of others.
>

Will do in next round.






Regards,
Jianyu Zhan