From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZ65n-0000do-Bj for qemu-devel@nongnu.org; Thu, 25 Feb 2016 19:17:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZ65k-0006Ia-Od for qemu-devel@nongnu.org; Thu, 25 Feb 2016 19:17:11 -0500 Date: Fri, 26 Feb 2016 10:30:09 +1100 From: David Gibson Message-ID: <20160225233009.GC20657@voom.fritz.box> References: <20160225180206.30801.25098.stgit@bahia.huguette.org> <20160225180224.30801.33181.stgit@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kVXhAStRUZ/+rrGn" Content-Disposition: inline In-Reply-To: <20160225180224.30801.33181.stgit@bahia.huguette.org> Subject: Re: [Qemu-devel] [PATCH 3/3] 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 --kVXhAStRUZ/+rrGn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. >=20 > The corresponding error traces get promotted to error messages. >=20 > This fixes the issues mentioned above. >=20 > Based on previous work from Brian W. Hart. >=20 > Signed-off-by: Greg Kurz > --- > 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(-) >=20 > 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; > } > =20 > -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 =3D &icp->ics[src]; > int irq; > @@ -720,15 +721,15 @@ 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); > - 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 =3D irq_hint; > } else { > irq =3D 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 +=3D ics->offset; > } > @@ -736,14 +737,15 @@ int xics_alloc(XICSState *icp, int src, int irq_hin= t, bool lsi) > ics_set_irq_type(ics, irq - ics->offset, lsi); > trace_xics_alloc(src, irq); > =20 > - return irq; > + *irqp =3D irq; > } > =20 > /* > * 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) > +void xics_alloc_block(XICSState *icp, int src, int num, bool lsi, bool a= lign, > + unsigned int *irqp, Error **errp) > { > int i, first =3D -1; > ICSState *ics =3D &icp->ics[src]; > @@ -763,6 +765,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, "cannot find a %d-IRQ block", num); > + return; > + } > =20 > if (first >=3D 0) { > for (i =3D first; i < first + num; ++i) { > @@ -773,7 +779,7 @@ int xics_alloc_block(XICSState *icp, int src, int num= , bool lsi, bool align) > =20 > trace_xics_alloc_block(src, first, num, lsi, align); > =20 > - return first; > + *irqp =3D first; > } > =20 > 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 =3D xics_alloc(spapr->icp, 0, 0, false); > + xics_alloc(spapr->icp, 0, 0, false, &spapr->check_exception_irq, NUL= L); I don't think you want NULL for the errp, you probably want &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..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, 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: > @@ -353,10 +354,11 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sP= APRMachineState *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); > + xics_alloc_block(spapr->icp, 0, req_num, false, > + ret_intr_type =3D=3D 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, 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"); > + 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; > } > =20 > 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 *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,10 @@ static void spapr_vio_busdev_realize(DeviceState *qd= ev, 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"); > + 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; > } > =20 > 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 { > =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); > +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 a= lign, > + unsigned int *irqp, 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 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 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 --kVXhAStRUZ/+rrGn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWz46BAAoJEGw4ysog2bOSeS0P/1+kRV2e/3Ps5n6KuCLlejif 9K8d/5/2nokJK5lr8RCFCc3e8/VmBajGiL0sYQ3mhpaO2L6QgTNuk8emYxZoS8JP HmiMuKnEZzprSOv2GVaoNXbUOOhnZZwDnnHVLOUeTIaQg02bVu8PVOHT5Qc9r4so zedMG2Qjz0GUBX+AmbxraDqyOoPh8Quk+biDJlMfh8t0uhs17ZCL7kyOc77mnlkY n44hdJFbP2J4w7UXWZoXsZaqFby6QWVnK239c98WbIUGGnvMM/VSTCHE3i7snuMm dwo8iuG0x4gdyLUOmejQxFuK8GYMzN2fQ/AWLvszwNsd6Ez/Sn009lZDD/wmojBC loycpghxm/0+AYlakWya6ThjGUgCtXxS0q7Q7UrgRbqNvmEMlan4UeKuSpFtPQWT FKcoRQMRlj43M5LtYGrT9OP0HXK7Qr2Smf4sR6gvYTkoavn4EctNcz9M5eqilBZ6 n7pc3SYlcjUZEKX7aAC8yOPWGWJ8CCyS1ZluLxerhoXIfSkaHdSKNKTO9aGt3iOe tk2NRFfBDvpvz0SjsNMXTZlc0FmCGAdrJf3onotEsKI/HauAjUX54s17LW/wuRBY NX66cT45i5YomugvyzQyIBGXTBSvKL0D92BMofbdSvxocBY34KoRK4ObcanDLmWA 3/VE+eYZ/uNiakprjPaA =Ag/e -----END PGP SIGNATURE----- --kVXhAStRUZ/+rrGn--