Re: PCI interrupt functions

[Mark Kettenis]

> From: "Theo de Raadt" 
> Date: Fri, 31 May 2019 15:34:18 -0600
> 
> > On arm64, pci_intr_handle_t is a pointer to an opaque struct.
> 
> That's a subtle trap.  How would someone realize the order is wrong...
> 
> Would it not be better if this was done like the other architectures,
> where the pci_intr_handle_t is a structure, not a pointer.
> 
> On arm64, the pci subsystems seem to use structures which are
> the same, and additional fields could be added easily if there
> was a need later.

Yes.  I suppose we anticipated that we would need different structs
for different host bridge drivers, but so far that hasn't happened.
I'm fairly confident that what we have now is sufficient for most
other hardware that we would like to support.

This actually allows me to remove some duplicated code.

ok?


Index: dev/fdt/dwpcie.c
===
RCS file: /cvs/src/sys/dev/fdt/dwpcie.c,v
retrieving revision 1.13
diff -u -p -r1.13 dwpcie.c
--- dev/fdt/dwpcie.c31 May 2019 10:35:49 -  1.13
+++ dev/fdt/dwpcie.c1 Jun 2019 18:00:30 -
@@ -224,9 +224,6 @@ pcireg_t dwpcie_conf_read(void *, pcitag
 void   dwpcie_conf_write(void *, pcitag_t, int, pcireg_t);
 
 intdwpcie_intr_map(struct pci_attach_args *, pci_intr_handle_t *);
-intdwpcie_intr_map_msi(struct pci_attach_args *, pci_intr_handle_t *);
-intdwpcie_intr_map_msix(struct pci_attach_args *, int,
-   pci_intr_handle_t *);
 const char *dwpcie_intr_string(void *, pci_intr_handle_t);
 void   *dwpcie_intr_establish(void *, pci_intr_handle_t, int,
int (*)(void *), void *, char *);
@@ -453,8 +450,8 @@ dwpcie_attach(struct device *parent, str
 
sc->sc_pc.pc_intr_v = sc;
sc->sc_pc.pc_intr_map = dwpcie_intr_map;
-   sc->sc_pc.pc_intr_map_msi = dwpcie_intr_map_msi;
-   sc->sc_pc.pc_intr_map_msix = dwpcie_intr_map_msix;
+   sc->sc_pc.pc_intr_map_msi = _pci_intr_map_msi;
+   sc->sc_pc.pc_intr_map_msix = _pci_intr_map_msix;
sc->sc_pc.pc_intr_string = dwpcie_intr_string;
sc->sc_pc.pc_intr_establish = dwpcie_intr_establish;
sc->sc_pc.pc_intr_disestablish = dwpcie_intr_disestablish;
@@ -903,21 +900,9 @@ dwpcie_conf_write(void *v, pcitag_t tag,
sc->sc_io_bus_addr, sc->sc_io_size);
 }
 
-#define PCI_INTX   0
-#define PCI_MSI1
-#define PCI_MSIX   2
-
-struct dwpcie_intr_handle {
-   pci_chipset_tag_t   ih_pc;
-   pcitag_tih_tag;
-   int ih_intrpin;
-   int ih_type;
-};
-
 int
 dwpcie_intr_map(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 {
-   struct dwpcie_intr_handle *ih;
int pin = pa->pa_rawintrpin;
 
if (pin == 0 || pin > PCI_INTERRUPT_PIN_MAX)
@@ -926,68 +911,18 @@ dwpcie_intr_map(struct pci_attach_args *
if (pa->pa_tag == 0)
return -1;
 
-   ih = malloc(sizeof(struct dwpcie_intr_handle), M_DEVBUF, M_WAITOK);
-   ih->ih_pc = pa->pa_pc;
-   ih->ih_tag = pa->pa_intrtag;
-   ih->ih_intrpin = pa->pa_intrpin;
-   ih->ih_type = PCI_INTX;
-   *ihp = (pci_intr_handle_t)ih;
-
-   return 0;
-}
-
-int
-dwpcie_intr_map_msi(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
-{
-   pci_chipset_tag_t pc = pa->pa_pc;
-   pcitag_t tag = pa->pa_tag;
-   struct dwpcie_intr_handle *ih;
-
-   if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-   pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
-   return -1;
-
-   ih = malloc(sizeof(struct dwpcie_intr_handle), M_DEVBUF, M_WAITOK);
-   ih->ih_pc = pa->pa_pc;
-   ih->ih_tag = pa->pa_tag;
-   ih->ih_type = PCI_MSI;
-   *ihp = (pci_intr_handle_t)ih;
-
-   return 0;
-}
-
-int
-dwpcie_intr_map_msix(struct pci_attach_args *pa, int vec,
-pci_intr_handle_t *ihp)
-{
-   pci_chipset_tag_t pc = pa->pa_pc;
-   pcitag_t tag = pa->pa_tag;
-   struct dwpcie_intr_handle *ih;
-   pcireg_t reg;
-
-   if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-   pci_get_capability(pc, tag, PCI_CAP_MSIX, NULL, ) == 0)
-   return -1;
-
-   if (vec > PCI_MSIX_MC_TBLSZ(reg))
-   return -1;
-
-   ih = malloc(sizeof(struct dwpcie_intr_handle), M_DEVBUF, M_WAITOK);
-   ih->ih_pc = pa->pa_pc;
-   ih->ih_tag = pa->pa_tag;
-   ih->ih_intrpin = vec;
-   ih->ih_type = PCI_MSIX;
-   *ihp = (pci_intr_handle_t)ih;
+   ihp->ih_pc = pa->pa_pc;
+   ihp->ih_tag = pa->pa_intrtag;
+   ihp->ih_intrpin = pa->pa_intrpin;
+   ihp->ih_type = PCI_INTX;
 
return 0;
 }
 
 const char *
-dwpcie_intr_string(void *v, pci_intr_handle_t ihp)
+dwpcie_intr_string(void *v, pci_intr_handle_t ih)
 {
-   struct dwpcie_intr_handle *ih = (struct dwpcie_intr_handle *)ihp;
-
-   switch (ih->ih_type) {
+   switch (ih.ih_type) {
case PCI_MSI:

Re: PCI interrupt functions

[Theo de Raadt]

> On arm64, pci_intr_handle_t is a pointer to an opaque struct.

That's a subtle trap.  How would someone realize the order is wrong...

Would it not be better if this was done like the other architectures,
where the pci_intr_handle_t is a structure, not a pointer.

On arm64, the pci subsystems seem to use structures which are
the same, and additional fields could be added easily if there
was a need later.

PCI interrupt functions

[Mark Kettenis]

To set up an interrupt handler for a PCI device, you need to do
something like this:


pci_intr_handle_t ih;

pci_intr_map(pa, );

printf(": %s", pci_intr_string(pa->pa_pc, ih));

pci_intr_establish(pa->pa_pc, ih, ...);


On arm64, pci_intr_handle_t is a pointer to an opaque struct.  The
pci_intr_map() function allocates that struct using malloc(9) and in
order not to leak the memory, pci_intr_establish() frees the struct
using free(9).  That is all fine, except that some drivers do things
in a slightly different order:


pci_intr_handle_t ih;

pci_intr_map(pa, );

pci_intr_establish(pa->pa_pc, ih, ...);

printf(": %s", pci_intr_string(pa->pa_pc, ih));


That means the call to pci_intr_string() is a use-after-free.  The
consequences are rather mild; it prints the wrong string.  But this
isn't right.

Now the big question is whether the arm64 implementation needs to be
changed, or whether we should fix the drivers.  Below is a fix to
nvme(4) for example.

Thoughts?


Index: dev/pci/nvme_pci.c
===
RCS file: /cvs/src/sys/dev/pci/nvme_pci.c,v
retrieving revision 1.7
diff -u -p -r1.7 nvme_pci.c
--- dev/pci/nvme_pci.c  10 Jan 2018 15:45:46 -  1.7
+++ dev/pci/nvme_pci.c  31 May 2019 20:33:46 -
@@ -90,6 +90,7 @@ nvme_pci_attach(struct device *parent, s
struct pci_attach_args *pa = aux;
pcireg_t maptype;
pci_intr_handle_t ih;
+   const char *intrstr;
int msi = 1;
 
psc->psc_pc = pa->pa_pc;
@@ -113,14 +114,18 @@ nvme_pci_attach(struct device *parent, s
msi = 0;
}
 
+   intrstr = pci_intr_string(pa->pa_pc, ih);
sc->sc_ih = pci_intr_establish(pa->pa_pc, ih, IPL_BIO,
msi ? nvme_intr : nvme_intr_intx, sc, DEVNAME(sc));
if (sc->sc_ih == NULL) {
-   printf(": unable to establish interrupt\n");
+   printf(": unable to establish interrupt");
+   if (intrstr != NULL)
+   printf(" at %s", intrstr);
+   printf("\n");
goto unmap;
}
 
-   printf(": %s", pci_intr_string(pa->pa_pc, ih));
+   printf(": %s", intrstr);
if (nvme_attach(sc) != 0) {
/* error printed by nvme_attach() */
goto disestablish;