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 <address@hidden> 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 <address@hidden>  
> 
> 
> > ---
> > 
> > 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"
> >   
>

pgpscrgRH2oHY.pgp
Description: OpenPGP digital signature