Description Bjoern A. Zeeb 2017-01-25 11:41:28 UTC

Structurally this seems entirely backwards that drivers do supply a isc_msix_bar value without checking if MSIX is available.  Writing code based on assumptions is not stable but iflib should guard against this.

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.

It seems that a lot more safeguarding and perhaps better structuring between drivers and iflib is needed to avoid these kinds of errors.

This patch just adds the safeguard in a single place in iflib, em(4) should ideally be improved to not signal an msix bar value anymore if MSIX is not avail.


Index: head-r312664.svn/sys/net/iflib.c
===================================================================
--- head-r312664.svn/sys/net/iflib.c    (revision 312664)
+++ head-r312664.svn/sys/net/iflib.c    (working copy)
@@ -3710,6 +3710,10 @@ iflib_device_register(device_t dev, void *sc, if_s
        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;
@@ -4754,15 +4758,21 @@ 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);
-               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, &rid) == 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;
+               }
        }

        /*