Re: [PATCH 4/4] xics: Merge TYPE_ICS_BASE and TYPE_ICS_SIMPLE classes

[Cédric Le Goater]

On 24/09/2019 06:59, David Gibson wrote:
> TYPE_ICS_SIMPLE is the only subtype of TYPE_ICS_BASE that's ever
> instantiated, and the only one we're ever likely to want.  The
> existence of different classes is just a hang over from when we
> (misguidedly) had separate subtypes for the KVM and non-KVM version of
> the device.
> 
> So, collapse the two classes together into just TYPE_ICS.


Well, I have been maintaining another subclass for the PHB3 MSI 
but it has never been merged and it will require some rework. 

Anyhow the base ICS code is cleaner with that patch and it
does not seem to break migration.

> 
> Signed-off-by: David Gibson <address@hidden>


Reviewed-by: Cédric Le Goater <address@hidden>

C.


> ---
>  hw/intc/xics.c        | 57 ++++++++++++++++---------------------------
>  hw/ppc/pnv_psi.c      |  2 +-
>  hw/ppc/spapr_irq.c    |  4 +--
>  include/hw/ppc/xics.h | 17 ++-----------
>  4 files changed, 26 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 9ae51bbc76..388dbba870 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -555,7 +555,7 @@ static void ics_reset_irq(ICSIRQState *irq)
>  
>  static void ics_reset(DeviceState *dev)
>  {
> -    ICSState *ics = ICS_BASE(dev);
> +    ICSState *ics = ICS(dev);
>      int i;
>      uint8_t flags[ics->nr_irqs];
>  
> @@ -573,7 +573,7 @@ static void ics_reset(DeviceState *dev)
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
>  
> -        ics_set_kvm_state(ICS_BASE(dev), &local_err);
> +        ics_set_kvm_state(ICS(dev), &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>          }
> @@ -587,7 +587,7 @@ static void ics_reset_handler(void *dev)
>  
>  static void ics_realize(DeviceState *dev, Error **errp)
>  {
> -    ICSState *ics = ICS_BASE(dev);
> +    ICSState *ics = ICS(dev);
>      Error *local_err = NULL;
>      Object *obj;
>  
> @@ -609,26 +609,14 @@ static void ics_realize(DeviceState *dev, Error **errp)
>      qemu_register_reset(ics_reset_handler, ics);
>  }
>  
> -static void ics_simple_class_init(ObjectClass *klass, void *data)
> +static void ics_instance_init(Object *obj)
>  {
> -}
> -
> -static const TypeInfo ics_simple_info = {
> -    .name = TYPE_ICS_SIMPLE,
> -    .parent = TYPE_ICS_BASE,
> -    .instance_size = sizeof(ICSState),
> -    .class_init = ics_simple_class_init,
> -    .class_size = sizeof(ICSStateClass),
> -};
> -
> -static void ics_base_instance_init(Object *obj)
> -{
> -    ICSState *ics = ICS_BASE(obj);
> +    ICSState *ics = ICS(obj);
>  
>      ics->offset = XICS_IRQ_BASE;
>  }
>  
> -static int ics_base_pre_save(void *opaque)
> +static int ics_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
>  
> @@ -639,7 +627,7 @@ static int ics_base_pre_save(void *opaque)
>      return 0;
>  }
>  
> -static int ics_base_post_load(void *opaque, int version_id)
> +static int ics_post_load(void *opaque, int version_id)
>  {
>      ICSState *ics = opaque;
>  
> @@ -657,7 +645,7 @@ static int ics_base_post_load(void *opaque, int 
> version_id)
>      return 0;
>  }
>  
> -static const VMStateDescription vmstate_ics_base_irq = {
> +static const VMStateDescription vmstate_ics_irq = {
>      .name = "ics/irq",
>      .version_id = 2,
>      .minimum_version_id = 1,
> @@ -671,46 +659,44 @@ static const VMStateDescription vmstate_ics_base_irq = {
>      },
>  };
>  
> -static const VMStateDescription vmstate_ics_base = {
> +static const VMStateDescription vmstate_ics = {
>      .name = "ics",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = ics_base_pre_save,
> -    .post_load = ics_base_post_load,
> +    .pre_save = ics_pre_save,
> +    .post_load = ics_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
>  
>          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
> -                                             vmstate_ics_base_irq,
> +                                             vmstate_ics_irq,
>                                               ICSIRQState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
>  
> -static Property ics_base_properties[] = {
> +static Property ics_properties[] = {
>      DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void ics_base_class_init(ObjectClass *klass, void *data)
> +static void ics_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = ics_realize;
> -    dc->props = ics_base_properties;
> +    dc->props = ics_properties;
>      dc->reset = ics_reset;
> -    dc->vmsd = &vmstate_ics_base;
> +    dc->vmsd = &vmstate_ics;
>  }
>  
> -static const TypeInfo ics_base_info = {
> -    .name = TYPE_ICS_BASE,
> +static const TypeInfo ics_info = {
> +    .name = TYPE_ICS,
>      .parent = TYPE_DEVICE,
> -    .abstract = true,
>      .instance_size = sizeof(ICSState),
> -    .instance_init = ics_base_instance_init,
> -    .class_init = ics_base_class_init,
> -    .class_size = sizeof(ICSStateClass),
> +    .instance_init = ics_instance_init,
> +    .class_init = ics_class_init,
>  };
>  
>  static const TypeInfo xics_fabric_info = {
> @@ -749,8 +735,7 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>  
>  static void xics_register_types(void)
>  {
> -    type_register_static(&ics_simple_info);
> -    type_register_static(&ics_base_info);
> +    type_register_static(&ics_info);
>      type_register_static(&icp_info);
>      type_register_static(&xics_fabric_info);
>  }
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 8ea81e9d8e..a997f16bb4 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -469,7 +469,7 @@ static void pnv_psi_power8_instance_init(Object *obj)
>      Pnv8Psi *psi8 = PNV8_PSI(obj);
>  
>      object_initialize_child(obj, "ics-psi",  &psi8->ics, sizeof(psi8->ics),
> -                            TYPE_ICS_SIMPLE, &error_abort, NULL);
> +                            TYPE_ICS, &error_abort, NULL);
>  }
>  
>  static const uint8_t irq_to_xivr[] = {
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index ac189c5796..6c45d2a3c0 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -98,7 +98,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, 
> int nr_irqs,
>      Object *obj;
>      Error *local_err = NULL;
>  
> -    obj = object_new(TYPE_ICS_SIMPLE);
> +    obj = object_new(TYPE_ICS);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                     &error_fatal);
> @@ -109,7 +109,7 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, 
> int nr_irqs,
>          return;
>      }
>  
> -    spapr->ics = ICS_BASE(obj);
> +    spapr->ics = ICS(obj);
>  
>      xics_spapr_init(spapr);
>  }
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 92628e7cab..d8cf206a69 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -89,21 +89,8 @@ struct PnvICPState {
>      uint32_t links[3];
>  };
>  
> -#define TYPE_ICS_BASE "ics-base"
> -#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
> -
> -/* Retain ics for sPAPR for migration from existing sPAPR guests */
> -#define TYPE_ICS_SIMPLE "ics"
> -#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
> -
> -#define ICS_BASE_CLASS(klass) \
> -     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE)
> -#define ICS_BASE_GET_CLASS(obj) \
> -     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE)
> -
> -struct ICSStateClass {
> -    DeviceClass parent_class;
> -};
> +#define TYPE_ICS "ics"
> +#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>  
>  struct ICSState {
>      /*< private >*/
>