Re: [patch] x86: handle legacy PIC interrupts on all the cpu's

[Ingo Molnar <mingo@elte.hu>]

* Yinghai Lu <yinghai@kernel.org> wrote:

> On 03/15/2010 11:54 PM, Suresh Siddha wrote:
> > On Mon, 2010-03-15 at 22:37 -0700, Ingo Molnar wrote:
> >> * Yinghai Lu <yinghai@kernel.org> wrote:
> >>
> >>> On 03/15/2010 03:56 PM, Suresh Siddha wrote:
> >>>> On Mon, 2010-03-15 at 14:51 -0700, Yinghai Lu wrote:
> >>>>>> +	for (irq = 0; irq < legacy_pic->nr_legacy_irqs; irq++)
> >>>>>> +		if (!IO_APIC_IRQ(irq))
> >>>>>> +			per_cpu(vector_irq, cpu)[IRQ0_VECTOR + irq] = irq;
> >>>>>
> >>>>> seems those three lines are not needed...
> >>>>
> >>>> Those are needed for !CONFIG_X86_IO_APIC case.
> >>>>
> >>> then we can have
> >>>
> >>> +#ifndef CONFIG_X86_IO_APIC
> >>> +	for (irq = 0; irq < legacy_pic->nr_legacy_irqs; irq++)
> >>> +		per_cpu(vector_irq, cpu)[IRQ0_VECTOR + irq] = irq;
> >>> +#endif
> >>>
> >>> then we don't punish most setup with ioapic controller.
> >>
> >> Ok - i've simplified the code with the above and have added your Acked-by - is 
> >> that is fine by you?
> > 
> > Ingo, Probably appended one is a better version. Yinghai can you please
> > Ack if it is ok. Thanks.
> > 
> > ---
> > From: Suresh Siddha <suresh.b.siddha@intel.com>
> > Subject: x86: handle legacy PIC interrupts on all the cpu's
> > 
> > Ingo Molnar reported that with the recent changes of not statically blocking
> > IRQ0_VECTOR..IRQ15_VECTOR's on all the cpu's, broke an AMD platform
> > (with Nvidia chipset) boot when "noapic" boot option is used.
> > 
> > On this platform, legacy PIC interrupts are getting delivered to all the
> > cpu's instead of just the boot cpu. Thus not initializing the vector to irq
> > mapping for the legacy irq's resulted in not handling certain interrupts
> > causing boot hang.
> > 
> > Fix this by initializing the vector to irq mapping on all the logical cpu's,
> > if the legacy IRQ is handled by the legacy PIC.
> > 
> > Reported-by: Ingo Molnar <mingo@elte.hu>
> > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > ---
> > 
> >  arch/x86/include/asm/hw_irq.h  |    3 +--
> >  arch/x86/kernel/apic/io_apic.c |    9 +++++++++
> >  arch/x86/kernel/irqinit.c      |   20 ++++++++++++++++++++
> >  3 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
> > index a929c9e..564a7a1 100644
> > --- a/arch/x86/include/asm/hw_irq.h
> > +++ b/arch/x86/include/asm/hw_irq.h
> > @@ -133,15 +133,14 @@ extern void (*__initconst interrupt[NR_VECTORS-FIRST_EXTERNAL_VECTOR])(void);
> >  
> >  typedef int vector_irq_t[NR_VECTORS];
> >  DECLARE_PER_CPU(vector_irq_t, vector_irq);
> > +extern void __setup_vector_irq(int cpu);
> >  
> >  #ifdef CONFIG_X86_IO_APIC
> >  extern void lock_vector_lock(void);
> >  extern void unlock_vector_lock(void);
> > -extern void __setup_vector_irq(int cpu);
> >  #else
> >  static inline void lock_vector_lock(void) {}
> >  static inline void unlock_vector_lock(void) {}
> > -static inline void __setup_vector_irq(int cpu) {}
> >  #endif
> >  
> >  #endif /* !ASSEMBLY_ */
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index e4e0ddc..fd3cecd 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -1268,6 +1268,15 @@ void __setup_vector_irq(int cpu)
> >  	/* Mark the inuse vectors */
> >  	for_each_irq_desc(irq, desc) {
> >  		cfg = desc->chip_data;
> > +
> > +		/*
> > +		 * If it is a legacy IRQ handled by the legacy PIC, be ready
> > +		 * to handle it on any CPU, as the PIC interrupts are delivered
> > +		 * to multiple cpu's on some platforms.
> > +		 */
> > +		if (irq < legacy_pic->nr_legacy_irqs && !IO_APIC_IRQ(irq))
> > +			cpumask_set_cpu(cpu, cfg->domain);
> > +
> >  		if (!cpumask_test_cpu(cpu, cfg->domain))
> >  			continue;
> >  		vector = cfg->vector;
> > diff --git a/arch/x86/kernel/irqinit.c b/arch/x86/kernel/irqinit.c
> > index ef257fc..afc3b31 100644
> > --- a/arch/x86/kernel/irqinit.c
> > +++ b/arch/x86/kernel/irqinit.c
> > @@ -141,6 +141,26 @@ void __init init_IRQ(void)
> >  	x86_init.irqs.intr_init();
> >  }
> >  
> > +#ifndef CONFIG_X86_IO_APIC
> > +/*
> > + * Setup the vector to irq mappings.
> > + */
> > +void __setup_vector_irq(int cpu)
> > +{
> > +	int irq;
> > +
> > +	/*
> > +	 * On most of the platforms, legacy PIC delivers the interrupts on the
> > +	 * boot cpu. But there are certain platforms where PIC interrupts are
> > +	 * delivered to multiple cpu's. For the new cpu that is coming online,
> > +	 * setup the static legacy vector to irq mapping.
> > +	 */
> > +	for (irq = 0; irq < legacy_pic->nr_legacy_irqs; irq++)
> > +		per_cpu(vector_irq, cpu)[IRQ0_VECTOR + irq] = irq;
> > +
> > +}
> > +#endif
> > +
> >  static void __init smp_intr_init(void)
> >  {
> >  #ifdef CONFIG_SMP
> > 
> 
> yes, less one one function prototype.
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Please submit the cleanup bit separately, we try to keep x86/urgent patches as 
small as possible.

Thanks,

	Ingo