[Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[xiaoqiang zhao]

This series of patch QOM'ify apic and ioapic.
Just replace old 'init' with QOM's 'realize', the call
logic is untouched.

the first patch of each is a cleanup. the second patch
complete the convertion. 
All patch have been compiled and tested successfully.

Changes since v2:
- fix code style check errors

Changes since v1:
- separate cleanup patch from the rest
- add & change some code comment
- QOM'ify icc_bus for consistency

xiaoqiang zhao (4):
  apic: Cleanup for QOM'ify
  apic: QOM'ify apic & icc_bus
  ioapic: Cleanup for QOM'ify
  ioapic: QOM'ify ioapic

 hw/cpu/icc_bus.c                  |   14 +++----
 hw/i386/kvm/apic.c                |   18 +++++----
 hw/i386/kvm/ioapic.c              |   15 +++++---
 hw/intc/apic.c                    |   52 ++++++++++++++------------
 hw/intc/apic_common.c             |   73 +++++++++++++++++++------------------
 hw/intc/ioapic.c                  |   21 ++++++++---
 hw/intc/ioapic_common.c           |   17 ++++++---
 include/hw/cpu/icc_bus.h          |    3 +-
 include/hw/i386/apic_internal.h   |    3 +-
 include/hw/i386/ioapic_internal.h |    3 +-
 10 files changed, 125 insertions(+), 94 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v3 1/4] apic: Cleanup for QOM'ify

[xiaoqiang zhao]

do some cleanup, includes:
1. remove DO_UPCAST() for APICCommonState
2. Change DeviceState pointers from 'd' to 'dev', better to understand
3. rename 'register_types' to specifically 'apic_common_register_types'

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/i386/kvm/apic.c    |    8 +++----
 hw/intc/apic.c        |   42 +++++++++++++++++-----------------
 hw/intc/apic_common.c |   60 ++++++++++++++++++++++++-------------------------
 3 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 5609063..84f6056 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -25,9 +25,9 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic,
     return *((uint32_t *)(kapic->regs + (reg_id << 4)));
 }
 
-void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
+void kvm_put_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     int i;
 
     memset(kapic, 0, sizeof(*kapic));
@@ -51,9 +51,9 @@ void kvm_put_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
     kvm_apic_set_reg(kapic, 0x3e, s->divide_conf);
 }
 
-void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic)
+void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     int i, v;
 
     s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index a913186..b542628 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -171,9 +171,9 @@ static void apic_local_deliver(APICCommonState *s, int vector)
     }
 }
 
-void apic_deliver_pic_intr(DeviceState *d, int level)
+void apic_deliver_pic_intr(DeviceState *dev, int level)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
 
     if (level) {
         apic_local_deliver(s, APIC_LVT_LINT0);
@@ -376,9 +376,9 @@ static void apic_update_irq(APICCommonState *s)
     }
 }
 
-void apic_poll_irq(DeviceState *d)
+void apic_poll_irq(DeviceState *dev)
 {
-    APICCommonState *s = APIC_COMMON(d);
+    APICCommonState *s = APIC_COMMON(dev);
 
     apic_sync_vapic(s, SYNC_FROM_VAPIC);
     apic_update_irq(s);
@@ -482,9 +482,9 @@ static void apic_startup(APICCommonState *s, int vector_num)
     cpu_interrupt(CPU(s->cpu), CPU_INTERRUPT_SIPI);
 }
 
-void apic_sipi(DeviceState *d)
+void apic_sipi(DeviceState *dev)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
 
     cpu_reset_interrupt(CPU(s->cpu), CPU_INTERRUPT_SIPI);
 
@@ -494,11 +494,11 @@ void apic_sipi(DeviceState *d)
     s->wait_for_sipi = 0;
 }
 
-static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
+static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
                          uint8_t delivery_mode, uint8_t vector_num,
                          uint8_t trigger_mode)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     uint32_t deliver_bitmask[MAX_APIC_WORDS];
     int dest_shorthand = (s->icr[0] >> 18) & 3;
     APICCommonState *apic_iter;
@@ -551,9 +551,9 @@ static bool apic_check_pic(APICCommonState *s)
     return true;
 }
 
-int apic_get_interrupt(DeviceState *d)
+int apic_get_interrupt(DeviceState *dev)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     int intno;
 
     /* if the APIC is installed or enabled, we let the 8259 handle the
@@ -585,9 +585,9 @@ int apic_get_interrupt(DeviceState *d)
     return intno;
 }
 
-int apic_accept_pic_intr(DeviceState *d)
+int apic_accept_pic_intr(DeviceState *dev)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     uint32_t lvt0;
 
     if (!s)
@@ -657,16 +657,16 @@ static void apic_mem_writew(void *opaque, hwaddr addr, uint32_t val)
 
 static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
 {
-    DeviceState *d;
+    DeviceState *dev;
     APICCommonState *s;
     uint32_t val;
     int index;
 
-    d = cpu_get_current_apic();
-    if (!d) {
+    dev = cpu_get_current_apic();
+    if (!dev) {
         return 0;
     }
-    s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    s = APIC_COMMON(dev);
 
     index = (addr >> 4) & 0xff;
     switch(index) {
@@ -752,7 +752,7 @@ static void apic_send_msi(hwaddr addr, uint32_t data)
 
 static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
 {
-    DeviceState *d;
+    DeviceState *dev;
     APICCommonState *s;
     int index = (addr >> 4) & 0xff;
     if (addr > 0xfff || !index) {
@@ -765,11 +765,11 @@ static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
         return;
     }
 
-    d = cpu_get_current_apic();
-    if (!d) {
+    dev = cpu_get_current_apic();
+    if (!dev) {
         return;
     }
-    s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    s = APIC_COMMON(dev);
 
     trace_apic_mem_writel(addr, val);
 
@@ -810,7 +810,7 @@ static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
         break;
     case 0x30:
         s->icr[0] = val;
-        apic_deliver(d, (s->icr[1] >> 24) & 0xff, (s->icr[0] >> 11) & 1,
+        apic_deliver(dev, (s->icr[1] >> 24) & 0xff, (s->icr[0] >> 11) & 1,
                      (s->icr[0] >> 8) & 7, (s->icr[0] & 0xff),
                      (s->icr[0] >> 15) & 1);
         break;
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index a0beb10..f3edf48 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -27,21 +27,21 @@
 static int apic_irq_delivered;
 bool apic_report_tpr_access;
 
-void cpu_set_apic_base(DeviceState *d, uint64_t val)
+void cpu_set_apic_base(DeviceState *dev, uint64_t val)
 {
     trace_cpu_set_apic_base(val);
 
-    if (d) {
-        APICCommonState *s = APIC_COMMON(d);
+    if (dev) {
+        APICCommonState *s = APIC_COMMON(dev);
         APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
         info->set_base(s, val);
     }
 }
 
-uint64_t cpu_get_apic_base(DeviceState *d)
+uint64_t cpu_get_apic_base(DeviceState *dev)
 {
-    if (d) {
-        APICCommonState *s = APIC_COMMON(d);
+    if (dev) {
+        APICCommonState *s = APIC_COMMON(dev);
         trace_cpu_get_apic_base((uint64_t)s->apicbase);
         return s->apicbase;
     } else {
@@ -50,39 +50,39 @@ uint64_t cpu_get_apic_base(DeviceState *d)
     }
 }
 
-void cpu_set_apic_tpr(DeviceState *d, uint8_t val)
+void cpu_set_apic_tpr(DeviceState *dev, uint8_t val)
 {
     APICCommonState *s;
     APICCommonClass *info;
 
-    if (!d) {
+    if (!dev) {
         return;
     }
 
-    s = APIC_COMMON(d);
+    s = APIC_COMMON(dev);
     info = APIC_COMMON_GET_CLASS(s);
 
     info->set_tpr(s, val);
 }
 
-uint8_t cpu_get_apic_tpr(DeviceState *d)
+uint8_t cpu_get_apic_tpr(DeviceState *dev)
 {
     APICCommonState *s;
     APICCommonClass *info;
 
-    if (!d) {
+    if (!dev) {
         return 0;
     }
 
-    s = APIC_COMMON(d);
+    s = APIC_COMMON(dev);
     info = APIC_COMMON_GET_CLASS(s);
 
     return info->get_tpr(s);
 }
 
-void apic_enable_tpr_access_reporting(DeviceState *d, bool enable)
+void apic_enable_tpr_access_reporting(DeviceState *dev, bool enable)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     apic_report_tpr_access = enable;
@@ -91,19 +91,19 @@ void apic_enable_tpr_access_reporting(DeviceState *d, bool enable)
     }
 }
 
-void apic_enable_vapic(DeviceState *d, hwaddr paddr)
+void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     s->vapic_paddr = paddr;
     info->vapic_base_update(s);
 }
 
-void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
+void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,
                                    TPRAccess access)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
 
     vapic_report_tpr_access(s->vapic, CPU(s->cpu), ip, access);
 }
@@ -129,9 +129,9 @@ int apic_get_irq_delivered(void)
     return apic_irq_delivered;
 }
 
-void apic_deliver_nmi(DeviceState *d)
+void apic_deliver_nmi(DeviceState *dev)
 {
-    APICCommonState *s = APIC_COMMON(d);
+    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
 
     info->external_nmi(s);
@@ -170,9 +170,9 @@ bool apic_next_timer(APICCommonState *s, int64_t current_time)
     return true;
 }
 
-void apic_init_reset(DeviceState *d)
+void apic_init_reset(DeviceState *dev)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     int i;
 
     if (!s) {
@@ -203,19 +203,19 @@ void apic_init_reset(DeviceState *d)
     s->timer_expiry = -1;
 }
 
-void apic_designate_bsp(DeviceState *d)
+void apic_designate_bsp(DeviceState *dev)
 {
-    if (d == NULL) {
+    if (dev == NULL) {
         return;
     }
 
-    APICCommonState *s = APIC_COMMON(d);
+    APICCommonState *s = APIC_COMMON(dev);
     s->apicbase |= MSR_IA32_APICBASE_BSP;
 }
 
-static void apic_reset_common(DeviceState *d)
+static void apic_reset_common(DeviceState *dev)
 {
-    APICCommonState *s = DO_UPCAST(APICCommonState, busdev.qdev, d);
+    APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
     bool bsp;
 
@@ -226,7 +226,7 @@ static void apic_reset_common(DeviceState *d)
     s->vapic_paddr = 0;
     info->vapic_base_update(s);
 
-    apic_init_reset(d);
+    apic_init_reset(dev);
 
     if (bsp) {
         /*
@@ -400,9 +400,9 @@ static const TypeInfo apic_common_type = {
     .abstract = true,
 };
 
-static void register_types(void)
+static void apic_common_register_types(void)
 {
     type_register_static(&apic_common_type);
 }
 
-type_init(register_types)
+type_init(apic_common_register_types)
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 2/4] apic: QOM'ify apic & icc_bus

[xiaoqiang zhao]

changes includes:
1. use type constant for apic and kvm_apic
2. convert function 'init' to QOM's 'realize' for apic/kvm_apic
3. for consistency, also QOM'ify apic's parent bus 'icc_bus'

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/cpu/icc_bus.c                |   14 ++++++--------
 hw/i386/kvm/apic.c              |   10 +++++++---
 hw/intc/apic.c                  |   10 +++++++---
 hw/intc/apic_common.c           |   13 +++++++------
 include/hw/cpu/icc_bus.h        |    3 ++-
 include/hw/i386/apic_internal.h |    3 ++-
 6 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 9a4ea7e..7f44c59 100644
--- a/hw/cpu/icc_bus.c
+++ b/hw/cpu/icc_bus.c
@@ -43,15 +43,13 @@ static const TypeInfo icc_bus_info = {
 
 static void icc_device_realize(DeviceState *dev, Error **errp)
 {
-    ICCDevice *id = ICC_DEVICE(dev);
-    ICCDeviceClass *idc = ICC_DEVICE_GET_CLASS(id);
-
-    if (idc->init) {
-        if (idc->init(id) < 0) {
-            error_setg(errp, "%s initialization failed.",
-                       object_get_typename(OBJECT(dev)));
-        }
+    ICCDeviceClass *idc = ICC_DEVICE_GET_CLASS(dev);
+
+    /* convert to QOM */
+    if (idc->realize) {
+        idc->realize(dev, errp);
     }
+
 }
 
 static void icc_device_class_init(ObjectClass *oc, void *data)
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 84f6056..55f4a53 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -13,6 +13,8 @@
 #include "hw/pci/msi.h"
 #include "sysemu/kvm.h"
 
+#define TYPE_KVM_APIC "kvm-apic"
+
 static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
                                     int reg_id, uint32_t val)
 {
@@ -171,8 +173,10 @@ static const MemoryRegionOps kvm_apic_io_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void kvm_apic_init(APICCommonState *s)
+static void kvm_apic_realize(DeviceState *dev, Error **errp)
 {
+    APICCommonState *s = APIC_COMMON(dev);
+
     memory_region_init_io(&s->io_memory, NULL, &kvm_apic_io_ops, s, "kvm-apic-msi",
                           APIC_SPACE_SIZE);
 
@@ -185,7 +189,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
-    k->init = kvm_apic_init;
+    k->realize = kvm_apic_realize;
     k->set_base = kvm_apic_set_base;
     k->set_tpr = kvm_apic_set_tpr;
     k->get_tpr = kvm_apic_get_tpr;
@@ -195,7 +199,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo kvm_apic_info = {
-    .name = "kvm-apic",
+    .name = TYPE_KVM_APIC,
     .parent = TYPE_APIC_COMMON,
     .instance_size = sizeof(APICCommonState),
     .class_init = kvm_apic_class_init,
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index b542628..2d7891d 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -32,6 +32,8 @@
 #define SYNC_TO_VAPIC                   0x2
 #define SYNC_ISR_IRR_TO_VAPIC           0x4
 
+#define TYPE_APIC "apic"
+
 static APICCommonState *local_apics[MAX_APICS + 1];
 
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode);
@@ -871,8 +873,10 @@ static const MemoryRegionOps apic_io_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void apic_init(APICCommonState *s)
+static void apic_realize(DeviceState *dev, Error **errp)
 {
+    APICCommonState *s = APIC_COMMON(dev);
+
     memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
                           APIC_SPACE_SIZE);
 
@@ -886,7 +890,7 @@ static void apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
 
-    k->init = apic_init;
+    k->realize = apic_realize;
     k->set_base = apic_set_base;
     k->set_tpr = apic_set_tpr;
     k->get_tpr = apic_get_tpr;
@@ -897,7 +901,7 @@ static void apic_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo apic_info = {
-    .name          = "apic",
+    .name          = TYPE_APIC,
     .instance_size = sizeof(APICCommonState),
     .parent        = TYPE_APIC_COMMON,
     .class_init    = apic_class_init,
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index f3edf48..2ab1e12 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -284,7 +284,7 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static int apic_init_common(ICCDevice *dev)
+static void apic_common_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
     APICCommonClass *info;
@@ -293,14 +293,16 @@ static int apic_init_common(ICCDevice *dev)
     static bool mmio_registered;
 
     if (apic_no >= MAX_APICS) {
-        return -1;
+        error_setg(errp, "%s initialization failed.",
+                   object_get_typename(OBJECT(dev)));
+        return;
     }
     s->idx = apic_no++;
 
     info = APIC_COMMON_GET_CLASS(s);
-    info->init(s);
+    info->realize(dev, errp);
     if (!mmio_registered) {
-        ICCBus *b = ICC_BUS(qdev_get_parent_bus(DEVICE(dev)));
+        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
         memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
         mmio_registered = true;
     }
@@ -315,7 +317,6 @@ static int apic_init_common(ICCDevice *dev)
         info->enable_tpr_reporting(s, true);
     }
 
-    return 0;
 }
 
 static void apic_dispatch_pre_save(void *opaque)
@@ -388,7 +389,7 @@ static void apic_common_class_init(ObjectClass *klass, void *data)
     dc->reset = apic_reset_common;
     dc->no_user = 1;
     dc->props = apic_properties_common;
-    idc->init = apic_init_common;
+    idc->realize = apic_common_realize;
 }
 
 static const TypeInfo apic_common_type = {
diff --git a/include/hw/cpu/icc_bus.h b/include/hw/cpu/icc_bus.h
index b550070..b32a549 100644
--- a/include/hw/cpu/icc_bus.h
+++ b/include/hw/cpu/icc_bus.h
@@ -66,7 +66,8 @@ typedef struct ICCDeviceClass {
     DeviceClass parent_class;
     /*< public >*/
 
-    int (*init)(ICCDevice *dev); /* TODO replace with QOM realize */
+    /* QOM realize */
+    DeviceRealize realize;
 } ICCDeviceClass;
 
 #define TYPE_ICC_DEVICE "icc-device"
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 1b0a7fb..0cbe26b 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -80,7 +80,8 @@ typedef struct APICCommonClass
 {
     ICCDeviceClass parent_class;
 
-    void (*init)(APICCommonState *s);
+    /* QOM realize */
+    DeviceRealize realize;
     void (*set_base)(APICCommonState *s, uint64_t val);
     void (*set_tpr)(APICCommonState *s, uint8_t val);
     uint8_t (*get_tpr)(APICCommonState *s);
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 3/4] ioapic: Cleanup for QOM'ify

[xiaoqiang zhao]

some cleanup:
1. ioapic_common.c: rename 'register_types' to 'ioapic_common_register_types'
2. change 'DEVICE(s)' to dev conversion by introducing local variable 'DeviceState *dev'

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/i386/kvm/ioapic.c    |    4 +++-
 hw/intc/ioapic.c        |    4 +++-
 hw/intc/ioapic_common.c |    4 ++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index f11a540..772a712 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -129,9 +129,11 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
 
 static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
 {
+    DeviceState *dev = DEVICE(s);
+
     memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
 
-    qdev_init_gpio_in(DEVICE(s), kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
+    qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
 }
 
 static Property kvm_ioapic_properties[] = {
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index d866e00..8842845 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -227,10 +227,12 @@ static const MemoryRegionOps ioapic_io_ops = {
 
 static void ioapic_init(IOAPICCommonState *s, int instance_no)
 {
+    DeviceState *dev = DEVICE(s);
+
     memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
                           "ioapic", 0x1000);
 
-    qdev_init_gpio_in(DEVICE(s), ioapic_set_irq, IOAPIC_NUM_PINS);
+    qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
 
     ioapics[instance_no] = s;
 }
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index 6b705c1..e55c6d1 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -110,9 +110,9 @@ static const TypeInfo ioapic_common_type = {
     .abstract = true,
 };
 
-static void register_types(void)
+static void ioapic_common_register_types(void)
 {
     type_register_static(&ioapic_common_type);
 }
 
-type_init(register_types)
+type_init(ioapic_common_register_types)
-- 
1.7.10.4

[Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[xiaoqiang zhao]

changes:
1. use type constant for kvm_ioapic and ioapic
2. convert 'init' to QOM's 'realize' for ioapic and kvm_ioapic
For QOM'ify, I move variable 'ioapic_no' from static to global.
Then we can drop the 'instance_no' argument. Now, it's child
that increase 'ioapic_no' counter.

Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
---
 hw/i386/kvm/ioapic.c              |   15 +++++++++------
 hw/intc/ioapic.c                  |   19 +++++++++++++------
 hw/intc/ioapic_common.c           |   13 ++++++++++---
 include/hw/i386/ioapic_internal.h |    3 ++-
 4 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 772a712..36f7de2 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -15,6 +15,8 @@
 #include "hw/i386/apic_internal.h"
 #include "sysemu/kvm.h"
 
+#define TYPE_KVM_IOAPIC "kvm-ioapic"
+
 /* PC Utility function */
 void kvm_pc_setup_irq_routing(bool pci_enabled)
 {
@@ -127,12 +129,12 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
     apic_report_irq_delivered(delivered);
 }
 
-static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
+static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *dev = DEVICE(s);
-
-    memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
+    IOAPICCommonState *s = IOAPIC_COMMON(dev);
 
+    memory_region_init_reservation(&s->io_memory, NULL,
+                                   TYPE_KVM_IOAPIC, 0x1000);
     qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
 }
 
@@ -143,10 +145,11 @@ static Property kvm_ioapic_properties[] = {
 
 static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
 {
+
     IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init      = kvm_ioapic_init;
+    k->realize   = kvm_ioapic_realize;
     k->pre_save  = kvm_ioapic_get;
     k->post_load = kvm_ioapic_put;
     dc->reset    = kvm_ioapic_reset;
@@ -154,7 +157,7 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo kvm_ioapic_info = {
-    .name  = "kvm-ioapic",
+    .name  = TYPE_KVM_IOAPIC,
     .parent = TYPE_IOAPIC_COMMON,
     .instance_size = sizeof(KVMIOAPICState),
     .class_init = kvm_ioapic_class_init,
diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 8842845..885f385 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -36,6 +36,10 @@
 
 static IOAPICCommonState *ioapics[MAX_IOAPICS];
 
+#define TYPE_IOAPIC "ioapic"
+/* global variable from ioapic_common.c */
+extern int ioapic_no;
+
 static void ioapic_service(IOAPICCommonState *s)
 {
     uint8_t i;
@@ -225,16 +229,19 @@ static const MemoryRegionOps ioapic_io_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void ioapic_init(IOAPICCommonState *s, int instance_no)
+static void ioapic_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *dev = DEVICE(s);
+    IOAPICCommonState *s = IOAPIC_COMMON(dev);
 
     memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
-                          "ioapic", 0x1000);
+                          TYPE_IOAPIC, 0x1000);
 
     qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
 
-    ioapics[instance_no] = s;
+    ioapics[ioapic_no] = s;
+
+    /* increase the counter */
+    ioapic_no++;
 }
 
 static void ioapic_class_init(ObjectClass *klass, void *data)
@@ -242,12 +249,12 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
     IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = ioapic_init;
+    k->realize = ioapic_realize;
     dc->reset = ioapic_reset_common;
 }
 
 static const TypeInfo ioapic_info = {
-    .name          = "ioapic",
+    .name          = TYPE_IOAPIC,
     .parent        = TYPE_IOAPIC_COMMON,
     .instance_size = sizeof(IOAPICCommonState),
     .class_init    = ioapic_class_init,
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index e55c6d1..aac0402 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -23,6 +23,14 @@
 #include "hw/i386/ioapic_internal.h"
 #include "hw/sysbus.h"
 
+/* ioapic_no count start from 0 to MAX_IOAPICS,
+ * remove as static variable from ioapic_common_init.
+ * now as a global variable, let child to increase the counter
+ * then we can drop the 'instance_no' argument
+ * and convert to our QOM's realize function
+ */
+int ioapic_no;
+
 void ioapic_reset_common(DeviceState *dev)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
@@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
 {
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
     IOAPICCommonClass *info;
-    static int ioapic_no;
 
     if (ioapic_no >= MAX_IOAPICS) {
         error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
@@ -69,10 +76,10 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
     }
 
     info = IOAPIC_COMMON_GET_CLASS(s);
-    info->init(s, ioapic_no);
+    info->realize(dev, errp);
 
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
-    ioapic_no++;
+
 }
 
 static const VMStateDescription vmstate_ioapic_common = {
diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
index 25576c8..cbe4744 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -83,7 +83,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
 
 typedef struct IOAPICCommonClass {
     SysBusDeviceClass parent_class;
-    void (*init)(IOAPICCommonState *s, int instance_no);
+    /* QOM realize */
+    DeviceRealize realize;
     void (*pre_save)(IOAPICCommonState *s);
     void (*post_load)(IOAPICCommonState *s);
 } IOAPICCommonClass;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Andreas Färber]

Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
> xiaoqiang zhao (4):
>   apic: Cleanup for QOM'ify
>   apic: QOM'ify apic & icc_bus
>   ioapic: Cleanup for QOM'ify
>   ioapic: QOM'ify ioapic

Thanks, I have rebased and queued them with changes on qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

In particular I have reverted TYPE_ introductions were unnecessary and
reverted usages of TYPE_ constants for MemoryRegions. Also two white
lines have been dropped. The ICC bus conversion has been split out as a
separate step, using NULL errp as intermediate step since ignored.
(CC'ing Peter C. and Luiz)

However I still have doubts about the last patch and would appreciate
feedback from Paolo, Anthony or anyone, cf. upcoming reply there.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[Andreas Färber]

Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
> changes:
> 1. use type constant for kvm_ioapic and ioapic
> 2. convert 'init' to QOM's 'realize' for ioapic and kvm_ioapic
> For QOM'ify, I move variable 'ioapic_no' from static to global.
> Then we can drop the 'instance_no' argument. Now, it's child
> that increase 'ioapic_no' counter.
> 
> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
> ---
>  hw/i386/kvm/ioapic.c              |   15 +++++++++------
>  hw/intc/ioapic.c                  |   19 +++++++++++++------
>  hw/intc/ioapic_common.c           |   13 ++++++++++---
>  include/hw/i386/ioapic_internal.h |    3 ++-
>  4 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
> index 772a712..36f7de2 100644
> --- a/hw/i386/kvm/ioapic.c
> +++ b/hw/i386/kvm/ioapic.c
> @@ -15,6 +15,8 @@
>  #include "hw/i386/apic_internal.h"
>  #include "sysemu/kvm.h"
>  
> +#define TYPE_KVM_IOAPIC "kvm-ioapic"
> +
>  /* PC Utility function */
>  void kvm_pc_setup_irq_routing(bool pci_enabled)
>  {
> @@ -127,12 +129,12 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
>      apic_report_irq_delivered(delivered);
>  }
>  
> -static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
> +static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(s);
> -
> -    memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>  
> +    memory_region_init_reservation(&s->io_memory, NULL,
> +                                   TYPE_KVM_IOAPIC, 0x1000);
>      qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
>  }
>  
> @@ -143,10 +145,11 @@ static Property kvm_ioapic_properties[] = {
>  
>  static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>  {
> +
>      IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    k->init      = kvm_ioapic_init;
> +    k->realize   = kvm_ioapic_realize;
>      k->pre_save  = kvm_ioapic_get;
>      k->post_load = kvm_ioapic_put;
>      dc->reset    = kvm_ioapic_reset;
> @@ -154,7 +157,7 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo kvm_ioapic_info = {
> -    .name  = "kvm-ioapic",
> +    .name  = TYPE_KVM_IOAPIC,
>      .parent = TYPE_IOAPIC_COMMON,
>      .instance_size = sizeof(KVMIOAPICState),
>      .class_init = kvm_ioapic_class_init,
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 8842845..885f385 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -36,6 +36,10 @@
>  
>  static IOAPICCommonState *ioapics[MAX_IOAPICS];
>  
> +#define TYPE_IOAPIC "ioapic"
> +/* global variable from ioapic_common.c */
> +extern int ioapic_no;
> +
>  static void ioapic_service(IOAPICCommonState *s)
>  {
>      uint8_t i;
> @@ -225,16 +229,19 @@ static const MemoryRegionOps ioapic_io_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static void ioapic_init(IOAPICCommonState *s, int instance_no)
> +static void ioapic_realize(DeviceState *dev, Error **errp)
>  {
> -    DeviceState *dev = DEVICE(s);
> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>  
>      memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
> -                          "ioapic", 0x1000);
> +                          TYPE_IOAPIC, 0x1000);
>  
>      qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
>  
> -    ioapics[instance_no] = s;
> +    ioapics[ioapic_no] = s;
> +
> +    /* increase the counter */
> +    ioapic_no++;

This increment used to happen in common code before, now it's done for
the non-KVM version only ...

>  }
>  
>  static void ioapic_class_init(ObjectClass *klass, void *data)
> @@ -242,12 +249,12 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
>      IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    k->init = ioapic_init;
> +    k->realize = ioapic_realize;
>      dc->reset = ioapic_reset_common;
>  }
>  
>  static const TypeInfo ioapic_info = {
> -    .name          = "ioapic",
> +    .name          = TYPE_IOAPIC,
>      .parent        = TYPE_IOAPIC_COMMON,
>      .instance_size = sizeof(IOAPICCommonState),
>      .class_init    = ioapic_class_init,
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index e55c6d1..aac0402 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -23,6 +23,14 @@
>  #include "hw/i386/ioapic_internal.h"
>  #include "hw/sysbus.h"
>  
> +/* ioapic_no count start from 0 to MAX_IOAPICS,
> + * remove as static variable from ioapic_common_init.
> + * now as a global variable, let child to increase the counter
> + * then we can drop the 'instance_no' argument
> + * and convert to our QOM's realize function
> + */
> +int ioapic_no;
> +
>  void ioapic_reset_common(DeviceState *dev)
>  {
>      IOAPICCommonState *s = IOAPIC_COMMON(dev);
> @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>  {
>      IOAPICCommonState *s = IOAPIC_COMMON(dev);
>      IOAPICCommonClass *info;
> -    static int ioapic_no;
>  
>      if (ioapic_no >= MAX_IOAPICS) {
>          error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);

... while the check for max. IOAPICs still happens in common code.

Do we need to count KVM IOAPICs as well? Or can we consolidate this into
the non-KVM version and keep it static there?

Regards,
Andreas

> @@ -69,10 +76,10 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>      }
>  
>      info = IOAPIC_COMMON_GET_CLASS(s);
> -    info->init(s, ioapic_no);
> +    info->realize(dev, errp);
>  
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
> -    ioapic_no++;
> +
>  }
>  
>  static const VMStateDescription vmstate_ioapic_common = {
> diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
> index 25576c8..cbe4744 100644
> --- a/include/hw/i386/ioapic_internal.h
> +++ b/include/hw/i386/ioapic_internal.h
> @@ -83,7 +83,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
>  
>  typedef struct IOAPICCommonClass {
>      SysBusDeviceClass parent_class;
> -    void (*init)(IOAPICCommonState *s, int instance_no);
> +    /* QOM realize */
> +    DeviceRealize realize;
>      void (*pre_save)(IOAPICCommonState *s);
>      void (*post_load)(IOAPICCommonState *s);
>  } IOAPICCommonClass;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Andreas Färber]

Am 18.12.2013 18:57, schrieb Andreas Färber:
> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
>> xiaoqiang zhao (4):
>>   apic: Cleanup for QOM'ify
>>   apic: QOM'ify apic & icc_bus
>>   ioapic: Cleanup for QOM'ify
>>   ioapic: QOM'ify ioapic
> 
> Thanks, I have rebased and queued them with changes on qom-next:
> https://github.com/afaerber/qemu-cpu/commits/qom-next
> 
> In particular I have reverted TYPE_ introductions were unnecessary and
> reverted usages of TYPE_ constants for MemoryRegions. Also two white
> lines have been dropped. The ICC bus conversion has been split out as a
> separate step, using NULL errp as intermediate step since ignored.
> (CC'ing Peter C. and Luiz)

Oh and you forgot to convert the Xen APIC, I trivially fixed that.
(CC'ing Stefano)

> 
> However I still have doubts about the last patch and would appreciate
> feedback from Paolo, Anthony or anyone, cf. upcoming reply there.
> 
> Regards,
> Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Stefano Stabellini]

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



On Wed, 18 Dec 2013, Andreas Färber wrote:

> Am 18.12.2013 18:57, schrieb Andreas Färber:
> > Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
> >> xiaoqiang zhao (4):
> >>   apic: Cleanup for QOM'ify
> >>   apic: QOM'ify apic & icc_bus
> >>   ioapic: Cleanup for QOM'ify
> >>   ioapic: QOM'ify ioapic
> > 
> > Thanks, I have rebased and queued them with changes on qom-next:
> > https://github.com/afaerber/qemu-cpu/commits/qom-next
> > 
> > In particular I have reverted TYPE_ introductions were unnecessary and
> > reverted usages of TYPE_ constants for MemoryRegions. Also two white
> > lines have been dropped. The ICC bus conversion has been split out as a
> > separate step, using NULL errp as intermediate step since ignored.
> > (CC'ing Peter C. and Luiz)
> 
> Oh and you forgot to convert the Xen APIC, I trivially fixed that.
> (CC'ing Stefano)

Yes, it should be really trivial. Give a look at hw/xen/xen_apic.c.

Re: [Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Andreas Färber]

Am 18.12.2013 20:20, schrieb Stefano Stabellini:
> On Wed, 18 Dec 2013, Andreas Färber wrote:
>> Am 18.12.2013 18:57, schrieb Andreas Färber:
>>> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
>>>> xiaoqiang zhao (4):
>>>>   apic: Cleanup for QOM'ify
>>>>   apic: QOM'ify apic & icc_bus
>>>>   ioapic: Cleanup for QOM'ify
>>>>   ioapic: QOM'ify ioapic
>>>
>>> Thanks, I have rebased and queued them with changes on qom-next:
>>> https://github.com/afaerber/qemu-cpu/commits/qom-next
>>>
>>> In particular I have reverted TYPE_ introductions were unnecessary and
>>> reverted usages of TYPE_ constants for MemoryRegions. Also two white
>>> lines have been dropped. The ICC bus conversion has been split out as a
>>> separate step, using NULL errp as intermediate step since ignored.
>>> (CC'ing Peter C. and Luiz)
>>
>> Oh and you forgot to convert the Xen APIC, I trivially fixed that.
>> (CC'ing Stefano)
> 
> Yes, it should be really trivial. Give a look at hw/xen/xen_apic.c.

https://github.com/afaerber/qemu-cpu/commit/40ae2368c4c9135971c69fa680949b5b1cfa4410#diff-3

Just wanted to give you a heads-up that we're messing in your code. :)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 0/4] QOM'ify apic and ioapic

[Stefano Stabellini]

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

On Wed, 18 Dec 2013, Andreas Färber wrote:
> Am 18.12.2013 20:20, schrieb Stefano Stabellini:
> > On Wed, 18 Dec 2013, Andreas Färber wrote:
> >> Am 18.12.2013 18:57, schrieb Andreas Färber:
> >>> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
> >>>> xiaoqiang zhao (4):
> >>>>   apic: Cleanup for QOM'ify
> >>>>   apic: QOM'ify apic & icc_bus
> >>>>   ioapic: Cleanup for QOM'ify
> >>>>   ioapic: QOM'ify ioapic
> >>>
> >>> Thanks, I have rebased and queued them with changes on qom-next:
> >>> https://github.com/afaerber/qemu-cpu/commits/qom-next
> >>>
> >>> In particular I have reverted TYPE_ introductions were unnecessary and
> >>> reverted usages of TYPE_ constants for MemoryRegions. Also two white
> >>> lines have been dropped. The ICC bus conversion has been split out as a
> >>> separate step, using NULL errp as intermediate step since ignored.
> >>> (CC'ing Peter C. and Luiz)
> >>
> >> Oh and you forgot to convert the Xen APIC, I trivially fixed that.
> >> (CC'ing Stefano)
> > 
> > Yes, it should be really trivial. Give a look at hw/xen/xen_apic.c.
> 
> https://github.com/afaerber/qemu-cpu/commit/40ae2368c4c9135971c69fa680949b5b1cfa4410#diff-3
> 
> Just wanted to give you a heads-up that we're messing in your code. :)

Ah OK, thank you :-)

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[Chen Fan]

On Wed, 2013-12-18 at 19:03 +0100, Andreas Färber wrote:
> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
> > changes:
> > 1. use type constant for kvm_ioapic and ioapic
> > 2. convert 'init' to QOM's 'realize' for ioapic and kvm_ioapic
> > For QOM'ify, I move variable 'ioapic_no' from static to global.
> > Then we can drop the 'instance_no' argument. Now, it's child
> > that increase 'ioapic_no' counter.
> > 
> > Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
> > ---
> >  hw/i386/kvm/ioapic.c              |   15 +++++++++------
> >  hw/intc/ioapic.c                  |   19 +++++++++++++------
> >  hw/intc/ioapic_common.c           |   13 ++++++++++---
> >  include/hw/i386/ioapic_internal.h |    3 ++-
> >  4 files changed, 34 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
> > index 772a712..36f7de2 100644
> > --- a/hw/i386/kvm/ioapic.c
> > +++ b/hw/i386/kvm/ioapic.c
> > @@ -15,6 +15,8 @@
> >  #include "hw/i386/apic_internal.h"
> >  #include "sysemu/kvm.h"
> >  
> > +#define TYPE_KVM_IOAPIC "kvm-ioapic"
> > +
> >  /* PC Utility function */
> >  void kvm_pc_setup_irq_routing(bool pci_enabled)
> >  {
> > @@ -127,12 +129,12 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
> >      apic_report_irq_delivered(delivered);
> >  }
> >  
> > -static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
> > +static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
> >  {
> > -    DeviceState *dev = DEVICE(s);
> > -
> > -    memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
> > +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
> >  
> > +    memory_region_init_reservation(&s->io_memory, NULL,
> > +                                   TYPE_KVM_IOAPIC, 0x1000);
> >      qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
> >  }
> >  
> > @@ -143,10 +145,11 @@ static Property kvm_ioapic_properties[] = {
> >  
> >  static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
> >  {
> > +
> >      IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    k->init      = kvm_ioapic_init;
> > +    k->realize   = kvm_ioapic_realize;
> >      k->pre_save  = kvm_ioapic_get;
> >      k->post_load = kvm_ioapic_put;
> >      dc->reset    = kvm_ioapic_reset;
> > @@ -154,7 +157,7 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
> >  }
> >  
> >  static const TypeInfo kvm_ioapic_info = {
> > -    .name  = "kvm-ioapic",
> > +    .name  = TYPE_KVM_IOAPIC,
> >      .parent = TYPE_IOAPIC_COMMON,
> >      .instance_size = sizeof(KVMIOAPICState),
> >      .class_init = kvm_ioapic_class_init,
> > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> > index 8842845..885f385 100644
> > --- a/hw/intc/ioapic.c
> > +++ b/hw/intc/ioapic.c
> > @@ -36,6 +36,10 @@
> >  
> >  static IOAPICCommonState *ioapics[MAX_IOAPICS];
> >  
> > +#define TYPE_IOAPIC "ioapic"
> > +/* global variable from ioapic_common.c */
> > +extern int ioapic_no;
> > +
> >  static void ioapic_service(IOAPICCommonState *s)
> >  {
> >      uint8_t i;
> > @@ -225,16 +229,19 @@ static const MemoryRegionOps ioapic_io_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> >  
> > -static void ioapic_init(IOAPICCommonState *s, int instance_no)
> > +static void ioapic_realize(DeviceState *dev, Error **errp)
> >  {
> > -    DeviceState *dev = DEVICE(s);
> > +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
> >  
> >      memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
> > -                          "ioapic", 0x1000);
> > +                          TYPE_IOAPIC, 0x1000);
> >  
> >      qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
> >  
> > -    ioapics[instance_no] = s;
> > +    ioapics[ioapic_no] = s;
> > +
> > +    /* increase the counter */
> > +    ioapic_no++;
> 
> This increment used to happen in common code before, now it's done for
> the non-KVM version only ...

Due to IOAPICS are not more then one, why did not we get rid of
ioapic_no and ioapics, to use one ioapics pointer instead of ioapics[] ?

Thanks,
Chen

> 
> >  }
> >  
> >  static void ioapic_class_init(ObjectClass *klass, void *data)
> > @@ -242,12 +249,12 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
> >      IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >  
> > -    k->init = ioapic_init;
> > +    k->realize = ioapic_realize;
> >      dc->reset = ioapic_reset_common;
> >  }
> >  
> >  static const TypeInfo ioapic_info = {
> > -    .name          = "ioapic",
> > +    .name          = TYPE_IOAPIC,
> >      .parent        = TYPE_IOAPIC_COMMON,
> >      .instance_size = sizeof(IOAPICCommonState),
> >      .class_init    = ioapic_class_init,
> > diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> > index e55c6d1..aac0402 100644
> > --- a/hw/intc/ioapic_common.c
> > +++ b/hw/intc/ioapic_common.c
> > @@ -23,6 +23,14 @@
> >  #include "hw/i386/ioapic_internal.h"
> >  #include "hw/sysbus.h"
> >  
> > +/* ioapic_no count start from 0 to MAX_IOAPICS,
> > + * remove as static variable from ioapic_common_init.
> > + * now as a global variable, let child to increase the counter
> > + * then we can drop the 'instance_no' argument
> > + * and convert to our QOM's realize function
> > + */
> > +int ioapic_no;
> > +
> >  void ioapic_reset_common(DeviceState *dev)
> >  {
> >      IOAPICCommonState *s = IOAPIC_COMMON(dev);
> > @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
> >  {
> >      IOAPICCommonState *s = IOAPIC_COMMON(dev);
> >      IOAPICCommonClass *info;
> > -    static int ioapic_no;
> >  
> >      if (ioapic_no >= MAX_IOAPICS) {
> >          error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
> 
> ... while the check for max. IOAPICs still happens in common code.
> 
> Do we need to count KVM IOAPICs as well? Or can we consolidate this into
> the non-KVM version and keep it static there?
> 
> Regards,
> Andreas
> 
> > @@ -69,10 +76,10 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      info = IOAPIC_COMMON_GET_CLASS(s);
> > -    info->init(s, ioapic_no);
> > +    info->realize(dev, errp);
> >  
> >      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
> > -    ioapic_no++;
> > +
> >  }
> >  
> >  static const VMStateDescription vmstate_ioapic_common = {
> > diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
> > index 25576c8..cbe4744 100644
> > --- a/include/hw/i386/ioapic_internal.h
> > +++ b/include/hw/i386/ioapic_internal.h
> > @@ -83,7 +83,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
> >  
> >  typedef struct IOAPICCommonClass {
> >      SysBusDeviceClass parent_class;
> > -    void (*init)(IOAPICCommonState *s, int instance_no);
> > +    /* QOM realize */
> > +    DeviceRealize realize;
> >      void (*pre_save)(IOAPICCommonState *s);
> >      void (*post_load)(IOAPICCommonState *s);
> >  } IOAPICCommonClass;
> 

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[赵小强]

于 12/19/2013 10:28 AM, Chen Fan 写道:
> On Wed, 2013-12-18 at 19:03 +0100, Andreas Färber wrote:
>> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
>>> changes:
>>> 1. use type constant for kvm_ioapic and ioapic
>>> 2. convert 'init' to QOM's 'realize' for ioapic and kvm_ioapic
>>> For QOM'ify, I move variable 'ioapic_no' from static to global.
>>> Then we can drop the 'instance_no' argument. Now, it's child
>>> that increase 'ioapic_no' counter.
>>>
>>> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
>>> ---
>>>   hw/i386/kvm/ioapic.c              |   15 +++++++++------
>>>   hw/intc/ioapic.c                  |   19 +++++++++++++------
>>>   hw/intc/ioapic_common.c           |   13 ++++++++++---
>>>   include/hw/i386/ioapic_internal.h |    3 ++-
>>>   4 files changed, 34 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
>>> index 772a712..36f7de2 100644
>>> --- a/hw/i386/kvm/ioapic.c
>>> +++ b/hw/i386/kvm/ioapic.c
>>> @@ -15,6 +15,8 @@
>>>   #include "hw/i386/apic_internal.h"
>>>   #include "sysemu/kvm.h"
>>>   
>>> +#define TYPE_KVM_IOAPIC "kvm-ioapic"
>>> +
>>>   /* PC Utility function */
>>>   void kvm_pc_setup_irq_routing(bool pci_enabled)
>>>   {
>>> @@ -127,12 +129,12 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
>>>       apic_report_irq_delivered(delivered);
>>>   }
>>>   
>>> -static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
>>> +static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
>>>   {
>>> -    DeviceState *dev = DEVICE(s);
>>> -
>>> -    memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
>>> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>>   
>>> +    memory_region_init_reservation(&s->io_memory, NULL,
>>> +                                   TYPE_KVM_IOAPIC, 0x1000);
>>>       qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
>>>   }
>>>   
>>> @@ -143,10 +145,11 @@ static Property kvm_ioapic_properties[] = {
>>>   
>>>   static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>>>   {
>>> +
>>>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>   
>>> -    k->init      = kvm_ioapic_init;
>>> +    k->realize   = kvm_ioapic_realize;
>>>       k->pre_save  = kvm_ioapic_get;
>>>       k->post_load = kvm_ioapic_put;
>>>       dc->reset    = kvm_ioapic_reset;
>>> @@ -154,7 +157,7 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>>>   }
>>>   
>>>   static const TypeInfo kvm_ioapic_info = {
>>> -    .name  = "kvm-ioapic",
>>> +    .name  = TYPE_KVM_IOAPIC,
>>>       .parent = TYPE_IOAPIC_COMMON,
>>>       .instance_size = sizeof(KVMIOAPICState),
>>>       .class_init = kvm_ioapic_class_init,
>>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>>> index 8842845..885f385 100644
>>> --- a/hw/intc/ioapic.c
>>> +++ b/hw/intc/ioapic.c
>>> @@ -36,6 +36,10 @@
>>>   
>>>   static IOAPICCommonState *ioapics[MAX_IOAPICS];
>>>   
>>> +#define TYPE_IOAPIC "ioapic"
>>> +/* global variable from ioapic_common.c */
>>> +extern int ioapic_no;
>>> +
>>>   static void ioapic_service(IOAPICCommonState *s)
>>>   {
>>>       uint8_t i;
>>> @@ -225,16 +229,19 @@ static const MemoryRegionOps ioapic_io_ops = {
>>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>>   };
>>>   
>>> -static void ioapic_init(IOAPICCommonState *s, int instance_no)
>>> +static void ioapic_realize(DeviceState *dev, Error **errp)
>>>   {
>>> -    DeviceState *dev = DEVICE(s);
>>> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>>   
>>>       memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
>>> -                          "ioapic", 0x1000);
>>> +                          TYPE_IOAPIC, 0x1000);
>>>   
>>>       qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
>>>   
>>> -    ioapics[instance_no] = s;
>>> +    ioapics[ioapic_no] = s;
>>> +
>>> +    /* increase the counter */
>>> +    ioapic_no++;
>> This increment used to happen in common code before, now it's done for
>> the non-KVM version only ...
> Due to IOAPICS are not more then one, why did not we get rid of
> ioapic_no and ioapics, to use one ioapics pointer instead of ioapics[] ?
>
> Thanks,
> Chen
>
>>>   }
>>>   
>>>   static void ioapic_class_init(ObjectClass *klass, void *data)
>>> @@ -242,12 +249,12 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
>>>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>>   
>>> -    k->init = ioapic_init;
>>> +    k->realize = ioapic_realize;
>>>       dc->reset = ioapic_reset_common;
>>>   }
>>>   
>>>   static const TypeInfo ioapic_info = {
>>> -    .name          = "ioapic",
>>> +    .name          = TYPE_IOAPIC,
>>>       .parent        = TYPE_IOAPIC_COMMON,
>>>       .instance_size = sizeof(IOAPICCommonState),
>>>       .class_init    = ioapic_class_init,
>>> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
>>> index e55c6d1..aac0402 100644
>>> --- a/hw/intc/ioapic_common.c
>>> +++ b/hw/intc/ioapic_common.c
>>> @@ -23,6 +23,14 @@
>>>   #include "hw/i386/ioapic_internal.h"
>>>   #include "hw/sysbus.h"
>>>   
>>> +/* ioapic_no count start from 0 to MAX_IOAPICS,
>>> + * remove as static variable from ioapic_common_init.
>>> + * now as a global variable, let child to increase the counter
>>> + * then we can drop the 'instance_no' argument
>>> + * and convert to our QOM's realize function
>>> + */
>>> +int ioapic_no;
>>> +
>>>   void ioapic_reset_common(DeviceState *dev)
>>>   {
>>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>> @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>>>   {
>>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>>       IOAPICCommonClass *info;
>>> -    static int ioapic_no;
>>>   
>>>       if (ioapic_no >= MAX_IOAPICS) {
>>>           error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
>> ... while the check for max. IOAPICs still happens in common code.
>>
>> Do we need to count KVM IOAPICs as well? Or can we consolidate this into
>> the non-KVM version and keep it static there?
>>
>> Regards,
>> Andreas
>>
>>> @@ -69,10 +76,10 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>>>       }
>>>   
>>>       info = IOAPIC_COMMON_GET_CLASS(s);
>>> -    info->init(s, ioapic_no);
>>> +    info->realize(dev, errp);
>>>   
>>>       sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
>>> -    ioapic_no++;
>>> +
>>>   }
>>>   
>>>   static const VMStateDescription vmstate_ioapic_common = {
>>> diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
>>> index 25576c8..cbe4744 100644
>>> --- a/include/hw/i386/ioapic_internal.h
>>> +++ b/include/hw/i386/ioapic_internal.h
>>> @@ -83,7 +83,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
>>>   
>>>   typedef struct IOAPICCommonClass {
>>>       SysBusDeviceClass parent_class;
>>> -    void (*init)(IOAPICCommonState *s, int instance_no);
>>> +    /* QOM realize */
>>> +    DeviceRealize realize;
>>>       void (*pre_save)(IOAPICCommonState *s);
>>>       void (*post_load)(IOAPICCommonState *s);
>>>   } IOAPICCommonClass;
>
> Due to IOAPICS are not more then one, why did not we get rid of
> ioapic_no and ioapics, to use one ioapics pointer instead of ioapics[] ?

     According to intel manul, in a system, IOAPIC can be more than one 
. So, i think developers want to keep
   this compatibility.

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[赵小强]

于 12/19/2013 02:03 AM, Andreas Färber 写道:
> Am 05.11.2013 11:16, schrieb xiaoqiang zhao:
>> changes:
>> 1. use type constant for kvm_ioapic and ioapic
>> 2. convert 'init' to QOM's 'realize' for ioapic and kvm_ioapic
>> For QOM'ify, I move variable 'ioapic_no' from static to global.
>> Then we can drop the 'instance_no' argument. Now, it's child
>> that increase 'ioapic_no' counter.
>>
>> Signed-off-by: xiaoqiang zhao <zxq_yx_007@163.com>
>> ---
>>   hw/i386/kvm/ioapic.c              |   15 +++++++++------
>>   hw/intc/ioapic.c                  |   19 +++++++++++++------
>>   hw/intc/ioapic_common.c           |   13 ++++++++++---
>>   include/hw/i386/ioapic_internal.h |    3 ++-
>>   4 files changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
>> index 772a712..36f7de2 100644
>> --- a/hw/i386/kvm/ioapic.c
>> +++ b/hw/i386/kvm/ioapic.c
>> @@ -15,6 +15,8 @@
>>   #include "hw/i386/apic_internal.h"
>>   #include "sysemu/kvm.h"
>>   
>> +#define TYPE_KVM_IOAPIC "kvm-ioapic"
>> +
>>   /* PC Utility function */
>>   void kvm_pc_setup_irq_routing(bool pci_enabled)
>>   {
>> @@ -127,12 +129,12 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
>>       apic_report_irq_delivered(delivered);
>>   }
>>   
>> -static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
>> +static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
>>   {
>> -    DeviceState *dev = DEVICE(s);
>> -
>> -    memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
>> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>   
>> +    memory_region_init_reservation(&s->io_memory, NULL,
>> +                                   TYPE_KVM_IOAPIC, 0x1000);
>>       qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
>>   }
>>   
>> @@ -143,10 +145,11 @@ static Property kvm_ioapic_properties[] = {
>>   
>>   static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>>   {
>> +
>>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>   
>> -    k->init      = kvm_ioapic_init;
>> +    k->realize   = kvm_ioapic_realize;
>>       k->pre_save  = kvm_ioapic_get;
>>       k->post_load = kvm_ioapic_put;
>>       dc->reset    = kvm_ioapic_reset;
>> @@ -154,7 +157,7 @@ static void kvm_ioapic_class_init(ObjectClass *klass, void *data)
>>   }
>>   
>>   static const TypeInfo kvm_ioapic_info = {
>> -    .name  = "kvm-ioapic",
>> +    .name  = TYPE_KVM_IOAPIC,
>>       .parent = TYPE_IOAPIC_COMMON,
>>       .instance_size = sizeof(KVMIOAPICState),
>>       .class_init = kvm_ioapic_class_init,
>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>> index 8842845..885f385 100644
>> --- a/hw/intc/ioapic.c
>> +++ b/hw/intc/ioapic.c
>> @@ -36,6 +36,10 @@
>>   
>>   static IOAPICCommonState *ioapics[MAX_IOAPICS];
>>   
>> +#define TYPE_IOAPIC "ioapic"
>> +/* global variable from ioapic_common.c */
>> +extern int ioapic_no;
>> +
>>   static void ioapic_service(IOAPICCommonState *s)
>>   {
>>       uint8_t i;
>> @@ -225,16 +229,19 @@ static const MemoryRegionOps ioapic_io_ops = {
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>>   };
>>   
>> -static void ioapic_init(IOAPICCommonState *s, int instance_no)
>> +static void ioapic_realize(DeviceState *dev, Error **errp)
>>   {
>> -    DeviceState *dev = DEVICE(s);
>> +    IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>   
>>       memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
>> -                          "ioapic", 0x1000);
>> +                          TYPE_IOAPIC, 0x1000);
>>   
>>       qdev_init_gpio_in(dev, ioapic_set_irq, IOAPIC_NUM_PINS);
>>   
>> -    ioapics[instance_no] = s;
>> +    ioapics[ioapic_no] = s;
>> +
>> +    /* increase the counter */
>> +    ioapic_no++;
> This increment used to happen in common code before, now it's done for
> the non-KVM version only ...
>
>>   }
>>   
>>   static void ioapic_class_init(ObjectClass *klass, void *data)
>> @@ -242,12 +249,12 @@ static void ioapic_class_init(ObjectClass *klass, void *data)
>>       IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>   
>> -    k->init = ioapic_init;
>> +    k->realize = ioapic_realize;
>>       dc->reset = ioapic_reset_common;
>>   }
>>   
>>   static const TypeInfo ioapic_info = {
>> -    .name          = "ioapic",
>> +    .name          = TYPE_IOAPIC,
>>       .parent        = TYPE_IOAPIC_COMMON,
>>       .instance_size = sizeof(IOAPICCommonState),
>>       .class_init    = ioapic_class_init,
>> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
>> index e55c6d1..aac0402 100644
>> --- a/hw/intc/ioapic_common.c
>> +++ b/hw/intc/ioapic_common.c
>> @@ -23,6 +23,14 @@
>>   #include "hw/i386/ioapic_internal.h"
>>   #include "hw/sysbus.h"
>>   
>> +/* ioapic_no count start from 0 to MAX_IOAPICS,
>> + * remove as static variable from ioapic_common_init.
>> + * now as a global variable, let child to increase the counter
>> + * then we can drop the 'instance_no' argument
>> + * and convert to our QOM's realize function
>> + */
>> +int ioapic_no;
>> +
>>   void ioapic_reset_common(DeviceState *dev)
>>   {
>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>> @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>>   {
>>       IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>       IOAPICCommonClass *info;
>> -    static int ioapic_no;
>>   
>>       if (ioapic_no >= MAX_IOAPICS) {
>>           error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
> ... while the check for max. IOAPICs still happens in common code.
>
> Do we need to count KVM IOAPICs as well? Or can we consolidate this into
> the non-KVM version and keep it static there?
>
> Regards,
> Andreas
>
>> @@ -69,10 +76,10 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>>       }
>>   
>>       info = IOAPIC_COMMON_GET_CLASS(s);
>> -    info->init(s, ioapic_no);
>> +    info->realize(dev, errp);
>>   
>>       sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
>> -    ioapic_no++;
>> +
>>   }
>>   
>>   static const VMStateDescription vmstate_ioapic_common = {
>> diff --git a/include/hw/i386/ioapic_internal.h b/include/hw/i386/ioapic_internal.h
>> index 25576c8..cbe4744 100644
>> --- a/include/hw/i386/ioapic_internal.h
>> +++ b/include/hw/i386/ioapic_internal.h
>> @@ -83,7 +83,8 @@ typedef struct IOAPICCommonState IOAPICCommonState;
>>   
>>   typedef struct IOAPICCommonClass {
>>       SysBusDeviceClass parent_class;
>> -    void (*init)(IOAPICCommonState *s, int instance_no);
>> +    /* QOM realize */
>> +    DeviceRealize realize;
>>       void (*pre_save)(IOAPICCommonState *s);
>>       void (*post_load)(IOAPICCommonState *s);
>>   } IOAPICCommonClass;
> This increment used to happen in common code before, now it's done for
> the non-KVM version only ...
  Yes, I think the increment should be done in kvm version also. 
Actually i missed it.
   The following patch should fix it:

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 36f7de2..031c87c 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -17,6 +17,9 @@

  #define TYPE_KVM_IOAPIC "kvm-ioapic"

+/* global variable from ioapic_common.c */
+extern int ioapic_no;
+
  /* PC Utility function */
  void kvm_pc_setup_irq_routing(bool pci_enabled)
  {
@@ -136,6 +139,9 @@ static void kvm_ioapic_realize(DeviceState *dev, 
Error **errp)
      memory_region_init_reservation(&s->io_memory, NULL,
                                     TYPE_KVM_IOAPIC, 0x1000);
      qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
+
+    /* increase the counter */
+    ioapic_no++;
  }

  static Property kvm_ioapic_properties[] = {

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[Paolo Bonzini]

Il 18/12/2013 19:03, Andreas Färber ha scritto:
>> > @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>> >  {
>> >      IOAPICCommonState *s = IOAPIC_COMMON(dev);
>> >      IOAPICCommonClass *info;
>> > -    static int ioapic_no;
>> >  
>> >      if (ioapic_no >= MAX_IOAPICS) {
>> >          error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
> ... while the check for max. IOAPICs still happens in common code.
> 
> Do we need to count KVM IOAPICs as well? Or can we consolidate this into
> the non-KVM version and keep it static there?

KVM only supports one IOAPIC.  Creating a second fails with EEXIST.

Paolo

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[Andreas Färber]

Am 19.12.2013 15:20, schrieb Paolo Bonzini:
> Il 18/12/2013 19:03, Andreas Färber ha scritto:
>>>> @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp)
>>>>  {
>>>>      IOAPICCommonState *s = IOAPIC_COMMON(dev);
>>>>      IOAPICCommonClass *info;
>>>> -    static int ioapic_no;
>>>>  
>>>>      if (ioapic_no >= MAX_IOAPICS) {
>>>>          error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS);
>> ... while the check for max. IOAPICs still happens in common code.
>>
>> Do we need to count KVM IOAPICs as well? Or can we consolidate this into
>> the non-KVM version and keep it static there?
> 
> KVM only supports one IOAPIC.  Creating a second fails with EEXIST.

As it turns out, MAX_IOAPICS is 1, so covers both cases. No KVM ioctl
actually happens on realize for using said EEXIST, so I moved the
increment back to where it was, minimizing changes.

If we want to redesign it, that can still be done in a follow-up patch.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 4/4] ioapic: QOM'ify ioapic

[<zxq_yx_007@163.com>]

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

OK. Thx Andreas！ 在2013年12月20日 19:18， Andreas Färber写道： Am 19.12.2013 15:20, schrieb Paolo Bonzini: > Il 18/12/2013 19:03, Andreas Färber ha scritto: >>>> @@ -61,7 +69,6 @@ static void ioapic_common_realize(DeviceState *dev, Error **errp) >>>>  { >>>>      IOAPICCommonState *s = IOAPIC_COMMON(dev); >>>>      IOAPICCommonClass *info; >>>> -    static int ioapic_no; >>>>   >>>>      if (ioapic_no >= MAX_IOAPICS) { >>>>          error_setg(errp, "Only %d ioapics allowed", MAX_IOAPICS); >> ... while the check for max. IOAPICs still happens in common code. >> >> Do we need to count KVM IOAPICs as well? Or can we consolidate this into >> the non-KVM version and keep it static there? > > KVM only supports one IOAPIC.  Creating a second fails with EEXIST. As it turns out, MAX_IOAPICS is 1, so covers both cases. No KVM ioctl actually happens on realize for using said EEXIST, so I moved the increment back to where it was, minimizing changes. If we want to redesign it, that can still be done in a follow-up patch. Thanks, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[-- Attachment #2: Type: text/html, Size: 1934 bytes --]