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: 4073 bytes --]

On Wed, Sep 28, 2016 at 07:22:01AM +0200, Hervé Poussineau wrote:
> 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 <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?
> 
> Yes, it seems correct.
> I can add in interface header that:
> "Returned pointer and statistics must remain valid until the BQL is
> next dropped"

Ok, makes sense.

> Does it require a v3?

Not for my sake.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

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