[Qemu-devel] [PATCH v2 0/6] intc: change 'info irq' and 'info pic' to be target-agnostic

[Qemu-devel] [PATCH v2 0/6] intc: change 'info irq' and 'info pic' to be target-agnostic

[Hervé Poussineau]

Hi,

This patchset aims at genericizing the 'info irq' and 'info pic' HMP commands, so
that it is available on all machines and can display details about more than one
interrupt controller per machine.

Patch 1 adds a new interface InterruptStatsProvider, which is used to:
- gather statistics for the 'info irq' command
- print some text when 'info pic' is called

Patches 2 to 4 implement InterruptStatsProvider interface on interrupt controllers
which have ad-hock code to handle 'info irq'/'info pic' commands.

Patch 5 removes ad-hock code, and replaces it by a generic version. You can get
details about multiple interrupt controllers per machine starting here.

Patch 6 makes 'info irq'/'info pic' commands available on all architectures.
For example, Alpha clipper machine is now able to display details about the
i8259 interrupt controller.

Changes since v1:
- renamed interface from IntCtrl to InterruptStatsProvider

Hervé

Hervé Poussineau (6):
  intc: add an interface to gather statistics/informations on interrupt
    controllers
  intc/i8259: implement InterruptStatsProvider interface
  intc/slavio_intctl: implement InterruptStatsProvider interface
  intc/lm32_pic: implement InterruptStatsProvider interface
  intc: make HMP 'info irq' and 'info pic' commands use
    InterruptStatsProvider interface
  intc: make HMP 'info irq' and 'info pic' commands available on all
    targets

 hmp-commands-info.hx       | 17 +----------
 hmp.c                      | 65 +++++++++++++++++++++++++++++++++++++++++
 hmp.h                      |  2 ++
 hw/intc/Makefile.objs      |  1 +
 hw/intc/i8259.c            | 73 +++++++++++++++++++++++-----------------------
 hw/intc/intc.c             | 41 ++++++++++++++++++++++++++
 hw/intc/lm32_pic.c         | 63 ++++++++++++++++++---------------------
 hw/intc/slavio_intctl.c    | 67 ++++++++++++++++++++++--------------------
 hw/sparc/sun4m.c           | 15 +---------
 include/hw/i386/pc.h       |  2 --
 include/hw/intc/intc.h     | 30 +++++++++++++++++++
 include/hw/lm32/lm32_pic.h |  3 --
 include/hw/sparc/sun4m.h   |  8 -----
 monitor.c                  |  6 ----
 14 files changed, 241 insertions(+), 152 deletions(-)
 create mode 100644 hw/intc/intc.c
 create mode 100644 include/hw/intc/intc.h

-- 
2.1.4

[Qemu-devel] [PATCH v2 1/6] intc: add an interface to gather statistics/informations on interrupt controllers

[Hervé Poussineau]

This interface will be used by HMP commands 'info irq' and 'info pic'.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/intc/Makefile.objs  |  1 +
 hw/intc/intc.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 include/hw/intc/intc.h | 30 ++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100644 hw/intc/intc.c
 create mode 100644 include/hw/intc/intc.h

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 05ec21b..f24c837 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -17,6 +17,7 @@ common-obj-$(CONFIG_ARM_GIC) += arm_gicv3.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_dist.o
 common-obj-$(CONFIG_ARM_GIC) += arm_gicv3_redist.o
 common-obj-$(CONFIG_OPENPIC) += openpic.o
+common-obj-y += intc.o
 
 obj-$(CONFIG_APIC) += apic.o apic_common.o
 obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
diff --git a/hw/intc/intc.c b/hw/intc/intc.c
new file mode 100644
index 0000000..2e1e29e
--- /dev/null
+++ b/hw/intc/intc.c
@@ -0,0 +1,41 @@
+/*
+ * QEMU Generic Interrupt Controller
+ *
+ * Copyright (c) 2016 Hervé Poussineau
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/intc/intc.h"
+#include "qemu/module.h"
+
+static const TypeInfo intctrl_info = {
+    .name = TYPE_INTERRUPT_STATS_PROVIDER,
+    .parent = TYPE_INTERFACE,
+    .class_size = sizeof(InterruptStatsProviderClass),
+};
+
+static void intc_register_types(void)
+{
+    type_register_static(&intctrl_info);
+}
+
+type_init(intc_register_types)
+
diff --git a/include/hw/intc/intc.h b/include/hw/intc/intc.h
new file mode 100644
index 0000000..2a97ea1
--- /dev/null
+++ b/include/hw/intc/intc.h
@@ -0,0 +1,30 @@
+#ifndef INTC_H
+#define INTC_H
+
+#include "qom/object.h"
+
+#define TYPE_INTERRUPT_STATS_PROVIDER "intctrl"
+
+#define INTERRUPT_STATS_PROVIDER_CLASS(klass) \
+    OBJECT_CLASS_CHECK(InterruptStatsProviderClass, (klass), \
+                       TYPE_INTERRUPT_STATS_PROVIDER)
+#define INTERRUPT_STATS_PROVIDER_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(InterruptStatsProviderClass, (obj), \
+                     TYPE_INTERRUPT_STATS_PROVIDER)
+#define INTERRUPT_STATS_PROVIDER(obj) \
+    INTERFACE_CHECK(InterruptStatsProvider, (obj), \
+                    TYPE_INTERRUPT_STATS_PROVIDER)
+
+typedef struct InterruptStatsProvider {
+    Object parent;
+} InterruptStatsProvider;
+
+typedef struct InterruptStatsProviderClass {
+    InterfaceClass parent;
+
+    bool (*get_statistics)(InterruptStatsProvider *obj, uint64_t **irq_counts,
+                           unsigned int *nb_irqs);
+    void (*print_info)(InterruptStatsProvider *obj, Monitor *mon);
+} InterruptStatsProviderClass;
+
+#endif
-- 
2.1.4

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

[Hervé Poussineau]

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;
+        *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)
-- 
2.1.4

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

[David Gibson]

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

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)

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

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

[Hervé Poussineau]

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;
>> +}
>> +

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

[David Gibson]

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

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

[Hervé Poussineau]

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;
>>>> +}
>>>> +
>>
>

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

[David Gibson]

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

[Qemu-devel] [PATCH v2 3/6] intc/slavio_intctl: implement InterruptStatsProvider interface

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/intc/slavio_intctl.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
index e82e893..a9acb64 100644
--- a/hw/intc/slavio_intctl.c
+++ b/hw/intc/slavio_intctl.c
@@ -26,6 +26,7 @@
 #include "hw/sparc/sun4m.h"
 #include "monitor/monitor.h"
 #include "hw/sysbus.h"
+#include "hw/intc/intc.h"
 #include "trace.h"
 
 //#define DEBUG_IRQ_COUNT
@@ -418,6 +419,31 @@ static void slavio_intctl_reset(DeviceState *d)
     slavio_check_interrupts(s, 0);
 }
 
+#ifdef DEBUG_IRQ_COUNT
+static bool slavio_intctl_get_statistics(InterruptStatsProvider *obj,
+                                         uint64_t **irq_counts,
+                                         unsigned int *nb_irqs)
+{
+    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
+    *irq_counts = s->irq_count;
+    *nb_irqs = ARRAY_SIZE(s->irq_count);
+    return true;
+}
+#endif
+
+static void slavio_intctl_print_info(InterruptStatsProvider *obj, Monitor *mon)
+{
+    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
+    int i;
+
+    for (i = 0; i < MAX_CPUS; i++) {
+        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
+                       s->slaves[i].intreg_pending);
+    }
+    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
+                   s->intregm_pending, s->intregm_disabled);
+}
+
 static void slavio_intctl_init(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -449,9 +475,14 @@ static void slavio_intctl_init(Object *obj)
 static void slavio_intctl_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
 
     dc->reset = slavio_intctl_reset;
     dc->vmsd = &vmstate_intctl;
+#ifdef DEBUG_IRQ_COUNT
+    ic->get_statistics = slavio_intctl_get_statistics;
+#endif
+    ic->print_info = slavio_intctl_print_info;
 }
 
 static const TypeInfo slavio_intctl_info = {
@@ -460,6 +491,10 @@ static const TypeInfo slavio_intctl_info = {
     .instance_size = sizeof(SLAVIO_INTCTLState),
     .instance_init = slavio_intctl_init,
     .class_init    = slavio_intctl_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_INTERRUPT_STATS_PROVIDER },
+        { }
+    },
 };
 
 static void slavio_intctl_register_types(void)
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 3/6] intc/slavio_intctl: implement InterruptStatsProvider interface

[Artyom Tarasenko]

Are slavio_pic_info and slavio_irq_info still used after this patch?

On Mon, Sep 26, 2016 at 10:23 PM, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/intc/slavio_intctl.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
> index e82e893..a9acb64 100644
> --- a/hw/intc/slavio_intctl.c
> +++ b/hw/intc/slavio_intctl.c
> @@ -26,6 +26,7 @@
>  #include "hw/sparc/sun4m.h"
>  #include "monitor/monitor.h"
>  #include "hw/sysbus.h"
> +#include "hw/intc/intc.h"
>  #include "trace.h"
>
>  //#define DEBUG_IRQ_COUNT
> @@ -418,6 +419,31 @@ static void slavio_intctl_reset(DeviceState *d)
>      slavio_check_interrupts(s, 0);
>  }
>
> +#ifdef DEBUG_IRQ_COUNT
> +static bool slavio_intctl_get_statistics(InterruptStatsProvider *obj,
> +                                         uint64_t **irq_counts,
> +                                         unsigned int *nb_irqs)
> +{
> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
> +    *irq_counts = s->irq_count;
> +    *nb_irqs = ARRAY_SIZE(s->irq_count);
> +    return true;
> +}
> +#endif
> +
> +static void slavio_intctl_print_info(InterruptStatsProvider *obj, Monitor *mon)
> +{
> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
> +    int i;
> +
> +    for (i = 0; i < MAX_CPUS; i++) {
> +        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
> +                       s->slaves[i].intreg_pending);
> +    }
> +    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
> +                   s->intregm_pending, s->intregm_disabled);
> +}
> +
>  static void slavio_intctl_init(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -449,9 +475,14 @@ static void slavio_intctl_init(Object *obj)
>  static void slavio_intctl_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
>
>      dc->reset = slavio_intctl_reset;
>      dc->vmsd = &vmstate_intctl;
> +#ifdef DEBUG_IRQ_COUNT
> +    ic->get_statistics = slavio_intctl_get_statistics;
> +#endif
> +    ic->print_info = slavio_intctl_print_info;
>  }
>
>  static const TypeInfo slavio_intctl_info = {
> @@ -460,6 +491,10 @@ static const TypeInfo slavio_intctl_info = {
>      .instance_size = sizeof(SLAVIO_INTCTLState),
>      .instance_init = slavio_intctl_init,
>      .class_init    = slavio_intctl_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> +        { }
> +    },
>  };
>
>  static void slavio_intctl_register_types(void)
> --
> 2.1.4
>
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCH v2 3/6] intc/slavio_intctl: implement InterruptStatsProvider interface

[Hervé Poussineau]

Le 27/09/2016 à 16:53, Artyom Tarasenko a écrit :
> Are slavio_pic_info and slavio_irq_info still used after this patch?

After this patch, yes.
However, slavio_pic_info/slavio_irq_info (and sun4m_hmp_info_pic/sun4m_hmp_info_irq) will be removed in patch 5/6.

Hervé

>
> On Mon, Sep 26, 2016 at 10:23 PM, Hervé Poussineau <hpoussin@reactos.org> wrote:
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  hw/intc/slavio_intctl.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
>> index e82e893..a9acb64 100644
>> --- a/hw/intc/slavio_intctl.c
>> +++ b/hw/intc/slavio_intctl.c
>> @@ -26,6 +26,7 @@
>>  #include "hw/sparc/sun4m.h"
>>  #include "monitor/monitor.h"
>>  #include "hw/sysbus.h"
>> +#include "hw/intc/intc.h"
>>  #include "trace.h"
>>
>>  //#define DEBUG_IRQ_COUNT
>> @@ -418,6 +419,31 @@ static void slavio_intctl_reset(DeviceState *d)
>>      slavio_check_interrupts(s, 0);
>>  }
>>
>> +#ifdef DEBUG_IRQ_COUNT
>> +static bool slavio_intctl_get_statistics(InterruptStatsProvider *obj,
>> +                                         uint64_t **irq_counts,
>> +                                         unsigned int *nb_irqs)
>> +{
>> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
>> +    *irq_counts = s->irq_count;
>> +    *nb_irqs = ARRAY_SIZE(s->irq_count);
>> +    return true;
>> +}
>> +#endif
>> +
>> +static void slavio_intctl_print_info(InterruptStatsProvider *obj, Monitor *mon)
>> +{
>> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
>> +    int i;
>> +
>> +    for (i = 0; i < MAX_CPUS; i++) {
>> +        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
>> +                       s->slaves[i].intreg_pending);
>> +    }
>> +    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
>> +                   s->intregm_pending, s->intregm_disabled);
>> +}
>> +
>>  static void slavio_intctl_init(Object *obj)
>>  {
>>      DeviceState *dev = DEVICE(obj);
>> @@ -449,9 +475,14 @@ static void slavio_intctl_init(Object *obj)
>>  static void slavio_intctl_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
>>
>>      dc->reset = slavio_intctl_reset;
>>      dc->vmsd = &vmstate_intctl;
>> +#ifdef DEBUG_IRQ_COUNT
>> +    ic->get_statistics = slavio_intctl_get_statistics;
>> +#endif
>> +    ic->print_info = slavio_intctl_print_info;
>>  }
>>
>>  static const TypeInfo slavio_intctl_info = {
>> @@ -460,6 +491,10 @@ static const TypeInfo slavio_intctl_info = {
>>      .instance_size = sizeof(SLAVIO_INTCTLState),
>>      .instance_init = slavio_intctl_init,
>>      .class_init    = slavio_intctl_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_INTERRUPT_STATS_PROVIDER },
>> +        { }
>> +    },
>>  };
>>
>>  static void slavio_intctl_register_types(void)
>> --
>> 2.1.4
>>
>>
>
>
>

Re: [Qemu-devel] [PATCH v2 3/6] intc/slavio_intctl: implement InterruptStatsProvider interface

[Artyom Tarasenko]

On Tue, Sep 27, 2016 at 8:39 PM, Hervé Poussineau <hpoussin@reactos.org> wrote:
> Le 27/09/2016 à 16:53, Artyom Tarasenko a écrit :
>>
>> Are slavio_pic_info and slavio_irq_info still used after this patch?
>
>
> After this patch, yes.
> However, slavio_pic_info/slavio_irq_info (and
> sun4m_hmp_info_pic/sun4m_hmp_info_irq) will be removed in patch 5/6.
>

Oh. The 5/6 somehow didn't make it to my inbox, so I missed it.

Acked-by: Artyom Tarasenko <atar4qemu@gmail.com>

>
>
>>
>> On Mon, Sep 26, 2016 at 10:23 PM, Hervé Poussineau <hpoussin@reactos.org>
>> wrote:
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> ---
>>>  hw/intc/slavio_intctl.c | 35 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>
>>> diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
>>> index e82e893..a9acb64 100644
>>> --- a/hw/intc/slavio_intctl.c
>>> +++ b/hw/intc/slavio_intctl.c
>>> @@ -26,6 +26,7 @@
>>>  #include "hw/sparc/sun4m.h"
>>>  #include "monitor/monitor.h"
>>>  #include "hw/sysbus.h"
>>> +#include "hw/intc/intc.h"
>>>  #include "trace.h"
>>>
>>>  //#define DEBUG_IRQ_COUNT
>>> @@ -418,6 +419,31 @@ static void slavio_intctl_reset(DeviceState *d)
>>>      slavio_check_interrupts(s, 0);
>>>  }
>>>
>>> +#ifdef DEBUG_IRQ_COUNT
>>> +static bool slavio_intctl_get_statistics(InterruptStatsProvider *obj,
>>> +                                         uint64_t **irq_counts,
>>> +                                         unsigned int *nb_irqs)
>>> +{
>>> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
>>> +    *irq_counts = s->irq_count;
>>> +    *nb_irqs = ARRAY_SIZE(s->irq_count);
>>> +    return true;
>>> +}
>>> +#endif
>>> +
>>> +static void slavio_intctl_print_info(InterruptStatsProvider *obj,
>>> Monitor *mon)
>>> +{
>>> +    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(obj);
>>> +    int i;
>>> +
>>> +    for (i = 0; i < MAX_CPUS; i++) {
>>> +        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
>>> +                       s->slaves[i].intreg_pending);
>>> +    }
>>> +    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
>>> +                   s->intregm_pending, s->intregm_disabled);
>>> +}
>>> +
>>>  static void slavio_intctl_init(Object *obj)
>>>  {
>>>      DeviceState *dev = DEVICE(obj);
>>> @@ -449,9 +475,14 @@ static void slavio_intctl_init(Object *obj)
>>>  static void slavio_intctl_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    InterruptStatsProviderClass *ic =
>>> INTERRUPT_STATS_PROVIDER_CLASS(klass);
>>>
>>>      dc->reset = slavio_intctl_reset;
>>>      dc->vmsd = &vmstate_intctl;
>>> +#ifdef DEBUG_IRQ_COUNT
>>> +    ic->get_statistics = slavio_intctl_get_statistics;
>>> +#endif
>>> +    ic->print_info = slavio_intctl_print_info;
>>>  }
>>>
>>>  static const TypeInfo slavio_intctl_info = {
>>> @@ -460,6 +491,10 @@ static const TypeInfo slavio_intctl_info = {
>>>      .instance_size = sizeof(SLAVIO_INTCTLState),
>>>      .instance_init = slavio_intctl_init,
>>>      .class_init    = slavio_intctl_class_init,
>>> +    .interfaces = (InterfaceInfo[]) {
>>> +        { TYPE_INTERRUPT_STATS_PROVIDER },
>>> +        { }
>>> +    },
>>>  };
>>>
>>>  static void slavio_intctl_register_types(void)
>>> --
>>> 2.1.4
>>>
>>>
>>
>>
>>
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

[Qemu-devel] [PATCH v2 4/6] intc/lm32_pic: implement InterruptStatsProvider interface

[Hervé Poussineau]

We have to change the vmstate version due to changes in statistics counters.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/intc/lm32_pic.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/hw/intc/lm32_pic.c b/hw/intc/lm32_pic.c
index 3dad01c..c045b99 100644
--- a/hw/intc/lm32_pic.c
+++ b/hw/intc/lm32_pic.c
@@ -25,6 +25,7 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "hw/lm32/lm32_pic.h"
+#include "hw/intc/intc.h"
 
 #define TYPE_LM32_PIC "lm32-pic"
 #define LM32_PIC(obj) OBJECT_CHECK(LM32PicState, (obj), TYPE_LM32_PIC)
@@ -38,7 +39,7 @@ struct LM32PicState {
     uint32_t irq_state;
 
     /* statistics */
-    uint32_t stats_irq_count[32];
+    uint64_t stats_irq_count[32];
 };
 typedef struct LM32PicState LM32PicState;
 
@@ -152,6 +153,22 @@ static void pic_reset(DeviceState *d)
     }
 }
 
+static bool lm32_get_statistics(InterruptStatsProvider *obj,
+                                uint64_t **irq_counts, unsigned int *nb_irqs)
+{
+    LM32PicState *s = LM32_PIC(obj);
+    *irq_counts = s->stats_irq_count;
+    *nb_irqs = ARRAY_SIZE(s->stats_irq_count);
+    return true;
+}
+
+static void lm32_print_info(InterruptStatsProvider *obj, Monitor *mon)
+{
+    LM32PicState *s = LM32_PIC(obj);
+    monitor_printf(mon, "lm32-pic: im=%08x ip=%08x irq_state=%08x\n",
+            s->im, s->ip, s->irq_state);
+}
+
 static void lm32_pic_init(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -166,13 +183,13 @@ static void lm32_pic_init(Object *obj)
 
 static const VMStateDescription vmstate_lm32_pic = {
     .name = "lm32-pic",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(im, LM32PicState),
         VMSTATE_UINT32(ip, LM32PicState),
         VMSTATE_UINT32(irq_state, LM32PicState),
-        VMSTATE_UINT32_ARRAY(stats_irq_count, LM32PicState, 32),
+        VMSTATE_UINT64_ARRAY(stats_irq_count, LM32PicState, 32),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -180,9 +197,12 @@ static const VMStateDescription vmstate_lm32_pic = {
 static void lm32_pic_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
 
     dc->reset = pic_reset;
     dc->vmsd = &vmstate_lm32_pic;
+    ic->get_statistics = lm32_get_statistics;
+    ic->print_info = lm32_print_info;
 }
 
 static const TypeInfo lm32_pic_info = {
@@ -191,6 +211,10 @@ static const TypeInfo lm32_pic_info = {
     .instance_size = sizeof(LM32PicState),
     .instance_init = lm32_pic_init,
     .class_init    = lm32_pic_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_INTERRUPT_STATS_PROVIDER },
+        { }
+    },
 };
 
 static void lm32_pic_register_types(void)
-- 
2.1.4

[Qemu-devel] [PATCH v2 5/6] intc: make HMP 'info irq' and 'info pic' commands use InterruptStatsProvider interface

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hmp-commands-info.hx       | 12 ---------
 hmp.c                      | 65 ++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h                      |  2 ++
 hw/intc/i8259.c            | 36 -------------------------
 hw/intc/lm32_pic.c         | 31 ----------------------
 hw/intc/slavio_intctl.c    | 32 -----------------------
 hw/sparc/sun4m.c           | 15 +----------
 include/hw/i386/pc.h       |  2 --
 include/hw/lm32/lm32_pic.h |  3 ---
 include/hw/sparc/sun4m.h   |  8 ------
 monitor.c                  |  6 -----
 11 files changed, 68 insertions(+), 144 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 19729e5..6a7c476 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -179,13 +179,7 @@ ETEXI
         .args_type  = "",
         .params     = "",
         .help       = "show the interrupts statistics (if available)",
-#ifdef TARGET_SPARC
-        .cmd        = sun4m_hmp_info_irq,
-#elif defined(TARGET_LM32)
-        .cmd        = lm32_hmp_info_irq,
-#else
         .cmd        = hmp_info_irq,
-#endif
     },
 
 STEXI
@@ -199,13 +193,7 @@ ETEXI
         .args_type  = "",
         .params     = "",
         .help       = "show i8259 (PIC) state",
-#ifdef TARGET_SPARC
-        .cmd        = sun4m_hmp_info_pic,
-#elif defined(TARGET_LM32)
-        .cmd        = lm32_hmp_info_pic,
-#else
         .cmd        = hmp_info_pic,
-#endif
     },
 #endif
 
diff --git a/hmp.c b/hmp.c
index 336e7bf..ec2e9ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -36,6 +36,7 @@
 #include "qemu-io.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "hw/intc/intc.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -787,6 +788,70 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
     }
 }
 
+static int hmp_info_irq_foreach(Object *obj, void *opaque)
+{
+    InterruptStatsProvider *intc;
+    InterruptStatsProviderClass *k;
+    Monitor *mon = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
+        intc = INTERRUPT_STATS_PROVIDER(obj);
+        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
+        uint64_t *irq_counts;
+        unsigned int nb_irqs, i;
+        if (k->get_statistics &&
+            k->get_statistics(intc, &irq_counts, &nb_irqs)) {
+            if (nb_irqs > 0) {
+                monitor_printf(mon, "IRQ statistics for %s:\n",
+                               object_get_typename(obj));
+                for (i = 0; i < nb_irqs; i++) {
+                    if (irq_counts[i] > 0) {
+                        monitor_printf(mon, "%2d: %" PRId64 "\n", i,
+                                       irq_counts[i]);
+                    }
+                }
+            }
+        } else {
+            monitor_printf(mon, "IRQ statistics not available for %s.\n",
+                           object_get_typename(obj));
+        }
+    }
+
+    return 0;
+}
+
+void hmp_info_irq(Monitor *mon, const QDict *qdict)
+{
+    object_child_foreach_recursive(object_get_root(),
+                                   hmp_info_irq_foreach, mon);
+}
+
+static int hmp_info_pic_foreach(Object *obj, void *opaque)
+{
+    InterruptStatsProvider *intc;
+    InterruptStatsProviderClass *k;
+    Monitor *mon = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
+        intc = INTERRUPT_STATS_PROVIDER(obj);
+        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
+        if (k->print_info) {
+            k->print_info(intc, mon);
+        } else {
+            monitor_printf(mon, "PIC informations not available for %s.\n",
+                           object_get_typename(obj));
+        }
+    }
+
+    return 0;
+}
+
+void hmp_info_pic(Monitor *mon, const QDict *qdict)
+{
+    object_child_foreach_recursive(object_get_root(),
+                                   hmp_info_pic_foreach, mon);
+}
+
 void hmp_info_pci(Monitor *mon, const QDict *qdict)
 {
     PciInfoList *info_list, *info;
diff --git a/hmp.h b/hmp.h
index 0876ec0..184769c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
 void hmp_info_vnc(Monitor *mon, const QDict *qdict);
 void hmp_info_spice(Monitor *mon, const QDict *qdict);
 void hmp_info_balloon(Monitor *mon, const QDict *qdict);
+void hmp_info_irq(Monitor *mon, const QDict *qdict);
+void hmp_info_pic(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_tpm(Monitor *mon, const QDict *qdict);
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 75c8d22..fe9ecd6 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -461,42 +461,6 @@ static void pic_realize(DeviceState *dev, Error **errp)
     pc->parent_realize(dev, errp);
 }
 
-void hmp_info_pic(Monitor *mon, const QDict *qdict)
-{
-    int i;
-    PICCommonState *s;
-
-    if (!isa_pic) {
-        return;
-    }
-    for (i = 0; i < 2; i++) {
-        s = i == 0 ? PIC_COMMON(isa_pic) : slave_pic;
-        monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
-                       "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n",
-                       i, s->irr, s->imr, s->isr, s->priority_add,
-                       s->irq_base, s->read_reg_select, s->elcr,
-                       s->special_fully_nested_mode);
-    }
-}
-
-void hmp_info_irq(Monitor *mon, const QDict *qdict)
-{
-#ifndef DEBUG_IRQ_COUNT
-    monitor_printf(mon, "irq statistic code not compiled.\n");
-#else
-    int i;
-    int64_t count;
-
-    monitor_printf(mon, "IRQ statistics:\n");
-    for (i = 0; i < 16; i++) {
-        count = irq_count[i];
-        if (count > 0) {
-            monitor_printf(mon, "%2d: %" PRId64 "\n", i, count);
-        }
-    }
-#endif
-}
-
 qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq)
 {
     qemu_irq *irq_set;
diff --git a/hw/intc/lm32_pic.c b/hw/intc/lm32_pic.c
index c045b99..09e1511 100644
--- a/hw/intc/lm32_pic.c
+++ b/hw/intc/lm32_pic.c
@@ -43,35 +43,6 @@ struct LM32PicState {
 };
 typedef struct LM32PicState LM32PicState;
 
-static LM32PicState *pic;
-void lm32_hmp_info_pic(Monitor *mon, const QDict *qdict)
-{
-    if (pic == NULL) {
-        return;
-    }
-
-    monitor_printf(mon, "lm32-pic: im=%08x ip=%08x irq_state=%08x\n",
-            pic->im, pic->ip, pic->irq_state);
-}
-
-void lm32_hmp_info_irq(Monitor *mon, const QDict *qdict)
-{
-    int i;
-    uint32_t count;
-
-    if (pic == NULL) {
-        return;
-    }
-
-    monitor_printf(mon, "IRQ statistics:\n");
-    for (i = 0; i < 32; i++) {
-        count = pic->stats_irq_count[i];
-        if (count > 0) {
-            monitor_printf(mon, "%2d: %u\n", i, count);
-        }
-    }
-}
-
 static void update_irq(LM32PicState *s)
 {
     s->ip |= s->irq_state;
@@ -177,8 +148,6 @@ static void lm32_pic_init(Object *obj)
 
     qdev_init_gpio_in(dev, irq_handler, 32);
     sysbus_init_irq(sbd, &s->parent_irq);
-
-    pic = s;
 }
 
 static const VMStateDescription vmstate_lm32_pic = {
diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
index a9acb64..84e0bee 100644
--- a/hw/intc/slavio_intctl.c
+++ b/hw/intc/slavio_intctl.c
@@ -211,38 +211,6 @@ static const MemoryRegionOps slavio_intctlm_mem_ops = {
     },
 };
 
-void slavio_pic_info(Monitor *mon, DeviceState *dev)
-{
-    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(dev);
-    int i;
-
-    for (i = 0; i < MAX_CPUS; i++) {
-        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
-                       s->slaves[i].intreg_pending);
-    }
-    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
-                   s->intregm_pending, s->intregm_disabled);
-}
-
-void slavio_irq_info(Monitor *mon, DeviceState *dev)
-{
-#ifndef DEBUG_IRQ_COUNT
-    monitor_printf(mon, "irq statistic code not compiled.\n");
-#else
-    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(dev);
-    int i;
-    int64_t count;
-
-    s = SLAVIO_INTCTL(dev);
-    monitor_printf(mon, "IRQ statistics:\n");
-    for (i = 0; i < 32; i++) {
-        count = s->irq_count[i];
-        if (count > 0)
-            monitor_printf(mon, "%2d: %" PRId64 "\n", i, count);
-    }
-#endif
-}
-
 static const uint32_t intbit_to_level[] = {
     2, 3, 5, 7, 9, 11, 13, 2,   3, 5, 7, 9, 11, 13, 12, 12,
     6, 13, 4, 10, 8, 9, 11, 0,  0, 0, 0, 15, 15, 15, 15, 0,
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 478fda8..b3915e4 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -159,20 +159,6 @@ static void nvram_init(Nvram *nvram, uint8_t *macaddr,
     }
 }
 
-static DeviceState *slavio_intctl;
-
-void sun4m_hmp_info_pic(Monitor *mon, const QDict *qdict)
-{
-    if (slavio_intctl)
-        slavio_pic_info(mon, slavio_intctl);
-}
-
-void sun4m_hmp_info_irq(Monitor *mon, const QDict *qdict)
-{
-    if (slavio_intctl)
-        slavio_irq_info(mon, slavio_intctl);
-}
-
 void cpu_check_irqs(CPUSPARCState *env)
 {
     CPUState *cs;
@@ -873,6 +859,7 @@ static void dummy_fdc_tc(void *opaque, int irq, int level)
 static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
                           MachineState *machine)
 {
+    DeviceState *slavio_intctl;
     const char *cpu_model = machine->cpu_model;
     unsigned int i;
     void *iommu, *espdma, *ledma, *nvram;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ab8e319..73178ae 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -181,8 +181,6 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq);
 qemu_irq *kvm_i8259_init(ISABus *bus);
 int pic_read_irq(DeviceState *d);
 int pic_get_output(DeviceState *d);
-void hmp_info_pic(Monitor *mon, const QDict *qdict);
-void hmp_info_irq(Monitor *mon, const QDict *qdict);
 
 /* ioapic.c */
 
diff --git a/include/hw/lm32/lm32_pic.h b/include/hw/lm32/lm32_pic.h
index 189fa38..e6479b8 100644
--- a/include/hw/lm32/lm32_pic.h
+++ b/include/hw/lm32/lm32_pic.h
@@ -8,7 +8,4 @@ uint32_t lm32_pic_get_im(DeviceState *d);
 void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
 void lm32_pic_set_im(DeviceState *d, uint32_t im);
 
-void lm32_hmp_info_pic(Monitor *mon, const QDict *qdict);
-void lm32_hmp_info_irq(Monitor *mon, const QDict *qdict);
-
 #endif /* QEMU_HW_LM32_PIC_H */
diff --git a/include/hw/sparc/sun4m.h b/include/hw/sparc/sun4m.h
index 9c17425..580d87b 100644
--- a/include/hw/sparc/sun4m.h
+++ b/include/hw/sparc/sun4m.h
@@ -24,14 +24,6 @@ static inline void sparc_iommu_memory_write(void *opaque,
     sparc_iommu_memory_rw(opaque, addr, buf, len, 1);
 }
 
-/* slavio_intctl.c */
-void slavio_pic_info(Monitor *mon, DeviceState *dev);
-void slavio_irq_info(Monitor *mon, DeviceState *dev);
-
-/* sun4m.c */
-void sun4m_hmp_info_pic(Monitor *mon, const QDict *qdict);
-void sun4m_hmp_info_irq(Monitor *mon, const QDict *qdict);
-
 /* sparc32_dma.c */
 #include "hw/sparc/sparc32_dma.h"
 
diff --git a/monitor.c b/monitor.c
index 83c4edf..d26c3bc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -81,12 +81,6 @@
 #include "qemu/cutils.h"
 #include "qapi/qmp/dispatch.h"
 
-/* for hmp_info_irq/pic */
-#if defined(TARGET_SPARC)
-#include "hw/sparc/sun4m.h"
-#endif
-#include "hw/lm32/lm32_pic.h"
-
 #if defined(TARGET_S390X)
 #include "hw/s390x/storage-keys.h"
 #endif
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 5/6] intc: make HMP 'info irq' and 'info pic' commands use InterruptStatsProvider interface

[Paolo Bonzini]

On 26/09/2016 22:23, Hervé Poussineau wrote:
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hmp-commands-info.hx       | 12 ---------
>  hmp.c                      | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h                      |  2 ++
>  hw/intc/i8259.c            | 36 -------------------------
>  hw/intc/lm32_pic.c         | 31 ----------------------
>  hw/intc/slavio_intctl.c    | 32 -----------------------
>  hw/sparc/sun4m.c           | 15 +----------
>  include/hw/i386/pc.h       |  2 --
>  include/hw/lm32/lm32_pic.h |  3 ---
>  include/hw/sparc/sun4m.h   |  8 ------
>  monitor.c                  |  6 -----
>  11 files changed, 68 insertions(+), 144 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 19729e5..6a7c476 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -179,13 +179,7 @@ ETEXI
>          .args_type  = "",
>          .params     = "",
>          .help       = "show the interrupts statistics (if available)",
> -#ifdef TARGET_SPARC
> -        .cmd        = sun4m_hmp_info_irq,
> -#elif defined(TARGET_LM32)
> -        .cmd        = lm32_hmp_info_irq,
> -#else
>          .cmd        = hmp_info_irq,
> -#endif
>      },
>  
>  STEXI
> @@ -199,13 +193,7 @@ ETEXI
>          .args_type  = "",
>          .params     = "",
>          .help       = "show i8259 (PIC) state",
> -#ifdef TARGET_SPARC
> -        .cmd        = sun4m_hmp_info_pic,
> -#elif defined(TARGET_LM32)
> -        .cmd        = lm32_hmp_info_pic,
> -#else
>          .cmd        = hmp_info_pic,
> -#endif
>      },
>  #endif
>  
> diff --git a/hmp.c b/hmp.c
> index 336e7bf..ec2e9ce 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -36,6 +36,7 @@
>  #include "qemu-io.h"
>  #include "qemu/cutils.h"
>  #include "qemu/error-report.h"
> +#include "hw/intc/intc.h"
>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -787,6 +788,70 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
>      }
>  }
>  
> +static int hmp_info_irq_foreach(Object *obj, void *opaque)
> +{
> +    InterruptStatsProvider *intc;
> +    InterruptStatsProviderClass *k;
> +    Monitor *mon = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
> +        intc = INTERRUPT_STATS_PROVIDER(obj);
> +        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
> +        uint64_t *irq_counts;
> +        unsigned int nb_irqs, i;
> +        if (k->get_statistics &&
> +            k->get_statistics(intc, &irq_counts, &nb_irqs)) {
> +            if (nb_irqs > 0) {
> +                monitor_printf(mon, "IRQ statistics for %s:\n",
> +                               object_get_typename(obj));
> +                for (i = 0; i < nb_irqs; i++) {
> +                    if (irq_counts[i] > 0) {
> +                        monitor_printf(mon, "%2d: %" PRId64 "\n", i,
> +                                       irq_counts[i]);
> +                    }
> +                }
> +            }
> +        } else {
> +            monitor_printf(mon, "IRQ statistics not available for %s.\n",
> +                           object_get_typename(obj));
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +void hmp_info_irq(Monitor *mon, const QDict *qdict)
> +{
> +    object_child_foreach_recursive(object_get_root(),
> +                                   hmp_info_irq_foreach, mon);
> +}
> +
> +static int hmp_info_pic_foreach(Object *obj, void *opaque)
> +{
> +    InterruptStatsProvider *intc;
> +    InterruptStatsProviderClass *k;
> +    Monitor *mon = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
> +        intc = INTERRUPT_STATS_PROVIDER(obj);
> +        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
> +        if (k->print_info) {
> +            k->print_info(intc, mon);
> +        } else {
> +            monitor_printf(mon, "PIC informations not available for %s.\n",

Replacing with "Interrupt controller information", otherwise I'm queuing
the series.

Paolo

> +                           object_get_typename(obj));
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +void hmp_info_pic(Monitor *mon, const QDict *qdict)
> +{
> +    object_child_foreach_recursive(object_get_root(),
> +                                   hmp_info_pic_foreach, mon);
> +}
> +
>  void hmp_info_pci(Monitor *mon, const QDict *qdict)
>  {
>      PciInfoList *info_list, *info;
> diff --git a/hmp.h b/hmp.h
> index 0876ec0..184769c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -36,6 +36,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
>  void hmp_info_vnc(Monitor *mon, const QDict *qdict);
>  void hmp_info_spice(Monitor *mon, const QDict *qdict);
>  void hmp_info_balloon(Monitor *mon, const QDict *qdict);
> +void hmp_info_irq(Monitor *mon, const QDict *qdict);
> +void hmp_info_pic(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 75c8d22..fe9ecd6 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -461,42 +461,6 @@ static void pic_realize(DeviceState *dev, Error **errp)
>      pc->parent_realize(dev, errp);
>  }
>  
> -void hmp_info_pic(Monitor *mon, const QDict *qdict)
> -{
> -    int i;
> -    PICCommonState *s;
> -
> -    if (!isa_pic) {
> -        return;
> -    }
> -    for (i = 0; i < 2; i++) {
> -        s = i == 0 ? PIC_COMMON(isa_pic) : slave_pic;
> -        monitor_printf(mon, "pic%d: irr=%02x imr=%02x isr=%02x hprio=%d "
> -                       "irq_base=%02x rr_sel=%d elcr=%02x fnm=%d\n",
> -                       i, s->irr, s->imr, s->isr, s->priority_add,
> -                       s->irq_base, s->read_reg_select, s->elcr,
> -                       s->special_fully_nested_mode);
> -    }
> -}
> -
> -void hmp_info_irq(Monitor *mon, const QDict *qdict)
> -{
> -#ifndef DEBUG_IRQ_COUNT
> -    monitor_printf(mon, "irq statistic code not compiled.\n");
> -#else
> -    int i;
> -    int64_t count;
> -
> -    monitor_printf(mon, "IRQ statistics:\n");
> -    for (i = 0; i < 16; i++) {
> -        count = irq_count[i];
> -        if (count > 0) {
> -            monitor_printf(mon, "%2d: %" PRId64 "\n", i, count);
> -        }
> -    }
> -#endif
> -}
> -
>  qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq)
>  {
>      qemu_irq *irq_set;
> diff --git a/hw/intc/lm32_pic.c b/hw/intc/lm32_pic.c
> index c045b99..09e1511 100644
> --- a/hw/intc/lm32_pic.c
> +++ b/hw/intc/lm32_pic.c
> @@ -43,35 +43,6 @@ struct LM32PicState {
>  };
>  typedef struct LM32PicState LM32PicState;
>  
> -static LM32PicState *pic;
> -void lm32_hmp_info_pic(Monitor *mon, const QDict *qdict)
> -{
> -    if (pic == NULL) {
> -        return;
> -    }
> -
> -    monitor_printf(mon, "lm32-pic: im=%08x ip=%08x irq_state=%08x\n",
> -            pic->im, pic->ip, pic->irq_state);
> -}
> -
> -void lm32_hmp_info_irq(Monitor *mon, const QDict *qdict)
> -{
> -    int i;
> -    uint32_t count;
> -
> -    if (pic == NULL) {
> -        return;
> -    }
> -
> -    monitor_printf(mon, "IRQ statistics:\n");
> -    for (i = 0; i < 32; i++) {
> -        count = pic->stats_irq_count[i];
> -        if (count > 0) {
> -            monitor_printf(mon, "%2d: %u\n", i, count);
> -        }
> -    }
> -}
> -
>  static void update_irq(LM32PicState *s)
>  {
>      s->ip |= s->irq_state;
> @@ -177,8 +148,6 @@ static void lm32_pic_init(Object *obj)
>  
>      qdev_init_gpio_in(dev, irq_handler, 32);
>      sysbus_init_irq(sbd, &s->parent_irq);
> -
> -    pic = s;
>  }
>  
>  static const VMStateDescription vmstate_lm32_pic = {
> diff --git a/hw/intc/slavio_intctl.c b/hw/intc/slavio_intctl.c
> index a9acb64..84e0bee 100644
> --- a/hw/intc/slavio_intctl.c
> +++ b/hw/intc/slavio_intctl.c
> @@ -211,38 +211,6 @@ static const MemoryRegionOps slavio_intctlm_mem_ops = {
>      },
>  };
>  
> -void slavio_pic_info(Monitor *mon, DeviceState *dev)
> -{
> -    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(dev);
> -    int i;
> -
> -    for (i = 0; i < MAX_CPUS; i++) {
> -        monitor_printf(mon, "per-cpu %d: pending 0x%08x\n", i,
> -                       s->slaves[i].intreg_pending);
> -    }
> -    monitor_printf(mon, "master: pending 0x%08x, disabled 0x%08x\n",
> -                   s->intregm_pending, s->intregm_disabled);
> -}
> -
> -void slavio_irq_info(Monitor *mon, DeviceState *dev)
> -{
> -#ifndef DEBUG_IRQ_COUNT
> -    monitor_printf(mon, "irq statistic code not compiled.\n");
> -#else
> -    SLAVIO_INTCTLState *s = SLAVIO_INTCTL(dev);
> -    int i;
> -    int64_t count;
> -
> -    s = SLAVIO_INTCTL(dev);
> -    monitor_printf(mon, "IRQ statistics:\n");
> -    for (i = 0; i < 32; i++) {
> -        count = s->irq_count[i];
> -        if (count > 0)
> -            monitor_printf(mon, "%2d: %" PRId64 "\n", i, count);
> -    }
> -#endif
> -}
> -
>  static const uint32_t intbit_to_level[] = {
>      2, 3, 5, 7, 9, 11, 13, 2,   3, 5, 7, 9, 11, 13, 12, 12,
>      6, 13, 4, 10, 8, 9, 11, 0,  0, 0, 0, 15, 15, 15, 15, 0,
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 478fda8..b3915e4 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -159,20 +159,6 @@ static void nvram_init(Nvram *nvram, uint8_t *macaddr,
>      }
>  }
>  
> -static DeviceState *slavio_intctl;
> -
> -void sun4m_hmp_info_pic(Monitor *mon, const QDict *qdict)
> -{
> -    if (slavio_intctl)
> -        slavio_pic_info(mon, slavio_intctl);
> -}
> -
> -void sun4m_hmp_info_irq(Monitor *mon, const QDict *qdict)
> -{
> -    if (slavio_intctl)
> -        slavio_irq_info(mon, slavio_intctl);
> -}
> -
>  void cpu_check_irqs(CPUSPARCState *env)
>  {
>      CPUState *cs;
> @@ -873,6 +859,7 @@ static void dummy_fdc_tc(void *opaque, int irq, int level)
>  static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>                            MachineState *machine)
>  {
> +    DeviceState *slavio_intctl;
>      const char *cpu_model = machine->cpu_model;
>      unsigned int i;
>      void *iommu, *espdma, *ledma, *nvram;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index ab8e319..73178ae 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -181,8 +181,6 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq);
>  qemu_irq *kvm_i8259_init(ISABus *bus);
>  int pic_read_irq(DeviceState *d);
>  int pic_get_output(DeviceState *d);
> -void hmp_info_pic(Monitor *mon, const QDict *qdict);
> -void hmp_info_irq(Monitor *mon, const QDict *qdict);
>  
>  /* ioapic.c */
>  
> diff --git a/include/hw/lm32/lm32_pic.h b/include/hw/lm32/lm32_pic.h
> index 189fa38..e6479b8 100644
> --- a/include/hw/lm32/lm32_pic.h
> +++ b/include/hw/lm32/lm32_pic.h
> @@ -8,7 +8,4 @@ uint32_t lm32_pic_get_im(DeviceState *d);
>  void lm32_pic_set_ip(DeviceState *d, uint32_t ip);
>  void lm32_pic_set_im(DeviceState *d, uint32_t im);
>  
> -void lm32_hmp_info_pic(Monitor *mon, const QDict *qdict);
> -void lm32_hmp_info_irq(Monitor *mon, const QDict *qdict);
> -
>  #endif /* QEMU_HW_LM32_PIC_H */
> diff --git a/include/hw/sparc/sun4m.h b/include/hw/sparc/sun4m.h
> index 9c17425..580d87b 100644
> --- a/include/hw/sparc/sun4m.h
> +++ b/include/hw/sparc/sun4m.h
> @@ -24,14 +24,6 @@ static inline void sparc_iommu_memory_write(void *opaque,
>      sparc_iommu_memory_rw(opaque, addr, buf, len, 1);
>  }
>  
> -/* slavio_intctl.c */
> -void slavio_pic_info(Monitor *mon, DeviceState *dev);
> -void slavio_irq_info(Monitor *mon, DeviceState *dev);
> -
> -/* sun4m.c */
> -void sun4m_hmp_info_pic(Monitor *mon, const QDict *qdict);
> -void sun4m_hmp_info_irq(Monitor *mon, const QDict *qdict);
> -
>  /* sparc32_dma.c */
>  #include "hw/sparc/sparc32_dma.h"
>  
> diff --git a/monitor.c b/monitor.c
> index 83c4edf..d26c3bc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -81,12 +81,6 @@
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/dispatch.h"
>  
> -/* for hmp_info_irq/pic */
> -#if defined(TARGET_SPARC)
> -#include "hw/sparc/sun4m.h"
> -#endif
> -#include "hw/lm32/lm32_pic.h"
> -
>  #if defined(TARGET_S390X)
>  #include "hw/s390x/storage-keys.h"
>  #endif
> 

[Qemu-devel] [PATCH v2 6/6] intc: make HMP 'info irq' and 'info pic' commands available on all targets

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hmp-commands-info.hx | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 6a7c476..55d50c4 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -172,8 +172,6 @@ STEXI
 Show the command line history.
 ETEXI
 
-#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
-    defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET_SPARC64))
     {
         .name       = "irq",
         .args_type  = "",
@@ -192,10 +190,9 @@ ETEXI
         .name       = "pic",
         .args_type  = "",
         .params     = "",
-        .help       = "show i8259 (PIC) state",
+        .help       = "show PIC state",
         .cmd        = hmp_info_pic,
     },
-#endif
 
 STEXI
 @item info pic
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 0/6] intc: change 'info irq' and 'info pic' to be target-agnostic

[David Gibson]

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

On Mon, Sep 26, 2016 at 10:23:22PM +0200, Hervé Poussineau wrote:
> Hi,
> 
> This patchset aims at genericizing the 'info irq' and 'info pic' HMP commands, so
> that it is available on all machines and can display details about more than one
> interrupt controller per machine.
> 
> Patch 1 adds a new interface InterruptStatsProvider, which is used to:
> - gather statistics for the 'info irq' command
> - print some text when 'info pic' is called
> 
> Patches 2 to 4 implement InterruptStatsProvider interface on interrupt controllers
> which have ad-hock code to handle 'info irq'/'info pic' commands.
> 
> Patch 5 removes ad-hock code, and replaces it by a generic version. You can get
> details about multiple interrupt controllers per machine starting here.
> 
> Patch 6 makes 'info irq'/'info pic' commands available on all architectures.
> For example, Alpha clipper machine is now able to display details about the
> i8259 interrupt controller.

Thanks for doing this.  I didn't spot your first version of this, but
it's a rather more thorough approach to some cleanups I attempted a
while back, but never got around to completing.  Looks like a nice
approach.

> 
> Changes since v1:
> - renamed interface from IntCtrl to InterruptStatsProvider
> 
> Hervé
> 
> Hervé Poussineau (6):
>   intc: add an interface to gather statistics/informations on interrupt
>     controllers
>   intc/i8259: implement InterruptStatsProvider interface
>   intc/slavio_intctl: implement InterruptStatsProvider interface
>   intc/lm32_pic: implement InterruptStatsProvider interface
>   intc: make HMP 'info irq' and 'info pic' commands use
>     InterruptStatsProvider interface
>   intc: make HMP 'info irq' and 'info pic' commands available on all
>     targets
> 
>  hmp-commands-info.hx       | 17 +----------
>  hmp.c                      | 65 +++++++++++++++++++++++++++++++++++++++++
>  hmp.h                      |  2 ++
>  hw/intc/Makefile.objs      |  1 +
>  hw/intc/i8259.c            | 73 +++++++++++++++++++++++-----------------------
>  hw/intc/intc.c             | 41 ++++++++++++++++++++++++++
>  hw/intc/lm32_pic.c         | 63 ++++++++++++++++++---------------------
>  hw/intc/slavio_intctl.c    | 67 ++++++++++++++++++++++--------------------
>  hw/sparc/sun4m.c           | 15 +---------
>  include/hw/i386/pc.h       |  2 --
>  include/hw/intc/intc.h     | 30 +++++++++++++++++++
>  include/hw/lm32/lm32_pic.h |  3 --
>  include/hw/sparc/sun4m.h   |  8 -----
>  monitor.c                  |  6 ----
>  14 files changed, 241 insertions(+), 152 deletions(-)
>  create mode 100644 hw/intc/intc.c
>  create mode 100644 include/hw/intc/intc.h
> 

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