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

[Hervé Poussineau]

Le 28/09/2016 à 03:37, David Gibson a écrit :
> On Tue, Sep 27, 2016 at 08:49:47PM +0200, Hervé Poussineau wrote:
> > 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:
>
> Ok, that's a pretty strong argument.
>
> > 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.
>
> 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é
> >
> > > > +        *nb_irqs = ARRAY_SIZE(irq_count);
> > > > +#else
> > > > +        return false;
> > > > +#endif
> > > > +    } else {
> > > > +        *irq_counts = NULL;
> > > > +        *nb_irqs = 0;
> > > > +    }
> > > > +    return true;
> > > > +}
> > > > +