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

[Cédric Le Goater]

On 25/09/2019 08:45, David Gibson 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>

The commit log clearly reflects what you think of the current solution 
in the code :) 

It is hideous, but it worked fine and this is even better :) 

Reviewed-by: Cédric Le Goater <address@hidden>

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