Re: [Qemu-devel] [PATCH v2 2/6] intc/i8259: implement InterruptStatsProvider interface

[Hervé Poussineau]

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;
> > +}
> > +