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