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

[Greg Kurz]

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 <address@hidden>
---
 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)
 {
     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"