On Tue, Jul 20, 2021 at 02:21:39PM +0200, Mark Kettenis wrote:
> > Date: Tue, 20 Jul 2021 21:55:56 +1000
> > From: Jonathan Matthew <[email protected]>
> >
> > 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.
>
> I don't really read misc@, so don't post your patches there.
Right, it was just there for testing.
>
> > Mark, what do you think?
>
> Yeah, making pci_intr_msix_count() should return 0 if MSIs are not
> supported. A bit strange though to pass both pa and pa->pa_tag. I'd
> change the function to only take pa as an argument.
Yes, on second look that makes sense. Here's a better diff with that change,
and that also doesn't break arches without __HAVE_PCI_MSIX. ok?
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 21 Jul 2021 03:24:44 -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);
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 21 Jul 2021 03:24:44 -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);
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 21 Jul 2021 03:24:44 -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);
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 21 Jul 2021 03:24:44 -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);
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 21 Jul 2021 03:24:44 -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);
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 21 Jul 2021 03:24:44 -0000
@@ -1694,11 +1694,15 @@ 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)
{
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, pa->pa_tag, PCI_CAP_MSIX, NULL,
+ ®) == 0)
return (0);
return (PCI_MSIX_MC_TBLSZ(reg) + 1);
@@ -1731,7 +1735,7 @@ 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)
{
return (0);
}
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 21 Jul 2021 03:24:44 -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 *);
uint16_t pci_requester_id(pci_chipset_tag_t, pcitag_t);
