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

From: Jianyu Zhan
Date: Sun Mar 13 2016 - 03:26:53 EST

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
>> 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
and on explicit CONFIG_X86_LOCAL_APIC(X86_UP_APIC).

So initial code:

#define first_system_vector NR_VECTORS

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

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

then comes the CONFIG_X86_LOCAL_APIC dependent vector definition.

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

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

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

static void __init system_intr_init(void)

alloc_intr_gate(IRQ_WORK_VECTOR, irq_work_interrupt);


>> >> -#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.

Jianyu Zhan