From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38642) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp4fP-0005Ep-R6 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bp4fK-00011H-OQ for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:14 -0400 Received: from ozlabs.org ([103.22.144.67]:42503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp4fK-0000zF-2J for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:10 -0400 Date: Wed, 28 Sep 2016 11:37:12 +1000 From: David Gibson Message-ID: <20160928013712.GD14447@umbus.fritz.box> References: <1474921408-24710-1-git-send-email-hpoussin@reactos.org> <1474921408-24710-3-git-send-email-hpoussin@reactos.org> <20160927041138.GF30322@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qGV0fN9tzfkG3CxV" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2 2/6] intc/i8259: implement InterruptStatsProvider interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Herv=E9?= Poussineau Cc: qemu-devel@nongnu.org, Paolo Bonzini , "Michael S. Tsirkin" , Luiz Capitulino --qGV0fN9tzfkG3CxV Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 27, 2016 at 08:49:47PM +0200, Herv=E9 Poussineau wrote: > Le 27/09/2016 =E0 06:11, David Gibson a =E9crit : > > On Mon, Sep 26, 2016 at 10:23:24PM +0200, Herv=E9 Poussineau wrote: > > > Signed-off-by: Herv=E9 Poussineau > > > --- > > > hw/intc/i8259.c | 37 +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 37 insertions(+) > > >=20 > > > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c > > > index c2607a5..75c8d22 100644 > > > --- a/hw/intc/i8259.c > > > +++ b/hw/intc/i8259.c > > > @@ -29,6 +29,7 @@ > > > #include "qemu/timer.h" > > > #include "qemu/log.h" > > > #include "hw/isa/i8259_internal.h" > > > +#include "hw/intc/intc.h" > > >=20 > > > /* debug PIC */ > > > //#define DEBUG_PIC > > > @@ -251,6 +252,35 @@ static void pic_reset(DeviceState *dev) > > > pic_init_reset(s); > > > } > > >=20 > > > +static bool pic_get_statistics(InterruptStatsProvider *obj, > > > + uint64_t **irq_counts, unsigned int *= nb_irqs) > > > +{ > > > + PICCommonState *s =3D PIC_COMMON(obj); > > > + > > > + if (s->master) { > > > +#ifdef DEBUG_IRQ_COUNT > > > + *irq_counts =3D irq_count; > >=20 > > So, the irq_counts return parameter is set to point at an internal > > structure of the intc, in this and the other implementations. > >=20 > > Is that safe, without some contract about how long the array pointer > > is valid and/or correct? Could it be a problem if in future we tried > > to implement this for an intc that doesn't keep irq stats as a simple > > array (e.g. kept the count in a structure also containing other > > information for each irq)? >=20 > I implemented the interface with more than 15 interrupt controllers in hw= /intc. > It worked well for all of them. In fact, most of the times, the device is= doing something like: Ok, that's a pretty strong argument. > my_device_irq_handler(int n) > { > MyDeviceState *s =3D ...; > qemu_irq_raise(s->master_irq); > } >=20 > realize() > { > qemu_allocate_irqs(my_device_irq_handler, NB_IRQS) > } >=20 > It's quite easy to add in MyDeviceState: > uint64_t irq_count[NB_IRQS] in MyDeviceState; > and adding in my_device_irq_handler > s->irq_count[n]++; >=20 > We can maybe add a note on the interface that: > - the pointer must remain valid for the whole life of the device, > - the contents may stale, but must not be invalid >=20 > For your intc, you'll need to have a second array irq_count, which is upd= ated on each > get_statistics() call. >=20 > > I'm wondering if a safer interface might be to actually copy out a > > snapshot of the counts, which the caller is responsible for freeing. >=20 > In that case, all implementations will have to do g_malloc + memcpy, and = caller will have to call g_free. > That's possible, but IMO less easy to implement on device side. True. I still feel a bit uneasy without having some sort of description of the length of validity of the pointer. With the current implementation and use cases, it seems like "until the BQL is next dropped" would be about right. Does that seem like it's correct to you? >=20 > Herv=E9 >=20 > >=20 > > > + *nb_irqs =3D ARRAY_SIZE(irq_count); > > > +#else > > > + return false; > > > +#endif > > > + } else { > > > + *irq_counts =3D NULL; > > > + *nb_irqs =3D 0; > > > + } > > > + return true; > > > +} > > > + >=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 --qGV0fN9tzfkG3CxV Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX6x7IAAoJEGw4ysog2bOSLcEQALZfl8YdvwhE15irHAVP3aG8 lysownVNuUddpSXDjoSy+M2nXsYyDcjQ5FBXdUP6cayvU84s4BL7xcsUyj+5Gkeq EoFZHOxteyxOEzvh/0jKb+d2AfKDgg3Y33KIU94nSiY1PsVMF8q6XmzSj3esOHH2 aqmdavBQ6pT8nSKYl68fbOJ8RGSdBeStE4NL+GTi8j9cxPc/YJsZWUur7Pq8vhJn pHWKFX2GtUnDc399WMGahHscbEbENsnT6lsdBUqMOqmyAOa2RkHjO3RgjzFMnPM8 31V+uq2qxt2Oe6By3S+xB3nE1Sz2ZBIbWvm7gOQLqRX9oR1hZ2SiInnnFEQxAIK9 AtfNfj2NG5gpHIWIZlatXzRMeexdNdP/kMM/2dEUZKAgTdHZSsBSklpX0wURhAke 3AAZruYWvV0ovSONaQX+IuSQ0xfsTk2WpBkxSmGPaSR86UPONwvCQoYWL2OtKST2 goSeCAFU9FvuJ35iheECjf80SxclTxES8tf0ifOZj5iHXZMAEL+ivncu0mZfW1Iy ZCmGJ78/8gwOBt9UIBJeGwErPSALyCV+rJ3rDWikINQfjgt3kD/8BOg1ktHQ/W6B Q8pthDx/14C8o6e/8b/7pIojyB6Nd8WvY2kfQQCQjKsoyfy3hfh+XM3J5JZIhBed ijxUmpWfkDMAKsk3kmw7 =YCME -----END PGP SIGNATURE----- --qGV0fN9tzfkG3CxV--