From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp9de-0004hu-61 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 03:50:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bp9dZ-0006Bn-KV for qemu-devel@nongnu.org; Wed, 28 Sep 2016 03:50:46 -0400 Received: from ozlabs.org ([103.22.144.67]:38763) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp9dY-0006Ao-9E for qemu-devel@nongnu.org; Wed, 28 Sep 2016 03:50:41 -0400 Date: Wed, 28 Sep 2016 17:29:08 +1000 From: David Gibson Message-ID: <20160928072908.GH18880@umbus> References: <1474921408-24710-1-git-send-email-hpoussin@reactos.org> <1474921408-24710-3-git-send-email-hpoussin@reactos.org> <20160927041138.GF30322@umbus.fritz.box> <20160928013712.GD14447@umbus.fritz.box> <2a051808-2982-4431-b312-f460b4566703@reactos.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="y9PDtDHaFrXNoMPU" Content-Disposition: inline In-Reply-To: <2a051808-2982-4431-b312-f460b4566703@reactos.org> 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 --y9PDtDHaFrXNoMPU Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 28, 2016 at 07:22:01AM +0200, Herv=E9 Poussineau wrote: > Le 28/09/2016 =E0 03:37, David Gibson a =E9crit : > > 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 i= nt *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 tri= ed > > > > to implement this for an intc that doesn't keep irq stats as a simp= le > > > > 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 i= n hw/intc. > > > It worked well for all of them. In fact, most of the times, the devic= e is doing something like: > >=20 > > Ok, that's a pretty strong argument. > >=20 > > > 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= updated 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. > >=20 > > True. > >=20 > > 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 > Yes, it seems correct. > I can add in interface header that: > "Returned pointer and statistics must remain valid until the BQL is > next dropped" Ok, makes sense. > Does it require a v3? Not for my sake. Reviewed-by: David Gibson --=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 --y9PDtDHaFrXNoMPU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX63FBAAoJEGw4ysog2bOSwzYP/RN8P3mfogidTbMKZ7xa014a 8uak/QYfsSpL80v81E+Q66tmA7t7uQnhhOdTfNNoWmfVjkBie3Jn6ux2iSnGDT4G ts5KZiZTQgeGFt9o3u/lb+JPxlIlHR4MJkImCKJ3ROgZmdnVwzofXioPu9vGS5et Ezd8MTbloHX3PV5TqA8rubIVfdjRVTvs1Wxmp0Pxg5DpXNU/pHrVsSAelPSijA3m 1oBRw5U6zijKHO9AuQPa7NQtvW4ZVgvdcwuZnD0rvvB4vkfj4SYZLVtU1bkRKucC KlqGFD//5fWAUUuBIb2eDp2kTJqpCIldjchu2xjuhKbBRNRQr/2SdHEd10cAeCro ErR0++tP8tg1LFzGTcVNmrE3VLRPjiG2DuS5R1wr28QFxmRRdy01ucwXJx6sIiL7 rbaa6RtufYw3CcEqUL+Mbds1qJUW9gZrhgsSX10cGt5+o1VngSb0rPeLB3VsoSM0 w4gThM3BC8nkr0QS0lums82ua4FO3urZSQRGqBmK5MZUQnEVoYwoUl90yKFUI1S0 K5NT8i9I3GbSGCrWxpJPmBl0BzmiGV3G5EzWE9Ycv98OhPsY3/XL3cZ1sW1bHcTn AavjCDsuDCAd7Ic6rflKu+YpTbns6AtvjMwn1vztqCNUZIIeelaIrCtjYzzgvFfZ qKulrWUoei2aC+rfTcrx =aNNa -----END PGP SIGNATURE----- --y9PDtDHaFrXNoMPU--