Re: [Qemu-devel] [PATCH] spapr: fix LSI interrupt specifiers in the device tree

[Greg Kurz]

On Sun, 3 Dec 2017 10:37:35 +1100
David Gibson  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   
> 
> 
> > ---
> > 
> > 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 

Re: [Qemu-devel] [PATCH] spapr: fix LSI interrupt specifiers in the device tree

[David Gibson]

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 


> ---
> 
> 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_RATE1
>  
> +#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"
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT 

Re: [Qemu-devel] [PATCH] spapr: fix LSI interrupt specifiers in the device tree

[Cédric Le Goater]

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 
> ---
> 
> 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 :00:00.0 enp0s0: link up, 100Mbps, full-duplex, 
lpa 0x05E1
   (initramfs) cat /proc/interrupts 
  CPU0   CPU1   CPU2   CPU3   
16:341635485538  XIVE-IPI0 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 
Tested-by: Cédric Le Goater 

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[] = { 

[Qemu-devel] [PATCH] spapr: fix LSI interrupt specifiers in the device tree

[Greg Kurz]

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 
---

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_RATE1
 
+#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"