| Message ID | 151224301160.13812.16487624528793386353.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: fix LSI interrupt specifiers in the device tree | expand |
On Sat, Dec 02, 2017 at 08:30:11PM +0100, Greg Kurz wrote: > PAPR 2.7 C.6.9.1.2 describes the "#interrupt-cells" property of the > PowerPC External Interrupt Source Controller node as follows: > > “#interrupt-cells” > > Standard property name to define the number of cells in an interrupt- > specifier within an interrupt domain. > > prop-encoded-array: An integer, encoded as with encode-int, that denotes > the number of cells required to represent an interrupt specifier in its > child nodes. > > The value of this property for the PowerPC External Interrupt option shall > be 2. Thus all interrupt specifiers (as used in the standard “interrupts” > property) shall consist of two cells, each containing an integer encoded > as with encode-int. The first integer represents the interrupt number the > second integer is the trigger code: 0 for edge triggered, 1 for level > triggered. > > This patch adds a second cell to the interrupt specifier stored in the > "interrupts" property of PCI device nodes. This property only exists if > the Interrupt Pin register is set, ie, the interrupt is level, the extra > cell is hence set to 1. Nack. This format of interrupt specifier is only needed for things wired directly to the xics. The PCI INTx interrupts aren't - they go through the interrupt nexus in the PHB. The interrupt-map is intended to remap the simple 1,2,3,4 for INTA..INTD to xics interrupt specifiers. > This also fixes the interrupt specifiers in the "interrupt-map" property > of the PHB node, that were setting the second cell to 8 (confusion with > IRQ_TYPE_LEVEL_LOW ?) instead of 1. Fixing that is correct, though I think. As might be the changes in other places I'll have to check. > While here, let's introduce defines for the interrupt specifier trigger > code, and patch other users in spapr. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > > This fixes /proc/interrupts in linux guests where LSIs appear as > Edge instead of Level. > --- > hw/ppc/spapr_events.c | 2 +- > hw/ppc/spapr_pci.c | 4 +++- > hw/ppc/spapr_vio.c | 3 ++- > include/hw/ppc/spapr.h | 3 +++ > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index e377fc7ddea2..4bcb98f948ea 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -283,7 +283,7 @@ void spapr_dt_events(sPAPRMachineState *spapr, void *fdt) > } > > interrupts[0] = cpu_to_be32(source->irq); > - interrupts[1] = 0; > + interrupts[1] = SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE; > > _FDT(node_offset = fdt_add_subnode(fdt, event_sources, source_name)); > _FDT(fdt_setprop(fdt, node_offset, "interrupts", interrupts, > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 5a3122a9f9f9..91fedbf0929c 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1231,6 +1231,8 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, > if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { > _FDT(fdt_setprop_cell(fdt, offset, "interrupts", > pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); > + _FDT(fdt_appendprop_cell(fdt, offset, "interrupts", > + SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL)); > } > > if (!is_bridge) { > @@ -2122,7 +2124,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, > irqmap[3] = cpu_to_be32(j+1); > irqmap[4] = cpu_to_be32(xics_phandle); > irqmap[5] = cpu_to_be32(phb->lsi_table[lsi_num].irq); > - irqmap[6] = cpu_to_be32(0x8); > + irqmap[6] = cpu_to_be32(SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL); > } > } > /* Write interrupt map */ > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index ea3bc8bd9e21..29a17651a17c 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -126,7 +126,8 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, > } > > if (dev->irq) { > - uint32_t ints_prop[] = {cpu_to_be32(dev->irq), 0}; > + uint32_t ints_prop[] = { cpu_to_be32(dev->irq), > + SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE }; > > ret = fdt_setprop(fdt, node_off, "interrupts", ints_prop, > sizeof(ints_prop)); > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 9d21ca9bde3a..8f6298bde59b 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -590,6 +590,9 @@ void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr); > > #define RTAS_EVENT_SCAN_RATE 1 > > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE 0 > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL 1 > + > typedef struct sPAPRTCETable sPAPRTCETable; > > #define TYPE_SPAPR_TCE_TABLE "spapr-tce-table" >
On 12/02/2017 08:30 PM, Greg Kurz wrote: > PAPR 2.7 C.6.9.1.2 describes the "#interrupt-cells" property of the > PowerPC External Interrupt Source Controller node as follows: > > “#interrupt-cells” > > Standard property name to define the number of cells in an interrupt- > specifier within an interrupt domain. > > prop-encoded-array: An integer, encoded as with encode-int, that denotes > the number of cells required to represent an interrupt specifier in its > child nodes. > > The value of this property for the PowerPC External Interrupt option shall > be 2. Thus all interrupt specifiers (as used in the standard “interrupts” > property) shall consist of two cells, each containing an integer encoded > as with encode-int. The first integer represents the interrupt number the > second integer is the trigger code: 0 for edge triggered, 1 for level > triggered. > > This patch adds a second cell to the interrupt specifier stored in the > "interrupts" property of PCI device nodes. This property only exists if > the Interrupt Pin register is set, ie, the interrupt is level, the extra > cell is hence set to 1. > > This also fixes the interrupt specifiers in the "interrupt-map" property > of the PHB node, that were setting the second cell to 8 (confusion with > IRQ_TYPE_LEVEL_LOW ?) instead of 1. > > While here, let's introduce defines for the interrupt specifier trigger > code, and patch other users in spapr. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > > This fixes /proc/interrupts in linux guests where LSIs appear as > Edge instead of Level. It does and also XIVE stops complaining with such warning when an LSI interrupt is configured : [ 20.137390] xive: Interrupt 17 (HW 0x1004) type mismatch, Linux says Edge, FW says Level and we know have : (initramfs) ip link set up dev enp0s0 [ 20.186717] 8139cp 0000:00:00.0 enp0s0: link up, 100Mbps, full-duplex, lpa 0x05E1 (initramfs) cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 16: 341 635 485 538 XIVE-IPI 0 Edge IPI 17: 0 0 0 5 XIVE-IRQ 4100 Level enp0s0 18: 0 0 0 0 XIVE-IRQ 4097 Edge RAS_HOTPLUG 19: 0 0 0 0 XIVE-IRQ 4096 Edge RAS_EPOW 20: 0 0 25 0 XIVE-IRQ 4098 Edge hvc_console The "interrupt-map" property is not obvious to understand but indeed the last 2 cells of a row are also determined by the #interrupt-cells property. Some comments would be welcomed. See device tree specification for that. Reviewed-by: Cédric Le Goater <clg@kaod.org> Tested-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/ppc/spapr_events.c | 2 +- > hw/ppc/spapr_pci.c | 4 +++- > hw/ppc/spapr_vio.c | 3 ++- > include/hw/ppc/spapr.h | 3 +++ > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index e377fc7ddea2..4bcb98f948ea 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -283,7 +283,7 @@ void spapr_dt_events(sPAPRMachineState *spapr, void *fdt) > } > > interrupts[0] = cpu_to_be32(source->irq); > - interrupts[1] = 0; > + interrupts[1] = SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE; > > _FDT(node_offset = fdt_add_subnode(fdt, event_sources, source_name)); > _FDT(fdt_setprop(fdt, node_offset, "interrupts", interrupts, > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 5a3122a9f9f9..91fedbf0929c 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1231,6 +1231,8 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, > if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { > _FDT(fdt_setprop_cell(fdt, offset, "interrupts", > pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); > + _FDT(fdt_appendprop_cell(fdt, offset, "interrupts", > + SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL)); > } > > if (!is_bridge) { > @@ -2122,7 +2124,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, > irqmap[3] = cpu_to_be32(j+1); > irqmap[4] = cpu_to_be32(xics_phandle); > irqmap[5] = cpu_to_be32(phb->lsi_table[lsi_num].irq); > - irqmap[6] = cpu_to_be32(0x8); > + irqmap[6] = cpu_to_be32(SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL); > } > } > /* Write interrupt map */ > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index ea3bc8bd9e21..29a17651a17c 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -126,7 +126,8 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, > } > > if (dev->irq) { > - uint32_t ints_prop[] = {cpu_to_be32(dev->irq), 0}; > + uint32_t ints_prop[] = { cpu_to_be32(dev->irq), > + SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE }; > > ret = fdt_setprop(fdt, node_off, "interrupts", ints_prop, > sizeof(ints_prop)); > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 9d21ca9bde3a..8f6298bde59b 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -590,6 +590,9 @@ void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr); > > #define RTAS_EVENT_SCAN_RATE 1 > > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE 0 > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL 1 > + > typedef struct sPAPRTCETable sPAPRTCETable; > > #define TYPE_SPAPR_TCE_TABLE "spapr-tce-table" >
On Sun, 3 Dec 2017 10:37:35 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Sat, Dec 02, 2017 at 08:30:11PM +0100, Greg Kurz wrote: > > PAPR 2.7 C.6.9.1.2 describes the "#interrupt-cells" property of the > > PowerPC External Interrupt Source Controller node as follows: > > > > “#interrupt-cells” > > > > Standard property name to define the number of cells in an interrupt- > > specifier within an interrupt domain. > > > > prop-encoded-array: An integer, encoded as with encode-int, that denotes > > the number of cells required to represent an interrupt specifier in its > > child nodes. > > > > The value of this property for the PowerPC External Interrupt option shall > > be 2. Thus all interrupt specifiers (as used in the standard “interrupts” > > property) shall consist of two cells, each containing an integer encoded > > as with encode-int. The first integer represents the interrupt number the > > second integer is the trigger code: 0 for edge triggered, 1 for level > > triggered. > > > > This patch adds a second cell to the interrupt specifier stored in the > > "interrupts" property of PCI device nodes. This property only exists if > > the Interrupt Pin register is set, ie, the interrupt is level, the extra > > cell is hence set to 1. > > Nack. This format of interrupt specifier is only needed for things > wired directly to the xics. The PCI INTx interrupts aren't - they go > through the interrupt nexus in the PHB. The interrupt-map is intended > to remap the simple 1,2,3,4 for INTA..INTD to xics interrupt specifiers. > Indeed you're right... and the “#interrupt-cells” property of the PHB is set to 1, ie, the interrupt specifier format in the child PCI isn't expected to have an extra cell... This should have rung a bell :) > > This also fixes the interrupt specifiers in the "interrupt-map" property > > of the PHB node, that were setting the second cell to 8 (confusion with > > IRQ_TYPE_LEVEL_LOW ?) instead of 1. > > Fixing that is correct, though I think. As might be the changes in > other places I'll have to check. > > > While here, let's introduce defines for the interrupt specifier trigger > > code, and patch other users in spapr. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > This fixes /proc/interrupts in linux guests where LSIs appear as > > Edge instead of Level. > > --- > > hw/ppc/spapr_events.c | 2 +- > > hw/ppc/spapr_pci.c | 4 +++- > > hw/ppc/spapr_vio.c | 3 ++- > > include/hw/ppc/spapr.h | 3 +++ > > 4 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > > index e377fc7ddea2..4bcb98f948ea 100644 > > --- a/hw/ppc/spapr_events.c > > +++ b/hw/ppc/spapr_events.c > > @@ -283,7 +283,7 @@ void spapr_dt_events(sPAPRMachineState *spapr, void *fdt) > > } > > > > interrupts[0] = cpu_to_be32(source->irq); > > - interrupts[1] = 0; > > + interrupts[1] = SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE; > > > > _FDT(node_offset = fdt_add_subnode(fdt, event_sources, source_name)); > > _FDT(fdt_setprop(fdt, node_offset, "interrupts", interrupts, > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 5a3122a9f9f9..91fedbf0929c 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1231,6 +1231,8 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, > > if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { > > _FDT(fdt_setprop_cell(fdt, offset, "interrupts", > > pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); > > + _FDT(fdt_appendprop_cell(fdt, offset, "interrupts", > > + SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL)); > > } > > > > if (!is_bridge) { > > @@ -2122,7 +2124,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, > > irqmap[3] = cpu_to_be32(j+1); > > irqmap[4] = cpu_to_be32(xics_phandle); > > irqmap[5] = cpu_to_be32(phb->lsi_table[lsi_num].irq); > > - irqmap[6] = cpu_to_be32(0x8); > > + irqmap[6] = cpu_to_be32(SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL); > > } > > } > > /* Write interrupt map */ > > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > > index ea3bc8bd9e21..29a17651a17c 100644 > > --- a/hw/ppc/spapr_vio.c > > +++ b/hw/ppc/spapr_vio.c > > @@ -126,7 +126,8 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, > > } > > > > if (dev->irq) { > > - uint32_t ints_prop[] = {cpu_to_be32(dev->irq), 0}; > > + uint32_t ints_prop[] = { cpu_to_be32(dev->irq), > > + SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE }; > > > > ret = fdt_setprop(fdt, node_off, "interrupts", ints_prop, > > sizeof(ints_prop)); > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index 9d21ca9bde3a..8f6298bde59b 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -590,6 +590,9 @@ void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr); > > > > #define RTAS_EVENT_SCAN_RATE 1 > > > > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE 0 > > +#define SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL 1 > > + > > typedef struct sPAPRTCETable sPAPRTCETable; > > > > #define TYPE_SPAPR_TCE_TABLE "spapr-tce-table" > > >
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index e377fc7ddea2..4bcb98f948ea 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -283,7 +283,7 @@ void spapr_dt_events(sPAPRMachineState *spapr, void *fdt) } interrupts[0] = cpu_to_be32(source->irq); - interrupts[1] = 0; + interrupts[1] = SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE; _FDT(node_offset = fdt_add_subnode(fdt, event_sources, source_name)); _FDT(fdt_setprop(fdt, node_offset, "interrupts", interrupts, diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 5a3122a9f9f9..91fedbf0929c 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1231,6 +1231,8 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset, if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) { _FDT(fdt_setprop_cell(fdt, offset, "interrupts", pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1))); + _FDT(fdt_appendprop_cell(fdt, offset, "interrupts", + SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL)); } if (!is_bridge) { @@ -2122,7 +2124,7 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb, irqmap[3] = cpu_to_be32(j+1); irqmap[4] = cpu_to_be32(xics_phandle); irqmap[5] = cpu_to_be32(phb->lsi_table[lsi_num].irq); - irqmap[6] = cpu_to_be32(0x8); + irqmap[6] = cpu_to_be32(SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL); } } /* Write interrupt map */ diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index ea3bc8bd9e21..29a17651a17c 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -126,7 +126,8 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, } if (dev->irq) { - uint32_t ints_prop[] = {cpu_to_be32(dev->irq), 0}; + uint32_t ints_prop[] = { cpu_to_be32(dev->irq), + SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE }; ret = fdt_setprop(fdt, node_off, "interrupts", ints_prop, sizeof(ints_prop)); diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 9d21ca9bde3a..8f6298bde59b 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -590,6 +590,9 @@ void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr); #define RTAS_EVENT_SCAN_RATE 1 +#define SPAPR_DT_INTERRUPT_IDENTIFIER_EDGE 0 +#define SPAPR_DT_INTERRUPT_IDENTIFIER_LEVEL 1 + typedef struct sPAPRTCETable sPAPRTCETable; #define TYPE_SPAPR_TCE_TABLE "spapr-tce-table"
PAPR 2.7 C.6.9.1.2 describes the "#interrupt-cells" property of the PowerPC External Interrupt Source Controller node as follows: “#interrupt-cells” Standard property name to define the number of cells in an interrupt- specifier within an interrupt domain. prop-encoded-array: An integer, encoded as with encode-int, that denotes the number of cells required to represent an interrupt specifier in its child nodes. The value of this property for the PowerPC External Interrupt option shall be 2. Thus all interrupt specifiers (as used in the standard “interrupts” property) shall consist of two cells, each containing an integer encoded as with encode-int. The first integer represents the interrupt number the second integer is the trigger code: 0 for edge triggered, 1 for level triggered. This patch adds a second cell to the interrupt specifier stored in the "interrupts" property of PCI device nodes. This property only exists if the Interrupt Pin register is set, ie, the interrupt is level, the extra cell is hence set to 1. This also fixes the interrupt specifiers in the "interrupt-map" property of the PHB node, that were setting the second cell to 8 (confusion with IRQ_TYPE_LEVEL_LOW ?) instead of 1. While here, let's introduce defines for the interrupt specifier trigger code, and patch other users in spapr. Signed-off-by: Greg Kurz <groug@kaod.org> --- This fixes /proc/interrupts in linux guests where LSIs appear as Edge instead of Level. --- hw/ppc/spapr_events.c | 2 +- hw/ppc/spapr_pci.c | 4 +++- hw/ppc/spapr_vio.c | 3 ++- include/hw/ppc/spapr.h | 3 +++ 4 files changed, 9 insertions(+), 3 deletions(-)