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

[David Gibson]

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

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