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

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

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