MSI: read-flush MSI-X table (rev 2)

PATCH In review Mitch Williams 4 participants 9 messages Mar 29, 2007 – Mar 30, 2007 PCI

lore

Share & tools

lore

Mitch Williams

author

2007-03-29 16:41 UTC #0 lore raw

PATCH 1/1 MSI: read-flush MSI-X table (rev 2)

This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.

Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.

This patch performs a read flush after writes to the MSI-X table for mask/unmask and rebalancing operations.

This patch has been validated with (unreleased) network hardware which uses MSI-X.

Revised with input from Eric Biederman.

Signed-off-by Mitch Williams <[email protected]>

arch/i386/kernel/io_apic.c +1 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/i386/kernel/io_apic.c linux-2.6.21-rc5/arch/i386/kernel/io_apic.c
@@ -2584,6 +2584,7 @@ static void set_msi_irq_affinity(unsigne
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */

arch/ia64/kernel/msi_ia64.c +1 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/ia64/kernel/msi_ia64.c linux-2.6.21-rc5/arch/ia64/kernel/msi_ia64.c
@@ -60,6 +60,7 @@ static void ia64_set_msi_irq_affinity(un
 	msg.address_lo = addr;
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */

arch/ia64/sn/kernel/msi_sn.c +1 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/ia64/sn/kernel/msi_sn.c linux-2.6.21-rc5/arch/ia64/sn/kernel/msi_sn.c
@@ -204,6 +204,7 @@ static void sn_set_msi_irq_affinity(unsi
 	msg.address_lo = (u32)(bus_addr & 0x00000000ffffffff);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */

arch/x86_64/kernel/io_apic.c +1 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/x86_64/kernel/io_apic.c linux-2.6.21-rc5/arch/x86_64/kernel/io_apic.c
@@ -1956,6 +1956,7 @@ static void set_msi_irq_affinity(unsigne
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */

drivers/pci/msi.c +25 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/drivers/pci/msi.c linux-2.6.21-rc5/drivers/pci/msi.c
@@ -68,6 +68,29 @@ static void msix_set_enable(struct pci_d
 	}
 }
 
+void msix_flush_writes(unsigned int irq)
+{
+	struct msi_desc *entry;
+
+	entry = get_irq_msi(irq);
+	BUG_ON(!entry || !entry->dev);
+	switch (entry->msi_attrib.type) {
+	case PCI_CAP_ID_MSI:
+		/* nothing to do */
+		break;
+	case PCI_CAP_ID_MSIX:
+	{
+		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
+			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
+		readl(entry->mask_base + offset);
+		break;
+	}
+	default:
+		BUG();
+		break;
+	}
+}
+
 static void msi_set_mask_bit(unsigned int irq, int flag)
 {
 	struct msi_desc *entry;

@@ -186,11 +209,13 @@ void write_msi_msg(unsigned int irq, str
 void mask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bit(irq, 1);
+	msix_flush_writes(irq);
 }
 
 void unmask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bit(irq, 0);
+	msix_flush_writes(irq);
 }
 
 static int msi_free_irq(struct pci_dev* dev, int irq);

include/linux/msi.h +1 -0

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/include/linux/msi.h linux-2.6.21-rc5/include/linux/msi.h
@@ -12,6 +12,7 @@ extern void mask_msi_irq(unsigned int ir
 extern void unmask_msi_irq(unsigned int irq);
 extern void read_msi_msg(unsigned int irq, struct msi_msg *msg);
 extern void write_msi_msg(unsigned int irq, struct msi_msg *msg);
+extern void msix_flush_writes(unsigned int irq);
 
 struct msi_desc {
 	struct {

Eric W. Biederman

2007-03-29 17:00 UTC #1 lore raw

Quoted 8 lines · Mitch Williams

This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.

Quoted 7 lines

This patch performs a read flush after writes to the MSI-X table for mask/unmask and rebalancing operations.
This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.

Do we still need the flush the set affinity routines? Shouldn't flush in mask and unmask should now be enough?

Quoted 120 lines

Signed-off-by Mitch Williams <[email protected]>
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/i386/kernel/io_apic.c linux-2.6.21-rc5/arch/i386/kernel/io_apic.c
--- linux-2.6.21-rc5-clean/arch/i386/kernel/io_apic.c 2007-03-28 10:05:22.000000000 -0700
+++ linux-2.6.21-rc5/arch/i386/kernel/io_apic.c 2007-03-28 10:19:14.000000000 -0700
arch/i386/kernel/io_apic.c +1 -0
@@ -2584,6 +2584,7 @@ static void set_msi_irq_affinity(unsigne
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/ia64/kernel/msi_ia64.c linux-2.6.21-rc5/arch/ia64/kernel/msi_ia64.c
--- linux-2.6.21-rc5-clean/arch/ia64/kernel/msi_ia64.c 2007-03-28 10:05:22.000000000 -0700
+++ linux-2.6.21-rc5/arch/ia64/kernel/msi_ia64.c 2007-03-28 09:34:27.000000000 -0700
arch/ia64/kernel/msi_ia64.c +1 -0
@@ -60,6 +60,7 @@ static void ia64_set_msi_irq_affinity(un
 	msg.address_lo = addr;
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/ia64/sn/kernel/msi_sn.c linux-2.6.21-rc5/arch/ia64/sn/kernel/msi_sn.c
--- linux-2.6.21-rc5-clean/arch/ia64/sn/kernel/msi_sn.c 2007-03-28 10:05:22.000000000 -0700
+++ linux-2.6.21-rc5/arch/ia64/sn/kernel/msi_sn.c 2007-03-28 09:34:06.000000000 -0700
arch/ia64/sn/kernel/msi_sn.c +1 -0
@@ -204,6 +204,7 @@ static void sn_set_msi_irq_affinity(unsi
 	msg.address_lo = (u32)(bus_addr & 0x00000000ffffffff);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = cpu_mask;
 }
 #endif /* CONFIG_SMP */
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/arch/x86_64/kernel/io_apic.c linux-2.6.21-rc5/arch/x86_64/kernel/io_apic.c
--- linux-2.6.21-rc5-clean/arch/x86_64/kernel/io_apic.c 2007-03-28 10:05:22.000000000 -0700
+++ linux-2.6.21-rc5/arch/x86_64/kernel/io_apic.c 2007-03-28 10:18:52.000000000 -0700
arch/i386/kernel/io_apic.c +1 -0
@@ -1956,6 +1956,7 @@ static void set_msi_irq_affinity(unsigne
 	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
 
 	write_msi_msg(irq, &msg);
+	msix_flush_writes(irq);
 	irq_desc[irq].affinity = mask;
 }
 #endif /* CONFIG_SMP */
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/drivers/pci/msi.c linux-2.6.21-rc5/drivers/pci/msi.c
drivers/pci/msi.c renamed from drivers/pci/msi.c 2007-03-28 10:05:24.000000000 -0700 +25 -0
@@ -68,6 +68,29 @@ static void msix_set_enable(struct pci_d
 	}
 }
 
+void msix_flush_writes(unsigned int irq)
+{
+	struct msi_desc *entry;
+
+	entry = get_irq_msi(irq);
+	BUG_ON(!entry || !entry->dev);
+	switch (entry->msi_attrib.type) {
+	case PCI_CAP_ID_MSI:
+		/* nothing to do */
+		break;
+	case PCI_CAP_ID_MSIX:
+	{
+		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
+			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
+		readl(entry->mask_base + offset);
+		break;
+	}
+	default:
+		BUG();
+		break;
+	}
+}
+
 static void msi_set_mask_bit(unsigned int irq, int flag)
 {
 	struct msi_desc *entry;
@@ -186,11 +209,13 @@ void write_msi_msg(unsigned int irq, str
 void mask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bit(irq, 1);
+	msix_flush_writes(irq);
 }
 
 void unmask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bit(irq, 0);
+	msix_flush_writes(irq);
 }
 
 static int msi_free_irq(struct pci_dev* dev, int irq);
diff -urpN -X dontdiff linux-2.6.21-rc5-clean/include/linux/msi.h linux-2.6.21-rc5/include/linux/msi.h
--- linux-2.6.21-rc5-clean/include/linux/msi.h 2007-03-28 10:05:25.000000000 -0700
+++ linux-2.6.21-rc5/include/linux/msi.h 2007-03-28 09:21:51.000000000 -0700
drivers/pci/msi.c renamed from drivers/pci/msi.c 2007-03-28 10:05:24.000000000 -0700 +1 -0
@@ -12,6 +12,7 @@ extern void mask_msi_irq(unsigned int ir
 extern void unmask_msi_irq(unsigned int irq);
 extern void read_msi_msg(unsigned int irq, struct msi_msg *msg);
 extern void write_msi_msg(unsigned int irq, struct msi_msg *msg);
+extern void msix_flush_writes(unsigned int irq);
 
 struct msi_desc {
 	struct {

Williams, Mitch A

author

2007-03-29 22:01 UTC #2 lore raw

Quoted 2 lines · Eric W. Biederman

Do we still need the flush the set affinity routines? Shouldn't flush in mask and unmask should now be enough?

Yeah, I think you're right. I've removed that call, and we're running some basic validation on the change. I'll post a new patch tomorrow AM.
-Mitch

Chuck Ebbert

2007-03-29 17:25 UTC #3 lore raw

In reply to Mitch Williams

Quoted 11 lines · Mitch Williams

This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.
This patch performs a read flush after writes to the MSI-X table for mask/unmask and rebalancing operations.

Quoted 7 lines

This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.
Signed-off-by Mitch Williams <[email protected]>

Are you going to post one for 2.6.20 as well? Some people might be interested...

Randy Dunlap

2007-03-29 17:31 UTC #4 lore raw

Quoted 3 lines · On Thu, 29 Mar 2007 13:25:34 -0400 Chuck Ebbert wrote:

Are you going to post one for 2.6.20 as well? Some people might be interested...

Please don't drop cc:s.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Williams, Mitch A

author

2007-03-29 22:06 UTC #5 lore raw

In reply to Chuck Ebbert

Quoted 2 lines · Chuck Ebbert

Are you going to post one for 2.6.20 as well? Some people might be interested...

The first time I posted this patch, Greg KH indicated that he thought it was too intrusive to add to -stable, especially considering that our MSI-X capable hardware isn't in the field yet.

So the answer to your question is, "probably not". -Mitch

Chuck Ebbert

2007-03-29 22:14 UTC #6 lore raw

Quoted 9 lines · Mitch A

Quoted 2 lines · Chuck Ebbert
Are you going to post one for 2.6.20 as well? Some people might be interested...
The first time I posted this patch, Greg KH indicated that he thought it was too intrusive to add to -stable, especially considering that our MSI-X capable hardware isn't in the field yet.
So the answer to your question is, "probably not".

So it's not useful in 2.6.20 for anything that's available today, from any vendor? We can put it in Fedora if you think it is.

Eric W. Biederman

2007-03-30 05:03 UTC #7 lore raw

In reply to Williams, Mitch A

Quoted 7 lines · Mitch A

Quoted 2 lines · Chuck Ebbert
Are you going to post one for 2.6.20 as well? Some people might be interested...
The first time I posted this patch, Greg KH indicated that he thought it was too intrusive to add to -stable, especially considering that our MSI-X capable hardware isn't in the field yet.

There is MSI-X capable hardware supported by the kernel in 2.6.20.

The last version of your patch with the code removed from the set_affinity path is not very intrusive at all, as it just touches msi.c

Eric

Eric W. Biederman

2007-03-30 05:03 UTC #8 lore raw

In reply to Williams, Mitch A

Quoted 7 lines · Mitch A

Quoted 2 lines · Chuck Ebbert
Are you going to post one for 2.6.20 as well? Some people might be interested...
The first time I posted this patch, Greg KH indicated that he thought it was too intrusive to add to -stable, especially considering that our MSI-X capable hardware isn't in the field yet.

There is MSI-X capable hardware supported by the kernel in 2.6.20.

The last version of your patch with the code removed from the set_affinity path is not very intrusive at all, as it just touches msi.c

Eric