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 <gk...@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
signature.asc
Description: PGP signature