Re: [PATCH 18/20] xive: Improve irq claim/free path

[Cédric Le Goater]

On 25/09/2019 08:45, David Gibson wrote:
> spapr_xive_irq_claim() returns a bool to indicate if it succeeded.  But
> most of the callers and one callee use a richer Error * instead.  So use
> that instead of a bool return so we can actually pass more informative
> errors up the stack.
> 
> In addition it didn't actually check if the irq was already claimed, which
> is one of the primary purposes of the claim path, so do that.
> 
> spapr_xive_irq_free() also returned a bool... which no callers checked, so
> just drop it.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/intc/spapr_xive.c        | 17 ++++++++++-------
>  hw/ppc/spapr_irq.c          | 12 ++++++++----
>  include/hw/ppc/spapr_xive.h |  5 +++--
>  3 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 47b5ec0b56..5a56a58299 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -528,12 +528,18 @@ static void spapr_xive_register_types(void)
>  
>  type_init(spapr_xive_register_types)
>  
> -bool spapr_xive_irq_claim(SpaprXive *xive, uint32_t lisn, bool lsi)
> +void spapr_xive_irq_claim(SpaprXive *xive, uint32_t lisn, bool lsi,
> +                          Error **errp)
>  {
>      XiveSource *xsrc = &xive->source;
>  
>      assert(lisn < xive->nr_irqs);
>  
> +    if (be64_to_cpu(xive->eat[lisn].w) & EAS_VALID) {

please use xive_eas_is_valid()

with that change,

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


C. 

> +        error_setg(errp, "IRQ %d is not free", lisn);
> +        return;
> +    }
> +
>      /*
>       * Set default values when allocating an IRQ number
>       */
> @@ -547,20 +553,17 @@ bool spapr_xive_irq_claim(SpaprXive *xive, uint32_t 
> lisn, bool lsi)
>  
>          kvmppc_xive_source_reset_one(xsrc, lisn, &local_err);
>          if (local_err) {
> -            error_report_err(local_err);
> -            return false;
> +            error_propagate(errp, local_err);
> +            return;
>          }
>      }
> -
> -    return true;
>  }
>  
> -bool spapr_xive_irq_free(SpaprXive *xive, uint32_t lisn)
> +void spapr_xive_irq_free(SpaprXive *xive, uint32_t lisn)
>  {
>      assert(lisn < xive->nr_irqs);
>  
>      xive->eat[lisn].w &= cpu_to_be64(~EAS_VALID);
> -    return true;
>  }
>  
>  /*
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2673a90604..f53544e45e 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -245,7 +245,13 @@ static void spapr_irq_init_xive(SpaprMachineState 
> *spapr, Error **errp)
>  
>      /* Enable the CPU IPIs */
>      for (i = 0; i < nr_servers; ++i) {
> -        spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false);
> +        Error *local_err = NULL;
> +
> +        spapr_xive_irq_claim(spapr->xive, SPAPR_IRQ_IPI + i, false, 
> &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>  
>      spapr_xive_hcall_init(spapr);
> @@ -254,9 +260,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, 
> Error **errp)
>  static void spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi,
>                                   Error **errp)
>  {
> -    if (!spapr_xive_irq_claim(spapr->xive, irq, lsi)) {
> -        error_setg(errp, "IRQ %d is invalid", irq);
> -    }
> +    spapr_xive_irq_claim(spapr->xive, irq, lsi, errp);
>  }
>  
>  static void spapr_irq_free_xive(SpaprMachineState *spapr, int irq)
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index bfd40f01d8..69df3793e1 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -54,8 +54,9 @@ typedef struct SpaprXive {
>   */
>  #define SPAPR_XIVE_BLOCK_ID 0x0
>  
> -bool spapr_xive_irq_claim(SpaprXive *xive, uint32_t lisn, bool lsi);
> -bool spapr_xive_irq_free(SpaprXive *xive, uint32_t lisn);
> +void spapr_xive_irq_claim(SpaprXive *xive, uint32_t lisn, bool lsi,
> +                          Error **errp);
> +void spapr_xive_irq_free(SpaprXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
>  int spapr_xive_post_load(SpaprXive *xive, int version_id);
>  
>