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