From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37121) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg4O5-0001wV-2B for qemu-devel@nongnu.org; Mon, 11 Nov 2013 22:11:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vg4Nz-00060e-Rd for qemu-devel@nongnu.org; Mon, 11 Nov 2013 22:11:32 -0500 Received: from m50-134.163.com ([123.125.50.134]:39252) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vg4Ny-00060A-JG for qemu-devel@nongnu.org; Mon, 11 Nov 2013 22:11:27 -0500 Message-ID: <52819C5B.5020400@163.com> Date: Tue, 12 Nov 2013 11:11:23 +0800 From: =?UTF-8?B?6LW15bCP5by6?= MIME-Version: 1.0 References: <1383638143-11812-1-git-send-email-zxq_yx_007@163.com> <1383638143-11812-3-git-send-email-zxq_yx_007@163.com> <1383639922.556.10.camel@G08FNSTD131468> <5278B1A3.40109@163.com> <528055EC.6080600@163.com> <1384219693.29198.19.camel@G08FNSTD131468> <52818A38.50500@163.com> <1384225361.29198.51.camel@G08FNSTD131468> In-Reply-To: <1384225361.29198.51.camel@G08FNSTD131468> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 2/4] apic: QOM'ify apic & icc_bus List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chen Fan Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, aliguori@amazon.com, afaerber@suse.de 于 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! ;-)