diff mbox

[3/3] xics: report errors with the QEMU Error API

Message ID 20160225180224.30801.33181.stgit@bahia.huguette.org (mailing list archive)
State New, archived
Headers show

Commit Message

Greg Kurz Feb. 25, 2016, 6:02 p.m. UTC 
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(-)

Comments

David Gibson Feb. 25, 2016, 11:30 p.m. UTC | #1 
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"
>
diff mbox

Patch

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)
 {
     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;
         }
         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);
     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"