Disabling msix interrupts

Disabling msix interrupts

[David Laight]

netdev probably isn't the right list for this, but I suspect people
reading it understand what happens.

I'm fairly sure that an msix interrupt can get raised after
the kernel thinks it has masked it.

When an msix interrupt is disabled I think msi_set_mask_bit()
(in drivers/pci/msi.c) is called to write a '1' to the card's
hardware MSIX mask register (the last 32bit word of the entry).
This function carefully reads back the mask register to flush
the write through the pcie bus.
Except it doesn't, it reads the 'address_lo' register instead! [1]

While this will stop the hardware raising any more interrupts,
it could easily be in the process of raising one.
ie have read the mask, found it zero, read the address and
data, and be in the process of issuing the pcie write.

The pcie write (to disable the interrupt) and readback are seen
by the hardware as (more or less) back to back transfers, so can
both easily overtake the request to raise the interrupt.
The pcie bus is also allowed to make a read completion tlp
overtake a write tlp.
Add in any host-side delays in raising the hardware interrupt
itself, and an interrupt could happen well after it was masked.

More worrying would be any code that tries to change the address
and data associated with an interrupt.
You'd need moderate guard times after the disable and before the
enable to ensure the hardware didn't raise an interrupt with
a mismatch of the old and new values.

[1] Maybe I'll look at the order those cycles actually arrive in.

	David

Re: Disabling msix interrupts

[Alexander Duyck]

On Mon, Feb 6, 2017 at 7:33 AM, David Laight <David.Laight@aculab.com> wrote:
> netdev probably isn't the right list for this, but I suspect people
> reading it understand what happens.
>
> I'm fairly sure that an msix interrupt can get raised after
> the kernel thinks it has masked it.
>
> When an msix interrupt is disabled I think msi_set_mask_bit()
> (in drivers/pci/msi.c) is called to write a '1' to the card's
> hardware MSIX mask register (the last 32bit word of the entry).
> This function carefully reads back the mask register to flush
> the write through the pcie bus.
> Except it doesn't, it reads the 'address_lo' register instead! [1]

Reading any register will force the write to be flushed as all PCI
writes must be completed before a PCI read.  For example in the Intel
drivers we read register 0 to flush a write of any of the other
registers.

> While this will stop the hardware raising any more interrupts,
> it could easily be in the process of raising one.
> ie have read the mask, found it zero, read the address and
> data, and be in the process of issuing the pcie write.
>
> The pcie write (to disable the interrupt) and readback are seen
> by the hardware as (more or less) back to back transfers, so can
> both easily overtake the request to raise the interrupt.

I don't believe this is correct.  On the PCI bus what you should see
is the device aware that interrupts are disabled before the completion
arrives and any MSI messages should arrive before the last read
completion.

> The pcie bus is also allowed to make a read completion tlp
> overtake a write tlp.
> Add in any host-side delays in raising the hardware interrupt
> itself, and an interrupt could happen well after it was masked.

That is only if relaxed ordering is enabled, which I am not sure is
supported for MSI-X interrupts.  That being said though I believe
there are some platforms that could end up seeing a delay in handling
the interrupt if for example the CPU the interrupt was delivered to
was in a sleep state.

> More worrying would be any code that tries to change the address
> and data associated with an interrupt.
> You'd need moderate guard times after the disable and before the
> enable to ensure the hardware didn't raise an interrupt with
> a mismatch of the old and new values.
>
> [1] Maybe I'll look at the order those cycles actually arrive in.
>
>         David

So I have thrown in my $.02 on this, and added the linux-pci mailing
list.  That is a much better place to bring this up rather than
netdev.

Thanks.

- Alex

RE: Disabling msix interrupts

[David Laight]

From: Alexander 
> Sent: 06 February 2017 16:27
> To: David Laight
> On Mon, Feb 6, 2017 at 7:33 AM, David Laight <David.Laight@aculab.com> wrote:
> > netdev probably isn't the right list for this, but I suspect people
> > reading it understand what happens.
> >
> > I'm fairly sure that an msix interrupt can get raised after
> > the kernel thinks it has masked it.
> >
> > When an msix interrupt is disabled I think msi_set_mask_bit()
> > (in drivers/pci/msi.c) is called to write a '1' to the card's
> > hardware MSIX mask register (the last 32bit word of the entry).
> > This function carefully reads back the mask register to flush
> > the write through the pcie bus.
> > Except it doesn't, it reads the 'address_lo' register instead! [1]
> 
> Reading any register will force the write to be flushed as all PCI
> writes must be completed before a PCI read.  For example in the Intel
> drivers we read register 0 to flush a write of any of the other
> registers.

Sorry brain fade on that one.
Although the 'store buffer' on the sparc cpus I used to use would
let reads overtake writes. So you did have to read back the address
of the last write - not sure about modern sparc cpus.

> > While this will stop the hardware raising any more interrupts,
> > it could easily be in the process of raising one.
> > ie have read the mask, found it zero, read the address and
> > data, and be in the process of issuing the pcie write.
> >
> > The pcie write (to disable the interrupt) and readback are seen
> > by the hardware as (more or less) back to back transfers, so can
> > both easily overtake the request to raise the interrupt.
> 
> I don't believe this is correct.  On the PCI bus what you should see
> is the device aware that interrupts are disabled before the completion
> arrives and any MSI messages should arrive before the last read
> completion.

You are making the assumption that it takes the hardware zero
time to raise msix interrupts, and/or that the hardware logic that
raises the interrupts is tightly coupled with that which generates
the read completions.

The fpga logic we use for raising interrupts reads the mask last,
but then has to do three 'bus' cycles to get the 64bit address
configured and request the pcie write.
Meanwhile the pcie <=> msix table logic will be running asynchronously
so will quite easily complete the readback before the request itself
is sent.
I'm pretty sure there is enough async in the 'write' path that even if
I aborted raising the interrupt if there was a write to the msix table
the read completion could still overtake the write.

> > The pcie bus is also allowed to make a read completion tlp
> > overtake a write tlp.
> > Add in any host-side delays in raising the hardware interrupt
> > itself, and an interrupt could happen well after it was masked.
> 
> That is only if relaxed ordering is enabled, which I am not sure is
> supported for MSI-X interrupts.

The PCIE book on my desk is not helpful wrt reordering.
I know reads and read completion can be reordered - I fixed the fpga
logic (from the manufacturer) so that it correctly processed interleaved
read completions.
I don't see anything obvious that would require writes and read completions
to be kept in order.

> That being said though I believe
> there are some platforms that could end up seeing a delay in handling
> the interrupt if for example the CPU the interrupt was delivered to
> was in a sleep state.
>
> > More worrying would be any code that tries to change the address
> > and data associated with an interrupt.
> > You'd need moderate guard times after the disable and before the
> > enable to ensure the hardware didn't raise an interrupt with
> > a mismatch of the old and new values.
> >
> > [1] Maybe I'll look at the order those cycles actually arrive in.
> >
> >         David
> 
> So I have thrown in my $.02 on this, and added the linux-pci mailing
> list.  That is a much better place to bring this up rather than
> netdev.
> 
> Thanks.
> 
> - Alex

RE: Disabling msix interrupts

[David Laight]

RnJvbTogQWxleGFuZGVyIA0KPiBTZW50OiAwNiBGZWJydWFyeSAyMDE3IDE2OjI3DQo+IFRvOiBE
YXZpZCBMYWlnaHQNCj4gT24gTW9uLCBGZWIgNiwgMjAxNyBhdCA3OjMzIEFNLCBEYXZpZCBMYWln
aHQgPERhdmlkLkxhaWdodEBhY3VsYWIuY29tPiB3cm90ZToNCj4gPiBuZXRkZXYgcHJvYmFibHkg
aXNuJ3QgdGhlIHJpZ2h0IGxpc3QgZm9yIHRoaXMsIGJ1dCBJIHN1c3BlY3QgcGVvcGxlDQo+ID4g
cmVhZGluZyBpdCB1bmRlcnN0YW5kIHdoYXQgaGFwcGVucy4NCj4gPg0KPiA+IEknbSBmYWlybHkg
c3VyZSB0aGF0IGFuIG1zaXggaW50ZXJydXB0IGNhbiBnZXQgcmFpc2VkIGFmdGVyDQo+ID4gdGhl
IGtlcm5lbCB0aGlua3MgaXQgaGFzIG1hc2tlZCBpdC4NCj4gPg0KPiA+IFdoZW4gYW4gbXNpeCBp
bnRlcnJ1cHQgaXMgZGlzYWJsZWQgSSB0aGluayBtc2lfc2V0X21hc2tfYml0KCkNCj4gPiAoaW4g
ZHJpdmVycy9wY2kvbXNpLmMpIGlzIGNhbGxlZCB0byB3cml0ZSBhICcxJyB0byB0aGUgY2FyZCdz
DQo+ID4gaGFyZHdhcmUgTVNJWCBtYXNrIHJlZ2lzdGVyICh0aGUgbGFzdCAzMmJpdCB3b3JkIG9m
IHRoZSBlbnRyeSkuDQo+ID4gVGhpcyBmdW5jdGlvbiBjYXJlZnVsbHkgcmVhZHMgYmFjayB0aGUg
bWFzayByZWdpc3RlciB0byBmbHVzaA0KPiA+IHRoZSB3cml0ZSB0aHJvdWdoIHRoZSBwY2llIGJ1
cy4NCj4gPiBFeGNlcHQgaXQgZG9lc24ndCwgaXQgcmVhZHMgdGhlICdhZGRyZXNzX2xvJyByZWdp
c3RlciBpbnN0ZWFkISBbMV0NCj4gDQo+IFJlYWRpbmcgYW55IHJlZ2lzdGVyIHdpbGwgZm9yY2Ug
dGhlIHdyaXRlIHRvIGJlIGZsdXNoZWQgYXMgYWxsIFBDSQ0KPiB3cml0ZXMgbXVzdCBiZSBjb21w
bGV0ZWQgYmVmb3JlIGEgUENJIHJlYWQuICBGb3IgZXhhbXBsZSBpbiB0aGUgSW50ZWwNCj4gZHJp
dmVycyB3ZSByZWFkIHJlZ2lzdGVyIDAgdG8gZmx1c2ggYSB3cml0ZSBvZiBhbnkgb2YgdGhlIG90
aGVyDQo+IHJlZ2lzdGVycy4NCg0KU29ycnkgYnJhaW4gZmFkZSBvbiB0aGF0IG9uZS4NCkFsdGhv
dWdoIHRoZSAnc3RvcmUgYnVmZmVyJyBvbiB0aGUgc3BhcmMgY3B1cyBJIHVzZWQgdG8gdXNlIHdv
dWxkDQpsZXQgcmVhZHMgb3ZlcnRha2Ugd3JpdGVzLiBTbyB5b3UgZGlkIGhhdmUgdG8gcmVhZCBi
YWNrIHRoZSBhZGRyZXNzDQpvZiB0aGUgbGFzdCB3cml0ZSAtIG5vdCBzdXJlIGFib3V0IG1vZGVy
biBzcGFyYyBjcHVzLg0KDQo+ID4gV2hpbGUgdGhpcyB3aWxsIHN0b3AgdGhlIGhhcmR3YXJlIHJh
aXNpbmcgYW55IG1vcmUgaW50ZXJydXB0cywNCj4gPiBpdCBjb3VsZCBlYXNpbHkgYmUgaW4gdGhl
IHByb2Nlc3Mgb2YgcmFpc2luZyBvbmUuDQo+ID4gaWUgaGF2ZSByZWFkIHRoZSBtYXNrLCBmb3Vu
ZCBpdCB6ZXJvLCByZWFkIHRoZSBhZGRyZXNzIGFuZA0KPiA+IGRhdGEsIGFuZCBiZSBpbiB0aGUg
cHJvY2VzcyBvZiBpc3N1aW5nIHRoZSBwY2llIHdyaXRlLg0KPiA+DQo+ID4gVGhlIHBjaWUgd3Jp
dGUgKHRvIGRpc2FibGUgdGhlIGludGVycnVwdCkgYW5kIHJlYWRiYWNrIGFyZSBzZWVuDQo+ID4g
YnkgdGhlIGhhcmR3YXJlIGFzIChtb3JlIG9yIGxlc3MpIGJhY2sgdG8gYmFjayB0cmFuc2ZlcnMs
IHNvIGNhbg0KPiA+IGJvdGggZWFzaWx5IG92ZXJ0YWtlIHRoZSByZXF1ZXN0IHRvIHJhaXNlIHRo
ZSBpbnRlcnJ1cHQuDQo+IA0KPiBJIGRvbid0IGJlbGlldmUgdGhpcyBpcyBjb3JyZWN0LiAgT24g
dGhlIFBDSSBidXMgd2hhdCB5b3Ugc2hvdWxkIHNlZQ0KPiBpcyB0aGUgZGV2aWNlIGF3YXJlIHRo
YXQgaW50ZXJydXB0cyBhcmUgZGlzYWJsZWQgYmVmb3JlIHRoZSBjb21wbGV0aW9uDQo+IGFycml2
ZXMgYW5kIGFueSBNU0kgbWVzc2FnZXMgc2hvdWxkIGFycml2ZSBiZWZvcmUgdGhlIGxhc3QgcmVh
ZA0KPiBjb21wbGV0aW9uLg0KDQpZb3UgYXJlIG1ha2luZyB0aGUgYXNzdW1wdGlvbiB0aGF0IGl0
IHRha2VzIHRoZSBoYXJkd2FyZSB6ZXJvDQp0aW1lIHRvIHJhaXNlIG1zaXggaW50ZXJydXB0cywg
YW5kL29yIHRoYXQgdGhlIGhhcmR3YXJlIGxvZ2ljIHRoYXQNCnJhaXNlcyB0aGUgaW50ZXJydXB0
cyBpcyB0aWdodGx5IGNvdXBsZWQgd2l0aCB0aGF0IHdoaWNoIGdlbmVyYXRlcw0KdGhlIHJlYWQg
Y29tcGxldGlvbnMuDQoNClRoZSBmcGdhIGxvZ2ljIHdlIHVzZSBmb3IgcmFpc2luZyBpbnRlcnJ1
cHRzIHJlYWRzIHRoZSBtYXNrIGxhc3QsDQpidXQgdGhlbiBoYXMgdG8gZG8gdGhyZWUgJ2J1cycg
Y3ljbGVzIHRvIGdldCB0aGUgNjRiaXQgYWRkcmVzcw0KY29uZmlndXJlZCBhbmQgcmVxdWVzdCB0
aGUgcGNpZSB3cml0ZS4NCk1lYW53aGlsZSB0aGUgcGNpZSA8PT4gbXNpeCB0YWJsZSBsb2dpYyB3
aWxsIGJlIHJ1bm5pbmcgYXN5bmNocm9ub3VzbHkNCnNvIHdpbGwgcXVpdGUgZWFzaWx5IGNvbXBs
ZXRlIHRoZSByZWFkYmFjayBiZWZvcmUgdGhlIHJlcXVlc3QgaXRzZWxmDQppcyBzZW50Lg0KSSdt
IHByZXR0eSBzdXJlIHRoZXJlIGlzIGVub3VnaCBhc3luYyBpbiB0aGUgJ3dyaXRlJyBwYXRoIHRo
YXQgZXZlbiBpZg0KSSBhYm9ydGVkIHJhaXNpbmcgdGhlIGludGVycnVwdCBpZiB0aGVyZSB3YXMg
YSB3cml0ZSB0byB0aGUgbXNpeCB0YWJsZQ0KdGhlIHJlYWQgY29tcGxldGlvbiBjb3VsZCBzdGls
bCBvdmVydGFrZSB0aGUgd3JpdGUuDQoNCj4gPiBUaGUgcGNpZSBidXMgaXMgYWxzbyBhbGxvd2Vk
IHRvIG1ha2UgYSByZWFkIGNvbXBsZXRpb24gdGxwDQo+ID4gb3ZlcnRha2UgYSB3cml0ZSB0bHAu
DQo+ID4gQWRkIGluIGFueSBob3N0LXNpZGUgZGVsYXlzIGluIHJhaXNpbmcgdGhlIGhhcmR3YXJl
IGludGVycnVwdA0KPiA+IGl0c2VsZiwgYW5kIGFuIGludGVycnVwdCBjb3VsZCBoYXBwZW4gd2Vs
bCBhZnRlciBpdCB3YXMgbWFza2VkLg0KPiANCj4gVGhhdCBpcyBvbmx5IGlmIHJlbGF4ZWQgb3Jk
ZXJpbmcgaXMgZW5hYmxlZCwgd2hpY2ggSSBhbSBub3Qgc3VyZSBpcw0KPiBzdXBwb3J0ZWQgZm9y
IE1TSS1YIGludGVycnVwdHMuDQoNClRoZSBQQ0lFIGJvb2sgb24gbXkgZGVzayBpcyBub3QgaGVs
cGZ1bCB3cnQgcmVvcmRlcmluZy4NCkkga25vdyByZWFkcyBhbmQgcmVhZCBjb21wbGV0aW9uIGNh
biBiZSByZW9yZGVyZWQgLSBJIGZpeGVkIHRoZSBmcGdhDQpsb2dpYyAoZnJvbSB0aGUgbWFudWZh
Y3R1cmVyKSBzbyB0aGF0IGl0IGNvcnJlY3RseSBwcm9jZXNzZWQgaW50ZXJsZWF2ZWQNCnJlYWQg
Y29tcGxldGlvbnMuDQpJIGRvbid0IHNlZSBhbnl0aGluZyBvYnZpb3VzIHRoYXQgd291bGQgcmVx
dWlyZSB3cml0ZXMgYW5kIHJlYWQgY29tcGxldGlvbnMNCnRvIGJlIGtlcHQgaW4gb3JkZXIuDQoN
Cj4gVGhhdCBiZWluZyBzYWlkIHRob3VnaCBJIGJlbGlldmUNCj4gdGhlcmUgYXJlIHNvbWUgcGxh
dGZvcm1zIHRoYXQgY291bGQgZW5kIHVwIHNlZWluZyBhIGRlbGF5IGluIGhhbmRsaW5nDQo+IHRo
ZSBpbnRlcnJ1cHQgaWYgZm9yIGV4YW1wbGUgdGhlIENQVSB0aGUgaW50ZXJydXB0IHdhcyBkZWxp
dmVyZWQgdG8NCj4gd2FzIGluIGEgc2xlZXAgc3RhdGUuDQo+DQo+ID4gTW9yZSB3b3JyeWluZyB3
b3VsZCBiZSBhbnkgY29kZSB0aGF0IHRyaWVzIHRvIGNoYW5nZSB0aGUgYWRkcmVzcw0KPiA+IGFu
ZCBkYXRhIGFzc29jaWF0ZWQgd2l0aCBhbiBpbnRlcnJ1cHQuDQo+ID4gWW91J2QgbmVlZCBtb2Rl
cmF0ZSBndWFyZCB0aW1lcyBhZnRlciB0aGUgZGlzYWJsZSBhbmQgYmVmb3JlIHRoZQ0KPiA+IGVu
YWJsZSB0byBlbnN1cmUgdGhlIGhhcmR3YXJlIGRpZG4ndCByYWlzZSBhbiBpbnRlcnJ1cHQgd2l0
aA0KPiA+IGEgbWlzbWF0Y2ggb2YgdGhlIG9sZCBhbmQgbmV3IHZhbHVlcy4NCj4gPg0KPiA+IFsx
XSBNYXliZSBJJ2xsIGxvb2sgYXQgdGhlIG9yZGVyIHRob3NlIGN5Y2xlcyBhY3R1YWxseSBhcnJp
dmUgaW4uDQo+ID4NCj4gPiAgICAgICAgIERhdmlkDQo+IA0KPiBTbyBJIGhhdmUgdGhyb3duIGlu
IG15ICQuMDIgb24gdGhpcywgYW5kIGFkZGVkIHRoZSBsaW51eC1wY2kgbWFpbGluZw0KPiBsaXN0
LiAgVGhhdCBpcyBhIG11Y2ggYmV0dGVyIHBsYWNlIHRvIGJyaW5nIHRoaXMgdXAgcmF0aGVyIHRo
YW4NCj4gbmV0ZGV2Lg0KPiANCj4gVGhhbmtzLg0KPiANCj4gLSBBbGV4DQo=

Re: Disabling msix interrupts

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Mon, 6 Feb 2017 17:23:54 +0000

> Although the 'store buffer' on the sparc cpus I used to use would
> let reads overtake writes. So you did have to read back the address
> of the last write - not sure about modern sparc cpus.

Never would any sparc cpu do so when any of the operations involved
were to "side effect" locations, as PCI config space is.

RE: Disabling msix interrupts

[David Laight]

From: David Miller
> Sent: 06 February 2017 19:15
> From: David Laight <David.Laight@ACULAB.COM>
> Date: Mon, 6 Feb 2017 17:23:54 +0000
> 
> > Although the 'store buffer' on the sparc cpus I used to use would
> > let reads overtake writes. So you did have to read back the address
> > of the last write - not sure about modern sparc cpus.
> 
> Never would any sparc cpu do so when any of the operations involved
> were to "side effect" locations, as PCI config space is.

I guess they used non-zero ASI, and that forced the flush??
Normal uncached memory reads would overtake writes.
(These were SuperSparc (Viking)).

	David

Re: Disabling msix interrupts

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Tue, 7 Feb 2017 09:55:47 +0000

> From: David Miller
>> Sent: 06 February 2017 19:15
>> From: David Laight <David.Laight@ACULAB.COM>
>> Date: Mon, 6 Feb 2017 17:23:54 +0000
>> 
>> > Although the 'store buffer' on the sparc cpus I used to use would
>> > let reads overtake writes. So you did have to read back the address
>> > of the last write - not sure about modern sparc cpus.
>> 
>> Never would any sparc cpu do so when any of the operations involved
>> were to "side effect" locations, as PCI config space is.
> 
> I guess they used non-zero ASI, and that forced the flush??
> Normal uncached memory reads would overtake writes.
> (These were SuperSparc (Viking)).

On sun4m it was controlled by bits in the physical address.