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

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

signature.asc
Description: PGP signature