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

[赵小强 <zxq_yx_007@163.com>]

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