From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37780) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aaBVa-0004Mv-Vd for qemu-devel@nongnu.org; Sun, 28 Feb 2016 19:16:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aaBVX-0000Fl-Je for qemu-devel@nongnu.org; Sun, 28 Feb 2016 19:16:18 -0500 Date: Mon, 29 Feb 2016 10:48:59 +1100 From: David Gibson Message-ID: <20160228234859.GA19643@voom.fritz.box> References: <20160226093816.6707.22148.stgit@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qMm9M+Fa2AknHoGS" Content-Disposition: inline In-Reply-To: <20160226093816.6707.22148.stgit@bahia.huguette.org> Subject: Re: [Qemu-devel] [PATCH v2] xics: report errors with the QEMU Error API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --qMm9M+Fa2AknHoGS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. >=20 > 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 (devi= ce > config address, MSI or MSIX). >=20 > This fixes the issues mentioned above. >=20 > Based on previous work from Brian W. Hart. >=20 > Signed-off-by: Greg Kurz 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() > --- >=20 > 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(-) >=20 > 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; > } > =20 > -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 =3D &icp->ics[src]; > int irq; > @@ -720,14 +720,14 @@ int xics_alloc(XICSState *icp, int src, int irq_hin= t, bool lsi) > if (irq_hint) { > assert(src =3D=3D 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", ir= q_hint); > return -1; > } > irq =3D irq_hint; > } else { > irq =3D 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 +=3D 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 firs= t IRQ in the block. > * If align=3D=3Dtrue, aligns the first IRQ number to num. > */ > -int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool al= ign) > +int xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool al= ign, > + Error **errp) > { > int i, first =3D -1; > ICSState *ics =3D &icp->ics[src]; > @@ -763,6 +764,10 @@ int xics_alloc_block(XICSState *icp, int src, int nu= m, bool lsi, bool align) > } else { > first =3D ics_find_free_block(ics, num, 1); > } > + if (first < 0) { > + error_setg(errp, "can't find a free %d-IRQ block", num); > + return -1; > + } > =20 > if (first >=3D 0) { > for (i =3D 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 =3D xics_alloc(spapr->icp, 0, 0, false); > + spapr->check_exception_irq =3D xics_alloc(spapr->icp, 0, 0, false, > + &error_fatal); > spapr->epow_notifier.notify =3D 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, sPAP= RMachineState *spapr, > PCIDevice *pdev =3D NULL; > spapr_pci_msi *msi; > int *config_addr_key; > + Error *err =3D NULL; > =20 > switch (func) { > case RTAS_CHANGE_MSI_FN: > @@ -354,9 +355,10 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPA= PRMachineState *spapr, > =20 > /* Allocate MSIs */ > irq =3D xics_alloc_block(spapr->icp, 0, req_num, false, > - ret_intr_type =3D=3D RTAS_TYPE_MSI); > - if (!irq) { > - error_report("Cannot allocate MSIs for device %x", config_addr); > + ret_intr_type =3D=3D 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, E= rror **errp) > /* Initialize the LSI table */ > for (i =3D 0; i < PCI_NUM_PINS; i++) { > uint32_t irq; > + Error *local_err =3D NULL; > =20 > - irq =3D xics_alloc_block(spapr->icp, 0, 1, true, false); > - if (!irq) { > - error_setg(errp, "spapr_allocate_lsi failed"); > + irq =3D xics_alloc_block(spapr->icp, 0, 1, true, false, &local_e= rr); > + if (local_err) { > + error_propagate(errp, local_err); > + error_prepend(errp, "can't allocate LSIs: "); > return; > } > =20 > 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 *qde= v, Error **errp) > VIOsPAPRDevice *dev =3D (VIOsPAPRDevice *)qdev; > VIOsPAPRDeviceClass *pc =3D VIO_SPAPR_DEVICE_GET_CLASS(dev); > char *id; > + Error *local_err =3D NULL; > =20 > if (dev->reg !=3D -1) { > /* > @@ -463,9 +464,9 @@ static void spapr_vio_busdev_realize(DeviceState *qde= v, Error **errp) > dev->qdev.id =3D id; > } > =20 > - dev->irq =3D xics_alloc(spapr->icp, 0, dev->irq, false); > - if (!dev->irq) { > - error_setg(errp, "can't allocate IRQ"); > + dev->irq =3D xics_alloc(spapr->icp, 0, dev->irq, false, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > return; > } > =20 > 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 { > =20 > 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 al= ign); > +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 al= ign, > + Error **errp); > void xics_free(XICSState *icp, int irq, int num); > =20 > 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 i= n 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) "sour= ce#%d, first irq %d, %d irqs, lsi=3D%d, alignnum %d" > xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d ir= qs" > xics_ics_free_warn(int src, int irq) "Source#%d, irq %d is already free" >=20 --=20 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 --qMm9M+Fa2AknHoGS Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW04drAAoJEGw4ysog2bOS1o0QAMnec+27ap2FgVqqUXKIRK61 quxXhd75b8+sHzJhAGOvvDP2Ry++u+UGCWtnS2NCgDANZR2vEPESEk0eVHo1EFHH relCGjtjQ55NBnqtsJacPUkjZDFopvZbUDyoxOagJY4mXnI49QoOhd4aMQGR/g5p QcJnd7++Hf8skGTNK4BdJ2DJhPwzkzsWKLWKFO4EiMEpnEvb1ddrJRgj/59SRn/w NNK3z+vJDmD1RSw9LQ6mntoPXURv7oXq/+B7lzAGN7WihkI8y9FGTFsQQH6neoel R2I/2ATGhANEBkszIn3imRBxabzS8v62oRHDYEiFlwl95ITWPvRyx2SwRiSiEBdN 6a0w6JnE4hkyHKP96iWImpMRNrwqpM2KwNsR0y8Cw1SGqYjMGuDfiMGwj2uvf7ce Fmet6ciQjb9t1eQUo0jG8K/OemG8SkwMumMV8NmggRTsmUbZdutsz8FBHcE8CLlB 5ylfA2pAmG1fhy4OU5MxBcR9ZmOUEdsspu71n5vUZCX0kEjAqSmVdjlzC96/QooT MMuuKsK8ujrXhCL8n1MtZZsq9TFSRUpir6EGOf07KG7MIU7QLud04gSa4eDxAkmB Suw5sDWZuYDuHnpJUyHucRnLnBkPSk/DbMabThL3ft184O4NTxdlsWbDnlSQP0kv qvfBN4sF1kRsU5z+jUA9 =hWql -----END PGP SIGNATURE----- --qMm9M+Fa2AknHoGS--