On Mon, Jul 19, 2021 at 07:37:10PM -0400, Ashton Fagg wrote:
> I have an Intel 82599 10 gigabit ethernet card I wanted to get working
> on my SiFive Unmatched board.
>
> I found the ix(4) driver has some weirdness around MSI-X
> interrupts. While the driver supports operating both with and without
> MSI-X support, it's hard-coded via a flag rather than dynamically checking
> if it's available. If the flag is set (which it always is right now),
> but MSI-X isn't available, the driver will throw an error and the device
> won't work:
>
> ix0 at pci7 dev 0 function 0 "Intel 82599" rev 0x01ixgbe_allocate_msix:
> pci_intr_map_msix vec 0 failed
>
> The root cause is this call failing in if_ix.c:
>
> if (pci_intr_map_msix(pa, i, &ih)) {
> printf("ixgbe_allocate_msix: "
> "pci_intr_map_msix vec %d failed\n", i);
> error = ENOMEM;
> goto fail;
> }
>
>
> Because in _pci_intr_map_msix (in sys/arch/riscv64/dev/pci_machdep.c):
>
> if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
> pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
> return -1;
>
> The PCI attach flags would not have PCI_FLAGS_MSI_ENABLED set.
>
> The following diff remedies that by checking if PCI_FLAGS_MSI_ENABLED is
> actually set, rather than just trying and failing because the hard-coded
> flag says so. It also enables ix(4) in the kernel config for
> riscv64. Effectively, the driver will now only try to use MSI-X if the
> machine is advertising it to be available.
I'd rather not have to do this in every driver. We otherwise check that flag
inside the pci interrupt functions rather than in the driver code, so we
should do so in pci_intr_msix_count() too, since that's what we call in
multi-queue nic drivers to decide whether to use MSI-X. Drivers that only
want a single vector will just call pci_intr_map_msix() and fall back to MSI
or legacy interrupts if that fails.
I posted the alternate version of this diff to misc@ a few days ago, which
repeats the checks used to set PCI_FLAGS_MSI_ENABLED in pci_intr_msix_count(),
rather than passing in struct pci_attach_args, in case we prefer to do it that
way.
Mark, what do you think?
Index: if_bnxt.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_bnxt.c,v
retrieving revision 1.32
diff -u -p -u -p -r1.32 if_bnxt.c
--- if_bnxt.c 24 Apr 2021 09:37:46 -0000 1.32
+++ if_bnxt.c 20 Jul 2021 11:23:22 -0000
@@ -537,7 +537,7 @@ bnxt_attach(struct device *parent, struc
sc->sc_flags |= BNXT_FLAG_MSIX;
intrstr = pci_intr_string(sc->sc_pc, ih);
- nmsix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
+ nmsix = pci_intr_msix_count(pa, pa->pa_tag);
if (nmsix > 1) {
sc->sc_ih = pci_intr_establish(sc->sc_pc, ih,
IPL_NET | IPL_MPSAFE, bnxt_admin_intr, sc,
DEVNAME(sc));
Index: if_ix.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_ix.c,v
retrieving revision 1.178
diff -u -p -u -p -r1.178 if_ix.c
--- if_ix.c 22 Dec 2020 23:25:37 -0000 1.178
+++ if_ix.c 20 Jul 2021 11:23:22 -0000
@@ -1783,7 +1783,7 @@ ixgbe_setup_msix(struct ix_softc *sc)
if (!ixgbe_enable_msix)
return;
- nmsix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
+ nmsix = pci_intr_msix_count(pa, pa->pa_tag);
if (nmsix <= 1)
return;
Index: if_ixl.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_ixl.c,v
retrieving revision 1.74
diff -u -p -u -p -r1.74 if_ixl.c
--- if_ixl.c 26 Mar 2021 08:02:34 -0000 1.74
+++ if_ixl.c 20 Jul 2021 11:23:22 -0000
@@ -1795,7 +1795,7 @@ ixl_attach(struct device *parent, struct
}
if (pci_intr_map_msix(pa, 0, &sc->sc_ih) == 0) {
- int nmsix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
+ int nmsix = pci_intr_msix_count(pa, pa->pa_tag);
if (nmsix > 1) { /* we used 1 (the 0th) for the adminq */
nmsix--;
Index: if_mcx.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_mcx.c,v
retrieving revision 1.101
diff -u -p -u -p -r1.101 if_mcx.c
--- if_mcx.c 2 Jun 2021 19:16:11 -0000 1.101
+++ if_mcx.c 20 Jul 2021 11:23:22 -0000
@@ -2831,7 +2831,7 @@ mcx_attach(struct device *parent, struct
goto teardown;
}
- msix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
+ msix = pci_intr_msix_count(pa, pa->pa_tag);
if (msix < 2) {
printf(": not enough msi-x vectors\n");
goto teardown;
Index: if_vmx.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v
retrieving revision 1.65
diff -u -p -u -p -r1.65 if_vmx.c
--- if_vmx.c 12 Dec 2020 11:48:53 -0000 1.65
+++ if_vmx.c 20 Jul 2021 11:23:22 -0000
@@ -271,7 +271,7 @@ vmxnet3_attach(struct device *parent, st
switch (intrcfg & VMXNET3_INTRCFG_TYPE_MASK) {
case VMXNET3_INTRCFG_TYPE_AUTO:
case VMXNET3_INTRCFG_TYPE_MSIX:
- msix = pci_intr_msix_count(pa->pa_pc, pa->pa_tag);
+ msix = pci_intr_msix_count(pa, pa->pa_tag);
if (msix > 0) {
if (pci_intr_map_msix(pa, 0, &ih) == 0) {
msix--; /* are there spares for tx/rx qs? */
Index: pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/pci.c,v
retrieving revision 1.119
diff -u -p -u -p -r1.119 pci.c
--- pci.c 8 Sep 2020 20:13:52 -0000 1.119
+++ pci.c 20 Jul 2021 11:23:22 -0000
@@ -1694,11 +1694,14 @@ pci_resume_msix(pci_chipset_tag_t pc, pc
}
int
-pci_intr_msix_count(pci_chipset_tag_t pc, pcitag_t tag)
+pci_intr_msix_count(struct pci_attach_args *pa, pcitag_t tag)
{
pcireg_t reg;
- if (pci_get_capability(pc, tag, PCI_CAP_MSIX, NULL, ®) == 0)
+ if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0)
+ return (0);
+
+ if (pci_get_capability(pa->pa_pc, tag, PCI_CAP_MSIX, NULL, ®) == 0)
return (0);
return (PCI_MSIX_MC_TBLSZ(reg) + 1);
Index: pcivar.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/pcivar.h,v
retrieving revision 1.75
diff -u -p -u -p -r1.75 pcivar.h
--- pcivar.h 1 May 2021 16:11:16 -0000 1.75
+++ pcivar.h 20 Jul 2021 11:23:22 -0000
@@ -247,7 +247,7 @@ void pci_suspend_msix(pci_chipset_tag_t,
void pci_resume_msix(pci_chipset_tag_t, pcitag_t, bus_space_tag_t,
pcireg_t, struct msix_vector *);
-int pci_intr_msix_count(pci_chipset_tag_t, pcitag_t);
+int pci_intr_msix_count(struct pci_attach_args *, pcitag_t);
uint16_t pci_requester_id(pci_chipset_tag_t, pcitag_t);
