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

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 <david@gibson.dropbear.id.au>
---
 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(-)

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 <david@gibson.dropbear.id.au>
> ---
>  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 <clg@kaod.org>


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

On Wed, Sep 25, 2019 at 09:25:47AM +0200, Cédric Le Goater wrote:
> 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 <david@gibson.dropbear.id.au>
> > ---
> >  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()

Oops, missed that that existed.  Fixed.

> with that change,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Oops, missed

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