From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aSasQ-0004qx-HS for qemu-devel@nongnu.org; Sun, 07 Feb 2016 20:44:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aSasP-0000El-1i for qemu-devel@nongnu.org; Sun, 07 Feb 2016 20:44:30 -0500 Date: Mon, 8 Feb 2016 11:45:19 +1000 From: David Gibson Message-ID: <20160208014519.GD3702@voom> References: <20160205084340.4178.52171.stgit@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="k4f25fnPtRuIRUb3" Content-Disposition: inline In-Reply-To: <20160205084340.4178.52171.stgit@bahia.huguette.org> Subject: Re: [Qemu-devel] [PATCH] hw/intc: fix failure return for xics_alloc_block() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-ppc@nongnu.org, Alexander Graf , qemu-devel@nongnu.org --k4f25fnPtRuIRUb3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: > From: Brian W. Hart >=20 > xics_alloc_block() does not return a clear error code when it > fails to allocate a block of interrupts. Instead it returns the > base interrupt number minus 1. This change updates it to return a > clear -1 in case of failure (following the example of xics_alloc()). >=20 > The two callers of xics_alloc_block() are updated to check for > a negative return as an error. They had previously checked for > a 0 return as an error, which wrongly treated most failures as > successes. >=20 > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > Signed-off-by: Brian W. Hart > [only pass src and num to trace_xics_alloc_block_failed_no_left, > added trace_xics_alloc_block_failed_no_left definition to trace-events] > Signed-off-by: Greg Kurz Hrm, it would probably be better to give xics_alloc_block() an Error ** argument so it can report errors using the new API. TBH the whole xics_alloc_block() interface is kind of dubious, or at least the ics_find_free_block() part of it. Dynamically allocating irqs to devices is basically awful for migration, so it's better to have fixed allocations of all interrupts at the machine level. > --- > hw/intc/xics.c | 10 ++++++---- > hw/ppc/spapr_pci.c | 9 +++++---- > trace-events | 1 + > 3 files changed, 12 insertions(+), 8 deletions(-) >=20 > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index cd91ddc4d1d9..3bb77ff96e7b 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -763,11 +763,13 @@ int xics_alloc_block(XICSState *icp, int src, int n= um, bool lsi, bool align) > } else { > first =3D ics_find_free_block(ics, num, 1); > } > + if (first < 0) { > + trace_xics_alloc_block_failed_no_left(src, num); > + return -1; > + } > =20 > - if (first >=3D 0) { > - for (i =3D first; i < first + num; ++i) { > - ics_set_irq_type(ics, i, lsi); > - } > + for (i =3D first; i < first + num; ++i) { > + ics_set_irq_type(ics, i, lsi); > } > first +=3D ics->offset; > =20 > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index cca9257fecc5..ba33cee2a465 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -275,7 +275,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > unsigned int req_num =3D rtas_ld(args, 4); /* 0 =3D=3D remove all */ > unsigned int seq_num =3D rtas_ld(args, 5); > unsigned int ret_intr_type; > - unsigned int irq, max_irqs =3D 0, num =3D 0; > + unsigned int max_irqs =3D 0, num =3D 0; > + int irq; > sPAPRPHBState *phb =3D NULL; > PCIDevice *pdev =3D NULL; > spapr_pci_msi *msi; > @@ -354,7 +355,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > /* Allocate MSIs */ > irq =3D xics_alloc_block(spapr->icp, 0, req_num, false, > ret_intr_type =3D=3D RTAS_TYPE_MSI); > - if (!irq) { > + if (irq < 0) { > error_report("Cannot allocate MSIs for device %x", config_addr); > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > return; > @@ -1359,10 +1360,10 @@ static void spapr_phb_realize(DeviceState *dev, E= rror **errp) > =20 > /* Initialize the LSI table */ > for (i =3D 0; i < PCI_NUM_PINS; i++) { > - uint32_t irq; > + int32_t irq; > =20 > irq =3D xics_alloc_block(spapr->icp, 0, 1, true, false); > - if (!irq) { > + if (irq < 0) { > error_setg(errp, "spapr_allocate_lsi failed"); > return; > } > diff --git a/trace-events b/trace-events > index c9ac144ceee4..07b0250aaf11 100644 > --- a/trace-events > +++ b/trace-events > @@ -1393,6 +1393,7 @@ 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_alloc_block_failed_no_left(int src, int num) "source#%d, cannot fin= d %d consecutive irqs" > 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 --=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 --k4f25fnPtRuIRUb3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWt/MvAAoJEGw4ysog2bOSAFwQALudap1uayadzzDd+Y1iaup7 5dkwxU+yGqAnA19IvjqQ1xNCy3E03SKHaj6pQ740EHWpIqD71W8E9tGIgpWv5/w2 8MEleOZfKJzbh7IjmXo9IJNS1vgGylmkVFgKPSNWJmrLpQWQMR2C4v01qQuoS5F8 DCwgbGj4iUcAOYPytwvTUE73Hx+OsG7jgs9X8DMsFrhBaZh6D2aPfdi8v7I6oOZy SrCZxhlALqL9r/dYejaZO790G9i95lQJZlmhjMz0Zn2TfO7lEdl9J3fBvMreylho i1p10ZglNKo5sx2p5ndMZQgf5oxoBdvbZAwqUMUG6wqKhTdNqZ7TO6PhAjkdNU1F +PfvCjVueNk4mibNVs2HOnxyXGzXXp+q+bi+eR+oluOCBiobO0qc4cVBum8t5ar0 RtgrM+50F8Ikq2+/SWDhFoumpWPheU/pbSSO8KIPW1GdDdzyocgRkJJ/BtcLXuLq 8SOGFGNV70EFjuSjR59Hf+yJ/+HZOQ3AlU0MQQUKq7uh3VT3dzio/6U03AIzkEN4 rWj8aBRF8OLwuq5YF+cLln/Xf8sbh1rIhjWCxnt0w0D361OUfU55js1kN+sbqdLu cVzPhtrOp1GxLJvMrkB3nwkku1VXDHdtO/0sTlHqmHyjIeq8o+sW3CiwRQ01yuap G0mfKue4945rvjyxqsJQ =xw5W -----END PGP SIGNATURE----- --k4f25fnPtRuIRUb3--