nvme_pci.c patch for MSI-X

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

nvme_pci.c patch for MSI-X

Jason Tubnor
Hi,

Below is a patch that fixes an issue where NVMe storage is presented only
via MSI-X.  This issue came about as the NVMe implementation in bhyve only
uses MSI-X.

Thanks to Chuck Tuffli for the initial patch.  It was adjusted to deal with
with both cases.

Thank,

Jason Tubnor

Index: sys/dev/pci/nvme_pci.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/nvme_pci.c,v
retrieving revision 1.7
diff -u -p -u -r1.7 nvme_pci.c
--- sys/dev/pci/nvme_pci.c 10 Jan 2018 15:45:46 -0000 1.7
+++ sys/dev/pci/nvme_pci.c 24 Mar 2019 08:22:42 -0000
@@ -105,7 +105,7 @@ nvme_pci_attach(struct device *parent, s
  return;
  }

- if (pci_intr_map_msi(pa, &ih) != 0) {
+ if ((pci_intr_map_msix(pa, 0, &ih) != 0) && (pci_intr_map_msi(pa, &ih) !=
0)) {
  if (pci_intr_map(pa, &ih) != 0) {
  printf(": unable to map interrupt\n");
  goto unmap;
Reply | Threaded
Open this post in threaded view
|

Re: nvme_pci.c patch for MSI-X

Chris Cappuccio
I think the current MSI-X implementation is a minimal skeleton, enough for some
devices under virtualization. I don't know if it's enough for NVMe on real
hardware.

Jason Tubnor [[hidden email]] wrote:

> Hi,
>
> Below is a patch that fixes an issue where NVMe storage is presented only
> via MSI-X.  This issue came about as the NVMe implementation in bhyve only
> uses MSI-X.
>
> Thanks to Chuck Tuffli for the initial patch.  It was adjusted to deal with
> with both cases.
>
> Thank,
>
> Jason Tubnor
>
> Index: sys/dev/pci/nvme_pci.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/nvme_pci.c,v
> retrieving revision 1.7
> diff -u -p -u -r1.7 nvme_pci.c
> --- sys/dev/pci/nvme_pci.c 10 Jan 2018 15:45:46 -0000 1.7
> +++ sys/dev/pci/nvme_pci.c 24 Mar 2019 08:22:42 -0000
> @@ -105,7 +105,7 @@ nvme_pci_attach(struct device *parent, s
>   return;
>   }
>
> - if (pci_intr_map_msi(pa, &ih) != 0) {
> + if ((pci_intr_map_msix(pa, 0, &ih) != 0) && (pci_intr_map_msi(pa, &ih) !=
> 0)) {
>   if (pci_intr_map(pa, &ih) != 0) {
>   printf(": unable to map interrupt\n");
>   goto unmap;

Reply | Threaded
Open this post in threaded view
|

Re: nvme_pci.c patch for MSI-X

Mark Kettenis
> Date: Thu, 28 Mar 2019 09:00:35 -0700
> From: Chris Cappuccio <[hidden email]>
>
> I think the current MSI-X implementation is a minimal skeleton,
> enough for some devices under virtualization. I don't know if it's
> enough for NVMe on real hardware.

The main problem is that the MSI-X implementation has
machine-depenedent bits that are not implemented on all platforms.

> Jason Tubnor [[hidden email]] wrote:
> > Hi,
> >
> > Below is a patch that fixes an issue where NVMe storage is presented only
> > via MSI-X.  This issue came about as the NVMe implementation in bhyve only
> > uses MSI-X.
> >
> > Thanks to Chuck Tuffli for the initial patch.  It was adjusted to deal with
> > with both cases.
> >
> > Thank,
> >
> > Jason Tubnor
> >
> > Index: sys/dev/pci/nvme_pci.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/pci/nvme_pci.c,v
> > retrieving revision 1.7
> > diff -u -p -u -r1.7 nvme_pci.c
> > --- sys/dev/pci/nvme_pci.c 10 Jan 2018 15:45:46 -0000 1.7
> > +++ sys/dev/pci/nvme_pci.c 24 Mar 2019 08:22:42 -0000
> > @@ -105,7 +105,7 @@ nvme_pci_attach(struct device *parent, s
> >   return;
> >   }
> >
> > - if (pci_intr_map_msi(pa, &ih) != 0) {
> > + if ((pci_intr_map_msix(pa, 0, &ih) != 0) && (pci_intr_map_msi(pa, &ih) !=
> > 0)) {
> >   if (pci_intr_map(pa, &ih) != 0) {
> >   printf(": unable to map interrupt\n");
> >   goto unmap;
>
>

Reply | Threaded
Open this post in threaded view
|

Re: nvme_pci.c patch for MSI-X

Ted Unangst-6
Mark Kettenis wrote:
> > Date: Thu, 28 Mar 2019 09:00:35 -0700
> > From: Chris Cappuccio <[hidden email]>
> >
> > I think the current MSI-X implementation is a minimal skeleton,
> > enough for some devices under virtualization. I don't know if it's
> > enough for NVMe on real hardware.
>
> The main problem is that the MSI-X implementation has
> machine-depenedent bits that are not implemented on all platforms.

We do have macros everywhere though.

#define         pci_intr_map_msix(pa, vec, ihp) (-1)

I think that should be good enough to allow the code to continue building on
all platforms and fallback, no?

> > >
> > > - if (pci_intr_map_msi(pa, &ih) != 0) {
> > > + if ((pci_intr_map_msix(pa, 0, &ih) != 0) && (pci_intr_map_msi(pa, &ih) !=
> > > 0)) {
> > >   if (pci_intr_map(pa, &ih) != 0) {
> > >   printf(": unable to map interrupt\n");
> > >   goto unmap;
> >
> >
>

Reply | Threaded
Open this post in threaded view
|

Re: nvme_pci.c patch for MSI-X

Mark Kettenis
> From: "Ted Unangst" <[hidden email]>
> Date: Mon, 01 Apr 2019 05:55:34 -0400
>
> Mark Kettenis wrote:
> > > Date: Thu, 28 Mar 2019 09:00:35 -0700
> > > From: Chris Cappuccio <[hidden email]>
> > >
> > > I think the current MSI-X implementation is a minimal skeleton,
> > > enough for some devices under virtualization. I don't know if it's
> > > enough for NVMe on real hardware.
> >
> > The main problem is that the MSI-X implementation has
> > machine-depenedent bits that are not implemented on all platforms.
>
> We do have macros everywhere though.
>
> #define         pci_intr_map_msix(pa, vec, ihp) (-1)
>
> I think that should be good enough to allow the code to continue building on
> all platforms and fallback, no?

Hmm, maybe, yes.

> > > > - if (pci_intr_map_msi(pa, &ih) != 0) {
> > > > + if ((pci_intr_map_msix(pa, 0, &ih) != 0) && (pci_intr_map_msi(pa, &ih) !=
> > > > 0)) {
> > > >   if (pci_intr_map(pa, &ih) != 0) {
> > > >   printf(": unable to map interrupt\n");
> > > >   goto unmap;
> > >
> > >
> >
>