Re: [PATCH 12/20] spapr: Simplify spapr_qirq() handling

[Cédric Le Goater]

On 25/09/2019 08:45, David Gibson wrote:
> Currently spapr_qirq() used to find the qemu_irq for an spapr global irq
> number, redirects through the SpaprIrq::qirq method.  But the array of
> qemu_irqs is allocated in the PAPR layer, not the backends, and so the
> method implementations all return the same thing, just differing in the
> preliminary checks they make.
> 
> So, we can remove the method, and just implement spapr_qirq() directly,
> including all the relevant checks in one place.  We change all those
> checks into assert()s as well, since a failure here indicates an error in
> the calling code.

nice,
 
> Signed-off-by: David Gibson <address@hidden>

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

C.

> ---
>  hw/ppc/spapr_irq.c         | 47 ++++++++++----------------------------
>  include/hw/ppc/spapr_irq.h |  1 -
>  2 files changed, 12 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 9a9e486eb5..038bfffff4 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -150,17 +150,6 @@ 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;
> -
> -    if (ics_valid_irq(ics, irq)) {
> -        return spapr->qirqs[irq];
> -    }
> -
> -    return NULL;
> -}
> -
>  static void spapr_irq_print_info_xics(SpaprMachineState *spapr, Monitor *mon)
>  {
>      CPUState *cs;
> @@ -242,7 +231,6 @@ SpaprIrq spapr_irq_xics = {
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
>      .free        = spapr_irq_free_xics,
> -    .qirq        = spapr_qirq_xics,
>      .print_info  = spapr_irq_print_info_xics,
>      .dt_populate = spapr_dt_xics,
>      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
> @@ -300,20 +288,6 @@ static void spapr_irq_free_xive(SpaprMachineState 
> *spapr, int irq, int num)
>      }
>  }
>  
> -static qemu_irq spapr_qirq_xive(SpaprMachineState *spapr, int irq)
> -{
> -    SpaprXive *xive = spapr->xive;
> -
> -    if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
> -        return NULL;
> -    }
> -
> -    /* The sPAPR machine/device should have claimed the IRQ before */
> -    assert(xive_eas_is_valid(&xive->eat[irq]));
> -
> -    return spapr->qirqs[irq];
> -}
> -
>  static void spapr_irq_print_info_xive(SpaprMachineState *spapr,
>                                        Monitor *mon)
>  {
> @@ -413,7 +387,6 @@ SpaprIrq spapr_irq_xive = {
>      .init        = spapr_irq_init_xive,
>      .claim       = spapr_irq_claim_xive,
>      .free        = spapr_irq_free_xive,
> -    .qirq        = spapr_qirq_xive,
>      .print_info  = spapr_irq_print_info_xive,
>      .dt_populate = spapr_dt_xive,
>      .cpu_intc_create = spapr_irq_cpu_intc_create_xive,
> @@ -487,11 +460,6 @@ static void spapr_irq_free_dual(SpaprMachineState 
> *spapr, int irq, int num)
>      spapr_irq_xive.free(spapr, irq, num);
>  }
>  
> -static qemu_irq spapr_qirq_dual(SpaprMachineState *spapr, int irq)
> -{
> -    return spapr_irq_current(spapr)->qirq(spapr, irq);
> -}
> -
>  static void spapr_irq_print_info_dual(SpaprMachineState *spapr, Monitor *mon)
>  {
>      spapr_irq_current(spapr)->print_info(spapr, mon);
> @@ -586,7 +554,6 @@ SpaprIrq spapr_irq_dual = {
>      .init        = spapr_irq_init_dual,
>      .claim       = spapr_irq_claim_dual,
>      .free        = spapr_irq_free_dual,
> -    .qirq        = spapr_qirq_dual,
>      .print_info  = spapr_irq_print_info_dual,
>      .dt_populate = spapr_irq_dt_populate_dual,
>      .cpu_intc_create = spapr_irq_cpu_intc_create_dual,
> @@ -700,7 +667,18 @@ void spapr_irq_free(SpaprMachineState *spapr, int irq, 
> int num)
>  
>  qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq)
>  {
> -    return spapr->irq->qirq(spapr, irq);
> +    assert(irq >= SPAPR_XIRQ_BASE);
> +    assert(irq < (spapr->irq->nr_xirqs + SPAPR_XIRQ_BASE));
> +
> +    if (spapr->ics) {
> +        assert(ics_valid_irq(spapr->ics, irq));
> +    }
> +    if (spapr->xive) {
> +        assert(irq < spapr->xive->nr_irqs);
> +        assert(xive_eas_is_valid(&spapr->xive->eat[irq]));
> +    }
> +
> +    return spapr->qirqs[irq];
>  }
>  
>  int spapr_irq_post_load(SpaprMachineState *spapr, int version_id)
> @@ -803,7 +781,6 @@ SpaprIrq spapr_irq_xics_legacy = {
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
>      .free        = spapr_irq_free_xics,
> -    .qirq        = spapr_qirq_xics,
>      .print_info  = spapr_irq_print_info_xics,
>      .dt_populate = spapr_dt_xics,
>      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 7e26288fcd..a4e790ef60 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -44,7 +44,6 @@ typedef struct SpaprIrq {
>      void (*init)(SpaprMachineState *spapr, Error **errp);
>      int (*claim)(SpaprMachineState *spapr, int irq, bool lsi, Error **errp);
>      void (*free)(SpaprMachineState *spapr, int irq, int num);
> -    qemu_irq (*qirq)(SpaprMachineState *spapr, int irq);
>      void (*print_info)(SpaprMachineState *spapr, Monitor *mon);
>      void (*dt_populate)(SpaprMachineState *spapr, uint32_t nr_servers,
>                          void *fdt, uint32_t phandle);
>