Re: svn commit: r312755 - head/sys/net
On Monday, January 30, 2017 09:18:56 AM Sean Bruno wrote: > > On 01/27/17 12:28, John Baldwin wrote: > > On Wednesday, January 25, 2017 02:37:05 PM Sean Bruno wrote: > >> Author: sbruno > >> Date: Wed Jan 25 14:37:05 2017 > >> New Revision: 312755 > >> URL: https://svnweb.freebsd.org/changeset/base/312755 > >> > >> Log: > >> Add error checking to the pci_find_cap(, PCIY_MSIX,) call that is returns > >> success and a good value. Only then try to use it and set the > >> MSIX_ENABLE > >> bit. > >> > >> With the current em(4) driver we have observed failures in this case in a > >> specific environment when pci_find_cap() would not return the assumed > >> value, which meant we ended up writing to PCI register 2 (PCI_DEVICE_ID) > >> which is read-only. > > > > Why is this writing directly to the MSIX registers at all? pci_alloc_msix() > > etc. handle those registers for all other drivers and proper suspend/resume > > depends on drivers using the existing PCI API for managing MSI and MSI-X. > > > > The comment above this code block explains what's up. Basically, > virtualized environments are sometimes "lazy" about correct register setup. Can you provide more details here? We already deploy workarounds for specific versions of Xen that emulate MSI-X incorrectly, though for Xen your change actually makes things worse (older versions of Xen don't "see" updates to the MSI-X table while MSI-X is enabled). However, you are still enabling MSI-X while there is uninitialized junk in the table meaning that if the device asserted an interrupt it could trigger a panic. If there are bugs with hypervisors, we need to work around them in the base PCI code as we have done with the Xen workarounds. They are not going to be e1000-specific (or even NIC-specific), so iflib is the wrong place to handle them. > If MSIX caps aren't set, try to enable them. If that fails, assume MSI. Enabling MSI-X with an uninitialized table is not safe. -- John Baldwin ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r312755 - head/sys/net
On 01/27/17 12:28, John Baldwin wrote: > On Wednesday, January 25, 2017 02:37:05 PM Sean Bruno wrote: >> Author: sbruno >> Date: Wed Jan 25 14:37:05 2017 >> New Revision: 312755 >> URL: https://svnweb.freebsd.org/changeset/base/312755 >> >> Log: >> Add error checking to the pci_find_cap(, PCIY_MSIX,) call that is returns >> success and a good value. Only then try to use it and set the MSIX_ENABLE >> bit. >> >> With the current em(4) driver we have observed failures in this case in a >> specific environment when pci_find_cap() would not return the assumed >> value, which meant we ended up writing to PCI register 2 (PCI_DEVICE_ID) >> which is read-only. > > Why is this writing directly to the MSIX registers at all? pci_alloc_msix() > etc. handle those registers for all other drivers and proper suspend/resume > depends on drivers using the existing PCI API for managing MSI and MSI-X. > The comment above this code block explains what's up. Basically, virtualized environments are sometimes "lazy" about correct register setup. If MSIX caps aren't set, try to enable them. If that fails, assume MSI. Later on the code does the proper pci_alloc_msix() calls in the proper sequence, IMO. sean signature.asc Description: OpenPGP digital signature
Re: svn commit: r312755 - head/sys/net
On Wednesday, January 25, 2017 02:37:05 PM Sean Bruno wrote: > Author: sbruno > Date: Wed Jan 25 14:37:05 2017 > New Revision: 312755 > URL: https://svnweb.freebsd.org/changeset/base/312755 > > Log: > Add error checking to the pci_find_cap(, PCIY_MSIX,) call that is returns > success and a good value. Only then try to use it and set the MSIX_ENABLE > bit. > > With the current em(4) driver we have observed failures in this case in a > specific environment when pci_find_cap() would not return the assumed > value, which meant we ended up writing to PCI register 2 (PCI_DEVICE_ID) > which is read-only. Why is this writing directly to the MSIX registers at all? pci_alloc_msix() etc. handle those registers for all other drivers and proper suspend/resume depends on drivers using the existing PCI API for managing MSI and MSI-X. > Modified: head/sys/net/iflib.c > == > --- head/sys/net/iflib.c Wed Jan 25 13:42:38 2017(r312754) > +++ head/sys/net/iflib.c Wed Jan 25 14:37:05 2017(r312755) > @@ -4779,15 +4783,20 @@ iflib_msix_init(if_ctx_t ctx) > uint16_t pci_cmd_word; > int msix_ctrl, rid; > > - rid = 0; > pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2); > pci_cmd_word |= PCIM_CMD_BUSMASTEREN; > pci_write_config(dev, PCIR_COMMAND, pci_cmd_word, 2); This should use 'pci_enable_busmaster()' like other drivers rather than manipulating registers directly. -- John Baldwin ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r312755 - head/sys/net
Author: sbruno
Date: Wed Jan 25 14:37:05 2017
New Revision: 312755
URL: https://svnweb.freebsd.org/changeset/base/312755
Log:
Add error checking to the pci_find_cap(, PCIY_MSIX,) call that is returns
success and a good value. Only then try to use it and set the MSIX_ENABLE
bit.
With the current em(4) driver we have observed failures in this case in a
specific environment when pci_find_cap() would not return the assumed
value, which meant we ended up writing to PCI register 2 (PCI_DEVICE_ID)
which is read-only.
PR: 216456
Submitted by: bz
Modified:
head/sys/net/iflib.c
Modified: head/sys/net/iflib.c
==
--- head/sys/net/iflib.cWed Jan 25 13:42:38 2017(r312754)
+++ head/sys/net/iflib.cWed Jan 25 14:37:05 2017(r312755)
@@ -3733,6 +3733,10 @@ iflib_device_register(device_t dev, void
if (sctx->isc_flags & IFLIB_SKIP_MSIX) {
msix = scctx->isc_vectors;
} else if (scctx->isc_msix_bar != 0)
+ /*
+ * The simple fact that isc_msix_bar is not 0 does not mean we
+ * we have a good value there that is known to work.
+ */
msix = iflib_msix_init(ctx);
else {
scctx->isc_vectors = 1;
@@ -4779,15 +4783,20 @@ iflib_msix_init(if_ctx_t ctx)
uint16_t pci_cmd_word;
int msix_ctrl, rid;
- rid = 0;
pci_cmd_word = pci_read_config(dev, PCIR_COMMAND, 2);
pci_cmd_word |= PCIM_CMD_BUSMASTEREN;
pci_write_config(dev, PCIR_COMMAND, pci_cmd_word, 2);
- pci_find_cap(dev, PCIY_MSIX, );
- rid += PCIR_MSIX_CTRL;
- msix_ctrl = pci_read_config(dev, rid, 2);
- msix_ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
- pci_write_config(dev, rid, msix_ctrl, 2);
+ rid = 0;
+ if (pci_find_cap(dev, PCIY_MSIX, ) == 0 && rid != 0) {
+ rid += PCIR_MSIX_CTRL;
+ msix_ctrl = pci_read_config(dev, rid, 2);
+ msix_ctrl |= PCIM_MSIXCTRL_MSIX_ENABLE;
+ pci_write_config(dev, rid, msix_ctrl, 2);
+ } else {
+ device_printf(dev, "PCIY_MSIX capability not found; "
+ "or rid %d == 0.\n", rid);
+ goto msi;
+ }
}
/*
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
