[Qemu-devel] [PATCH 0/5] i8259: cleanups and enhancements

[Qemu-devel] [PATCH 0/5] i8259: cleanups and enhancements

[Peter Xu]

It's mostly a cleanup, but patch 4 allows kvm-i8259 to support "info
pic" and "info irq" too.

I'm thinking whether it'll be good to move on this work to spread
these commands to IOAPICs too, by removing "info ioapic" command and
let "info pic" dump the things altogether (after all I think ioapic is
also one type of PIC).  Also, we can let ioapic to have statistics
too.  Let me know if anyone thinks this can be useful/cleaner and want
me to continue (which won't need too much time for sure).

Please review.  Thanks,

Peter Xu (5):
  i8259: convert DPRINTFs into trace
  i8259: use DEBUG_IRQ_COUNT always
  i8259: generalize statistics into common code
  kvm-i8259: support "info pic" and "info irq"
  i8259: move TYPE_INTERRUPT_STATS_PROVIDER upper

 hw/i386/kvm/i8259.c             |  1 +
 hw/intc/i8259.c                 | 86 ++++++-----------------------------------
 hw/intc/i8259_common.c          | 49 +++++++++++++++++++++++
 hw/intc/trace-events            |  7 ++++
 include/hw/isa/i8259_internal.h |  7 +++-
 5 files changed, 74 insertions(+), 76 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/5] i8259: convert DPRINTFs into trace

[Peter Xu]

One thing to mention is that in pic_set_irq() I need to uncomment a few
lines in the macros to make sure IRQ value calculation is correct.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/i8259.c      | 26 +++++++++++---------------
 hw/intc/trace-events |  7 +++++++
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index fe9ecd6bd4..f12e0b27f1 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -30,17 +30,11 @@
 #include "qemu/log.h"
 #include "hw/isa/i8259_internal.h"
 #include "hw/intc/intc.h"
+#include "trace.h"
 
 /* debug PIC */
 //#define DEBUG_PIC
 
-#ifdef DEBUG_PIC
-#define DPRINTF(fmt, ...)                                       \
-    do { printf("pic: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...)
-#endif
-
 //#define DEBUG_IRQ_LATENCY
 //#define DEBUG_IRQ_COUNT
 
@@ -122,8 +116,7 @@ static void pic_update_irq(PICCommonState *s)
 
     irq = pic_get_irq(s);
     if (irq >= 0) {
-        DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
-                s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
+        trace_pic_update_irq(s->master, s->imr, s->irr, s->priority_add);
         qemu_irq_raise(s->int_out[0]);
     } else {
         qemu_irq_lower(s->int_out[0]);
@@ -140,9 +133,11 @@ static void pic_set_irq(void *opaque, int irq, int level)
     defined(DEBUG_IRQ_LATENCY)
     int irq_index = s->master ? irq : irq + 8;
 #endif
+
+    trace_pic_set_irq(s->master, irq, level);
+
 #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
     if (level != irq_level[irq_index]) {
-        DPRINTF("pic_set_irq: irq=%d level=%d\n", irq_index, level);
         irq_level[irq_index] = level;
 #ifdef DEBUG_IRQ_COUNT
         if (level == 1) {
@@ -223,18 +218,18 @@ int pic_read_irq(DeviceState *d)
         intno = s->irq_base + irq;
     }
 
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
     if (irq == 2) {
         irq = irq2 + 8;
     }
-#endif
+
 #ifdef DEBUG_IRQ_LATENCY
     printf("IRQ%d latency=%0.3fus\n",
            irq,
            (double)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
                     irq_time[irq]) * 1000000.0 / NANOSECONDS_PER_SECOND);
 #endif
-    DPRINTF("pic_interrupt: irq=%d\n", irq);
+
+    trace_pic_interrupt(irq, intno);
     return intno;
 }
 
@@ -289,7 +284,8 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
     uint32_t val = val64;
     int priority, cmd, irq;
 
-    DPRINTF("write: addr=0x%02x val=0x%02x\n", addr, val);
+    trace_pic_ioport_write(s->master, addr, val);
+
     if (addr == 0) {
         if (val & 0x10) {
             pic_init_reset(s);
@@ -402,7 +398,7 @@ static uint64_t pic_ioport_read(void *opaque, hwaddr addr,
             ret = s->imr;
         }
     }
-    DPRINTF("read: addr=0x%02" HWADDR_PRIx " val=0x%02x\n", addr, ret);
+    trace_pic_ioport_read(s->master, addr, ret);
     return ret;
 }
 
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index b298fac7c6..c72b37c5cf 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -1,5 +1,12 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/intc/i8259.c
+pic_update_irq(bool master, uint8_t imr, uint8_t irr, uint8_t padd) "master %d imr %"PRIu8" irr %"PRIu8" padd %"PRIu8
+pic_set_irq(bool master, int irq, int level) "master %d irq %d level %d"
+pic_interrupt(int irq, int intno) "irq %d intno %d"
+pic_ioport_write(bool master, uint64_t addr, uint64_t val) "master %d addr 0x%"PRIx64" val 0x%"PRIx64
+pic_ioport_read(bool master, uint64_t addr, int val) "master %d addr 0x%"PRIx64" val 0x%x"
+
 # hw/intc/apic_common.c
 cpu_set_apic_base(uint64_t val) "0x%016"PRIx64
 cpu_get_apic_base(uint64_t val) "0x%016"PRIx64
-- 
2.14.3

[Qemu-devel] [PATCH 2/5] i8259: use DEBUG_IRQ_COUNT always

[Peter Xu]

It's not really scary to even enable it forever.  After all it's i8259,
and it's even not the kernel one.

Then we can remove quite a few of lines to make it cleaner.  And "info
irq" will always work for it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/i8259.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index f12e0b27f1..20c9d0a58b 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -36,7 +36,6 @@
 //#define DEBUG_PIC
 
 //#define DEBUG_IRQ_LATENCY
-//#define DEBUG_IRQ_COUNT
 
 #define TYPE_I8259 "isa-i8259"
 #define PIC_CLASS(class) OBJECT_CLASS_CHECK(PICClass, (class), TYPE_I8259)
@@ -52,12 +51,8 @@ typedef struct PICClass {
     DeviceRealize parent_realize;
 } PICClass;
 
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
 static int irq_level[16];
-#endif
-#ifdef DEBUG_IRQ_COUNT
 static uint64_t irq_count[16];
-#endif
 #ifdef DEBUG_IRQ_LATENCY
 static int64_t irq_time[16];
 #endif
@@ -128,24 +123,17 @@ static void pic_set_irq(void *opaque, int irq, int level)
 {
     PICCommonState *s = opaque;
     int mask = 1 << irq;
-
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT) || \
-    defined(DEBUG_IRQ_LATENCY)
     int irq_index = s->master ? irq : irq + 8;
-#endif
 
     trace_pic_set_irq(s->master, irq, level);
 
-#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
     if (level != irq_level[irq_index]) {
         irq_level[irq_index] = level;
-#ifdef DEBUG_IRQ_COUNT
         if (level == 1) {
             irq_count[irq_index]++;
         }
-#endif
     }
-#endif
+
 #ifdef DEBUG_IRQ_LATENCY
     if (level) {
         irq_time[irq_index] = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -253,12 +241,8 @@ static bool pic_get_statistics(InterruptStatsProvider *obj,
     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;
-- 
2.14.3

[Qemu-devel] [PATCH 3/5] i8259: generalize statistics into common code

[Peter Xu]

It was only for userspace i8259.  Move it to general code so that
kvm-i8259 can also use it in the future.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/intc/i8259.c                 | 37 +------------------------------------
 hw/intc/i8259_common.c          | 41 +++++++++++++++++++++++++++++++++++++++++
 include/hw/isa/i8259_internal.h |  7 +++++--
 3 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 20c9d0a58b..d9b9666aff 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -25,11 +25,9 @@
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/isa.h"
-#include "monitor/monitor.h"
 #include "qemu/timer.h"
 #include "qemu/log.h"
 #include "hw/isa/i8259_internal.h"
-#include "hw/intc/intc.h"
 #include "trace.h"
 
 /* debug PIC */
@@ -51,8 +49,6 @@ typedef struct PICClass {
     DeviceRealize parent_realize;
 } PICClass;
 
-static int irq_level[16];
-static uint64_t irq_count[16];
 #ifdef DEBUG_IRQ_LATENCY
 static int64_t irq_time[16];
 #endif
@@ -126,13 +122,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     int irq_index = s->master ? irq : irq + 8;
 
     trace_pic_set_irq(s->master, irq, level);
-
-    if (level != irq_level[irq_index]) {
-        irq_level[irq_index] = level;
-        if (level == 1) {
-            irq_count[irq_index]++;
-        }
-    }
+    pic_stat_update_irq(irq_index, level);
 
 #ifdef DEBUG_IRQ_LATENCY
     if (level) {
@@ -235,31 +225,6 @@ 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) {
-        *irq_counts = irq_count;
-        *nb_irqs = ARRAY_SIZE(irq_count);
-    } 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)
 {
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index 18427b459a..a3caddeefb 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -25,6 +25,10 @@
 #include "qemu/osdep.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/i8259_internal.h"
+#include "monitor/monitor.h"
+
+static int irq_level[16];
+static uint64_t irq_count[16];
 
 void pic_reset_common(PICCommonState *s)
 {
@@ -98,6 +102,43 @@ ISADevice *i8259_init_chip(const char *name, ISABus *bus, bool master)
     return isadev;
 }
 
+void pic_stat_update_irq(int irq, int level)
+{
+    if (level != irq_level[irq]) {
+        irq_level[irq] = level;
+        if (level == 1) {
+            irq_count[irq]++;
+        }
+    }
+}
+
+bool pic_get_statistics(InterruptStatsProvider *obj,
+                        uint64_t **irq_counts, unsigned int *nb_irqs)
+{
+    PICCommonState *s = PIC_COMMON(obj);
+
+    if (s->master) {
+        *irq_counts = irq_count;
+        *nb_irqs = ARRAY_SIZE(irq_count);
+    } else {
+        *irq_counts = NULL;
+        *nb_irqs = 0;
+    }
+
+    return true;
+}
+
+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 const VMStateDescription vmstate_pic_common = {
     .name = "i8259",
     .version_id = 1,
diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
index 6954b6ec5f..f742c2a726 100644
--- a/include/hw/isa/i8259_internal.h
+++ b/include/hw/isa/i8259_internal.h
@@ -28,6 +28,7 @@
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/isa.h"
+#include "hw/intc/intc.h"
 
 typedef struct PICCommonState PICCommonState;
 
@@ -76,8 +77,10 @@ struct PICCommonState {
 };
 
 void pic_reset_common(PICCommonState *s);
-
 ISADevice *i8259_init_chip(const char *name, ISABus *bus, bool master);
-
+void pic_stat_update_irq(int irq, int level);
+bool pic_get_statistics(InterruptStatsProvider *obj,
+                        uint64_t **irq_counts, unsigned int *nb_irqs);
+void pic_print_info(InterruptStatsProvider *obj, Monitor *mon);
 
 #endif /* QEMU_I8259_INTERNAL_H */
-- 
2.14.3

[Qemu-devel] [PATCH 4/5] kvm-i8259: support "info pic" and "info irq"

[Peter Xu]

Let's leverage the i8259 common code for kvm-i8259 too.

I think it's still possible that stats can lost when i8259 is in kernel
and meanwhile when irqfd is used, e.g., by vfio or vhost devices.
However that should be rare IMHO since they should be using MSIs mostly
if they really want performance (that's why people use vhost and device
assignment), and no old INTx should be used.  As long as the INTx users
are emulated in QEMU the stats will be correct.

For "info pic", it should be always accurate since we fetch kvm regs
before dump.

More importantly, it's just too simple to do this now - it's only 10+
LOC to gain this feature.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/kvm/i8259.c    | 8 ++++++++
 hw/intc/i8259_common.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
index 11d1b726b6..57abe091b0 100644
--- a/hw/i386/kvm/i8259.c
+++ b/hw/i386/kvm/i8259.c
@@ -111,6 +111,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level)
 {
     int delivered;
 
+    pic_stat_update_irq(irq, level);
     delivered = kvm_set_irq(kvm_state, irq, level);
     apic_report_irq_delivered(delivered);
 }
@@ -139,12 +140,15 @@ static void kvm_i8259_class_init(ObjectClass *klass, void *data)
     KVMPICClass *kpc = KVM_PIC_CLASS(klass);
     PICCommonClass *k = PIC_COMMON_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
 
     dc->reset     = kvm_pic_reset;
     kpc->parent_realize = dc->realize;
     dc->realize   = kvm_pic_realize;
     k->pre_save   = kvm_pic_get;
     k->post_load  = kvm_pic_put;
+    ic->get_statistics = pic_get_statistics;
+    ic->print_info = pic_print_info;
 }
 
 static const TypeInfo kvm_i8259_info = {
@@ -153,6 +157,10 @@ static const TypeInfo kvm_i8259_info = {
     .instance_size = sizeof(PICCommonState),
     .class_init = kvm_i8259_class_init,
     .class_size = sizeof(KVMPICClass),
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_INTERRUPT_STATS_PROVIDER },
+        { }
+    },
 };
 
 static void kvm_pic_register_types(void)
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index a3caddeefb..7efd2e8012 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -132,6 +132,7 @@ void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
 {
     PICCommonState *s = PIC_COMMON(obj);
 
+    pic_dispatch_pre_save(s);
     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,
-- 
2.14.3

[Qemu-devel] [PATCH 5/5] i8259: move TYPE_INTERRUPT_STATS_PROVIDER upper

[Peter Xu]

Now both classes (i8259, i8259-kvm) support this.  Move this upper to
the common class code.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/kvm/i8259.c    | 7 -------
 hw/intc/i8259.c        | 7 -------
 hw/intc/i8259_common.c | 7 +++++++
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
index 57abe091b0..b91e98074e 100644
--- a/hw/i386/kvm/i8259.c
+++ b/hw/i386/kvm/i8259.c
@@ -140,15 +140,12 @@ static void kvm_i8259_class_init(ObjectClass *klass, void *data)
     KVMPICClass *kpc = KVM_PIC_CLASS(klass);
     PICCommonClass *k = PIC_COMMON_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
-    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
 
     dc->reset     = kvm_pic_reset;
     kpc->parent_realize = dc->realize;
     dc->realize   = kvm_pic_realize;
     k->pre_save   = kvm_pic_get;
     k->post_load  = kvm_pic_put;
-    ic->get_statistics = pic_get_statistics;
-    ic->print_info = pic_print_info;
 }
 
 static const TypeInfo kvm_i8259_info = {
@@ -157,10 +154,6 @@ static const TypeInfo kvm_i8259_info = {
     .instance_size = sizeof(PICCommonState),
     .class_init = kvm_i8259_class_init,
     .class_size = sizeof(KVMPICClass),
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_INTERRUPT_STATS_PROVIDER },
-        { }
-    },
 };
 
 static void kvm_pic_register_types(void)
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index d9b9666aff..1602255a87 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -442,13 +442,10 @@ 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 = {
@@ -457,10 +454,6 @@ 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)
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index 7efd2e8012..c75c880157 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -178,6 +178,7 @@ static Property pic_properties_common[] = {
 static void pic_common_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
 
     dc->vmsd = &vmstate_pic_common;
     dc->props = pic_properties_common;
@@ -189,6 +190,8 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
      * code.
      */
     dc->user_creatable = false;
+    ic->get_statistics = pic_get_statistics;
+    ic->print_info = pic_print_info;
 }
 
 static const TypeInfo pic_common_type = {
@@ -198,6 +201,10 @@ static const TypeInfo pic_common_type = {
     .class_size = sizeof(PICCommonClass),
     .class_init = pic_common_class_init,
     .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_INTERRUPT_STATS_PROVIDER },
+        { }
+    },
 };
 
 static void pic_common_register_types(void)
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/5] i8259: convert DPRINTFs into trace

[Philippe Mathieu-Daudé]

On 12/10/2017 03:38 AM, Peter Xu wrote:
> One thing to mention is that in pic_set_irq() I need to uncomment a few
> lines in the macros to make sure IRQ value calculation is correct.

Yep, this is correct.

> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/intc/i8259.c      | 26 +++++++++++---------------
>  hw/intc/trace-events |  7 +++++++
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index fe9ecd6bd4..f12e0b27f1 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -30,17 +30,11 @@
>  #include "qemu/log.h"
>  #include "hw/isa/i8259_internal.h"
>  #include "hw/intc/intc.h"
> +#include "trace.h"
>  
>  /* debug PIC */
>  //#define DEBUG_PIC
>  
> -#ifdef DEBUG_PIC
> -#define DPRINTF(fmt, ...)                                       \
> -    do { printf("pic: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...)
> -#endif
> -
>  //#define DEBUG_IRQ_LATENCY
>  //#define DEBUG_IRQ_COUNT
>  
> @@ -122,8 +116,7 @@ static void pic_update_irq(PICCommonState *s)
>  
>      irq = pic_get_irq(s);
>      if (irq >= 0) {
> -        DPRINTF("pic%d: imr=%x irr=%x padd=%d\n",
> -                s->master ? 0 : 1, s->imr, s->irr, s->priority_add);
> +        trace_pic_update_irq(s->master, s->imr, s->irr, s->priority_add);
>          qemu_irq_raise(s->int_out[0]);
>      } else {
>          qemu_irq_lower(s->int_out[0]);
> @@ -140,9 +133,11 @@ static void pic_set_irq(void *opaque, int irq, int level)
>      defined(DEBUG_IRQ_LATENCY)
>      int irq_index = s->master ? irq : irq + 8;
>  #endif
> +
> +    trace_pic_set_irq(s->master, irq, level);
> +
>  #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
>      if (level != irq_level[irq_index]) {
> -        DPRINTF("pic_set_irq: irq=%d level=%d\n", irq_index, level);
>          irq_level[irq_index] = level;
>  #ifdef DEBUG_IRQ_COUNT
>          if (level == 1) {
> @@ -223,18 +218,18 @@ int pic_read_irq(DeviceState *d)
>          intno = s->irq_base + irq;
>      }
>  
> -#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_LATENCY)
>      if (irq == 2) {
>          irq = irq2 + 8;
>      }
> -#endif
> +
>  #ifdef DEBUG_IRQ_LATENCY
>      printf("IRQ%d latency=%0.3fus\n",
>             irq,
>             (double)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
>                      irq_time[irq]) * 1000000.0 / NANOSECONDS_PER_SECOND);
>  #endif
> -    DPRINTF("pic_interrupt: irq=%d\n", irq);
> +
> +    trace_pic_interrupt(irq, intno);
>      return intno;
>  }
>  
> @@ -289,7 +284,8 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>      uint32_t val = val64;
>      int priority, cmd, irq;
>  
> -    DPRINTF("write: addr=0x%02x val=0x%02x\n", addr, val);
> +    trace_pic_ioport_write(s->master, addr, val);
> +
>      if (addr == 0) {
>          if (val & 0x10) {
>              pic_init_reset(s);
> @@ -402,7 +398,7 @@ static uint64_t pic_ioport_read(void *opaque, hwaddr addr,
>              ret = s->imr;
>          }
>      }
> -    DPRINTF("read: addr=0x%02" HWADDR_PRIx " val=0x%02x\n", addr, ret);
> +    trace_pic_ioport_read(s->master, addr, ret);
>      return ret;
>  }
>  
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index b298fac7c6..c72b37c5cf 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -1,5 +1,12 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>  
> +# hw/intc/i8259.c
> +pic_update_irq(bool master, uint8_t imr, uint8_t irr, uint8_t padd) "master %d imr %"PRIu8" irr %"PRIu8" padd %"PRIu8
> +pic_set_irq(bool master, int irq, int level) "master %d irq %d level %d"
> +pic_interrupt(int irq, int intno) "irq %d intno %d"
> +pic_ioport_write(bool master, uint64_t addr, uint64_t val) "master %d addr 0x%"PRIx64" val 0x%"PRIx64
> +pic_ioport_read(bool master, uint64_t addr, int val) "master %d addr 0x%"PRIx64" val 0x%x"
> +
>  # hw/intc/apic_common.c
>  cpu_set_apic_base(uint64_t val) "0x%016"PRIx64
>  cpu_get_apic_base(uint64_t val) "0x%016"PRIx64
> 

Re: [Qemu-devel] [PATCH 2/5] i8259: use DEBUG_IRQ_COUNT always

[Philippe Mathieu-Daudé]

On 12/10/2017 03:38 AM, Peter Xu wrote:
> It's not really scary to even enable it forever.  After all it's i8259,
> and it's even not the kernel one.
> 
> Then we can remove quite a few of lines to make it cleaner.  And "info
> irq" will always work for it.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/intc/i8259.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index f12e0b27f1..20c9d0a58b 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -36,7 +36,6 @@
>  //#define DEBUG_PIC
>  
>  //#define DEBUG_IRQ_LATENCY
> -//#define DEBUG_IRQ_COUNT
>  
>  #define TYPE_I8259 "isa-i8259"
>  #define PIC_CLASS(class) OBJECT_CLASS_CHECK(PICClass, (class), TYPE_I8259)
> @@ -52,12 +51,8 @@ typedef struct PICClass {
>      DeviceRealize parent_realize;
>  } PICClass;
>  
> -#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
>  static int irq_level[16];
> -#endif
> -#ifdef DEBUG_IRQ_COUNT
>  static uint64_t irq_count[16];
> -#endif

I'll be so happy once this device get QOMified...

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  #ifdef DEBUG_IRQ_LATENCY
>  static int64_t irq_time[16];
>  #endif
> @@ -128,24 +123,17 @@ static void pic_set_irq(void *opaque, int irq, int level)
>  {
>      PICCommonState *s = opaque;
>      int mask = 1 << irq;
> -
> -#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT) || \
> -    defined(DEBUG_IRQ_LATENCY)
>      int irq_index = s->master ? irq : irq + 8;
> -#endif
>  
>      trace_pic_set_irq(s->master, irq, level);
>  
> -#if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
>      if (level != irq_level[irq_index]) {
>          irq_level[irq_index] = level;
> -#ifdef DEBUG_IRQ_COUNT
>          if (level == 1) {
>              irq_count[irq_index]++;
>          }
> -#endif
>      }
> -#endif
> +
>  #ifdef DEBUG_IRQ_LATENCY
>      if (level) {
>          irq_time[irq_index] = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -253,12 +241,8 @@ static bool pic_get_statistics(InterruptStatsProvider *obj,
>      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;
> 

Re: [Qemu-devel] [PATCH 3/5] i8259: generalize statistics into common code

[Philippe Mathieu-Daudé]

On 12/10/2017 03:38 AM, Peter Xu wrote:
> It was only for userspace i8259.  Move it to general code so that
> kvm-i8259 can also use it in the future.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/intc/i8259.c                 | 37 +------------------------------------
>  hw/intc/i8259_common.c          | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/isa/i8259_internal.h |  7 +++++--
>  3 files changed, 47 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 20c9d0a58b..d9b9666aff 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -25,11 +25,9 @@
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/isa.h"
> -#include "monitor/monitor.h"
>  #include "qemu/timer.h"
>  #include "qemu/log.h"
>  #include "hw/isa/i8259_internal.h"
> -#include "hw/intc/intc.h"
>  #include "trace.h"
>  
>  /* debug PIC */
> @@ -51,8 +49,6 @@ typedef struct PICClass {
>      DeviceRealize parent_realize;
>  } PICClass;
>  
> -static int irq_level[16];
> -static uint64_t irq_count[16];
>  #ifdef DEBUG_IRQ_LATENCY
>  static int64_t irq_time[16];
>  #endif
> @@ -126,13 +122,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>      int irq_index = s->master ? irq : irq + 8;
>  
>      trace_pic_set_irq(s->master, irq, level);
> -
> -    if (level != irq_level[irq_index]) {
> -        irq_level[irq_index] = level;
> -        if (level == 1) {
> -            irq_count[irq_index]++;
> -        }
> -    }
> +    pic_stat_update_irq(irq_index, level);
>  
>  #ifdef DEBUG_IRQ_LATENCY
>      if (level) {
> @@ -235,31 +225,6 @@ 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) {
> -        *irq_counts = irq_count;
> -        *nb_irqs = ARRAY_SIZE(irq_count);
> -    } 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)
>  {
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index 18427b459a..a3caddeefb 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -25,6 +25,10 @@
>  #include "qemu/osdep.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/i8259_internal.h"
> +#include "monitor/monitor.h"
> +
> +static int irq_level[16];
> +static uint64_t irq_count[16];
>  
>  void pic_reset_common(PICCommonState *s)
>  {
> @@ -98,6 +102,43 @@ ISADevice *i8259_init_chip(const char *name, ISABus *bus, bool master)
>      return isadev;
>  }
>  
> +void pic_stat_update_irq(int irq, int level)
> +{
> +    if (level != irq_level[irq]) {
> +        irq_level[irq] = level;
> +        if (level == 1) {
> +            irq_count[irq]++;
> +        }
> +    }
> +}
> +
> +bool pic_get_statistics(InterruptStatsProvider *obj,
> +                        uint64_t **irq_counts, unsigned int *nb_irqs)
> +{
> +    PICCommonState *s = PIC_COMMON(obj);
> +
> +    if (s->master) {
> +        *irq_counts = irq_count;
> +        *nb_irqs = ARRAY_SIZE(irq_count);
> +    } else {
> +        *irq_counts = NULL;
> +        *nb_irqs = 0;
> +    }
> +
> +    return true;
> +}
> +
> +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 const VMStateDescription vmstate_pic_common = {
>      .name = "i8259",
>      .version_id = 1,
> diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
> index 6954b6ec5f..f742c2a726 100644
> --- a/include/hw/isa/i8259_internal.h
> +++ b/include/hw/isa/i8259_internal.h
> @@ -28,6 +28,7 @@
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/isa.h"
> +#include "hw/intc/intc.h"
>  
>  typedef struct PICCommonState PICCommonState;
>  
> @@ -76,8 +77,10 @@ struct PICCommonState {
>  };
>  
>  void pic_reset_common(PICCommonState *s);
> -
>  ISADevice *i8259_init_chip(const char *name, ISABus *bus, bool master);
> -
> +void pic_stat_update_irq(int irq, int level);
> +bool pic_get_statistics(InterruptStatsProvider *obj,
> +                        uint64_t **irq_counts, unsigned int *nb_irqs);
> +void pic_print_info(InterruptStatsProvider *obj, Monitor *mon);

can you rename pic -> i8259?

i8259_get_statistics() or i8259pic_get_statistics()

>  #endif /* QEMU_I8259_INTERNAL_H */
> 

Re: [Qemu-devel] [PATCH 4/5] kvm-i8259: support "info pic" and "info irq"

[Philippe Mathieu-Daudé]

On 12/10/2017 03:38 AM, Peter Xu wrote:
> Let's leverage the i8259 common code for kvm-i8259 too.
> 
> I think it's still possible that stats can lost when i8259 is in kernel
> and meanwhile when irqfd is used, e.g., by vfio or vhost devices.
> However that should be rare IMHO since they should be using MSIs mostly
> if they really want performance (that's why people use vhost and device
> assignment), and no old INTx should be used.  As long as the INTx users
> are emulated in QEMU the stats will be correct.
> 
> For "info pic", it should be always accurate since we fetch kvm regs
> before dump.
> 
> More importantly, it's just too simple to do this now - it's only 10+
> LOC to gain this feature.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/i386/kvm/i8259.c    | 8 ++++++++
>  hw/intc/i8259_common.c | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
> index 11d1b726b6..57abe091b0 100644
> --- a/hw/i386/kvm/i8259.c
> +++ b/hw/i386/kvm/i8259.c
> @@ -111,6 +111,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level)
>  {
>      int delivered;
>  
> +    pic_stat_update_irq(irq, level);
>      delivered = kvm_set_irq(kvm_state, irq, level);
>      apic_report_irq_delivered(delivered);
>  }
> @@ -139,12 +140,15 @@ static void kvm_i8259_class_init(ObjectClass *klass, void *data)
>      KVMPICClass *kpc = KVM_PIC_CLASS(klass);
>      PICCommonClass *k = PIC_COMMON_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
>  
>      dc->reset     = kvm_pic_reset;
>      kpc->parent_realize = dc->realize;
>      dc->realize   = kvm_pic_realize;
>      k->pre_save   = kvm_pic_get;
>      k->post_load  = kvm_pic_put;
> +    ic->get_statistics = pic_get_statistics;
> +    ic->print_info = pic_print_info;
>  }
>  
>  static const TypeInfo kvm_i8259_info = {
> @@ -153,6 +157,10 @@ static const TypeInfo kvm_i8259_info = {
>      .instance_size = sizeof(PICCommonState),
>      .class_init = kvm_i8259_class_init,
>      .class_size = sizeof(KVMPICClass),
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> +        { }
> +    },
>  };
>  
>  static void kvm_pic_register_types(void)
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index a3caddeefb..7efd2e8012 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -132,6 +132,7 @@ void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
>  {
>      PICCommonState *s = PIC_COMMON(obj);
>  
> +    pic_dispatch_pre_save(s);
>      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,
> 

Re: [Qemu-devel] [PATCH 5/5] i8259: move TYPE_INTERRUPT_STATS_PROVIDER upper

[Philippe Mathieu-Daudé]

On 12/10/2017 03:38 AM, Peter Xu wrote:
> Now both classes (i8259, i8259-kvm) support this.  Move this upper to
> the common class code.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/i386/kvm/i8259.c    | 7 -------
>  hw/intc/i8259.c        | 7 -------
>  hw/intc/i8259_common.c | 7 +++++++
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/i386/kvm/i8259.c b/hw/i386/kvm/i8259.c
> index 57abe091b0..b91e98074e 100644
> --- a/hw/i386/kvm/i8259.c
> +++ b/hw/i386/kvm/i8259.c
> @@ -140,15 +140,12 @@ static void kvm_i8259_class_init(ObjectClass *klass, void *data)
>      KVMPICClass *kpc = KVM_PIC_CLASS(klass);
>      PICCommonClass *k = PIC_COMMON_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
>  
>      dc->reset     = kvm_pic_reset;
>      kpc->parent_realize = dc->realize;
>      dc->realize   = kvm_pic_realize;
>      k->pre_save   = kvm_pic_get;
>      k->post_load  = kvm_pic_put;
> -    ic->get_statistics = pic_get_statistics;
> -    ic->print_info = pic_print_info;
>  }
>  
>  static const TypeInfo kvm_i8259_info = {
> @@ -157,10 +154,6 @@ static const TypeInfo kvm_i8259_info = {
>      .instance_size = sizeof(PICCommonState),
>      .class_init = kvm_i8259_class_init,
>      .class_size = sizeof(KVMPICClass),
> -    .interfaces = (InterfaceInfo[]) {
> -        { TYPE_INTERRUPT_STATS_PROVIDER },
> -        { }
> -    },
>  };
>  
>  static void kvm_pic_register_types(void)
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index d9b9666aff..1602255a87 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -442,13 +442,10 @@ 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 = {
> @@ -457,10 +454,6 @@ 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)
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index 7efd2e8012..c75c880157 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -178,6 +178,7 @@ static Property pic_properties_common[] = {
>  static void pic_common_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(klass);
>  
>      dc->vmsd = &vmstate_pic_common;
>      dc->props = pic_properties_common;
> @@ -189,6 +190,8 @@ static void pic_common_class_init(ObjectClass *klass, void *data)
>       * code.
>       */
>      dc->user_creatable = false;
> +    ic->get_statistics = pic_get_statistics;
> +    ic->print_info = pic_print_info;
>  }
>  
>  static const TypeInfo pic_common_type = {
> @@ -198,6 +201,10 @@ static const TypeInfo pic_common_type = {
>      .class_size = sizeof(PICCommonClass),
>      .class_init = pic_common_class_init,
>      .abstract = true,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> +        { }
> +    },
>  };
>  
>  static void pic_common_register_types(void)
> 

Re: [Qemu-devel] [PATCH 3/5] i8259: generalize statistics into common code

[Peter Xu]

On Fri, Dec 15, 2017 at 08:28:07AM -0300, Philippe Mathieu-Daudé wrote:

[...]

> > diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
> > index 6954b6ec5f..f742c2a726 100644
> > --- a/include/hw/isa/i8259_internal.h
> > +++ b/include/hw/isa/i8259_internal.h
> > @@ -28,6 +28,7 @@
> >  #include "hw/hw.h"
> >  #include "hw/i386/pc.h"
> >  #include "hw/isa/isa.h"
> > +#include "hw/intc/intc.h"
> >  
> >  typedef struct PICCommonState PICCommonState;
> >  
> > @@ -76,8 +77,10 @@ struct PICCommonState {
> >  };
> >  
> >  void pic_reset_common(PICCommonState *s);
> > -
> >  ISADevice *i8259_init_chip(const char *name, ISABus *bus, bool master);
> > -
> > +void pic_stat_update_irq(int irq, int level);
> > +bool pic_get_statistics(InterruptStatsProvider *obj,
> > +                        uint64_t **irq_counts, unsigned int *nb_irqs);
> > +void pic_print_info(InterruptStatsProvider *obj, Monitor *mon);
> 
> can you rename pic -> i8259?
> 
> i8259_get_statistics() or i8259pic_get_statistics()

Yes I can.

But AFAICT it does not really matter now since at least we are still
mostly using pic_* in intc/i8259.c and kvm_pic_* in kvm/i8259.c for
namings.  So IMHO it's even more consistent to still use pci_* prefix
for them.

We can have another single patch to convert all of them to i8259
prefix if you wish to, as a work upon.

Just let me know if you still insist. :-)

Thanks for reviewing!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/5] i8259: cleanups and enhancements

[Paolo Bonzini]

On 10/12/2017 07:38, Peter Xu wrote:
> It's mostly a cleanup, but patch 4 allows kvm-i8259 to support "info
> pic" and "info irq" too.
> 
> I'm thinking whether it'll be good to move on this work to spread
> these commands to IOAPICs too, by removing "info ioapic" command and
> let "info pic" dump the things altogether (after all I think ioapic is
> also one type of PIC).  Also, we can let ioapic to have statistics
> too.

Yes, I agree.  I queued this series.

Paolo