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

Greg Kurz posted 1 patch 6 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/151239850618.30749.18071098158203257789.stgit@bahia.lan
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/ppc/spapr_events.c  |    3 +--
hw/ppc/spapr_pci.c     |    5 +++--
hw/ppc/spapr_vio.c     |    3 ++-
include/hw/ppc/spapr.h |   11 +++++++++++
4 files changed, 17 insertions(+), 5 deletions(-)
[Qemu-devel] [PATCH v2] spapr: fix LSI interrupt specifiers in the device tree
Posted by Greg Kurz 6 years, 3 months ago
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 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.

VIO devices and RTAS event sources use the same format for interrupt
specifiers: while here, we introduce a common helper to handle the
encoding details.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>
--
v2: - drop the erroneous changes to the "interrupts" prop in PCI device nodes
    - introduce a common helper to encode interrupt specifiers
---
 hw/ppc/spapr_events.c  |    3 +--
 hw/ppc/spapr_pci.c     |    5 +++--
 hw/ppc/spapr_vio.c     |    3 ++-
 include/hw/ppc/spapr.h |   11 +++++++++++
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 7dc87fc7bd77..c8205795b0c8 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -282,8 +282,7 @@ void spapr_dt_events(sPAPRMachineState *spapr, void *fdt)
             continue;
         }
 
-        interrupts[0] = cpu_to_be32(source->irq);
-        interrupts[1] = 0;
+        spapr_dt_encode_interrupt_specifier(interrupts, source->irq, false);
 
         _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 39134f0ef0a3..78769e833dd0 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2121,8 +2121,9 @@ int spapr_populate_pci_dt(sPAPRPHBState *phb,
             irqmap[2] = 0;
             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);
+            spapr_dt_encode_interrupt_specifier(&irqmap[5],
+                                                phb->lsi_table[lsi_num].irq,
+                                                true);
         }
     }
     /* Write interrupt map */
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index bb7ed2c537b0..582b50a0443d 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -126,8 +126,9 @@ static int vio_make_devnode(VIOsPAPRDevice *dev,
     }
 
     if (dev->irq) {
-        uint32_t ints_prop[] = {cpu_to_be32(dev->irq), 0};
+        uint32_t ints_prop[2];
 
+        spapr_dt_encode_interrupt_specifier(ints_prop, dev->irq, false);
         ret = fdt_setprop(fdt, node_off, "interrupts", ints_prop,
                           sizeof(ints_prop));
         if (ret < 0) {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 6b8e04c78771..423e72338309 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -590,6 +590,17 @@ void spapr_load_rtas(sPAPRMachineState *spapr, void *fdt, hwaddr addr);
 
 #define RTAS_EVENT_SCAN_RATE    1
 
+/* This helper should be used to encode interrupt specifiers when the related
+ * "interrupt-controller" node has its "#interrupt-cells" property set to 2 (ie,
+ * VIO devices, RTAS event sources and PHBs).
+ */
+static inline void spapr_dt_encode_interrupt_specifier(uint32_t *intspec,
+                                                       int irq, bool is_lsi)
+{
+    intspec[0] = cpu_to_be32(irq);
+    intspec[1] = is_lsi ? cpu_to_be32(1) : 0;
+}
+
 typedef struct sPAPRTCETable sPAPRTCETable;
 
 #define TYPE_SPAPR_TCE_TABLE "spapr-tce-table"


Re: [Qemu-devel] [PATCH v2] spapr: fix LSI interrupt specifiers in the device tree
Posted by David Gibson 6 years, 3 months ago
On Mon, Dec 04, 2017 at 03:41:46PM +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 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.
> 
> VIO devices and RTAS event sources use the same format for interrupt
> specifiers: while here, we introduce a common helper to handle the
> encoding details.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Tested-by: Cédric Le Goater <clg@kaod.org>

Looks good, but I'd request two minor changes.

1) Please reference LoPAPR instead of the internal PAPR, since other
people can't see it.

2) Change the helper's name to spapr_dt_xics_irq(), for both brevity
and specificity.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH v2] spapr: fix LSI interrupt specifiers in the device tree
Posted by Greg Kurz 6 years, 3 months ago
On Wed, 6 Dec 2017 11:25:44 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Dec 04, 2017 at 03:41:46PM +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 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.
> > 
> > VIO devices and RTAS event sources use the same format for interrupt
> > specifiers: while here, we introduce a common helper to handle the
> > encoding details.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Tested-by: Cédric Le Goater <clg@kaod.org>  
> 
> Looks good, but I'd request two minor changes.
> 
> 1) Please reference LoPAPR instead of the internal PAPR, since other
> people can't see it.
> 
> 2) Change the helper's name to spapr_dt_xics_irq(), for both brevity
> and specificity.
> 

Sure, I'll do that. Thanks!