|
From: | Hervé Poussineau |
Subject: | Re: [Qemu-devel] [PATCH v2 2/6] intc/i8259: implement InterruptStatsProvider interface |
Date: | Tue, 27 Sep 2016 20:49:47 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0 |
Le 27/09/2016 à 06:11, David Gibson a écrit :
On Mon, Sep 26, 2016 at 10:23:24PM +0200, Hervé Poussineau wrote:Signed-off-by: Hervé Poussineau <address@hidden> --- 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 = PIC_COMMON(obj); + + if (s->master) { +#ifdef DEBUG_IRQ_COUNT + *irq_counts = 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: my_device_irq_handler(int n) { MyDeviceState *s = ...; 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, and caller will have to call g_free. That's possible, but IMO less easy to implement on device side. Hervé
+ *nb_irqs = ARRAY_SIZE(irq_count); +#else + return false; +#endif + } else { + *irq_counts = NULL; + *nb_irqs = 0; + } + return true; +} +
[Prev in Thread] | Current Thread | [Next in Thread] |