> Date: Wed, 21 Jul 2021 15:15:11 +1000
> From: Jonathan Matthew <[email protected]>
>
> 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?
ok kettenis@
> 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);
>
>
