From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHP6P-00052H-B6 for qemu-devel@nongnu.org; Mon, 27 Jun 2016 01:28:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHP6L-0000FZ-6g for qemu-devel@nongnu.org; Mon, 27 Jun 2016 01:28:56 -0400 Date: Mon, 27 Jun 2016 15:30:12 +1000 From: David Gibson Message-ID: <20160627053012.GM4242@voom.fritz.box> References: <1466704050-15108-1-git-send-email-nikunj@linux.vnet.ibm.com> <1466704050-15108-7-git-send-email-nikunj@linux.vnet.ibm.com> <20160627042008.GF4242@voom.fritz.box> <87por344c5.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3FyYKcuUbgqNYeqV" Content-Disposition: inline In-Reply-To: <87por344c5.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Subject: Re: [Qemu-devel] [PATCH v1 06/11] ppc/xics: Make the ICSState a list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, clg@kaod.org, Benjamin Herrenschmidt --3FyYKcuUbgqNYeqV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 27, 2016 at 10:26:42AM +0530, Nikunj A Dadhania wrote: > David Gibson writes: >=20 > > [ Unknown signature status ] > > On Thu, Jun 23, 2016 at 11:17:25PM +0530, Nikunj A Dadhania wrote: > >> From: Benjamin Herrenschmidt > >>=20 > >> Instead of an array of fixed sized blocks, use a list, as we will need > >> to have sources with variable number of interrupts. SPAPR only uses > >> a single entry. Native will create more. If performance becomes an > >> issue we can add some hashed lookup but for now this will do fine. > >>=20 > >> Signed-off-by: Benjamin Herrenschmidt > >> Signed-off-by: Nikunj A Dadhania > >> --- > >> hw/intc/xics.c | 78 +++++++++++++++++++++++++++---------------= ------ > >> hw/intc/xics_kvm.c | 29 +++++++++++++----- > >> hw/intc/xics_spapr.c | 82 +++++++++++++++++++++++++++++-------------= --------- > >> hw/ppc/spapr_events.c | 2 +- > >> hw/ppc/spapr_pci.c | 5 ++-- > >> hw/ppc/spapr_vio.c | 2 +- > >> include/hw/ppc/xics.h | 13 ++++---- > >> 7 files changed, 124 insertions(+), 87 deletions(-) > >>=20 > >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c > >> index 38e51fc..ef2a1e4 100644 > >> --- a/hw/intc/xics.c > >> +++ b/hw/intc/xics.c > >> @@ -96,13 +96,16 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *c= pu) > >> static void xics_common_reset(DeviceState *d) > >> { > >> XICSState *xics =3D XICS_COMMON(d); > >> + ICSState *ics; > >> int i; > >> =20 > >> for (i =3D 0; i < xics->nr_servers; i++) { > >> device_reset(DEVICE(&xics->ss[i])); > >> } > >> =20 > >> - device_reset(DEVICE(xics->ics)); > >> + QLIST_FOREACH(ics, &xics->ics, list) { > >> + device_reset(DEVICE(ics)); > >> + } > >> } > >> =20 > >> static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char= *name, > >> @@ -134,7 +137,6 @@ static void xics_prop_set_nr_irqs(Object *obj, Vis= itor *v, const char *name, > >> } > >> =20 > >> assert(info->set_nr_irqs); > >> - assert(xics->ics); > >> info->set_nr_irqs(xics, value, errp); > >> } > >> =20 > >> @@ -212,33 +214,35 @@ static void ics_reject(ICSState *ics, int nr); > >> static void ics_resend(ICSState *ics); > >> static void ics_eoi(ICSState *ics, int nr); > >> =20 > >> -static void icp_check_ipi(XICSState *xics, int server) > >> +static void icp_check_ipi(ICPState *ss, int server) > > > > Since you're now passing ICPState, you don't need the server > > parameter, since its only purpose is to locate the right ICPState in > > the XICSState. >=20 > Right, i had retained this as there was a trace function using it. Maybe > we can then move the trace to the caller. Or just change the trace template. > >> @@ -361,13 +375,16 @@ int xics_spapr_alloc(XICSState *xics, int src, i= nt irq_hint, bool lsi, > >> * Allocate block of consecutive IRQs, and return the number of the f= irst IRQ in > >> * the block. If align=3D=3Dtrue, aligns the first IRQ number to num. > >> */ > >> -int xics_spapr_alloc_block(XICSState *xics, int src, int num, bool ls= i, > >> - bool align, Error **errp) > >> +int xics_spapr_alloc_block(XICSState *xics, int num, bool lsi, bool a= lign, > >> + Error **errp) > >> { > >> + ICSState *ics =3D QLIST_FIRST(&xics->ics); > >> int i, first =3D -1; > >> - ICSState *ics =3D &xics->ics[src]; > > > > Ouch.. AFAICT this would have SEGVed if it was ever called with src != =3D 0. > > > >> =20 > >> - assert(src =3D=3D 0); > >> + if (!ics) { > >> + return -1; > >> + } > >> + > >> /* > >> * MSIMesage::data is used for storing VIRQ so > >> * it has to be aligned to num to support multiple > >> @@ -394,7 +411,7 @@ int xics_spapr_alloc_block(XICSState *xics, int sr= c, int num, bool lsi, > >> } > >> first +=3D ics->offset; > >> =20 > >> - trace_xics_alloc_block(src, first, num, lsi, align); > >> + trace_xics_alloc_block(0, first, num, lsi, align); > > > > You should remove the src from the trave definition since it has to be > > 0. >=20 > Sure. >=20 > >> =20 > >> return first; > >> } > >> @@ -405,7 +422,7 @@ static void ics_free(ICSState *ics, int srcno, int= num) > >> =20 > >> for (i =3D srcno; i < srcno + num; ++i) { > >> if (ICS_IRQ_FREE(ics, i)) { > >> - trace_xics_ics_free_warn(ics - ics->xics->ics, i + ics->o= ffset); > >> + trace_xics_ics_free_warn(0, i + ics->offset); > > > > Likewise here you should remove the always 0 parameter from the trace > > definition. >=20 > Sure. >=20 > Regards > Nikunj >=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 --3FyYKcuUbgqNYeqV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXcLnkAAoJEGw4ysog2bOSg2gQANquTiNwnUHIyD5qD3GJmF5T 2MDqDn0eZtm6MmIjJhTzfHli7Czv1R9vLUa13CilHt9W9cskLliZoeCuBbnU49I6 TBYu7mn+rgyWCySnH7f6eLC6TUgyWHM6Z1bevQaOySTlriOp5DXbmsblUgpTwTd/ Xq5qzgu6tfr/lSUx7so04DsQ68BO4DsgMNZewDRqocZaNsSJ9V2PC8UipZDt16nE 5xnB1trHIwg2F3F2XloDJ5A06+nQ2EgDH7IfMc4MF8xV3djRfori10LXdrTIeZ3Q 4FC18dwfT+wof6wvcxS6XbuoLod1HHHTEpOhpAa4sPZVCD976S/jUGgM6U3H5ci1 AnLyzQiZGo+i5RZns6uJ/UQcMt527vxLRu7SZm535bL5y68YaGXaytpp7JiRx0P9 kDFDrdPC98ofME538aDYTdSMK3K3xaz/jAovbwEFgFpkTwBAD01IDyAJu9ptrsjb IvqgswfW5Rnyq6OHwPOeuz2pbqHj1GYarBN0N1BmPPRs80XJlWimPO4s82ZaWJoO 9WL8bx75KoSC5SQopsX9KPHQUR82yneUedZ9QiR19DdhWT3kvWNhut1UWnMdrXyt 1Msp+IJGEOWD5SWUWJC3a7gBwZ/uJCo+ZPQE5xhZ6Av6IA/Rk5/U0+ZG7EBmag8i TG1c3WWDr7JlAwyedz0d =2IuF -----END PGP SIGNATURE----- --3FyYKcuUbgqNYeqV--