| Message ID | 1474921408-24710-3-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
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'm wondering if a safer interface might be to actually copy out a snapshot of the counts, which the caller is responsible for freeing. > + *nb_irqs = ARRAY_SIZE(irq_count); > +#else > + return false; > +#endif > + } else { > + *irq_counts = NULL; > + *nb_irqs = 0; > + } > + return true; > +} > + > +static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon) > +{ > + PICCommonState *s = PIC_COMMON(obj); > + monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d " > + "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n", > + s->master ? 0 : 1, s->irr, s->imr, s->isr, s->priority_add, > + s->irq_base, s->read_reg_select, s->elcr, > + s->special_fully_nested_mode); > +} > + > static void pic_ioport_write(void *opaque, hwaddr addr64, > uint64_t val64, unsigned size) > { > @@ -503,10 +533,13 @@ static void i8259_class_init(ObjectClass *klass, void *data) > { > PICClass *k = PIC_CLASS(klass); > DeviceClass *dc = DEVICE_CLASS(klass); > + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); > > k->parent_realize = dc->realize; > dc->realize = pic_realize; > dc->reset = pic_reset; > + ic->get_statistics = pic_get_statistics; > + ic->print_info = pic_print_info; > } > > static const TypeInfo i8259_info = { > @@ -515,6 +548,10 @@ static const TypeInfo i8259_info = { > .parent = TYPE_PIC_COMMON, > .class_init = i8259_class_init, > .class_size = sizeof(PICClass), > + .interfaces = (InterfaceInfo[]) { > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { } > + }, > }; > > static void pic_register_types(void)
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: 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; >> +} >> +
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; > > > +} > > > + >
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" 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; >>>> +} >>>> + >> >
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>
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; + *nb_irqs = ARRAY_SIZE(irq_count); +#else + return false; +#endif + } else { + *irq_counts = NULL; + *nb_irqs = 0; + } + return true; +} + +static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon) +{ + PICCommonState *s = PIC_COMMON(obj); + monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d " + "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n", + s->master ? 0 : 1, s->irr, s->imr, s->isr, s->priority_add, + s->irq_base, s->read_reg_select, s->elcr, + s->special_fully_nested_mode); +} + static void pic_ioport_write(void *opaque, hwaddr addr64, uint64_t val64, unsigned size) { @@ -503,10 +533,13 @@ static void i8259_class_init(ObjectClass *klass, void *data) { PICClass *k = PIC_CLASS(klass); DeviceClass *dc = DEVICE_CLASS(klass); + InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass); k->parent_realize = dc->realize; dc->realize = pic_realize; dc->reset = pic_reset; + ic->get_statistics = pic_get_statistics; + ic->print_info = pic_print_info; } static const TypeInfo i8259_info = { @@ -515,6 +548,10 @@ static const TypeInfo i8259_info = { .parent = TYPE_PIC_COMMON, .class_init = i8259_class_init, .class_size = sizeof(PICClass), + .interfaces = (InterfaceInfo[]) { + { TYPE_INTERRUPT_STATS_PROVIDER }, + { } + }, }; static void pic_register_types(void)
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> --- hw/intc/i8259.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)