[PATCH 2.6.20.3] Flush writes to MSI-X table

[PATCH 2.6.20.3] Flush writes to MSI-X table

[Mitch Williams]

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
enable/disable and rebalancing operations.  Because this is an expensive
operation, we do not perform the read flush after mask/unmask
operations.  Hardware which supports MSI-X typically also supports some
sort of interrupt moderation, so a read-flush is not necessary for
mask/unmask operations.

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

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>

diff -urpN -X dontdiff linux-2.6.20.3-clean/arch/i386/kernel/io_apic.c linux-2.6.20.3/arch/i386/kernel/io_apic.c
--- linux-2.6.20.3-clean/arch/i386/kernel/io_apic.c	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/arch/i386/kernel/io_apic.c	2007-03-22 10:33:47.000000000 -0700
@@ -2597,6 +2597,8 @@ static void set_msi_irq_affinity(unsigne
  */
 static struct irq_chip msi_chip = {
 	.name		= "PCI-MSI",
+	.enable		= enable_msi_irq,
+	.disable	= disable_msi_irq,
 	.unmask		= unmask_msi_irq,
 	.mask		= mask_msi_irq,
 	.ack		= ack_ioapic_irq,
diff -urpN -X dontdiff linux-2.6.20.3-clean/arch/ia64/kernel/msi_ia64.c linux-2.6.20.3/arch/ia64/kernel/msi_ia64.c
--- linux-2.6.20.3-clean/arch/ia64/kernel/msi_ia64.c	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/arch/ia64/kernel/msi_ia64.c	2007-03-22 10:33:47.000000000 -0700
@@ -116,6 +116,8 @@ static int ia64_msi_retrigger_irq(unsign
  */
 static struct irq_chip ia64_msi_chip = {
 	.name		= "PCI-MSI",
+	.enable		= enable_msi_irq,
+	.disable	= disable_msi_irq,
 	.mask		= mask_msi_irq,
 	.unmask		= unmask_msi_irq,
 	.ack		= ia64_ack_msi_irq,
diff -urpN -X dontdiff linux-2.6.20.3-clean/arch/ia64/sn/kernel/msi_sn.c linux-2.6.20.3/arch/ia64/sn/kernel/msi_sn.c
--- linux-2.6.20.3-clean/arch/ia64/sn/kernel/msi_sn.c	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/arch/ia64/sn/kernel/msi_sn.c	2007-03-22 10:33:47.000000000 -0700
@@ -216,6 +216,8 @@ static int sn_msi_retrigger_irq(unsigned
 
 static struct irq_chip sn_msi_chip = {
 	.name		= "PCI-MSI",
+	.enable		= enable_msi_irq,
+	.disable	= disable_msi_irq,
 	.mask		= mask_msi_irq,
 	.unmask		= unmask_msi_irq,
 	.ack		= sn_ack_msi_irq,
diff -urpN -X dontdiff linux-2.6.20.3-clean/arch/x86_64/kernel/io_apic.c linux-2.6.20.3/arch/x86_64/kernel/io_apic.c
--- linux-2.6.20.3-clean/arch/x86_64/kernel/io_apic.c	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/arch/x86_64/kernel/io_apic.c	2007-03-22 10:36:03.000000000 -0700
@@ -1923,6 +1923,7 @@ static void set_msi_irq_affinity(unsigne
 
 	cpus_and(mask, tmp, CPU_MASK_ALL);
 
+	msix_flush_writes(irq);
 	vector = assign_irq_vector(irq, mask, &tmp);
 	if (vector < 0)
 		return;
@@ -1937,6 +1938,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);
 	set_native_irq_info(irq, mask);
 }
 #endif /* CONFIG_SMP */
@@ -1947,6 +1949,8 @@ static void set_msi_irq_affinity(unsigne
  */
 static struct irq_chip msi_chip = {
 	.name		= "PCI-MSI",
+	.enable		= enable_msi_irq,
+	.disable	= disable_msi_irq,
 	.unmask		= unmask_msi_irq,
 	.mask		= mask_msi_irq,
 	.ack		= ack_apic_edge,
diff -urpN -X dontdiff linux-2.6.20.3-clean/drivers/pci/msi.c linux-2.6.20.3/drivers/pci/msi.c
--- linux-2.6.20.3-clean/drivers/pci/msi.c	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/drivers/pci/msi.c	2007-03-22 10:33:47.000000000 -0700
@@ -40,6 +40,29 @@ static int msi_cache_init(void)
 	return 0;
 }
 
+void msix_flush_writes(unsigned int irq)
+{
+	struct msi_desc *entry;
+
+	entry = msi_desc[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;
@@ -161,6 +184,17 @@ void unmask_msi_irq(unsigned int irq)
 	msi_set_mask_bit(irq, 0);
 }
 
+void disable_msi_irq(unsigned int irq)
+{
+	msi_set_mask_bit(irq, 1);
+	msix_flush_writes(irq);
+}
+
+void enable_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);
 static int msi_init(void)
 {
diff -urpN -X dontdiff linux-2.6.20.3-clean/include/linux/msi.h linux-2.6.20.3/include/linux/msi.h
--- linux-2.6.20.3-clean/include/linux/msi.h	2007-02-04 10:44:54.000000000 -0800
+++ linux-2.6.20.3/include/linux/msi.h	2007-03-22 10:33:47.000000000 -0700
@@ -10,9 +10,12 @@ struct msi_msg {
 /* Heper functions */
 extern void mask_msi_irq(unsigned int irq);
 extern void unmask_msi_irq(unsigned int irq);
+extern void disable_msi_irq(unsigned int irq);
+extern void enable_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 {

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Greg KH]

On Thu, Mar 22, 2007 at 02:08:19PM -0700, Mitch Williams wrote:
> 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
> enable/disable and rebalancing operations.  Because this is an expensive
> operation, we do not perform the read flush after mask/unmask
> operations.  Hardware which supports MSI-X typically also supports some
> sort of interrupt moderation, so a read-flush is not necessary for
> mask/unmask operations.
> 
> This patch has been validated with (unreleased) network hardware which
> uses MSI-X.

Is this needed for any hardware that is public today?

Also, it seems a bit too big of a patch for -stable right now,
especially as the mainline patch will not make it into 2.6.22 at the
earliest.

thanks,

greg k-h

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Kok, Auke]

Greg KH wrote:
> On Thu, Mar 22, 2007 at 02:08:19PM -0700, Mitch Williams wrote:
>> 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
>> enable/disable and rebalancing operations.  Because this is an expensive
>> operation, we do not perform the read flush after mask/unmask
>> operations.  Hardware which supports MSI-X typically also supports some
>> sort of interrupt moderation, so a read-flush is not necessary for
>> mask/unmask operations.
>>
>> This patch has been validated with (unreleased) network hardware which
>> uses MSI-X.
> 
> Is this needed for any hardware that is public today?

yes. Every msi-x capable piece of hardware in the field will crash if it does 
any form of interrupt balancing. (okay that is not that much stuff out there... 
I know, but the patch is not that big at all - all it does is subtly add a few 
read flushes to make sure that critical changes in the msix vector tables are 
pushed out at the proper time).

> Also, it seems a bit too big of a patch for -stable right now,
> especially as the mainline patch will not make it into 2.6.22 at the
> earliest.

I think Mitch was way too sensitive when he worded his e-mail. We should really 
be trying to get this fix into 2.6.21 at least.

Mitch, can you re-post this and include Eric Biederman, linux-pci, our intel 
platform guys and perhaps Linus and Andrew?

A lot of vendors (not just us) will be pushing msi-x capable hardware out, and 
this fix is absolutely needed. Getting it in soon is really preferred. Not to 
mention that Mitch has spent well over 8 weeks I think making sure that this is 
indeed the issue and the proper fix...

Auke

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Greg KH]

On Fri, Mar 23, 2007 at 02:50:45PM -0700, Kok, Auke wrote:
> Greg KH wrote:
> >On Thu, Mar 22, 2007 at 02:08:19PM -0700, Mitch Williams wrote:
> >>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
> >>enable/disable and rebalancing operations.  Because this is an expensive
> >>operation, we do not perform the read flush after mask/unmask
> >>operations.  Hardware which supports MSI-X typically also supports some
> >>sort of interrupt moderation, so a read-flush is not necessary for
> >>mask/unmask operations.
> >>
> >>This patch has been validated with (unreleased) network hardware which
> >>uses MSI-X.
> >
> >Is this needed for any hardware that is public today?
> 
> yes. Every msi-x capable piece of hardware in the field will crash if it 
> does any form of interrupt balancing. (okay that is not that much stuff out 
> there... I know, but the patch is not that big at all - all it does is 
> subtly add a few read flushes to make sure that critical changes in the 
> msix vector tables are pushed out at the proper time).
> 
> >Also, it seems a bit too big of a patch for -stable right now,
> >especially as the mainline patch will not make it into 2.6.22 at the
> >earliest.
> 
> I think Mitch was way too sensitive when he worded his e-mail. We should 
> really be trying to get this fix into 2.6.21 at least.
> 
> Mitch, can you re-post this and include Eric Biederman, linux-pci, our 
> intel platform guys and perhaps Linus and Andrew?
> 
> A lot of vendors (not just us) will be pushing msi-x capable hardware out, 
> and this fix is absolutely needed. Getting it in soon is really preferred. 
> Not to mention that Mitch has spent well over 8 weeks I think making sure 
> that this is indeed the issue and the proper fix...

Well, I'm sure you can agree that it is _very_ late in the 2.6.21
release cycle to expect to get this in for that kernel.  How about
waiting for 2.6.22 and if it's a big deal, getting it into the
2.6.21-stable tree if needed.

So far I have not seen any bug reports that this patch would fix, have
you?

thanks,

greg k-h

RE: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Williams, Mitch A]

Greg KH wrote:
>Well, I'm sure you can agree that it is _very_ late in the 2.6.21
>release cycle to expect to get this in for that kernel.  How about
>waiting for 2.6.22 and if it's a big deal, getting it into the
>2.6.21-stable tree if needed.
>
>So far I have not seen any bug reports that this patch would fix, have
>you?

Well, I've seen several bug reports on this issue -- but they're all
internal to Intel.

However, we do have here a real bug, which shows up on real hardware,
which will be released soon.  Obviously, I can't discuss release
schedules, but "soon" is a good word to use.  You might find out more if
you read The Register (wink, wink).

Given the time frame for release of 2.6.21, I'd be fine with skipping
2.6.20.x, and putting this in 2.6.21.  But we really don't want to wait
for 2.6.22.

I'll take Auke's suggestion and repost this Monday and cc linux-pci and
a slew of other people.

-Mitch

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Greg KH]

On Fri, Mar 23, 2007 at 05:24:23PM -0700, Williams, Mitch A wrote:
> Greg KH wrote:
> >Well, I'm sure you can agree that it is _very_ late in the 2.6.21
> >release cycle to expect to get this in for that kernel.  How about
> >waiting for 2.6.22 and if it's a big deal, getting it into the
> >2.6.21-stable tree if needed.
> >
> >So far I have not seen any bug reports that this patch would fix, have
> >you?
> 
> Well, I've seen several bug reports on this issue -- but they're all
> internal to Intel.
> 
> However, we do have here a real bug, which shows up on real hardware,
> which will be released soon.  Obviously, I can't discuss release
> schedules, but "soon" is a good word to use.  You might find out more if
> you read The Register (wink, wink).

Ok, but again, as this is something that no one outside of a company can
see, it doesn't really make sense to rush it into the kernel.

> Given the time frame for release of 2.6.21, I'd be fine with skipping
> 2.6.20.x, and putting this in 2.6.21.  But we really don't want to wait
> for 2.6.22.

I think it needs to wait, especially given that there is no public
hardware yet.

I'll add this to my queue.

thanks,

greg k-h

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Greg KH]

On Fri, Mar 23, 2007 at 05:28:02PM -0700, Greg KH wrote:
> On Fri, Mar 23, 2007 at 05:24:23PM -0700, Williams, Mitch A wrote:
> > Greg KH wrote:
> > >Well, I'm sure you can agree that it is _very_ late in the 2.6.21
> > >release cycle to expect to get this in for that kernel.  How about
> > >waiting for 2.6.22 and if it's a big deal, getting it into the
> > >2.6.21-stable tree if needed.
> > >
> > >So far I have not seen any bug reports that this patch would fix, have
> > >you?
> > 
> > Well, I've seen several bug reports on this issue -- but they're all
> > internal to Intel.
> > 
> > However, we do have here a real bug, which shows up on real hardware,
> > which will be released soon.  Obviously, I can't discuss release
> > schedules, but "soon" is a good word to use.  You might find out more if
> > you read The Register (wink, wink).
> 
> Ok, but again, as this is something that no one outside of a company can
> see, it doesn't really make sense to rush it into the kernel.
> 
> > Given the time frame for release of 2.6.21, I'd be fine with skipping
> > 2.6.20.x, and putting this in 2.6.21.  But we really don't want to wait
> > for 2.6.22.
> 
> I think it needs to wait, especially given that there is no public
> hardware yet.
> 
> I'll add this to my queue.

No, nevermind, I'll wait till it hits linux-pci and gets review from the
people there, as there are a _ton_ of other pending MSI patches that you
will need to be aware of, as they might conflict with this patch.
Please see the linux-pci archives for details of them.

thanks,

greg k-h

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Kok, Auke]

Greg KH wrote:
> On Fri, Mar 23, 2007 at 05:28:02PM -0700, Greg KH wrote:
>> On Fri, Mar 23, 2007 at 05:24:23PM -0700, Williams, Mitch A wrote:
>>> Greg KH wrote:
>>>> Well, I'm sure you can agree that it is _very_ late in the 2.6.21
>>>> release cycle to expect to get this in for that kernel.  How about
>>>> waiting for 2.6.22 and if it's a big deal, getting it into the
>>>> 2.6.21-stable tree if needed.
>>>>
>>>> So far I have not seen any bug reports that this patch would fix, have
>>>> you?
>>> Well, I've seen several bug reports on this issue -- but they're all
>>> internal to Intel.
>>>
>>> However, we do have here a real bug, which shows up on real hardware,
>>> which will be released soon.  Obviously, I can't discuss release
>>> schedules, but "soon" is a good word to use.  You might find out more if
>>> you read The Register (wink, wink).
>> Ok, but again, as this is something that no one outside of a company can
>> see, it doesn't really make sense to rush it into the kernel.
>>
>>> Given the time frame for release of 2.6.21, I'd be fine with skipping
>>> 2.6.20.x, and putting this in 2.6.21.  But we really don't want to wait
>>> for 2.6.22.
>> I think it needs to wait, especially given that there is no public
>> hardware yet.
>>
>> I'll add this to my queue.
> 
> No, nevermind, I'll wait till it hits linux-pci and gets review from the
> people there, as there are a _ton_ of other pending MSI patches that you
> will need to be aware of, as they might conflict with this patch.
> Please see the linux-pci archives for details of them.

Actually Mitch and me have been monitoring those and applying them as they came 
in for the last two months as some of those partially impacted (improved) the 
issue. The read flush to update the msi-x tables is the only thing missing right 
now.

Auke'

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Greg KH]

On Sat, Mar 24, 2007 at 04:33:41PM -0700, Kok, Auke wrote:
> Greg KH wrote:
> >On Fri, Mar 23, 2007 at 05:28:02PM -0700, Greg KH wrote:
> >>On Fri, Mar 23, 2007 at 05:24:23PM -0700, Williams, Mitch A wrote:
> >>>Greg KH wrote:
> >>>>Well, I'm sure you can agree that it is _very_ late in the 2.6.21
> >>>>release cycle to expect to get this in for that kernel.  How about
> >>>>waiting for 2.6.22 and if it's a big deal, getting it into the
> >>>>2.6.21-stable tree if needed.
> >>>>
> >>>>So far I have not seen any bug reports that this patch would fix, have
> >>>>you?
> >>>Well, I've seen several bug reports on this issue -- but they're all
> >>>internal to Intel.
> >>>
> >>>However, we do have here a real bug, which shows up on real hardware,
> >>>which will be released soon.  Obviously, I can't discuss release
> >>>schedules, but "soon" is a good word to use.  You might find out more if
> >>>you read The Register (wink, wink).
> >>Ok, but again, as this is something that no one outside of a company can
> >>see, it doesn't really make sense to rush it into the kernel.
> >>
> >>>Given the time frame for release of 2.6.21, I'd be fine with skipping
> >>>2.6.20.x, and putting this in 2.6.21.  But we really don't want to wait
> >>>for 2.6.22.
> >>I think it needs to wait, especially given that there is no public
> >>hardware yet.
> >>
> >>I'll add this to my queue.
> >
> >No, nevermind, I'll wait till it hits linux-pci and gets review from the
> >people there, as there are a _ton_ of other pending MSI patches that you
> >will need to be aware of, as they might conflict with this patch.
> >Please see the linux-pci archives for details of them.
> 
> Actually Mitch and me have been monitoring those and applying them as they 
> came in for the last two months as some of those partially impacted 
> (improved) the issue. The read flush to update the msi-x tables is the only 
> thing missing right now.

Are you including the 21 set MSI patch that want to linux-pci two days
ago?

I don't understand your need to try to rush an api change like this in
so quickly in an area that has a lot of churn and disagreement lately.
_Especially_ so late in the release cycle, and with no hardware publicly
availble.  What is the pressing need here?

thanks,

greg k-h

Re: [PATCH 2.6.20.3] Flush writes to MSI-X table

[Roland Dreier]

 > I don't understand your need to try to rush an api change like this in
 > so quickly in an area that has a lot of churn and disagreement lately.
 > _Especially_ so late in the release cycle, and with no hardware publicly
 > availble.

I'm not sure I understood this thread properly, but if I did understand
correctly then this bug affects IRQ balancing on any device with MSI-X
enabled.  In which case, there's plenty of publicly available hardware
with MSI-X support (including drivers in the mainline tree for a long
time).  A quick grep for pci_enable_msix finds plenty of drivers using
MSI-X now: cciss, ib_mthca, cxgb3, forcedeth, s2io, qla2xxx.

 - R.