* [Qemu-devel] [PATCH v2 1/4] apic: Cleanup for QOM'ify
2013-11-05 7:55 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
@ 2013-11-05 7:55 ` xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus xiaoqiang zhao
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 7:55 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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'
---
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 7:55 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 1/4] apic: Cleanup for QOM'ify xiaoqiang zhao
@ 2013-11-05 7:55 ` xiaoqiang zhao
2013-11-05 8:25 ` Chen Fan
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 3/4] ioapic: Cleanup for QOM'ify xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 4/4] ioapic: QOM'ify ioapic xiaoqiang zhao
3 siblings, 1 reply; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 7:55 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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'
---
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 | 5 +++--
6 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 9a4ea7e..1cc64ac 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..5a413cc 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..bd3a5fc 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus xiaoqiang zhao
@ 2013-11-05 8:25 ` Chen Fan
2013-11-05 8:51 ` 赵小强
0 siblings, 1 reply; 23+ messages in thread
From: Chen Fan @ 2013-11-05 8:25 UTC (permalink / raw)
To: xiaoqiang zhao; +Cc: pbonzini, qemu-devel, aliguori, afaerber
On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
> 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'
> ---
> 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 | 5 +++--
> 6 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> index 9a4ea7e..1cc64ac 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..5a413cc 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;
> }
Indentation looks bad.
> 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;
Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
class has included 'realize' field.
> } 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..bd3a5fc 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
> typedef struct APICCommonClass
> {
> ICCDeviceClass parent_class;
> -
> - void (*init)(APICCommonState *s);
> +
> + /* QOM realize */
> + DeviceRealize realize;
as above.
Thanks,
Chen
> void (*set_base)(APICCommonState *s, uint64_t val);
> void (*set_tpr)(APICCommonState *s, uint8_t val);
> uint8_t (*get_tpr)(APICCommonState *s);
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 8:25 ` Chen Fan
@ 2013-11-05 8:51 ` 赵小强
2013-11-11 3:58 ` 赵小强
0 siblings, 1 reply; 23+ messages in thread
From: 赵小强 @ 2013-11-05 8:51 UTC (permalink / raw)
To: Chen Fan; +Cc: pbonzini, qemu-devel, aliguori, afaerber
于 2013年11月05日 16:25, Chen Fan 写道:
> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
>> 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'
>> ---
>> 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 | 5 +++--
>> 6 files changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
>> index 9a4ea7e..1cc64ac 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..5a413cc 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;
>> }
> Indentation looks bad.
>
>> 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;
> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
> class has included 'realize' field.
>
>> } 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..bd3a5fc 100644
>> --- a/include/hw/i386/apic_internal.h
>> +++ b/include/hw/i386/apic_internal.h
>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>> typedef struct APICCommonClass
>> {
>> ICCDeviceClass parent_class;
>> -
>> - void (*init)(APICCommonState *s);
>> +
>> + /* QOM realize */
>> + DeviceRealize realize;
> as above.
>
> Thanks,
> Chen
>> void (*set_base)(APICCommonState *s, uint64_t val);
>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>> uint8_t (*get_tpr)(APICCommonState *s);
>
Thanks for your review!!
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 8:51 ` 赵小强
@ 2013-11-11 3:58 ` 赵小强
2013-11-12 1:28 ` Chen Fan
2013-11-12 14:52 ` Andreas Färber
0 siblings, 2 replies; 23+ messages in thread
From: 赵小强 @ 2013-11-11 3:58 UTC (permalink / raw)
To: Chen Fan; +Cc: pbonzini, qemu-devel, aliguori, afaerber
于 11/05/2013 04:51 PM, 赵小强 写道:
> 于 2013年11月05日 16:25, Chen Fan 写道:
>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
>>> 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'
>>> ---
>>> 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 | 5 +++--
>>> 6 files changed, 32 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
>>> index 9a4ea7e..1cc64ac 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..5a413cc 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;
>>> }
>> Indentation looks bad.
>>
>>> 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;
>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>> class has included 'realize' field.
>>
>>> } 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..bd3a5fc 100644
>>> --- a/include/hw/i386/apic_internal.h
>>> +++ b/include/hw/i386/apic_internal.h
>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>> typedef struct APICCommonClass
>>> {
>>> ICCDeviceClass parent_class;
>>> -
>>> - void (*init)(APICCommonState *s);
>>> +
>>> + /* QOM realize */
>>> + DeviceRealize realize;
>> as above.
>>
>> Thanks,
>> Chen
>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>> uint8_t (*get_tpr)(APICCommonState *s);
>>
> Thanks for your review!!
Hi, Chen Fan:
In my understanding, If we use only one 'realize'(which in
'DeviceClass'), I think all the initialization work must be done in the
leaf child. if we add 'redundant' realize to each parent, then we can
call the initialization chain from DeviceClass down to leaf child's
parent, with each parent complete a bit further(of cause, a parent can
do nothing and pass to it's child directly).
What do you think?
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-11 3:58 ` 赵小强
@ 2013-11-12 1:28 ` Chen Fan
2013-11-12 1:54 ` 赵小强
2013-11-12 16:20 ` Andreas Färber
2013-11-12 14:52 ` Andreas Färber
1 sibling, 2 replies; 23+ messages in thread
From: Chen Fan @ 2013-11-12 1:28 UTC (permalink / raw)
To: 赵小强; +Cc: pbonzini, qemu-devel, aliguori, afaerber
On Mon, 2013-11-11 at 11:58 +0800, 赵小强 wrote:
> 于 11/05/2013 04:51 PM, 赵小强 写道:
> > 于 2013年11月05日 16:25, Chen Fan 写道:
> >> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
> >>> 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'
> >>> ---
> >>> 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 | 5 +++--
> >>> 6 files changed, 32 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> >>> index 9a4ea7e..1cc64ac 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..5a413cc 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;
> >>> }
> >> Indentation looks bad.
> >>
> >>> 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;
> >> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
> >> class has included 'realize' field.
> >>
> >>> } 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..bd3a5fc 100644
> >>> --- a/include/hw/i386/apic_internal.h
> >>> +++ b/include/hw/i386/apic_internal.h
> >>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
> >>> typedef struct APICCommonClass
> >>> {
> >>> ICCDeviceClass parent_class;
> >>> -
> >>> - void (*init)(APICCommonState *s);
> >>> +
> >>> + /* QOM realize */
> >>> + DeviceRealize realize;
> >> as above.
> >>
> >> Thanks,
> >> Chen
> >>> void (*set_base)(APICCommonState *s, uint64_t val);
> >>> void (*set_tpr)(APICCommonState *s, uint8_t val);
> >>> uint8_t (*get_tpr)(APICCommonState *s);
> >>
> > Thanks for your review!!
> Hi, Chen Fan:
>
> In my understanding, If we use only one 'realize'(which in
> 'DeviceClass'), I think all the initialization work must be done in the
> leaf child. if we add 'redundant' realize to each parent, then we can
> call the initialization chain from DeviceClass down to leaf child's
> parent, with each parent complete a bit further(of cause, a parent can
> do nothing and pass to it's child directly).
>
Hi Zhao,
I may not agree with your point of view, As far as I know，usually, we
must override the 'dc->realize' pointer function to initialize a device
via setting 'realize' property to 'true', e.g.
object_property_set_bool('realize', true), this would call the realize
function of the device, If we want call the initialization chain from
'DeviceClass' down to leaf, we should add a 'parent_realize' field to
realize parent device. not long ago， I had sent a path about refactor
apic, though, there was not commented, if you are interested in it，
please help to review it:
https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg02823.html
Thanks,
Chen
> What do you think?
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 1:28 ` Chen Fan
@ 2013-11-12 1:54 ` 赵小强
2013-11-12 3:02 ` Chen Fan
2013-11-12 16:20 ` Andreas Färber
1 sibling, 1 reply; 23+ messages in thread
From: 赵小强 @ 2013-11-12 1:54 UTC (permalink / raw)
To: Chen Fan; +Cc: pbonzini, qemu-devel, aliguori, afaerber
[-- Attachment #1: Type: text/plain, Size: 10545 bytes --]
于 11/12/2013 09:28 AM, Chen Fan 写道:
> On Mon, 2013-11-11 at 11:58 +0800, 赵小强 wrote:
>> 于 11/05/2013 04:51 PM, 赵小强 写道:
>>> 于 2013年11月05日 16:25, Chen Fan 写道:
>>>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
>>>>> 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'
>>>>> ---
>>>>> 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 | 5 +++--
>>>>> 6 files changed, 32 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
>>>>> index 9a4ea7e..1cc64ac 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..5a413cc 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;
>>>>> }
>>>> Indentation looks bad.
>>>>
>>>>> 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;
>>>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>>>> class has included 'realize' field.
>>>>
>>>>> } 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..bd3a5fc 100644
>>>>> --- a/include/hw/i386/apic_internal.h
>>>>> +++ b/include/hw/i386/apic_internal.h
>>>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>>>> typedef struct APICCommonClass
>>>>> {
>>>>> ICCDeviceClass parent_class;
>>>>> -
>>>>> - void (*init)(APICCommonState *s);
>>>>> +
>>>>> + /* QOM realize */
>>>>> + DeviceRealize realize;
>>>> as above.
>>>>
>>>> Thanks,
>>>> Chen
>>>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>>>> uint8_t (*get_tpr)(APICCommonState *s);
>>> Thanks for your review!!
>> Hi, Chen Fan:
>>
>> In my understanding, If we use only one 'realize'(which in
>> 'DeviceClass'), I think all the initialization work must be done in the
>> leaf child. if we add 'redundant' realize to each parent, then we can
>> call the initialization chain from DeviceClass down to leaf child's
>> parent, with each parent complete a bit further(of cause, a parent can
>> do nothing and pass to it's child directly).
>>
> Hi Zhao,
> I may not agree with your point of view, As far as I know，usually, we
> must override the 'dc->realize' pointer function to initialize a device
> via setting 'realize' property to 'true', e.g.
> object_property_set_bool('realize', true), this would call the realize
> function of the device, If we want call the initialization chain from
> 'DeviceClass' down to leaf, we should add a 'parent_realize' field to
> realize parent device. not long ago， I had sent a path about refactor
> apic, though, there was not commented, if you are interested in it，
> please help to review it:
> https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg02823.html
>
> Thanks,
> Chen
>
>> What do you think?
>>
>>
>
Yes, you are right and I know that.//As it happens, I also have submited
the similar patch:
////https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg04450.html
But with little difference, I add parent_realize in the leaf class. eg:
typedef struct KVMAPICClass {
APICCommonClass parent_class;
DeviceRealize parent_realize;
} KVMAPICClass;
which not in APICCommonClass. But Andreas said this to me:
The main issue though is that Anthony has requested to change the
pattern for overriding virtual methods: Can you please update the patch
so that void (*init)(APICCommonState *dev); simply gets replaced by
DeviceRealize realize;? I.e. drop the two new ...Class'es and
parent_realize, leave the call logic as it is now and just update the
function signature. Same for the ioapic.
He asked me to drop the parent_realize. so in v2, I just replace the
'init' with 'realize'.
[-- Attachment #2: Type: text/html, Size: 11192 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 1:54 ` 赵小强
@ 2013-11-12 3:02 ` Chen Fan
2013-11-12 3:11 ` 赵小强
2013-11-12 16:41 ` Andreas Färber
0 siblings, 2 replies; 23+ messages in thread
From: Chen Fan @ 2013-11-12 3:02 UTC (permalink / raw)
To: 赵小强; +Cc: pbonzini, qemu-devel, aliguori, afaerber
On Tue, 2013-11-12 at 09:54 +0800, 赵小强 wrote:
> 于 11/12/2013 09:28 AM, Chen Fan 写道:
>
> > On Mon, 2013-11-11 at 11:58 +0800, 赵小强 wrote:
> > > 于 11/05/2013 04:51 PM, 赵小强 写道:
> > > > 于 2013年11月05日 16:25, Chen Fan 写道:
> > > > > On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
> > > > > > 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'
> > > > > > ---
> > > > > > 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 | 5 +++--
> > > > > > 6 files changed, 32 insertions(+), 23 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> > > > > > index 9a4ea7e..1cc64ac 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..5a413cc 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;
> > > > > > }
> > > > > Indentation looks bad.
> > > > >
> > > > > > 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;
> > > > > Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
> > > > > class has included 'realize' field.
> > > > >
> > > > > > } 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..bd3a5fc 100644
> > > > > > --- a/include/hw/i386/apic_internal.h
> > > > > > +++ b/include/hw/i386/apic_internal.h
> > > > > > @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
> > > > > > typedef struct APICCommonClass
> > > > > > {
> > > > > > ICCDeviceClass parent_class;
> > > > > > -
> > > > > > - void (*init)(APICCommonState *s);
> > > > > > +
> > > > > > + /* QOM realize */
> > > > > > + DeviceRealize realize;
> > > > > as above.
> > > > >
> > > > > Thanks,
> > > > > Chen
> > > > > > void (*set_base)(APICCommonState *s, uint64_t val);
> > > > > > void (*set_tpr)(APICCommonState *s, uint8_t val);
> > > > > > uint8_t (*get_tpr)(APICCommonState *s);
> > > > Thanks for your review!!
> > > Hi, Chen Fan:
> > >
> > > In my understanding, If we use only one 'realize'(which in
> > > 'DeviceClass'), I think all the initialization work must be done in the
> > > leaf child. if we add 'redundant' realize to each parent, then we can
> > > call the initialization chain from DeviceClass down to leaf child's
> > > parent, with each parent complete a bit further(of cause, a parent can
> > > do nothing and pass to it's child directly).
> > >
> > Hi Zhao,
> > I may not agree with your point of view, As far as I know，usually, we
> > must override the 'dc->realize' pointer function to initialize a device
> > via setting 'realize' property to 'true', e.g.
> > object_property_set_bool('realize', true), this would call the realize
> > function of the device, If we want call the initialization chain from
> > 'DeviceClass' down to leaf, we should add a 'parent_realize' field to
> > realize parent device. not long ago， I had sent a path about refactor
> > apic, though, there was not commented, if you are interested in it，
> > please help to review it:
> > https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg02823.html
> >
> > Thanks,
> > Chen
> >
> > > What do you think?
> > >
> > >
> >
> Yes, you are right and I know that. As it happens, I also have
> submited the similar patch:
> https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg04450.html
> But with little difference, I add parent_realize in the leaf class.
> eg:
>
> typedef struct KVMAPICClass {
> APICCommonClass parent_class;
>
> DeviceRealize parent_realize;
> } KVMAPICClass;
>
> which not in APICCommonClass. But Andreas said this to me:
> The main issue though is that Anthony has requested to change the
> pattern for overriding virtual methods: Can you please update the patch
> so that void (*init)(APICCommonState *dev); simply gets replaced by
> DeviceRealize realize;? I.e. drop the two new ...Class'es and
> parent_realize, leave the call logic as it is now and just update the
> function signature. Same for the ioapic.
>
> He asked me to drop the parent_realize. so in v2, I just replace the
> 'init' with 'realize'.
>
Hmm，I'm confused, Go through the entire QEMU source code, there was no
DeviceRealize realize field in leaf child, I think Andreas's purpose for
you was not simple to replace the 'init' by 'realize'. Maybe I'm
mistaken, I hope Andreas could give some comments about this.
on the other hand, I feel good regarding refactor QOM'ify apic, I'm now
making adding 'cpu-del' supported patches. which need unrealize the apic
device. so please rebase your work on Andreas's git source:
git://github.com/afaerber/qemu-cpu.git qom-cpu.
then we can do for them together.
Thanks,
Chen
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 3:02 ` Chen Fan
@ 2013-11-12 3:11 ` 赵小强
2013-11-12 16:41 ` Andreas Färber
1 sibling, 0 replies; 23+ messages in thread
From: 赵小强 @ 2013-11-12 3:11 UTC (permalink / raw)
To: Chen Fan; +Cc: pbonzini, qemu-devel, aliguori, afaerber
于 11/12/2013 11:02 AM, Chen Fan 写道:
> On Tue, 2013-11-12 at 09:54 +0800, 赵小强 wrote:
>> 于 11/12/2013 09:28 AM, Chen Fan 写道:
>>
>>> On Mon, 2013-11-11 at 11:58 +0800, 赵小强 wrote:
>>>> 于 11/05/2013 04:51 PM, 赵小强 写道:
>>>>> 于 2013年11月05日 16:25, Chen Fan 写道:
>>>>>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
>>>>>>> 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'
>>>>>>> ---
>>>>>>> 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 | 5 +++--
>>>>>>> 6 files changed, 32 insertions(+), 23 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
>>>>>>> index 9a4ea7e..1cc64ac 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..5a413cc 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;
>>>>>>> }
>>>>>> Indentation looks bad.
>>>>>>
>>>>>>> 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;
>>>>>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>>>>>> class has included 'realize' field.
>>>>>>
>>>>>>> } 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..bd3a5fc 100644
>>>>>>> --- a/include/hw/i386/apic_internal.h
>>>>>>> +++ b/include/hw/i386/apic_internal.h
>>>>>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>>>>>> typedef struct APICCommonClass
>>>>>>> {
>>>>>>> ICCDeviceClass parent_class;
>>>>>>> -
>>>>>>> - void (*init)(APICCommonState *s);
>>>>>>> +
>>>>>>> + /* QOM realize */
>>>>>>> + DeviceRealize realize;
>>>>>> as above.
>>>>>>
>>>>>> Thanks,
>>>>>> Chen
>>>>>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>>>>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>>>>>> uint8_t (*get_tpr)(APICCommonState *s);
>>>>> Thanks for your review!!
>>>> Hi, Chen Fan:
>>>>
>>>> In my understanding, If we use only one 'realize'(which in
>>>> 'DeviceClass'), I think all the initialization work must be done in the
>>>> leaf child. if we add 'redundant' realize to each parent, then we can
>>>> call the initialization chain from DeviceClass down to leaf child's
>>>> parent, with each parent complete a bit further(of cause, a parent can
>>>> do nothing and pass to it's child directly).
>>>>
>>> Hi Zhao,
>>> I may not agree with your point of view, As far as I know，usually, we
>>> must override the 'dc->realize' pointer function to initialize a device
>>> via setting 'realize' property to 'true', e.g.
>>> object_property_set_bool('realize', true), this would call the realize
>>> function of the device, If we want call the initialization chain from
>>> 'DeviceClass' down to leaf, we should add a 'parent_realize' field to
>>> realize parent device. not long ago， I had sent a path about refactor
>>> apic, though, there was not commented, if you are interested in it，
>>> please help to review it:
>>> https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg02823.html
>>>
>>> Thanks,
>>> Chen
>>>
>>>> What do you think?
>>>>
>>>>
>> Yes, you are right and I know that. As it happens, I also have
>> submited the similar patch:
>> https://lists.gnu.org/archive/html/qemu-devel/2013-09/msg04450.html
>> But with little difference, I add parent_realize in the leaf class.
>> eg:
>>
>> typedef struct KVMAPICClass {
>> APICCommonClass parent_class;
>>
>> DeviceRealize parent_realize;
>> } KVMAPICClass;
>>
>> which not in APICCommonClass. But Andreas said this to me:
>> The main issue though is that Anthony has requested to change the
>> pattern for overriding virtual methods: Can you please update the patch
>> so that void (*init)(APICCommonState *dev); simply gets replaced by
>> DeviceRealize realize;? I.e. drop the two new ...Class'es and
>> parent_realize, leave the call logic as it is now and just update the
>> function signature. Same for the ioapic.
>>
>> He asked me to drop the parent_realize. so in v2, I just replace the
>> 'init' with 'realize'.
>>
> Hmm，I'm confused, Go through the entire QEMU source code, there was no
> DeviceRealize realize field in leaf child, I think Andreas's purpose for
> you was not simple to replace the 'init' by 'realize'. Maybe I'm
> mistaken, I hope Andreas could give some comments about this.
> on the other hand, I feel good regarding refactor QOM'ify apic, I'm now
> making adding 'cpu-del' supported patches. which need unrealize the apic
> device. so please rebase your work on Andreas's git source:
> git://github.com/afaerber/qemu-cpu.git qom-cpu.
> then we can do for them together.
>
> Thanks,
> Chen
>
>>
>
Ok! I am new to qemu and am happy to work with you together! ;-)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 3:02 ` Chen Fan
2013-11-12 3:11 ` 赵小强
@ 2013-11-12 16:41 ` Andreas Färber
2013-11-13 8:47 ` Chen Fan
1 sibling, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-11-12 16:41 UTC (permalink / raw)
To: Chen Fan; +Cc: 赵小强, pbonzini, qemu-devel, aliguori
Am 12.11.2013 04:02, schrieb Chen Fan:
> On Tue, 2013-11-12 at 09:54 +0800, 赵小强 wrote:
>> He asked me to drop the parent_realize. so in v2, I just replace the
>> 'init' with 'realize'.
>>
> Hmm，I'm confused, Go through the entire QEMU source code, there was no
> DeviceRealize realize field in leaf child, I think Andreas's purpose for
> you was not simple to replace the 'init' by 'realize'. Maybe I'm
> mistaken, I hope Andreas could give some comments about this.
I was serious about that. It appears that my earlier reply didn't arrive
on the list, so I resent it today, hope that sheds some light.
My virtio series had been reworked in the same fashion but is *still* in
need of sending to the list...
> on the other hand, I feel good regarding refactor QOM'ify apic, I'm now
> making adding 'cpu-del' supported patches. which need unrealize the apic
> device. so please rebase your work on Andreas's git source:
> git://github.com/afaerber/qemu-cpu.git qom-cpu.
> then we can do for them together.
Actually the only pending work is Igor's x86 properties on qom-cpu-next
as of now. Both qom-cpu and qom-next have recently been merged so should
be no different from upstream qemu.git.
The QOM device refactorings are usually taking place on qom-next branch
at the same repository. We'll have to see how that works out with the
CPU work - ideally I'd merge qom-next first, then continue on qom-cpu.
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 16:41 ` Andreas Färber
@ 2013-11-13 8:47 ` Chen Fan
0 siblings, 0 replies; 23+ messages in thread
From: Chen Fan @ 2013-11-13 8:47 UTC (permalink / raw)
To: Andreas Färber
Cc: 赵小强, pbonzini, qemu-devel, aliguori
On Tue, 2013-11-12 at 17:41 +0100, Andreas Färber wrote:
> Am 12.11.2013 04:02, schrieb Chen Fan:
> > On Tue, 2013-11-12 at 09:54 +0800, 赵小强 wrote:
> >> He asked me to drop the parent_realize. so in v2, I just replace the
> >> 'init' with 'realize'.
> >>
> > Hmm，I'm confused, Go through the entire QEMU source code, there was no
> > DeviceRealize realize field in leaf child, I think Andreas's purpose for
> > you was not simple to replace the 'init' by 'realize'. Maybe I'm
> > mistaken, I hope Andreas could give some comments about this.
>
> I was serious about that. It appears that my earlier reply didn't arrive
> on the list, so I resent it today, hope that sheds some light.
>
> My virtio series had been reworked in the same fashion but is *still* in
> need of sending to the list...
>
> > on the other hand, I feel good regarding refactor QOM'ify apic, I'm now
> > making adding 'cpu-del' supported patches. which need unrealize the apic
> > device. so please rebase your work on Andreas's git source:
> > git://github.com/afaerber/qemu-cpu.git qom-cpu.
> > then we can do for them together.
>
> Actually the only pending work is Igor's x86 properties on qom-cpu-next
> as of now. Both qom-cpu and qom-next have recently been merged so should
> be no different from upstream qemu.git.
>
> The QOM device refactorings are usually taking place on qom-next branch
> at the same repository. We'll have to see how that works out with the
> CPU work - ideally I'd merge qom-next first, then continue on qom-cpu.
>
Thanks for your clarification !
Chen
> Regards,
> Andreas
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 1:28 ` Chen Fan
2013-11-12 1:54 ` 赵小强
@ 2013-11-12 16:20 ` Andreas Färber
1 sibling, 0 replies; 23+ messages in thread
From: Andreas Färber @ 2013-11-12 16:20 UTC (permalink / raw)
To: Chen Fan; +Cc: 赵小强, pbonzini, qemu-devel, aliguori
Hi,
Am 12.11.2013 02:28, schrieb Chen Fan:
> [...] not long ago， I had sent a path about refactor
> apic, though, there was not commented, if you are interested in it，
> please help to review it:
> https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg02823.html
You sent your series on Oct 22nd during KVM Forum, whereas I have only
recently just reviewed the previous APIC patch from Sep 27th. Sorry, but
I am lagging behind with review of several series and unless there's
reasons not to, I try to honor whomever was first with a patch.
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-11 3:58 ` 赵小强
2013-11-12 1:28 ` Chen Fan
@ 2013-11-12 14:52 ` Andreas Färber
2013-11-13 6:06 ` 赵小强
1 sibling, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-11-12 14:52 UTC (permalink / raw)
To: 赵小强; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
Resending yesterday's message since it hasn't arrived on qemu-devel...
Am 11.11.2013 04:58, schrieb 赵小强:
> 于 11/05/2013 04:51 PM, 赵小强 写道:
>> 于 2013年11月05日 16:25, Chen Fan 写道:
>>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
[...]
>>>> 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;
>>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>>> class has included 'realize' field.
>>>
>>>> } 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..bd3a5fc 100644
>>>> --- a/include/hw/i386/apic_internal.h
>>>> +++ b/include/hw/i386/apic_internal.h
>>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>>> typedef struct APICCommonClass
>>>> {
>>>> ICCDeviceClass parent_class;
>>>> -
>>>> - void (*init)(APICCommonState *s);
>>>> +
>>>> + /* QOM realize */
>>>> + DeviceRealize realize;
>>> as above.
>>>
>>> Thanks,
>>> Chen
>>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>>> uint8_t (*get_tpr)(APICCommonState *s);
>>>
>> Thanks for your review!!
> Hi, Chen Fan:
>
> In my understanding, If we use only one 'realize'(which in
> 'DeviceClass'), I think all the initialization work must be done in the
> leaf child. if we add 'redundant' realize to each parent, then we can
> call the initialization chain from DeviceClass down to leaf child's
> parent, with each parent complete a bit further(of cause, a parent can
> do nothing and pass to it's child directly).
>
> What do you think?
Your analysis is correct. v2 is like I requested you to do it and v3
still does iirc, so let's stick with that for "consistency". Bad word in
this context, I know. ;)
If you have some time, it would be nice if you could check whether these
devices (the non-KVM versions at least) are covered by make check. For
ICC bus I am certain that it is. In particular I'm wondering if we need
certain -cpu arguments to enable the *APIC devices and have the realize
functions actually exercised? (My assumption is no, but a confirmation
would save time.)
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-12 14:52 ` Andreas Färber
@ 2013-11-13 6:06 ` 赵小强
2013-11-29 1:26 ` 赵小强
0 siblings, 1 reply; 23+ messages in thread
From: 赵小强 @ 2013-11-13 6:06 UTC (permalink / raw)
To: Andreas Färber; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
于 11/12/2013 10:52 PM, Andreas Färber 写道:
> Resending yesterday's message since it hasn't arrived on qemu-devel...
>
> Am 11.11.2013 04:58, schrieb 赵小强:
>> 于 11/05/2013 04:51 PM, 赵小强 写道:
>>> 于 2013年11月05日 16:25, Chen Fan 写道:
>>>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
> [...]
>>>>> 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;
>>>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>>>> class has included 'realize' field.
>>>>
>>>>> } 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..bd3a5fc 100644
>>>>> --- a/include/hw/i386/apic_internal.h
>>>>> +++ b/include/hw/i386/apic_internal.h
>>>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>>>> typedef struct APICCommonClass
>>>>> {
>>>>> ICCDeviceClass parent_class;
>>>>> -
>>>>> - void (*init)(APICCommonState *s);
>>>>> +
>>>>> + /* QOM realize */
>>>>> + DeviceRealize realize;
>>>> as above.
>>>>
>>>> Thanks,
>>>> Chen
>>>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>>>> uint8_t (*get_tpr)(APICCommonState *s);
>>> Thanks for your review!!
>> Hi, Chen Fan:
>>
>> In my understanding, If we use only one 'realize'(which in
>> 'DeviceClass'), I think all the initialization work must be done in the
>> leaf child. if we add 'redundant' realize to each parent, then we can
>> call the initialization chain from DeviceClass down to leaf child's
>> parent, with each parent complete a bit further(of cause, a parent can
>> do nothing and pass to it's child directly).
>>
>> What do you think?
> Your analysis is correct. v2 is like I requested you to do it and v3
> still does iirc, so let's stick with that for "consistency". Bad word in
> this context, I know. ;)
>
> If you have some time, it would be nice if you could check whether these
> devices (the non-KVM versions at least) are covered by make check. For
> ICC bus I am certain that it is. In particular I'm wondering if we need
> certain -cpu arguments to enable the *APIC devices and have the realize
> functions actually exercised? (My assumption is no, but a confirmation
> would save time.)
>
> Regards,
> Andreas
>
I see. thanks for your reply!
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-13 6:06 ` 赵小强
@ 2013-11-29 1:26 ` 赵小强
2013-11-29 3:48 ` Andreas Färber
0 siblings, 1 reply; 23+ messages in thread
From: 赵小强 @ 2013-11-29 1:26 UTC (permalink / raw)
To: Andreas Färber; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
于 11/13/2013 02:06 PM, 赵小强 写道:
> 于 11/12/2013 10:52 PM, Andreas Färber 写道:
>> Resending yesterday's message since it hasn't arrived on qemu-devel...
>>
>> Am 11.11.2013 04:58, schrieb 赵小强:
>>> 于 11/05/2013 04:51 PM, 赵小强 写道:
>>>> 于 2013年11月05日 16:25, Chen Fan 写道:
>>>>> On Tue, 2013-11-05 at 15:55 +0800, xiaoqiang zhao wrote:
>> [...]
>>>>>> 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;
>>>>> Maybe adding 'realize' in ICCDeviceClass is redundant, because parent
>>>>> class has included 'realize' field.
>>>>>
>>>>>> } 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..bd3a5fc 100644
>>>>>> --- a/include/hw/i386/apic_internal.h
>>>>>> +++ b/include/hw/i386/apic_internal.h
>>>>>> @@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
>>>>>> typedef struct APICCommonClass
>>>>>> {
>>>>>> ICCDeviceClass parent_class;
>>>>>> -
>>>>>> - void (*init)(APICCommonState *s);
>>>>>> +
>>>>>> + /* QOM realize */
>>>>>> + DeviceRealize realize;
>>>>> as above.
>>>>>
>>>>> Thanks,
>>>>> Chen
>>>>>> void (*set_base)(APICCommonState *s, uint64_t val);
>>>>>> void (*set_tpr)(APICCommonState *s, uint8_t val);
>>>>>> uint8_t (*get_tpr)(APICCommonState *s);
>>>> Thanks for your review!!
>>> Hi, Chen Fan:
>>>
>>> In my understanding, If we use only one 'realize'(which in
>>> 'DeviceClass'), I think all the initialization work must be done in the
>>> leaf child. if we add 'redundant' realize to each parent, then we can
>>> call the initialization chain from DeviceClass down to leaf child's
>>> parent, with each parent complete a bit further(of cause, a parent can
>>> do nothing and pass to it's child directly).
>>>
>>> What do you think?
>> Your analysis is correct. v2 is like I requested you to do it and v3
>> still does iirc, so let's stick with that for "consistency". Bad word in
>> this context, I know. ;)
>>
>> If you have some time, it would be nice if you could check whether these
>> devices (the non-KVM versions at least) are covered by make check. For
>> ICC bus I am certain that it is. In particular I'm wondering if we need
>> certain -cpu arguments to enable the *APIC devices and have the realize
>> functions actually exercised? (My assumption is no, but a confirmation
>> would save time.)
>>
>> Regards,
>> Andreas
>>
> I see. thanks for your reply!
I have been busy with my work this days. I wish I could have some
time soon. A few questions about your last reply.
> it would be nice if you could check whether these
> devices (the non-KVM versions at least) are covered by make check. For
> ICC bus I am certain that it is.
1. Does "make check" mean the build target in the Makefile ? if it
is, I can not find anything about "icc_bus" under "tests" directory. Or
you refer to something else?
2. What does "enable the *APIC devices and have the realize functions
actually exercised?" mean?
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-29 1:26 ` 赵小强
@ 2013-11-29 3:48 ` Andreas Färber
2013-11-29 5:29 ` 赵小强
2013-11-29 7:22 ` 赵小强
0 siblings, 2 replies; 23+ messages in thread
From: Andreas Färber @ 2013-11-29 3:48 UTC (permalink / raw)
To: 赵小强; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
Am 29.11.2013 02:26, schrieb 赵小强:
> 于 11/12/2013 10:52 PM, Andreas Färber 写道:
>> it would be nice if you could check whether these
>> devices (the non-KVM versions at least) are covered by make check. For
>> ICC bus I am certain that it is.
> 1. Does "make check" mean the build target in the Makefile ?
Yes, in particular the make check-qtest target (check-qtest-x86_64).
> if it
> is, I can not find anything about "icc_bus" under "tests" directory. Or
> you refer to something else?
IIUC there is no strict need for an icc_bus-test.c because any test
using -M pc (or default for i386 or x86_64) is implicitly using icc_bus
and I don't recall any MMIO or similar implementation for ICC. It was
just a hot-pluggable version of SysBus. My question was, does this
implicit testing apply to the other devices touched here as well or not.
> 2. What does "enable the *APIC devices and have the realize functions
> actually exercised?" mean?
It may mean for instance, add printfs (or assertions or breakpoints or
trace points in core QOM code...) to the initialization code of those
devices, run make followed by make check to see if all of them show up.
If not, add apic-test.c, ioapic-test.c etc. using whatever -device or
-cpu command lines necessary to trigger the printfs - and then obviously
drop any debug printfs agains. But if I knew the answer already I
wouldn't ask you to investigate. :)
What I am not asking for is functional tests for registers or whatever -
you did not write that code, you should just assure that your realize
refactorings do not break device initialization accidentally.
If you do find time to work on this and find that some non-KVM device is
not covered: I was going to split off icc_bus into its own patch from 2/4.
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-29 3:48 ` Andreas Färber
@ 2013-11-29 5:29 ` 赵小强
2013-11-29 7:22 ` 赵小强
1 sibling, 0 replies; 23+ messages in thread
From: 赵小强 @ 2013-11-29 5:29 UTC (permalink / raw)
To: Andreas Färber; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
于 11/29/2013 11:48 AM, Andreas Färber 写道:
> Am 29.11.2013 02:26, schrieb 赵小强:
>> 于 11/12/2013 10:52 PM, Andreas Färber 写道:
>>> it would be nice if you could check whether these
>>> devices (the non-KVM versions at least) are covered by make check. For
>>> ICC bus I am certain that it is.
>> 1. Does "make check" mean the build target in the Makefile ?
> Yes, in particular the make check-qtest target (check-qtest-x86_64).
>
>> if it
>> is, I can not find anything about "icc_bus" under "tests" directory. Or
>> you refer to something else?
> IIUC there is no strict need for an icc_bus-test.c because any test
> using -M pc (or default for i386 or x86_64) is implicitly using icc_bus
> and I don't recall any MMIO or similar implementation for ICC. It was
> just a hot-pluggable version of SysBus. My question was, does this
> implicit testing apply to the other devices touched here as well or not.
>
>> 2. What does "enable the *APIC devices and have the realize functions
>> actually exercised?" mean?
> It may mean for instance, add printfs (or assertions or breakpoints or
> trace points in core QOM code...) to the initialization code of those
> devices, run make followed by make check to see if all of them show up.
> If not, add apic-test.c, ioapic-test.c etc. using whatever -device or
> -cpu command lines necessary to trigger the printfs - and then obviously
> drop any debug printfs agains. But if I knew the answer already I
> wouldn't ask you to investigate. :)
>
> What I am not asking for is functional tests for registers or whatever -
> you did not write that code, you should just assure that your realize
> refactorings do not break device initialization accidentally.
>
> If you do find time to work on this and find that some non-KVM device is
> not covered: I was going to split off icc_bus into its own patch from 2/4.
>
> Regards,
> Andreas
>
Ok, got it !
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-29 3:48 ` Andreas Färber
2013-11-29 5:29 ` 赵小强
@ 2013-11-29 7:22 ` 赵小强
1 sibling, 0 replies; 23+ messages in thread
From: 赵小强 @ 2013-11-29 7:22 UTC (permalink / raw)
To: Andreas Färber; +Cc: Chen Fan, pbonzini, qemu-devel, aliguori
于 11/29/2013 11:48 AM, Andreas Färber 写道:
> Am 29.11.2013 02:26, schrieb 赵小强:
>> 于 11/12/2013 10:52 PM, Andreas Färber 写道:
>>> it would be nice if you could check whether these
>>> devices (the non-KVM versions at least) are covered by make check. For
>>> ICC bus I am certain that it is.
>> 1. Does "make check" mean the build target in the Makefile ?
> Yes, in particular the make check-qtest target (check-qtest-x86_64).
>
>> if it
>> is, I can not find anything about "icc_bus" under "tests" directory. Or
>> you refer to something else?
> IIUC there is no strict need for an icc_bus-test.c because any test
> using -M pc (or default for i386 or x86_64) is implicitly using icc_bus
> and I don't recall any MMIO or similar implementation for ICC. It was
> just a hot-pluggable version of SysBus. My question was, does this
> implicit testing apply to the other devices touched here as well or not.
>
>> 2. What does "enable the *APIC devices and have the realize functions
>> actually exercised?" mean?
> It may mean for instance, add printfs (or assertions or breakpoints or
> trace points in core QOM code...) to the initialization code of those
> devices, run make followed by make check to see if all of them show up.
> If not, add apic-test.c, ioapic-test.c etc. using whatever -device or
> -cpu command lines necessary to trigger the printfs - and then obviously
> drop any debug printfs agains. But if I knew the answer already I
> wouldn't ask you to investigate. :)
>
> What I am not asking for is functional tests for registers or whatever -
> you did not write that code, you should just assure that your realize
> refactorings do not break device initialization accidentally.
>
> If you do find time to work on this and find that some non-KVM device is
> not covered: I was going to split off icc_bus into its own patch from 2/4.
>
> Regards,
> Andreas
>
Hi, Andreas:
As you said, I spent some time check my code. ( also by compile and
run a real gentoo guest)
I find ALL the non-KVM device (apic, ioapic) are covered by make
check . The KVM version are also worked correctly!
the "realize" is called as expect and device initialization is
success in both cases. (KVM and non-KVM)
So far, I can say that the changes I made is harmless :-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] ioapic: Cleanup for QOM'ify
2013-11-05 7:55 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 1/4] apic: Cleanup for QOM'ify xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus xiaoqiang zhao
@ 2013-11-05 7:55 ` xiaoqiang zhao
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 4/4] ioapic: QOM'ify ioapic xiaoqiang zhao
3 siblings, 0 replies; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 7:55 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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'
---
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [Qemu-devel] [PATCH v2 4/4] ioapic: QOM'ify ioapic
2013-11-05 7:55 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
` (2 preceding siblings ...)
2013-11-05 7:55 ` [Qemu-devel] [PATCH v2 3/4] ioapic: Cleanup for QOM'ify xiaoqiang zhao
@ 2013-11-05 7:55 ` xiaoqiang zhao
3 siblings, 0 replies; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 7:55 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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.
---
hw/i386/kvm/ioapic.c | 13 ++++++++-----
hw/intc/ioapic.c | 19 +++++++++++++------
hw/intc/ioapic_common.c | 13 ++++++++++---
include/hw/i386/ioapic_internal.h | 3 ++-
4 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 772a712..fe5e5c7 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,13 +129,14 @@ 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);
+ IOAPICCommonState *s = IOAPIC_COMMON(dev);
- memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
+ memory_region_init_reservation(&s->io_memory, NULL, TYPE_KVM_IOAPIC, 0x1000);
qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
+
}
static Property kvm_ioapic_properties[] = {
@@ -146,7 +149,7 @@ 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..718b5c0 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 = 0;
+
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 8:16 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
@ 2013-11-05 8:16 ` xiaoqiang zhao
0 siblings, 0 replies; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 8:16 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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 | 5 +++--
6 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 9a4ea7e..1cc64ac 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..5a413cc 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..bd3a5fc 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus
2013-11-05 7:53 [Qemu-devel] [PATCH v2 0/4] QOM'ify apic and ioapic xiaoqiang zhao
@ 2013-11-05 7:53 ` xiaoqiang zhao
0 siblings, 0 replies; 23+ messages in thread
From: xiaoqiang zhao @ 2013-11-05 7:53 UTC (permalink / raw)
To: qemu-devel; +Cc: xiaoqiang zhao, pbonzini, afaerber, aliguori
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'
---
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 | 5 +++--
6 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
index 9a4ea7e..1cc64ac 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..5a413cc 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..bd3a5fc 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -79,8 +79,9 @@ typedef struct APICCommonState APICCommonState;
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
^ permalink raw reply related [flat|nested] 23+ messages in thread