RE: [PATCH 09/14] x86_64 irq: Begin consolidating per_irq data in structures.

RE: [PATCH 09/14] x86_64 irq: Begin consolidating per_irq data in structures.

[Lu, Yinghai]

-----Original Message-----
From: ebiederm@xmission.com [mailto:ebiederm@xmission.com] 
Sent: Friday, February 23, 2007 3:33 AM

>+struct irq_cfg {
>+	cpumask_t domain;
>+	u8 vector;
>+};
>+
>+/* irq_cfg is indexed by the sum of all RTEs in all I/O APICs. */
>+struct irq_cfg irq_cfg[NR_IRQS] __read_mostly = {
>+	[0] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 0 },
>+	[1] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 1 },
>+	[2] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 2 },
>+	[3] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 3 },
>+	[4] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 4 },
>+	[5] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 5 },
>+	[6] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 6 },
>+	[7] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 7 },
>+	[8] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 8 },
>+	[9] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 9 },
>+	[10] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 10 },
>+	[11] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 11 },
>+	[12] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 12 },
>+	[13] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 13 },
>+	[14] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 14 },
>+	[15] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR
+ 15 },
>+};
>+
> static int assign_irq_vector(int irq, cpumask_t mask, cpumask_t
*result);

Why not use

IRQ0_VECTOR... IRQ15_VECTOR here. 

YH

Re: [PATCH 09/14] x86_64 irq: Begin consolidating per_irq data in structures.

[Eric W. Biederman]

"Lu, Yinghai" <yinghai.lu@amd.com> writes:
>
> Why not use
>
> IRQ0_VECTOR... IRQ15_VECTOR here. 

I do by the end of the patch series it was a patch ordering issue.

Eric

Re: [PATCH 2/2] x86_64 irq: Handle irqs pending in IRR during irq migration.

[Eric W. Biederman]

Ingo Molnar <mingo@elte.hu> writes:

> * Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> Ingo would it be reasonable to get a wait queue so I can wait for an 
>> irq that needs the delayed disable action to actually become masked?
>
> that might make sense, but what will do the wakeup - incidental IRQ 
> arriving on the new CPU? Isnt that a bit risky - maybe the device wont 
> generate IRQs for a really long time.

I still need to test this, but I believe I have found a simpler
way to avoid irr problems during migration, and I believe the code
works equally well with either edge or level triggered interrupts.

The idea is this: Instead of trying test for and handle when irr
occurs, simply enable local interrupts after disabling and
acknowledging the irq so that anything pending will be processed,
before we perform the migration operation.

I don't think the edge case cares about the mask/ack order but
but masking before acking appears important for the level triggered
case, so we might as well use that order for both.

Does this look like a sane way to handle this?

static void ack_apic(unsigned int irq)
{
	struct irq_desc *desc = irq_desc + irq;
	int do_unmask_irq = 0;
	if (unlikely((irq_desc[irq].status & IRQ_MOVE_PENDING) && !in_irq()) {
		do_unmask_irq = 1;
		desc->chip->mask(irq);
	}
	ack_APIC_irq();
	if (unlikely(do_unmask_irq)) {
		/* Don't let pending irqs accumulate */
		local_irq_enable();
                syncrhonize_irq(irq);
		move_masked_irq(irq);
		local_irq_disable();
		desc->chip->unmask(irq);
	}
}

Eric

Re: [PATCH 2/2] x86_64 irq: Handle irqs pending in IRR during irq migration.

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Ingo Molnar <mingo@elte.hu> writes:
>
>> * Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>>> Ingo would it be reasonable to get a wait queue so I can wait for an 
>>> irq that needs the delayed disable action to actually become masked?
>>
>> that might make sense, but what will do the wakeup - incidental IRQ 
>> arriving on the new CPU? Isnt that a bit risky - maybe the device wont 
>> generate IRQs for a really long time.
>
> I still need to test this, but I believe I have found a simpler
> way to avoid irr problems during migration, and I believe the code
> works equally well with either edge or level triggered interrupts.
>
> The idea is this: Instead of trying test for and handle when irr
> occurs, simply enable local interrupts after disabling and
> acknowledging the irq so that anything pending will be processed,
> before we perform the migration operation.
>
> I don't think the edge case cares about the mask/ack order but
> but masking before acking appears important for the level triggered
> case, so we might as well use that order for both.
>
> Does this look like a sane way to handle this?

The version I would up testing is below, and it doesn't work.
I still get "No irq handler for vector" warnings as well as
a couple of complaints from lock/irq debugging.    The debugging
doesn't worry me.  The fact that I don't have a good way to ensure
I have no more irqs in flight does.

So unless someone can find a sure way to drain the irqs in flight,
I can't migrate an irq from process context, and looking at irr and
handling a pending irq appears required.  '

Eric

static void ack_apic(unsigned int irq)
{
#if defined(CONFIG_GENERIC_PENDING_IRQ) || defined(CONFIG_IRQBALANCE)
	if (unlikely((irq_desc[irq].status & IRQ_MOVE_PENDING) && 
		     (hardirq_count() == HARDIRQ_OFFSET))) {
		struct irq_desc *desc = irq_desc + irq;
		desc->chip->mask(irq);
		ack_APIC_irq();

		/* Ensure all of the irq handlers for this irq have completed
		 * before we migrate it.
		 */
		raw_local_irq_enable();
		cpu_relax();
		raw_local_irq_disable();
		synchronize_irq(irq);

		move_masked_irq(irq);
		desc->chip->unmask(irq);
		return;
	}
#endif
	ack_APIC_irq();
}

Re: [PATCH 2/2] x86_64 irq: Handle irqs pending in IRR during irq migration.

[Eric W. Biederman]

>
> The version I would up testing is below, and it doesn't work.
> I still get "No irq handler for vector" warnings as well as
> a couple of complaints from lock/irq debugging.    The debugging
> doesn't worry me.  The fact that I don't have a good way to ensure
> I have no more irqs in flight does.
>
> So unless someone can find a sure way to drain the irqs in flight,
> I can't migrate an irq from process context, and looking at irr and
> handling a pending irq appears required.  '

Bah.  I had not taken into account that the local apic despite
being tightly coupled with the cpu is for programming purposes
an asynchronous device.  If I want to give it time to react to something
I need to read from it.

The routine below actually works. 

My remaining practical question is can this been done cleanly.

Ingo's lock debugging dislikes this routine.  
By using raw_local_irq_enable I have avoided all but a message on
the irq return path, I haven't quite worked out where.

But at least this version feels like it could be done better
(less inline? different helpers?) someone.

For interrupts coming through a sane interrupt controller moving
this into process context would certainly simplify things.  For edge
triggered interrupts coming through an io_apic I'm not at all certain
what makes sense.

When the routine below is used to ack an edge triggered interrupt
it runs before the edge triggered interrupt handler so losing an
edge shouldn't happen (we haven't acknowledged the hardware yet)
and even if we do the device driver gets to run at least once.

So doing migration in the irq handler still looks like the best
solution even if it is ugly.  As long as the little bit of
stack overhead isn't a problem I think enabling interrupts to
clear out any pending irqs certainly looks simpler. 

In another vein.  I went and looked through all of Intel's and
AMD's public errata that I could find and there weren't any associated
with irr or isr, so I think my previous version of the code is still
sane, and not likely to break.

I can improve it a little by getting the vector as:
"vector = ~ get_irq_regs()->orig_rax;" instead of reading
ISR.  That still leaves reading the pending bit in ISR and the
other funny tricks.

I'm conflicted between the two approaches a little because playing
games with enabling interrupts in an interrupt handler seems to
have some weird corner cases.

static void ack_apic(unsigned int irq)
{
#if defined(CONFIG_GENERIC_PENDING_IRQ) || defined(CONFIG_IRQBALANCE)
	struct irq_desc *desc;
	desc = irq_desc + irq;
	if (likely(!(desc->status & IRQ_MOVE_PENDING)))
		goto simple;

	if (hardirq_count() != HARDIRQ_OFFSET)
		goto simple;

	desc->chip->mask(irq);
	ack_APIC_irq();

	/* Ensure all of the irq handlers for this irq have completed
	 * before we migrate it.
	 */
	spin_unlock(&desc->lock);
	raw_local_irq_enable();
	apic_read(APIC_ID);
	raw_local_irq_disable();
	spin_lock(&desc->lock);

	move_masked_irq(irq);
	desc->chip->unmask(irq);
	return;
simple:
#endif
	ack_APIC_irq();
}


BUG: at /home/eric/projects/linux/linux-2.6-devel/kernel/lockdep.c:1860 trace_hardirqs_on()

Call Trace:
 <IRQ>  [<ffffffff8048562f>] trace_hardirqs_on_thunk+0x35/0x37
 [<ffffffff80290401>] generic_delete_inode+0x0/0x13e
 [<ffffffff8020a0fc>] restore_args+0x0/0x30
 [<ffffffff80290401>] generic_delete_inode+0x0/0x13e
 [<ffffffff8021648d>] ack_apic+0x63/0x99
 [<ffffffff80216485>] ack_apic+0x5b/0x99
 [<ffffffff8025881e>] handle_fasteoi_irq+0xc1/0xd1
 [<ffffffff80290401>] generic_delete_inode+0x0/0x13e
 [<ffffffff8020c0de>] do_IRQ+0x89/0xf3
 [<ffffffff80208ce8>] default_idle+0x35/0x51
 [<ffffffff80208cb3>] default_idle+0x0/0x51
 [<ffffffff8020a0a6>] ret_from_intr+0x0/0xf
 <EOI>  [<ffffffff80290401>] generic_delete_inode+0x0/0x13e
 [<ffffffff80208cb3>] default_idle+0x0/0x51
 [<ffffffff80208ce8>] default_idle+0x35/0x51
 [<ffffffff80208cea>] default_idle+0x37/0x51
 [<ffffffff80208ce8>] default_idle+0x35/0x51
 [<ffffffff80208d5a>] cpu_idle+0x56/0x75
 [<ffffffff808b9a69>] start_secondary+0x481/0x490

What are the real ioapic rte programming constraints?

[Eric W. Biederman]

I have recently been investigating why we reprogram ioapic irqs in the
interrupt handler, because it significantly complicates the code, and
makes things more fragile.  Eventually I found the commit with the
justification, see below.

There are not enough details in the justification to really understand
the issue so I'm asking to see if someone has some more details.

The description makes the assertion that reprograming the ioapic
when an  interrupt is pending is the only safe way to handle this.
Since edge triggered interrupts cannot be pending at the ioapic I know
it is not talking level triggered interrupts.

However it is not possible to fully reprogram a level triggered
interrupt when the interrupt is pending as the ioapic will not
receive the interrupt acknowledgement.  So it turns out I have
broken this change for several kernel releases without people
screaming at me about io_apic problems.

Currently I am disabling the irq on the ioapic before reprogramming
it so I do not run into issues.  Does that solve the concerns that
were patched around by only reprogramming interrupt redirection
table entry in interrupt handlers?

If it does I can solve and simply the code by moving it all back
into process context.

commit 54d5d42404e7705cf3804593189e963350d470e5
Author: Ashok Raj <ashok.raj@intel.com>
Date:   Tue Sep 6 15:16:15 2005 -0700

    [PATCH] x86/x86_64: deferred handling of writes to /proc/irqxx/smp_affinity
    
    When handling writes to /proc/irq, current code is re-programming rte
    entries directly. This is not recommended and could potentially cause
    chipset's to lockup, or cause missing interrupts.
    
    CONFIG_IRQ_BALANCE does this correctly, where it re-programs only when the
    interrupt is pending. The same needs to be done for /proc/irq handling as well.
    Otherwise user space irq balancers are really not doing the right thing.
    
    - Changed pending_irq_balance_cpumask to pending_irq_migrate_cpumask for
      lack of a generic name.
    - added move_irq out of IRQ_BALANCE, and added this same to X86_64
    - Added new proc handler for write, so we can do deferred write at irq
      handling time.
    - Display of /proc/irq/XX/smp_affinity used to display CPU_MASKALL, instead
      it now shows only active cpu masks, or exactly what was set.
    - Provided a common move_irq implementation, instead of duplicating
      when using generic irq framework.
    
    Tested on i386/x86_64 and ia64 with CONFIG_PCI_MSI turned on and off.
    Tested UP builds as well.
    
    MSI testing: tbd: I have cards, need to look for a x-over cable, although I
    did test an earlier version of this patch.  Will test in a couple days.
    
    Signed-off-by: Ashok Raj <ashok.raj@intel.com>
    Acked-by: Zwane Mwaikambo <zwane@holomorphy.com>
    Grudgingly-acked-by: Andi Kleen <ak@muc.de>
    Signed-off-by: Coywolf Qi Hunt <coywolf@lovecn.org>
    Signed-off-by: Ashok Raj <ashok.raj@intel.com>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Re: What are the real ioapic rte programming constraints?

[Zwane Mwaikambo]

On Sat, 10 Feb 2007, Eric W. Biederman wrote:

> There are not enough details in the justification to really understand
> the issue so I'm asking to see if someone has some more details.
> 
> The description makes the assertion that reprograming the ioapic
> when an  interrupt is pending is the only safe way to handle this.
> Since edge triggered interrupts cannot be pending at the ioapic I know
> it is not talking level triggered interrupts.
> 
> However it is not possible to fully reprogram a level triggered
> interrupt when the interrupt is pending as the ioapic will not
> receive the interrupt acknowledgement.  So it turns out I have
> broken this change for several kernel releases without people
> screaming at me about io_apic problems.
> 
> Currently I am disabling the irq on the ioapic before reprogramming
> it so I do not run into issues.  Does that solve the concerns that
> were patched around by only reprogramming interrupt redirection
> table entry in interrupt handlers?

Hi Eric,
	Could you outline in pseudocode where you're issuing the mask? If 
it's done whilst an irq is pending some (intel 7500 based) chipsets will 
not actually mask it but treat it as a 'legacy' IRQ and deliver it 
anyway. Using the masked whilst pending logic avoids all of that.

Cheers,
	Zwane

Re: What are the real ioapic rte programming constraints?

[Eric W. Biederman]

Zwane Mwaikambo <zwane@infradead.org> writes:

> On Sat, 10 Feb 2007, Eric W. Biederman wrote:
>
>> There are not enough details in the justification to really understand
>> the issue so I'm asking to see if someone has some more details.
>> 
>> The description makes the assertion that reprograming the ioapic
>> when an  interrupt is pending is the only safe way to handle this.
>> Since edge triggered interrupts cannot be pending at the ioapic I know
>> it is not talking level triggered interrupts.
>> 
>> However it is not possible to fully reprogram a level triggered
>> interrupt when the interrupt is pending as the ioapic will not
>> receive the interrupt acknowledgement.  So it turns out I have
>> broken this change for several kernel releases without people
>> screaming at me about io_apic problems.
>> 
>> Currently I am disabling the irq on the ioapic before reprogramming
>> it so I do not run into issues.  Does that solve the concerns that
>> were patched around by only reprogramming interrupt redirection
>> table entry in interrupt handlers?
>
> Hi Eric,
> 	Could you outline in pseudocode where you're issuing the mask? If 
> it's done whilst an irq is pending some (intel 7500 based) chipsets will 
> not actually mask it but treat it as a 'legacy' IRQ and deliver it 
> anyway. Using the masked whilst pending logic avoids all of that.

The code currently in the kernel does:

pending
mask
read io_apic
ack
reprogram vector and destination
unmask

So I guess it does retain the bug fix.

What I am looking at doing is:

mask
read io_apic 
-- Past this point no more irqs are expected from the io_apic
-- Now I work to drain any inflight/pending instances of the irq 
send ipi to all irq destinations cpus and wait for it to return
read lapic
disable local irqs
take irq lock
-- Now no more irqs are expected to arrive
reprogram vector and destination
enable local irqs
unmask

What I need to ensure is that I have a point where I will not receive any
new messages from an ioapic about a particular irq anymore.  Even if
everything is working perfectly setting the disable bit is not enough
because there could be an irq message in flight. So I need to give any
in flight irqs a chance to complete.  

With a little luck that logic will cover your 7500 disable race as
well. If not and there is a reasonable work around we should look at
that.  This is not a speed critical path so we can afford to do a
little more work.

The version of this that I am currently testing is below.

Eric

/*
 * Synchronize the local APIC and the CPU by doing
 * a dummy read from the local APIC
 */
static inline void lapic_sync(void)
{
	apic_read(APIC_ID);
}

static void affinity_noop(void *info)
{
	return;
}

static void mask_get_irq(unsigned int irq)
{
	struct irq_desc *desc = irq_desc + irq;
	int cpu;

	spin_lock(&vector_lock);

	/*
	 * Mask the irq so it will no longer occur
	 */
	desc->chip->mask(irq);

	/* If I can run a lower priority vector on another cpu
	 * then obviously the irq has completed on that cpu.  SMP call
	 * function is lower priority then all of the hardware
	 * irqs.
	 */
	for_each_cpu_mask(cpu, desc->affinity)
		smp_call_function_single(cpu, affinity_noop, NULL, 0, 1);

	/*
	 * Ensure irqs have cleared the local cpu
	 */
	lapic_sync();
	local_irq_disable();
	lapic_sync();
	spin_lock(&desc->lock);
}

static void unmask_put_irq(unsigned int irq)
{
	struct irq_desc *desc = irq_desc + irq;

	spin_unlock(&desc->lock);
	local_irq_enable();
	desc->chip->unmask(irq);
	spin_unlock(&vector_lock);
}

static void set_ioapic_affinity_level_irq(unsigned int irq, cpumask_t mask)
{
	unsigned int dest;
	int vector;

	/*
	 * Ensure all of the irq handlers for this irq have completed.
	 * i.e. drain all pending irqs
	 */
	mask_get_irq(irq);

	cpus_and(mask, mask, cpu_online_map);
	if (cpus_empty(mask))
		goto out;

	vector = __assign_irq_vector(irq, mask, &mask);
	if (vector < 0)
		goto out;

	dest = cpu_mask_to_apicid(mask);

	/*
	 * Only the high 8 bits are valid
	 */
	dest = SET_APIC_LOGICAL_ID(dest);

	spin_lock(&ioapic_lock);
	__target_IO_APIC_irq(irq, dest, vector);
	spin_unlock(&ioapic_lock);

	set_native_irq_info(irq, mask);
out:
	unmask_put_irq(irq);
}

Re: What are the real ioapic rte programming constraints?

[Zwane Mwaikambo]

On Sun, 11 Feb 2007, Eric W. Biederman wrote:

> What I am looking at doing is:
> 
> mask
> read io_apic 
> -- Past this point no more irqs are expected from the io_apic
> -- Now I work to drain any inflight/pending instances of the irq 
> send ipi to all irq destinations cpus and wait for it to return
> read lapic
> disable local irqs
> take irq lock
> -- Now no more irqs are expected to arrive
> reprogram vector and destination
> enable local irqs
> unmask
> 
> What I need to ensure is that I have a point where I will not receive any
> new messages from an ioapic about a particular irq anymore.  Even if
> everything is working perfectly setting the disable bit is not enough
> because there could be an irq message in flight. So I need to give any
> in flight irqs a chance to complete.  
> 
> With a little luck that logic will cover your 7500 disable race as
> well. If not and there is a reasonable work around we should look at
> that.  This is not a speed critical path so we can afford to do a
> little more work.

The 7500 issue isn't actually a race but a disease, if you mask a pending 
irq in its RTE, the PCI hub generates an INTx message corresponding to 
that irq. This apparently was done to support booting OSes without APIC 
support. So the following would occur;

- irqN pending on IOAPIC
- mask
=> INTx message for irqN

Unfortunately it appears the below code would also be affected by this as 
well, the appropriate reference is;

2.15.2 PCI Express* Legacy INTx Support and Boot Interrupt
http://download.intel.com/design/chipsets/datashts/30262802.pdf

> static void mask_get_irq(unsigned int irq)
> {
> 	struct irq_desc *desc = irq_desc + irq;
> 	int cpu;
> 
> 	spin_lock(&vector_lock);
> 
> 	/*
> 	 * Mask the irq so it will no longer occur
> 	 */
> 	desc->chip->mask(irq);
> 
> 	/* If I can run a lower priority vector on another cpu
> 	 * then obviously the irq has completed on that cpu.  SMP call
> 	 * function is lower priority then all of the hardware
> 	 * irqs.
> 	 */
> 	for_each_cpu_mask(cpu, desc->affinity)
> 		smp_call_function_single(cpu, affinity_noop, NULL, 0, 1);
> 
> 	/*
> 	 * Ensure irqs have cleared the local cpu
> 	 */
> 	lapic_sync();
> 	local_irq_disable();
> 	lapic_sync();
> 	spin_lock(&desc->lock);
> }

Re: What are the real ioapic rte programming constraints?

[Eric W. Biederman]

Zwane Mwaikambo <zwane@infradead.org> writes:
>
> The 7500 issue isn't actually a race but a disease, if you mask a pending 
> irq in its RTE, the PCI hub generates an INTx message corresponding to 
> that irq. This apparently was done to support booting OSes without APIC 
> support. So the following would occur;
>
> - irqN pending on IOAPIC
> - mask
> => INTx message for irqN
>
> Unfortunately it appears the below code would also be affected by this as 
> well, the appropriate reference is;
>
> 2.15.2 PCI Express* Legacy INTx Support and Boot Interrupt
> http://download.intel.com/design/chipsets/datashts/30262802.pdf

Ouch.  And this kind of thing isn't exactly uncommon.

However if we have the irqs also disabled in the i8259 we should
be safe from actually receiving this interrupt (even if it generates
bus traffic), and when we enable the irq since it is level triggered  
we should still get an interrupt message.

It isn't immediately obvious where the i8259 irq enable/disable
happens.  So i"m having trouble auditing that bit of code.

Plus we can get very strange things like the irq number changing
and the sharing rules being different when going through the i8259.
So irqN may be irqM when going through the i8259.

As long as we aren't using anything on the i8259 including the timer
in ExtINT mode we can disable every interrupt pin and not worry about
interrupts from that source.

Eric

Re: What are the real ioapic rte programming constraints?

[Zwane Mwaikambo]

On Sun, 11 Feb 2007, Eric W. Biederman wrote:

> > 2.15.2 PCI Express* Legacy INTx Support and Boot Interrupt
> > http://download.intel.com/design/chipsets/datashts/30262802.pdf
> 
> Ouch.  And this kind of thing isn't exactly uncommon.
> 
> However if we have the irqs also disabled in the i8259 we should
> be safe from actually receiving this interrupt (even if it generates
> bus traffic), and when we enable the irq since it is level triggered  
> we should still get an interrupt message.
>
> It isn't immediately obvious where the i8259 irq enable/disable
> happens.  So i"m having trouble auditing that bit of code.
>
> Plus we can get very strange things like the irq number changing
> and the sharing rules being different when going through the i8259.
> So irqN may be irqM when going through the i8259.
> 
> As long as we aren't using anything on the i8259 including the timer
> in ExtINT mode we can disable every interrupt pin and not worry about
> interrupts from that source.

We do the 8259 mask in setup_IO_APIC_irq. does anyone have access to an 
E7520/E7320 system for testing?

Cheers,
	Zwane

Re: What are the real ioapic rte programming constraints?

[Eric W. Biederman]

Zwane Mwaikambo <zwane@infradead.org> writes:

> On Sun, 11 Feb 2007, Eric W. Biederman wrote:
>
>> > 2.15.2 PCI Express* Legacy INTx Support and Boot Interrupt
>> > http://download.intel.com/design/chipsets/datashts/30262802.pdf
>> 
>> Ouch.  And this kind of thing isn't exactly uncommon.
>> 
>> However if we have the irqs also disabled in the i8259 we should
>> be safe from actually receiving this interrupt (even if it generates
>> bus traffic), and when we enable the irq since it is level triggered  
>> we should still get an interrupt message.
>>
>> It isn't immediately obvious where the i8259 irq enable/disable
>> happens.  So i"m having trouble auditing that bit of code.
>>
>> Plus we can get very strange things like the irq number changing
>> and the sharing rules being different when going through the i8259.
>> So irqN may be irqM when going through the i8259.
>> 
>> As long as we aren't using anything on the i8259 including the timer
>> in ExtINT mode we can disable every interrupt pin and not worry about
>> interrupts from that source.
>
> We do the 8259 mask in setup_IO_APIC_irq. does anyone have access to an 
> E7520/E7320 system for testing?

I think I do, I need to double check. 

The thing is this logic is different in that it uses INTx instead of pins
but otherwise is quite standard for chipsets and their IOAPICs.  I'm not
at all certain this behavior is what the original concern was about.
The description is enough different you may have found a completely
different set of behavior we have to worry about.

Since the legacy/non legacy behavior is common invoked by the ioapic
mask bit working with just about any recent chipset should get a taste
of that.  But I will still try and dig up an E7520 and see what
happens.

Eric

Conclusions from my investigation about ioapic programming

[Eric W. Biederman]

Ok. This is just an email to summarize my findings after investigating
the ioapic programming.

The ioapics on the E75xx chipset do have issues if you attempt to
reprogramming them outside of the irq handler.  I have on several
instances caused the state machine to get stuck such that an
individual ioapic entry was no longer capable of delivering
interrupts.  I suspect the remote IRR bit was set stuck on such that
switch the irq to edge triggered and back to level triggered would not
clear it but I did not confirm this.  I just know that I was switching
the irq to between level and edge triggered with the irq masked
and the irq did not fire.


The ioapics on the AMD 8xxx chipset do have issues if you attempt
to reprogram them outside of the irq handler.  I would up with 
remote IRR set and never clearing.  But by temporarily switching
the irq to edge triggered while it was masked I could clear
this condition.

I could not hit verifiable bugs in the ioapics on the Nforce4
chipset.  It's amazing one part of that chipset that I can't find
issues with.



I did find an algorithm that will work successfully for migrating
IRQs in process context if you have an ioapic that will follow pci
ordering rules.  In particulars the properties that the algorithm
depend on are reads guaranteeing that outstanding writes are flushed,
and in this context irqs in flight are considered writes.  I have
assumed that to devices outside of the cpu asic the cpu and the local
apic appear as the same device.

The algorithm was:
- Be running with interrupts enabled in process context.
- Mask the ioapic.
- Read the ioapic to flush outstanding reads to the local apic.
- Read the local apic to flush outstanding irqs to be send the cpu.

- Now that all of the irqs have been delivered and the irq is masked
  that irq is finally quiescent.

- With the irq quiescent it is safe to reprogram interrupt controller
  and the irq reception data structures.

There were a lot more details but that was the essence.

What I discovered was that except on the nforce chipset masking the
ioapic and then issue a read did not behave as if the interrupts were
flushed to the local apic. 

I did not look close enough to tell if local apics suffered from this
issue.  With local apics at least a read was necessary before you
could guarantee the local apic would deliver pending irqs.  A work
around on the local apics is to simply issue a low priority interrupt
as an IPI and wait for it to be processed.  This guarantees that all
higher priority interrupts have been flushed from the apic, and that
the local apic has processed interrupts.

For ioapics because they cannot be stimulated to send any irq by
stimulation from the cpu side not similar work around was possible.



** Conclusions.

*IRQs must be reprogramed in interrupt context.

The result of this is investigation is that I am convinced we need
to perform the irq migration activities in interrupt context although
I am not convinced it is completely safe.  I suspect multiple irqs
firing closely enough to each other may hit the same issues as
migrating irqs from process context.  However the odds are on our
side, when we are in irq context.

The reasoning for this is simply that.
- Before we reprogram a level triggered irq it's remote irr bit
  must be cleared by the irq being acknowledged before the can be
  safely reprogrammed.

- There is no generally effective way short of receiving an additional
  irq to ensure that the irq handler has run.  Polling the ioapics
  remote irr bit does not work.


* The CPU hotplug is currently very buggy.

Irq migration in the cpu hotplug case is a serious problem.  If we can
only safely migrate irqs from interrupt context and we cannot control
when those interrupts fire, then we cannot bound the amount of time it
will take to migrate the irqs away from a cpu.   The current cpu
hotplug code currently calls chip->set_affinity directly which is
wrong, as it does not take the necessary locks, and it does not
attempt to delay execution until we are in process context.

* Only an additional irq can signal the completion of an irq movement.

The attempt to rebuild the irq migration code from first principles
did bear some fruit.  I asked the question: "When is it safe to tear
down the data structures for irq movement?".  The only answer I have
is when I have received an irq provably from after the irq was
reprogrammed.  This is because the only way I can reliably synchronize
with irq delivery from an apic is to receive an additional irq.

Currently this is a problem both for cpu hotplug on x86_64 and i386
and for general irq migration on x86_64.

Patches to follow shortly.

Eric

[PATCH 0/14] x86_64 irq related fixes and cleanups.

[Eric W. Biederman]

My recent ioapic investigation has been painful and has taken me all
over the place.  One of my conclusions is that our code for dealing with
ioapics on i386 and x86_64 is a mess that has been growing for years.

I saw tremendous amounts of fodder for cleanup patches.  Ugh.

When deciding how much to include in the patchset for addressing the irq
migrations problems that have cropped up in the x86_64 code I really
wanted to keep the patchset small and minimal.  But I needed to make
some substantial changes so I would have a low priority IPI I could use
for ensuring irq migration was complete.  When trying to throw out
patches I kept finding things that were either bug fixes or relevant
cleanups to the problem I was addressing.

So this patchset is bigger than I would for addressing the issue irq
migration issue but there is nothing that isn't a simple cleanup or
isn't a real bug fix.  So I think we need it all one way or another.

I have lost track of who has what in which tree or who it makes sense
to send this to.  So this patchset is against Linus's latest git tree.
And hopefully we can include this in 2.6.21. 

Eric

[PATCH 01/14] x86_64 irq: Simplfy __assign_irq_vector

[Eric W. Biederman]

By precomputing old_mask I remove an extra if statement, remove an
indentation level and make the code slightly easier to read.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 950682f..357b354 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -685,6 +685,7 @@ static int __assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 	 * 0x80, because int 0x80 is hm, kind of importantish. ;)
 	 */
 	static int current_vector = FIRST_DEVICE_VECTOR, current_offset = 0;
+	cpumask_t old_mask = CPU_MASK_NONE;
 	int old_vector = -1;
 	int cpu;
 
@@ -699,11 +700,12 @@ static int __assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 		cpus_and(*result, irq_domain[irq], mask);
 		if (!cpus_empty(*result))
 			return old_vector;
+		cpus_and(old_mask, irq_domain[irq], cpu_online_map);
 	}
 
 	for_each_cpu_mask(cpu, mask) {
 		cpumask_t domain, new_mask;
-		int new_cpu;
+		int new_cpu, old_cpu;
 		int vector, offset;
 
 		domain = vector_allocation_domain(cpu);
@@ -728,13 +730,8 @@ next:
 		/* Found one! */
 		current_vector = vector;
 		current_offset = offset;
-		if (old_vector >= 0) {
-			cpumask_t old_mask;
-			int old_cpu;
-			cpus_and(old_mask, irq_domain[irq], cpu_online_map);
-			for_each_cpu_mask(old_cpu, old_mask)
-				per_cpu(vector_irq, old_cpu)[old_vector] = -1;
-		}
+		for_each_cpu_mask(old_cpu, old_mask)
+			per_cpu(vector_irq, old_cpu)[old_vector] = -1;
 		for_each_cpu_mask(new_cpu, new_mask)
 			per_cpu(vector_irq, new_cpu)[vector] = irq;
 		irq_vector[irq] = vector;
-- 
1.5.0.g53756

[PATCH 02/14] irq: Remove set_native_irq_info

[Eric W. Biederman]

This patch replaces all instances of "set_native_irq_info(irq, mask)"
with "irq_desc[irq].affinity = mask".  The latter form is clearer
uses fewer abstractions, and makes access to this field uniform
accross different architectures.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/i386/kernel/io_apic.c   |   10 +++++-----
 arch/ia64/kernel/msi_ia64.c  |    2 +-
 arch/ia64/sn/kernel/msi_sn.c |    2 +-
 arch/x86_64/kernel/io_apic.c |   10 +++++-----
 include/linux/irq.h          |   11 -----------
 5 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/arch/i386/kernel/io_apic.c b/arch/i386/kernel/io_apic.c
index 4ccebd4..6fec4da 100644
--- a/arch/i386/kernel/io_apic.c
+++ b/arch/i386/kernel/io_apic.c
@@ -343,7 +343,7 @@ static void set_ioapic_affinity_irq(unsigned int irq, cpumask_t cpumask)
 			break;
 		entry = irq_2_pin + entry->next;
 	}
-	set_native_irq_info(irq, cpumask);
+	irq_desc[irq].affinity = cpumask;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 
@@ -1354,7 +1354,7 @@ static void __init setup_IO_APIC_irqs(void)
 		}
 		spin_lock_irqsave(&ioapic_lock, flags);
 		__ioapic_write_entry(apic, pin, entry);
-		set_native_irq_info(irq, TARGET_CPUS);
+		irq_desc[irq].affinity = TARGET_CPUS;
 		spin_unlock_irqrestore(&ioapic_lock, flags);
 	}
 	}
@@ -2585,7 +2585,7 @@ static void set_msi_irq_affinity(unsigned int irq, cpumask_t mask)
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
-	set_native_irq_info(irq, mask);
+	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */
 
@@ -2669,7 +2669,7 @@ static void set_ht_irq_affinity(unsigned int irq, cpumask_t mask)
 	dest = cpu_mask_to_apicid(mask);
 
 	target_ht_irq(irq, dest);
-	set_native_irq_info(irq, mask);
+	irq_desc[irq].affinity = mask;
 }
 #endif
 
@@ -2875,7 +2875,7 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a
 
 	spin_lock_irqsave(&ioapic_lock, flags);
 	__ioapic_write_entry(ioapic, pin, entry);
-	set_native_irq_info(irq, TARGET_CPUS);
+	irq_desc[irq].affinity = TARGET_CPUS;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 
 	return 0;
diff --git a/arch/ia64/kernel/msi_ia64.c b/arch/ia64/kernel/msi_ia64.c
index 0d05450..e722090 100644
--- a/arch/ia64/kernel/msi_ia64.c
+++ b/arch/ia64/kernel/msi_ia64.c
@@ -60,7 +60,7 @@ static void ia64_set_msi_irq_affinity(unsigned int irq, cpumask_t cpu_mask)
 	msg.address_lo = addr;
 
 	write_msi_msg(irq, &msg);
-	set_native_irq_info(irq, cpu_mask);
+	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */
 
diff --git a/arch/ia64/sn/kernel/msi_sn.c b/arch/ia64/sn/kernel/msi_sn.c
index ea3dc38..49873aa 100644
--- a/arch/ia64/sn/kernel/msi_sn.c
+++ b/arch/ia64/sn/kernel/msi_sn.c
@@ -204,7 +204,7 @@ static void sn_set_msi_irq_affinity(unsigned int irq, cpumask_t cpu_mask)
 	msg.address_lo = (u32)(bus_addr & 0x00000000ffffffff);
 
 	write_msi_msg(irq, &msg);
-	set_native_irq_info(irq, cpu_mask);
+	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */
 
diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 357b354..1847213 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -261,7 +261,7 @@ static void set_ioapic_affinity_irq(unsigned int irq, cpumask_t mask)
 
 	spin_lock_irqsave(&ioapic_lock, flags);
 	__target_IO_APIC_irq(irq, dest, vector);
-	set_native_irq_info(irq, mask);
+	irq_desc[irq].affinity = mask;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 #endif
@@ -857,7 +857,7 @@ static void __init setup_IO_APIC_irq(int apic, int pin, int idx, int irq)
 	ioapic_write_entry(apic, pin, entry);
 
 	spin_lock_irqsave(&ioapic_lock, flags);
-	set_native_irq_info(irq, TARGET_CPUS);
+	irq_desc[irq].affinity = TARGET_CPUS;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 
 }
@@ -1930,7 +1930,7 @@ static void set_msi_irq_affinity(unsigned int irq, cpumask_t mask)
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
-	set_native_irq_info(irq, mask);
+	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */
 
@@ -2018,7 +2018,7 @@ static void set_ht_irq_affinity(unsigned int irq, cpumask_t mask)
 	dest = cpu_mask_to_apicid(tmp);
 
 	target_ht_irq(irq, dest, vector);
-	set_native_irq_info(irq, mask);
+	irq_desc[irq].affinity = mask;
 }
 #endif
 
@@ -2143,7 +2143,7 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int triggering, int p
 	ioapic_write_entry(ioapic, pin, entry);
 
 	spin_lock_irqsave(&ioapic_lock, flags);
-	set_native_irq_info(irq, TARGET_CPUS);
+	irq_desc[irq].affinity = TARGET_CPUS;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
 
 	return 0;
diff --git a/include/linux/irq.h b/include/linux/irq.h
index aa5b3e6..b0a44b8 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -200,17 +200,6 @@ extern int setup_irq(unsigned int irq, struct irqaction *new);
 #endif
 
 #ifdef CONFIG_SMP
-static inline void set_native_irq_info(int irq, cpumask_t mask)
-{
-	irq_desc[irq].affinity = mask;
-}
-#else
-static inline void set_native_irq_info(int irq, cpumask_t mask)
-{
-}
-#endif
-
-#ifdef CONFIG_SMP
 
 #if defined(CONFIG_GENERIC_PENDING_IRQ) || defined(CONFIG_IRQBALANCE)
 
-- 
1.5.0.g53756

[PATCH 03/14] x86_64 irq: Kill declaration of removed array, interrupt

[Eric W. Biederman]

It's dead Jim.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 1847213..2d154e1 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -793,8 +793,6 @@ void __setup_vector_irq(int cpu)
 }
 
 
-extern void (*interrupt[NR_IRQS])(void);
-
 static struct irq_chip ioapic_chip;
 
 #define IOAPIC_AUTO	-1
-- 
1.5.0.g53756

[PATCH 04/14] x86_64 irq: Remove the unused vector parameter from ioapic_register_intr

[Eric W. Biederman]

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 2d154e1..a69c38b 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -799,7 +799,7 @@ static struct irq_chip ioapic_chip;
 #define IOAPIC_EDGE	0
 #define IOAPIC_LEVEL	1
 
-static void ioapic_register_intr(int irq, int vector, unsigned long trigger)
+static void ioapic_register_intr(int irq, unsigned long trigger)
 {
 	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
 			trigger == IOAPIC_LEVEL)
@@ -847,7 +847,7 @@ static void __init setup_IO_APIC_irq(int apic, int pin, int idx, int irq)
 		entry.dest = cpu_mask_to_apicid(mask);
 		entry.vector = vector;
 
-		ioapic_register_intr(irq, vector, IOAPIC_AUTO);
+		ioapic_register_intr(irq, IOAPIC_AUTO);
 		if (!apic && (irq < 16))
 			disable_8259A_irq(irq);
 	}
@@ -2133,7 +2133,7 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int triggering, int p
 	       mp_ioapics[ioapic].mpc_apicid, pin, entry.vector, irq,
 	       triggering, polarity);
 
-	ioapic_register_intr(irq, entry.vector, triggering);
+	ioapic_register_intr(irq, triggering);
 
 	if (!ioapic && (irq < 16))
 		disable_8259A_irq(irq);
-- 
1.5.0.g53756

[PATCH 05/14] x86_64 irq: Refactor setup_IO_APIC_irq

[Eric W. Biederman]

Currently we have two routines that do practically the same thing
setup_IO_APIC_irq and io_apic_set_pci_routing.  This patch makes
setup_IO_APIC_irq the common factor of these two previous routines.
For setup_IO_APIC_irq all that was needed was to pass the trigger
and polarity to make the code a proper subset of io_apic_set_pci_routing.

Hopefully consolidating these two routines will improve maintenance
there were several differences that simply appear to be one routine
or the other getting it wrong.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |  126 +++++++++++------------------------------
 1 files changed, 34 insertions(+), 92 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index a69c38b..e064838 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -615,22 +615,6 @@ static int pin_2_irq(int idx, int apic, int pin)
 	return irq;
 }
 
-static inline int IO_APIC_irq_trigger(int irq)
-{
-	int apic, idx, pin;
-
-	for (apic = 0; apic < nr_ioapics; apic++) {
-		for (pin = 0; pin < nr_ioapic_registers[apic]; pin++) {
-			idx = find_irq_entry(apic,pin,mp_INT);
-			if ((idx != -1) && (irq == pin_2_irq(idx,apic,pin)))
-				return irq_trigger(idx);
-		}
-	}
-	/*
-	 * nonexistent IRQs are edge default
-	 */
-	return 0;
-}
 
 /* irq_vectors is indexed by the sum of all RTEs in all I/O APICs. */
 static u8 irq_vector[NR_IRQ_VECTORS] __read_mostly = {
@@ -795,26 +779,36 @@ void __setup_vector_irq(int cpu)
 
 static struct irq_chip ioapic_chip;
 
-#define IOAPIC_AUTO	-1
-#define IOAPIC_EDGE	0
-#define IOAPIC_LEVEL	1
-
 static void ioapic_register_intr(int irq, unsigned long trigger)
 {
-	if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
-			trigger == IOAPIC_LEVEL)
+	if (trigger)
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_fasteoi_irq, "fasteoi");
 	else
 		set_irq_chip_and_handler_name(irq, &ioapic_chip,
 					      handle_edge_irq, "edge");
 }
-static void __init setup_IO_APIC_irq(int apic, int pin, int idx, int irq)
+
+static void setup_IO_APIC_irq(int apic, int pin, unsigned int irq,
+			      int trigger, int polarity)
 {
 	struct IO_APIC_route_entry entry;
+	cpumask_t mask;
 	int vector;
 	unsigned long flags;
 
+	if (!IO_APIC_IRQ(irq))
+		return;
+
+	vector = assign_irq_vector(irq, TARGET_CPUS, &mask);
+	if (vector < 0)
+		return;
+
+	apic_printk(APIC_VERBOSE,KERN_DEBUG
+		    "IOAPIC[%d]: Set routing entry (%d-%d -> 0x%x -> "
+		    "IRQ %d Mode:%i Active:%i)\n",
+		    apic, mp_ioapics[apic].mpc_apicid, pin, vector,
+		    irq, trigger, polarity);
 
 	/*
 	 * add it to the IO-APIC irq-routing table:
@@ -823,41 +817,27 @@ static void __init setup_IO_APIC_irq(int apic, int pin, int idx, int irq)
 
 	entry.delivery_mode = INT_DELIVERY_MODE;
 	entry.dest_mode = INT_DEST_MODE;
+	entry.dest = cpu_mask_to_apicid(mask);
 	entry.mask = 0;				/* enable IRQ */
-	entry.dest = cpu_mask_to_apicid(TARGET_CPUS);
-
-	entry.trigger = irq_trigger(idx);
-	entry.polarity = irq_polarity(idx);
+	entry.trigger = trigger;
+	entry.polarity = polarity;
+	entry.vector = vector;
 
-	if (irq_trigger(idx)) {
-		entry.trigger = 1;
+	/* Mask level triggered irqs.
+	 * Use IRQ_DELAYED_DISABLE for edge triggered irqs.
+	 */
+	if (trigger)
 		entry.mask = 1;
-		entry.dest = cpu_mask_to_apicid(TARGET_CPUS);
-	}
-
-	if (!apic && !IO_APIC_IRQ(irq))
-		return;
 
-	if (IO_APIC_IRQ(irq)) {
-		cpumask_t mask;
-		vector = assign_irq_vector(irq, TARGET_CPUS, &mask);
-		if (vector < 0)
-			return;
-
-		entry.dest = cpu_mask_to_apicid(mask);
-		entry.vector = vector;
-
-		ioapic_register_intr(irq, IOAPIC_AUTO);
-		if (!apic && (irq < 16))
-			disable_8259A_irq(irq);
-	}
+	ioapic_register_intr(irq, trigger);
+	if (irq < 16)
+		disable_8259A_irq(irq);
 
 	ioapic_write_entry(apic, pin, entry);
 
 	spin_lock_irqsave(&ioapic_lock, flags);
 	irq_desc[irq].affinity = TARGET_CPUS;
 	spin_unlock_irqrestore(&ioapic_lock, flags);
-
 }
 
 static void __init setup_IO_APIC_irqs(void)
@@ -882,8 +862,8 @@ static void __init setup_IO_APIC_irqs(void)
 		irq = pin_2_irq(idx, apic, pin);
 		add_pin_to_irq(irq, apic, pin);
 
-		setup_IO_APIC_irq(apic, pin, idx, irq);
-
+		setup_IO_APIC_irq(apic, pin, irq,
+				  irq_trigger(idx), irq_polarity(idx));
 	}
 	}
 
@@ -2090,11 +2070,6 @@ int __init io_apic_get_redir_entries (int ioapic)
 
 int io_apic_set_pci_routing (int ioapic, int pin, int irq, int triggering, int polarity)
 {
-	struct IO_APIC_route_entry entry;
-	unsigned long flags;
-	int vector;
-	cpumask_t mask;
-
 	if (!IO_APIC_IRQ(irq)) {
 		apic_printk(APIC_QUIET,KERN_ERR "IOAPIC[%d]: Invalid reference to IRQ 0\n",
 			ioapic);
@@ -2107,42 +2082,7 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int triggering, int p
 	if (irq >= 16)
 		add_pin_to_irq(irq, ioapic, pin);
 
-
-	vector = assign_irq_vector(irq, TARGET_CPUS, &mask);
-	if (vector < 0)
-		return vector;
-
-	/*
-	 * Generate a PCI IRQ routing entry and program the IOAPIC accordingly.
-	 * Note that we mask (disable) IRQs now -- these get enabled when the
-	 * corresponding device driver registers for this IRQ.
-	 */
-
-	memset(&entry,0,sizeof(entry));
-
-	entry.delivery_mode = INT_DELIVERY_MODE;
-	entry.dest_mode = INT_DEST_MODE;
-	entry.dest = cpu_mask_to_apicid(mask);
-	entry.trigger = triggering;
-	entry.polarity = polarity;
-	entry.mask = 1;					 /* Disabled (masked) */
-	entry.vector = vector & 0xff;
-
-	apic_printk(APIC_VERBOSE,KERN_DEBUG "IOAPIC[%d]: Set PCI routing entry (%d-%d -> 0x%x -> "
-		"IRQ %d Mode:%i Active:%i)\n", ioapic, 
-	       mp_ioapics[ioapic].mpc_apicid, pin, entry.vector, irq,
-	       triggering, polarity);
-
-	ioapic_register_intr(irq, triggering);
-
-	if (!ioapic && (irq < 16))
-		disable_8259A_irq(irq);
-
-	ioapic_write_entry(ioapic, pin, entry);
-
-	spin_lock_irqsave(&ioapic_lock, flags);
-	irq_desc[irq].affinity = TARGET_CPUS;
-	spin_unlock_irqrestore(&ioapic_lock, flags);
+	setup_IO_APIC_irq(ioapic, pin, irq, triggering, polarity);
 
 	return 0;
 }
@@ -2175,7 +2115,9 @@ void __init setup_ioapic_dest(void)
 			 * cpu is online.
 			 */
 			if(!irq_vector[irq])
-				setup_IO_APIC_irq(ioapic, pin, irq_entry, irq);
+				setup_IO_APIC_irq(ioapic, pin, irq,
+						  irq_trigger(irq_entry),
+						  irq_polarity(irq_entry));
 			else
 				set_ioapic_affinity_irq(irq, TARGET_CPUS);
 		}
-- 
1.5.0.g53756

[PATCH 06/14] x86_64 irq: Simplfiy the set_affinity logic.

[Eric W. Biederman]

For some reason the code has been picking TARGET_CPUS when asked to
set the affinity to an empty set of cpus.  That is just silly it's
extra work.  Instead if there are no cpus to set the affinity to we
should just give up immediately.  That is simpler and a little more
intuitive.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index e064838..74671de 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -244,9 +244,7 @@ static void set_ioapic_affinity_irq(unsigned int irq, cpumask_t mask)
 
 	cpus_and(tmp, mask, cpu_online_map);
 	if (cpus_empty(tmp))
-		tmp = TARGET_CPUS;
-
-	cpus_and(mask, tmp, CPU_MASK_ALL);
+		return;
 
 	vector = assign_irq_vector(irq, mask, &tmp);
 	if (vector < 0)
@@ -1890,9 +1888,7 @@ static void set_msi_irq_affinity(unsigned int irq, cpumask_t mask)
 
 	cpus_and(tmp, mask, cpu_online_map);
 	if (cpus_empty(tmp))
-		tmp = TARGET_CPUS;
-
-	cpus_and(mask, tmp, CPU_MASK_ALL);
+		return;
 
 	vector = assign_irq_vector(irq, mask, &tmp);
 	if (vector < 0)
@@ -1985,9 +1981,7 @@ static void set_ht_irq_affinity(unsigned int irq, cpumask_t mask)
 
 	cpus_and(tmp, mask, cpu_online_map);
 	if (cpus_empty(tmp))
-		tmp = TARGET_CPUS;
-
-	cpus_and(mask, tmp, CPU_MASK_ALL);
+		return;
 
 	vector = assign_irq_vector(irq, mask, &tmp);
 	if (vector < 0)
-- 
1.5.0.g53756

[PATCH 07/14] x86_64 irq: In __DO_ACTION perform the FINAL action for every entry.

[Eric W. Biederman]

If we have an irq that comes from multiple io_apic pins the FINAL action
(which is io_apic_sync or nothing) needs to be called for every entry or
else if the two pins come from different io_apics we may not wait until
after the action happens on the io_apic.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 74671de..52fc19b 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -149,11 +149,11 @@ static inline void io_apic_sync(unsigned int apic)
 		reg = io_apic_read(entry->apic, 0x10 + R + pin*2);	\
 		reg ACTION;						\
 		io_apic_modify(entry->apic, reg);			\
+		FINAL;							\
 		if (!entry->next)					\
 			break;						\
 		entry = irq_2_pin + entry->next;			\
 	}								\
-	FINAL;								\
 }
 
 union entry_union {
-- 
1.5.0.g53756

[PATCH 08/14] x86_64 irq: Use NR_IRQS not NR_IRQ_VECTORS

[Eric W. Biederman]

NR_IRQ_VECTORS is currently a compatiblity define set to NR_IRQs.
This patch updates the users of NR_IRQ_VECTORS to use NR_IRQs instead
so that NR_IRQ_VECTORS can be removed.

There is still shared code with arch/i386 that uses NR_IRQ_VECTORS 
so we can't remove the #define just yet :(

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 52fc19b..65d7218 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -74,7 +74,7 @@ int nr_ioapic_registers[MAX_IO_APICS];
  * Rough estimation of how many shared IRQs there are, can
  * be changed anytime.
  */
-#define MAX_PLUS_SHARED_IRQS NR_IRQ_VECTORS
+#define MAX_PLUS_SHARED_IRQS NR_IRQS
 #define PIN_MAP_SIZE (MAX_PLUS_SHARED_IRQS + NR_IRQS)
 
 /*
@@ -615,7 +615,7 @@ static int pin_2_irq(int idx, int apic, int pin)
 
 
 /* irq_vectors is indexed by the sum of all RTEs in all I/O APICs. */
-static u8 irq_vector[NR_IRQ_VECTORS] __read_mostly = {
+static u8 irq_vector[NR_IRQS] __read_mostly = {
 	[0] = FIRST_EXTERNAL_VECTOR + 0,
 	[1] = FIRST_EXTERNAL_VECTOR + 1,
 	[2] = FIRST_EXTERNAL_VECTOR + 2,
@@ -634,7 +634,7 @@ static u8 irq_vector[NR_IRQ_VECTORS] __read_mostly = {
 	[15] = FIRST_EXTERNAL_VECTOR + 15,
 };
 
-static cpumask_t irq_domain[NR_IRQ_VECTORS] __read_mostly = {
+static cpumask_t irq_domain[NR_IRQS] __read_mostly = {
 	[0] = CPU_MASK_ALL,
 	[1] = CPU_MASK_ALL,
 	[2] = CPU_MASK_ALL,
@@ -671,7 +671,7 @@ static int __assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 	int old_vector = -1;
 	int cpu;
 
-	BUG_ON((unsigned)irq >= NR_IRQ_VECTORS);
+	BUG_ON((unsigned)irq >= NR_IRQS);
 
 	/* Only try and allocate irqs on cpus that are present */
 	cpus_and(mask, mask, cpu_online_map);
@@ -758,7 +758,7 @@ void __setup_vector_irq(int cpu)
 	int irq, vector;
 
 	/* Mark the inuse vectors */
-	for (irq = 0; irq < NR_IRQ_VECTORS; ++irq) {
+	for (irq = 0; irq < NR_IRQS; ++irq) {
 		if (!cpu_isset(cpu, irq_domain[irq]))
 			continue;
 		vector = irq_vector[irq];
-- 
1.5.0.g53756

[PATCH 09/14] x86_64 irq: Begin consolidating per_irq data in structures.

[Eric W. Biederman]

Currently the io_apic.c has several parallel arrays for different
kinds of data that can be know about an irq.  The parallel arrays
make the code harder to maintain and make it difficult to remove
the static limits on the number of the number of irqs.

This patch pushes irq_data and irq_vector into a irq_cfg array and
updates the code to use it. 

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86_64/kernel/io_apic.c |  111 +++++++++++++++++++-----------------------
 1 files changed, 50 insertions(+), 61 deletions(-)

diff --git a/arch/x86_64/kernel/io_apic.c b/arch/x86_64/kernel/io_apic.c
index 65d7218..dd6580c 100644
--- a/arch/x86_64/kernel/io_apic.c
+++ b/arch/x86_64/kernel/io_apic.c
@@ -47,6 +47,31 @@
 #include <asm/msidef.h>
 #include <asm/hypertransport.h>
 
+struct irq_cfg {
+	cpumask_t domain;
+	u8 vector;
+};
+
+/* irq_cfg is indexed by the sum of all RTEs in all I/O APICs. */
+struct irq_cfg irq_cfg[NR_IRQS] __read_mostly = {
+	[0] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 0 },
+	[1] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 1 },
+	[2] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 2 },
+	[3] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 3 },
+	[4] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 4 },
+	[5] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 5 },
+	[6] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 6 },
+	[7] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 7 },
+	[8] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 8 },
+	[9] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 9 },
+	[10] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 10 },
+	[11] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 11 },
+	[12] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 12 },
+	[13] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 13 },
+	[14] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 14 },
+	[15] = { .domain = CPU_MASK_ALL, .vector = FIRST_EXTERNAL_VECTOR + 15 },
+};
+
 static int assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result);
 
 #define __apicdebuginit  __init
@@ -613,46 +638,6 @@ static int pin_2_irq(int idx, int apic, int pin)
 	return irq;
 }
 
-
-/* irq_vectors is indexed by the sum of all RTEs in all I/O APICs. */
-static u8 irq_vector[NR_IRQS] __read_mostly = {
-	[0] = FIRST_EXTERNAL_VECTOR + 0,
-	[1] = FIRST_EXTERNAL_VECTOR + 1,
-	[2] = FIRST_EXTERNAL_VECTOR + 2,
-	[3] = FIRST_EXTERNAL_VECTOR + 3,
-	[4] = FIRST_EXTERNAL_VECTOR + 4,
-	[5] = FIRST_EXTERNAL_VECTOR + 5,
-	[6] = FIRST_EXTERNAL_VECTOR + 6,
-	[7] = FIRST_EXTERNAL_VECTOR + 7,
-	[8] = FIRST_EXTERNAL_VECTOR + 8,
-	[9] = FIRST_EXTERNAL_VECTOR + 9,
-	[10] = FIRST_EXTERNAL_VECTOR + 10,
-	[11] = FIRST_EXTERNAL_VECTOR + 11,
-	[12] = FIRST_EXTERNAL_VECTOR + 12,
-	[13] = FIRST_EXTERNAL_VECTOR + 13,
-	[14] = FIRST_EXTERNAL_VECTOR + 14,
-	[15] = FIRST_EXTERNAL_VECTOR + 15,
-};
-
-static cpumask_t irq_domain[NR_IRQS] __read_mostly = {
-	[0] = CPU_MASK_ALL,
-	[1] = CPU_MASK_ALL,
-	[2] = CPU_MASK_ALL,
-	[3] = CPU_MASK_ALL,
-	[4] = CPU_MASK_ALL,
-	[5] = CPU_MASK_ALL,
-	[6] = CPU_MASK_ALL,
-	[7] = CPU_MASK_ALL,
-	[8] = CPU_MASK_ALL,
-	[9] = CPU_MASK_ALL,
-	[10] = CPU_MASK_ALL,
-	[11] = CPU_MASK_ALL,
-	[12] = CPU_MASK_ALL,
-	[13] = CPU_MASK_ALL,
-	[14] = CPU_MASK_ALL,
-	[15] = CPU_MASK_ALL,
-};
-
 static int __assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 {
 	/*
@@ -670,19 +655,21 @@ static int __assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 	cpumask_t old_mask = CPU_MASK_NONE;
 	int old_vector = -1;
 	int cpu;
+	struct irq_cfg *cfg;
 
 	BUG_ON((unsigned)irq >= NR_IRQS);
+	cfg = &irq_cfg[irq];
 
 	/* Only try and allocate irqs on cpus that are present */
 	cpus_and(mask, mask, cpu_online_map);
 
-	if (irq_vector[irq] > 0)
-		old_vector = irq_vector[irq];
+	if (cfg->vector > 0)
+		old_vector = cfg->vector;
 	if (old_vector > 0) {
-		cpus_and(*result, irq_domain[irq], mask);
+		cpus_and(*result, cfg->domain, mask);
 		if (!cpus_empty(*result))
 			return old_vector;
-		cpus_and(old_mask, irq_domain[irq], cpu_online_map);
+		cpus_and(old_mask, cfg->domain, cpu_online_map);
 	}
 
 	for_each_cpu_mask(cpu, mask) {
@@ -716,8 +703,8 @@ next:
 			per_cpu(vector_irq, old_cpu)[old_vector] = -1;
 		for_each_cpu_mask(new_cpu, new_mask)
 			per_cpu(vector_irq, new_cpu)[vector] = irq;
-		irq_vector[irq] = vector;
-		irq_domain[irq] = domain;
+		cfg->vector = vector;
+		cfg->domain = domain;
 		cpus_and(*result, domain, mask);
 		return vector;
 	}
@@ -737,18 +724,21 @@ static int assign_irq_vector(int irq, cpumask_t mask, cpumask_t *result)
 
 static void __clear_irq_vector(int irq)
 {
+	struct irq_cfg *cfg;
 	cpumask_t mask;
 	int cpu, vector;
 
-	BUG_ON(!irq_vector[irq]);
+	BUG_ON((unsigned)irq >= NR_IRQS);
+	cfg = &irq_cfg[irq];
+	BUG_ON(!cfg->vector);
 
-	vector = irq_vector[irq];
-	cpus_and(mask, irq_domain[irq], cpu_online_map);
+	vector = cfg->vector;
+	cpus_and(mask, cfg->domain, cpu_online_map);
 	for_each_cpu_mask(cpu, mask)
 		per_cpu(vector_irq, cpu)[vector] = -1;
 
-	irq_vector[irq] = 0;
-	irq_domain[irq] = CPU_MASK_NONE;
+	cfg->vector = 0;
+	cfg->domain = CPU_MASK_NONE;
 }
 
 void __setup_vector_irq(int cpu)
@@ -759,9 +749,9 @@ void __setup_vector_irq(int cpu)
 
 	/* Mark the inuse vectors */
 	for (irq = 0; irq < NR_IRQS; ++irq) {
-		if (!cpu_isset(cpu, irq_domain[irq]))
+		if (!cpu_isset(cpu, irq_cfg[irq].domain))
 			continue;
-		vector = irq_vector[irq];
+		vector = irq_cfg[irq].vector;
 		per_cpu(vector_irq, cpu)[vector] = irq;
 	}
 	/* Mark the free vectors */
@@ -769,7 +759,7 @@ void __setup_vector_irq(int cpu)
 		irq = per_cpu(vector_irq, cpu)[vector];
 		if (irq < 0)
 			continue;
-		if (!cpu_isset(cpu, irq_domain[irq]))
+		if (!cpu_isset(cpu, irq_cfg[irq].domain))
 			per_cpu(vector_irq, cpu)[vector] = -1;
 	}
 }
@@ -1346,16 +1336,15 @@ static unsigned int startup_ioapic_irq(unsigned int irq)
 
 static int ioapic_retrigger_irq(unsigned int irq)
 {
+	struct irq_cfg *cfg = &irq_cfg[irq];
 	cpumask_t mask;
-	unsigned vector;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vector_lock, flags);
-	vector = irq_vector[irq];
 	cpus_clear(mask);
-	cpu_set(first_cpu(irq_domain[irq]), mask);
+	cpu_set(first_cpu(cfg->domain), mask);
 
-	send_IPI_mask(mask, vector);
+	send_IPI_mask(mask, cfg->vector);
 	spin_unlock_irqrestore(&vector_lock, flags);
 
 	return 1;
@@ -1430,7 +1419,7 @@ static inline void init_IO_APIC_traps(void)
 	 */
 	for (irq = 0; irq < NR_IRQS ; irq++) {
 		int tmp = irq;
-		if (IO_APIC_IRQ(tmp) && !irq_vector[tmp]) {
+		if (IO_APIC_IRQ(tmp) && !irq_cfg[tmp].vector) {
 			/*
 			 * Hmm.. We don't have an entry for this,
 			 * so default to an old-fashioned 8259
@@ -1816,7 +1805,7 @@ int create_irq(void)
 	for (new = (NR_IRQS - 1); new >= 0; new--) {
 		if (platform_legacy_irq(new))
 			continue;
-		if (irq_vector[new] != 0)
+		if (irq_cfg[new].vector != 0)
 			continue;
 		vector = __assign_irq_vector(new, TARGET_CPUS, &mask);
 		if (likely(vector > 0))
@@ -2108,7 +2097,7 @@ void __init setup_ioapic_dest(void)
 			 * when you have too many devices, because at that time only boot
 			 * cpu is online.
 			 */
-			if(!irq_vector[irq])
+			if (!irq_cfg[irq].vector)
 				setup_IO_APIC_irq(ioapic, pin, irq,
 						  irq_trigger(irq_entry),
 						  irq_polarity(irq_entry));
-- 
1.5.0.g53756