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

[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);