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

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

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

On Fri, Feb 26, 2016 at 10:44:07AM +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 adds an errp argument to xics_alloc() and xics_alloc_block() to
> report errors. The return value of these functions is a valid IRQ number
> if errp is NULL. It is undefined otherwise.
> 
> The corresponding error traces get promotted to error messages. Note that
> the "can't allocate IRQ" error message in spapr_vio_busdev_realize() also
> moves to xics_alloc(). Similar error message consolidation isn't really
> applicable to xics_alloc_block() because callers have extra context (device
> config address, MSI or MSIX).
> 
> This fixes the issues mentioned above.
> 
> Based on previous work from Brian W. Hart.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Merged, thanks.

> ---
> v2: - reverted to non-void xics_alloc() and xics_alloc_block()
>     - consolidated error message in xics_alloc()
>     - pass &error_fatal instead of NULL in spapr_events_init()
> ---
> 
>  hw/intc/xics.c        |   13 +++++++++----
>  hw/ppc/spapr_events.c |    3 ++-
>  hw/ppc/spapr_pci.c    |   16 ++++++++++------
>  hw/ppc/spapr_vio.c    |    7 ++++---
>  include/hw/ppc/xics.h |    5 +++--
>  trace-events          |    2 --
>  6 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e66ae328819a..213a3709254c 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -712,7 +712,7 @@ 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)
> +int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi, Error **errp)
>  {
>      ICSState *ics = &icp->ics[src];
>      int irq;
> @@ -720,14 +720,14 @@ 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);
> +            error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint);
>              return -1;
>          }
>          irq = irq_hint;
>      } else {
>          irq = ics_find_free_block(ics, 1, 1);
>          if (irq < 0) {
> -            trace_xics_alloc_failed_no_left(src);
> +            error_setg(errp, "can't allocate IRQ: no IRQ left");
>              return -1;
>          }
>          irq += ics->offset;
> @@ -743,7 +743,8 @@ int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi)
>   * 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)
> +int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                     Error **errp)
>  {
>      int i, first = -1;
>      ICSState *ics = &icp->ics[src];
> @@ -763,6 +764,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, "can't find a free %d-IRQ block", num);
> +        return -1;
> +    }
>  
>      if (first >= 0) {
>          for (i = first; i < first + num; ++i) {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index f5eac4b5441c..39f4682f957f 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -588,7 +588,8 @@ 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);
> +    spapr->check_exception_irq = xics_alloc(spapr->icp, 0, 0, false,
> +                                            &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..e8edad3ab7c3 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:
> @@ -354,9 +355,10 @@ 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);
> +                           ret_intr_type == RTAS_TYPE_MSI, &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");
> +        irq = xics_alloc_block(spapr->icp, 0, 1, true, false, &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..0f61a550cb2e 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,9 @@ 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");
> +    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 355a96623c70..f60b06ae829e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,8 +161,9 @@ 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);
> +int xics_alloc(XICSState *icp, int src, int irq_hint, bool lsi, Error **errp);
> +int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool align,
> +                     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 61a133f6eea9..075ec271007d 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1409,8 +1409,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 --]