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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

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 <hpoussin@reactos.org>
> > > ---
> > >  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?

> 
> Hervé
> 
> > 
> > > +        *nb_irqs = ARRAY_SIZE(irq_count);
> > > +#else
> > > +        return false;
> > > +#endif
> > > +    } else {
> > > +        *irq_counts = NULL;
> > > +        *nb_irqs = 0;
> > > +    }
> > > +    return true;
> > > +}
> > > +
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]