Message ID | 20160226093816.6707.22148.stgit@bahia.huguette.org |
---|---|
State | New |
Headers | show |
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" >
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"
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> --- 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(-)