From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47976) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp7K2-00059U-9J for qemu-devel@nongnu.org; Wed, 28 Sep 2016 01:22:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bp7Jw-0004Np-25 for qemu-devel@nongnu.org; Wed, 28 Sep 2016 01:22:21 -0400 Received: from smtp2-g21.free.fr ([212.27.42.2]:26271) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp7Jv-0004Mj-OU for qemu-devel@nongnu.org; Wed, 28 Sep 2016 01:22:15 -0400 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> From: =?UTF-8?Q?Herv=c3=a9_Poussineau?= Message-ID: <2a051808-2982-4431-b312-f460b4566703@reactos.org> Date: Wed, 28 Sep 2016 07:22:01 +0200 MIME-Version: 1.0 In-Reply-To: <20160928013712.GD14447@umbus.fritz.box> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable 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: David Gibson Cc: qemu-devel@nongnu.org, Paolo Bonzini , "Michael S. Tsirkin" , Luiz Capitulino 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(+) >>>> >>>> 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" >>>> >>>> /* debug PIC */ >>>> //#define DEBUG_PIC >>>> @@ -251,6 +252,35 @@ static void pic_reset(DeviceState *dev) >>>> pic_init_reset(s); >>>> } >>>> >>>> +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; >>> >>> So, the irq_counts return parameter is set to point at an internal >>> structure of the intc, in this and the other implementations. >>> >>> 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)? >> >> 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); >> } >> >> realize() >> { >> qemu_allocate_irqs(my_device_irq_handler, NB_IRQS) >> } >> >> 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]++; >> >> 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 >> >> For your intc, you'll need to have a second array irq_count, which is = updated on each >> get_statistics() call. >> >>> 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. >> >> In that case, all implementations will have to do g_malloc + memcpy, a= nd 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= ? Yes, it seems correct. I can add in interface header that: "Returned pointer and statistics must remain valid until the BQL is next = dropped" Does it require a v3? > >> >> Herv=E9 >> >>> >>>> + *nb_irqs =3D ARRAY_SIZE(irq_count); >>>> +#else >>>> + return false; >>>> +#endif >>>> + } else { >>>> + *irq_counts =3D NULL; >>>> + *nb_irqs =3D 0; >>>> + } >>>> + return true; >>>> +} >>>> + >> >