Re: [PATCH 11/20] spapr: Fix indexing of XICS irqs

[Greg Kurz]

On Wed, 25 Sep 2019 16:45:25 +1000
David Gibson <address@hidden> wrote:

> spapr global irq numbers are different from the source numbers on the ICS
> when using XICS - they're offset by XICS_IRQ_BASE (0x1000).  But
> spapr_irq_set_irq_xics() was passing through the global irq number to
> the ICS code unmodified.
> 
> We only got away with this because of a counteracting bug - we were
> incorrectly adjusting the qemu_irq we returned for a requested global irq
> number.
> 
> That approach mostly worked but is very confusing, incorrectly relies on
> the way the qemu_irq array is allocated, and undermines the intention of
> having the global array of qemu_irqs for spapr have a consistent meaning
> regardless of irq backend.
> 
> So, fix both set_irq and qemu_irq indexing.  We rename some parameters at
> the same time to make it clear that they are referring to spapr global
> irq numbers.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---

Reviewed-by: Greg Kurz <address@hidden>

Further cleanup could be to have the XICS backend to only take global
irq numbers and to convert them to ICS source numbers internally. This
would put an end to the confusion between srcno/irq in the frontend
code.

>  hw/ppc/spapr_irq.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 300c65be3a..9a9e486eb5 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -153,10 +153,9 @@ static void spapr_irq_free_xics(SpaprMachineState 
> *spapr, int irq, int num)
>  static qemu_irq spapr_qirq_xics(SpaprMachineState *spapr, int irq)
>  {
>      ICSState *ics = spapr->ics;
> -    uint32_t srcno = irq - ics->offset;
>  
>      if (ics_valid_irq(ics, irq)) {
> -        return spapr->qirqs[srcno];
> +        return spapr->qirqs[irq];
>      }
>  
>      return NULL;
> @@ -204,9 +203,10 @@ static int spapr_irq_post_load_xics(SpaprMachineState 
> *spapr, int version_id)
>      return 0;
>  }
>  
> -static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
> +static void spapr_irq_set_irq_xics(void *opaque, int irq, int val)
>  {
>      SpaprMachineState *spapr = opaque;
> +    uint32_t srcno = irq - spapr->ics->offset;
>  
>      ics_set_irq(spapr->ics, srcno, val);
>  }
> @@ -377,14 +377,14 @@ static void spapr_irq_reset_xive(SpaprMachineState 
> *spapr, Error **errp)
>      spapr_xive_mmio_set_enabled(spapr->xive, true);
>  }
>  
> -static void spapr_irq_set_irq_xive(void *opaque, int srcno, int val)
> +static void spapr_irq_set_irq_xive(void *opaque, int irq, int val)
>  {
>      SpaprMachineState *spapr = opaque;
>  
>      if (kvm_irqchip_in_kernel()) {
> -        kvmppc_xive_source_set_irq(&spapr->xive->source, srcno, val);
> +        kvmppc_xive_source_set_irq(&spapr->xive->source, irq, val);
>      } else {
> -        xive_source_set_irq(&spapr->xive->source, srcno, val);
> +        xive_source_set_irq(&spapr->xive->source, irq, val);
>      }
>  }
>  
> @@ -563,11 +563,11 @@ static void spapr_irq_reset_dual(SpaprMachineState 
> *spapr, Error **errp)
>      spapr_irq_current(spapr)->reset(spapr, errp);
>  }
>  
> -static void spapr_irq_set_irq_dual(void *opaque, int srcno, int val)
> +static void spapr_irq_set_irq_dual(void *opaque, int irq, int val)
>  {
>      SpaprMachineState *spapr = opaque;
>  
> -    spapr_irq_current(spapr)->set_irq(spapr, srcno, val);
> +    spapr_irq_current(spapr)->set_irq(spapr, irq, val);
>  }
>  
>  static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)