|
于 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'.
|