Re: [Qemu-devel] [PATCH 3/3] xics: report errors with the QEMU Error API

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9646 bytes --]

On Thu, Feb 25, 2016 at 07:02:25PM +0100, Greg Kurz wrote:
> Using the return value to report errors is error prone:
> - xics_alloc() returns -1 on error but spapr_vio_busdev_realize() errors
>   on 0
> - xics_alloc_block() returns the unclear value of ics->offset - 1 on error
>   but both rtas_ibm_change_msi() and spapr_phb_realize() error on 0
> 
> This patch turns xics_alloc() and xics_alloc_block() into void functions
> with an errp argument as only way to report errors, and an unsigned int *
> argument to return the IRQ number in case of success.
> 
> The corresponding error traces get promotted to error messages.
> 
> This fixes the issues mentioned above.
> 
> Based on previous work from Brian W. Hart.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/intc/xics.c        |   22 ++++++++++++++--------
>  hw/ppc/spapr_events.c |    2 +-
>  hw/ppc/spapr_pci.c    |   18 +++++++++++-------
>  hw/ppc/spapr_vio.c    |    8 +++++---
>  include/hw/ppc/xics.h |    6 ++++--
>  trace-events          |    2 --
>  6 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e66ae328819a..32f28f1693fd 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -712,7 +712,8 @@ static int ics_find_free_block(ICSState *ics, int num, int alignnum)
>      return -1;
>  }
>  
> -int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
> +void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
> +                unsigned int *irqp, Error **errp)

So, I like adding the error API, but I don't really like the change to
a void function.  I think it would be better to keep returning the irq
number through the return value in the success case, and just return
the error values.

>  {
>      ICSState *ics = &icp->ics[src];
>      int irq;
> @@ -720,15 +721,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
>      if (irq_hint) {
>          assert(src == xics_find_source(icp, irq_hint));
>          if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) {
> -            trace_xics_alloc_failed_hint(src, irq_hint);
> -            return -1;
> +            error_setg(errp, "IRQ %d is already in use", irq_hint);
> +            return;

So, I know the callers add extra information, but I don't think you
should really count on that.  It would be better to have a more
complete error message right here, in case this appears without
context.

>          }
>          irq = irq_hint;
>      } else {
>          irq = ics_find_free_block(ics, 1, 1);
>          if (irq < 0) {
> -            trace_xics_alloc_failed_no_left(src);
> -            return -1;
> +            error_setg(errp, "no IRQ left");
> +            return;
>          }
>          irq += ics->offset;
>      }
> @@ -736,14 +737,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
>      ics_set_irq_type(ics, irq - ics->offset, lsi);
>      trace_xics_alloc(src, irq);
>  
> -    return irq;
> +    *irqp = irq;
>  }
>  
>  /*
>   * Allocate block of consecutive IRQs, and return the number of the first IRQ in the block.
>   * If align==true, aligns the first IRQ number to num.
>   */
> -int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
> +void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                      unsigned int *irqp, Error **errp)
>  {
>      int i, first = -1;
>      ICSState *ics = &icp->ics[src];
> @@ -763,6 +765,10 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
>      } else {
>          first = ics_find_free_block(ics, num, 1);
>      }
> +    if (first < 0) {
> +        error_setg(errp, "cannot find a %d-IRQ block", num);
> +        return;
> +    }
>  
>      if (first >= 0) {
>          for (i = first; i < first + num; ++i) {
> @@ -773,7 +779,7 @@ int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align)
>  
>      trace_xics_alloc_block(src, first, num, lsi, align);
>  
> -    return first;
> +    *irqp = first;
>  }
>  
>  static void ics_free(ICSState *ics, int srcno, int num)
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index f5eac4b5441c..d6741d300952 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -588,7 +588,7 @@ out_no_events:
>  void spapr_events_init(sPAPRMachineState *spapr)
>  {
>      QTAILQ_INIT(&spapr->pending_events);
> -    spapr->check_exception_irq = xics_alloc(spapr->icp, 0, 0, false);
> +    xics_alloc(spapr->icp, 0, 0, false, &spapr->check_exception_irq, NULL);

I don't think you want NULL for the errp, you probably want &error_fatal.

>      spapr->epow_notifier.notify = spapr_powerdown_req;
>      qemu_register_powerdown_notifier(&spapr->epow_notifier);
>      spapr_rtas_register(RTAS_CHECK_EXCEPTION, "check-exception",
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 9b2b546b541c..9d1a2b7f7279 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -280,6 +280,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      PCIDevice *pdev = NULL;
>      spapr_pci_msi *msi;
>      int *config_addr_key;
> +    Error *err = NULL;
>  
>      switch (func) {
>      case RTAS_CHANGE_MSI_FN:
> @@ -353,10 +354,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      }
>  
>      /* Allocate MSIs */
> -    irq = xics_alloc_block(spapr->icp, 0, req_num, false,
> -                           ret_intr_type == RTAS_TYPE_MSI);
> -    if (!irq) {
> -        error_report("Cannot allocate MSIs for device %x", config_addr);
> +    xics_alloc_block(spapr->icp, 0, req_num, false,
> +                     ret_intr_type == RTAS_TYPE_MSI, &irq, &err);
> +    if (err) {
> +        error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> +                          config_addr);
>          rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
>          return;
>      }
> @@ -1367,10 +1369,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      /* Initialize the LSI table */
>      for (i = 0; i < PCI_NUM_PINS; i++) {
>          uint32_t irq;
> +        Error *local_err = NULL;
>  
> -        irq = xics_alloc_block(spapr->icp, 0, 1, true, false);
> -        if (!irq) {
> -            error_setg(errp, "spapr_allocate_lsi failed");
> +        xics_alloc_block(spapr->icp, 0, 1, true, false, &irq, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "can't allocate LSIs: ");
>              return;
>          }
>  
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index ac6666a90be7..91b1e8e75385 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -431,6 +431,7 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>      VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev;
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
>      char *id;
> +    Error *local_err = NULL;
>  
>      if (dev->reg != -1) {
>          /*
> @@ -463,9 +464,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          dev->qdev.id = id;
>      }
>  
> -    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false);
> -    if (!dev->irq) {
> -        error_setg(errp, "can't allocate IRQ");
> +    xics_alloc(spapr->icp, 0, dev->irq, false, &dev->irq, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "can't allocate IRQ: ");
>          return;
>      }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 355a96623c70..3c10fbac8852 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,8 +161,10 @@ struct ICSIRQState {
>  
>  qemu_irq xics_get_qirq(XICSState *icp, int irq);
>  void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
> -int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi);
> -int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align);
> +void xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi,
> +                unsigned int *irqp, Error **errp);
> +void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                      unsigned int *irqp, Error **errp);
>  void xics_free(XICSState *icp, int irq, int num);
>  
>  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> diff --git a/trace-events b/trace-events
> index f986c81dada5..3072b45d4081 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1412,8 +1412,6 @@ xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_
>  xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>  xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>  xics_alloc(int src, int irq) "source#%d, irq %d"
> -xics_alloc_failed_hint(int src, int irq) "source#%d, irq %d is already in use"
> -xics_alloc_failed_no_left(int src) "source#%d, no irq left"
>  xics_alloc_block(int src, int first, int num, bool lsi, int align) "source#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>  xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free"
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]