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 <hpous...@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 <da...@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
signature.asc
Description: PGP signature
