Re: [Qemu-devel] [PATCH v2 01/22] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 15022 bytes --]

On Thu, Feb 16, 2017 at 02:47:24PM +0100, Cédric Le Goater wrote:
> Today, the ICS (Interrupt Controller Source) object is created and
> realized by the init and realize routines of the XICS object, but some
> of the parameters are only known at the machine level.
> 
> These parameters are passed from the sPAPR machine to the ICS object
> in a rather convoluted way using property handlers and a class handler
> of the XICS object. The number of irqs required to allocate the IRQ
> state objects in the ICS realize routine is one of them.
> 
> Let's simplify the process by creating the ICS object along with the
> XICS object at the machine level and link the ICS into the XICS list
> of ICSs at this level also. In the sPAPR machine, there is only a
> single ICS but that will change with the PowerNV machine.
> 
> Also, QOMify the creation of the objects and get rid of the
> superfluous code.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Changes since v1:
> 
>  - added a XICS link to the ICS object
> 
>  hw/intc/xics.c        | 51 ++++++++++++++++---------------------------------
>  hw/intc/xics_kvm.c    | 44 ++++++++++--------------------------------
>  hw/intc/xics_spapr.c  | 34 ---------------------------------
>  hw/ppc/spapr.c        | 53 +++++++++++++++++++++++++++++++++++----------------
>  include/hw/ppc/xics.h |  2 --
>  5 files changed, 63 insertions(+), 121 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 372b8311fb8b..e70d3b8b1095 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -151,38 +151,6 @@ static void xics_common_reset(DeviceState *d)
>      }
>  }
>  
> -static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    XICSState *xics = XICS_COMMON(obj);
> -    int64_t value = xics->nr_irqs;
> -
> -    visit_type_int(v, name, &value, errp);
> -}
> -
> -static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
> -                                  void *opaque, Error **errp)
> -{
> -    XICSState *xics = XICS_COMMON(obj);
> -    XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
> -    Error *error = NULL;
> -    int64_t value;
> -
> -    visit_type_int(v, name, &value, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -    if (xics->nr_irqs) {
> -        error_setg(errp, "Number of interrupts is already set to %u",
> -                   xics->nr_irqs);
> -        return;
> -    }
> -
> -    assert(info->set_nr_irqs);
> -    info->set_nr_irqs(xics, value, errp);
> -}
> -
>  void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>                           const char *typename, Error **errp)
>  {
> @@ -241,9 +209,6 @@ static void xics_common_initfn(Object *obj)
>      XICSState *xics = XICS_COMMON(obj);
>  
>      QLIST_INIT(&xics->ics);
> -    object_property_add(obj, "nr_irqs", "int",
> -                        xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
> -                        NULL, NULL, NULL);
>      object_property_add(obj, "nr_servers", "int",
>                          xics_prop_get_nr_servers, xics_prop_set_nr_servers,
>                          NULL, NULL, NULL);
> @@ -737,6 +702,16 @@ static void ics_simple_initfn(Object *obj)
>  static void ics_simple_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_SIMPLE(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +
> +    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> +    if (!obj) {
> +        error_setg(errp, "%s: required link 'xics' not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +    ics->xics = XICS_COMMON(obj);
>  
>      if (!ics->nr_irqs) {
>          error_setg(errp, "Number of interrupts needs to be greater 0");
> @@ -746,12 +721,18 @@ static void ics_simple_realize(DeviceState *dev, Error **errp)
>      ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>  }
>  
> +static Property ics_simple_properties[] = {
> +    DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ics_simple_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      ICSStateClass *isc = ICS_BASE_CLASS(klass);
>  
>      dc->realize = ics_simple_realize;
> +    dc->props = ics_simple_properties;
>      dc->vmsd = &vmstate_ics_simple;
>      dc->reset = ics_simple_reset;
>      isc->post_load = ics_simple_post_load;
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 17694eaa8709..4a6c0522feb6 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -294,6 +294,16 @@ static void ics_kvm_reset(DeviceState *dev)
>  static void ics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_SIMPLE(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +
> +    obj = object_property_get_link(OBJECT(dev), "xics", &err);
> +    if (!obj) {
> +        error_setg(errp, "%s: required link 'xics' not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +    ics->xics = XICS_COMMON(obj);

Couldn't you have this logic in the base class rather than repeating
it in ics simple and ics kvm?

>      if (!ics->nr_irqs) {
>          error_setg(errp, "Number of interrupts needs to be greater 0");
> @@ -358,18 +368,6 @@ static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
>      ss->cap_irq_xics_enabled = true;
>  }
>  
> -static void xics_kvm_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
> -                                 Error **errp)
> -{
> -    ICSState *ics = QLIST_FIRST(&xics->ics);
> -
> -    /* This needs to be deprecated ... */
> -    xics->nr_irqs = nr_irqs;
> -    if (ics) {
> -        ics->nr_irqs = nr_irqs;
> -    }
> -}
> -
>  static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>                                      Error **errp)
>  {
> @@ -389,7 +387,6 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
>      XICSState *xics = XICS_COMMON(dev);
> -    ICSState *ics;
>      int i, rc;
>      Error *error = NULL;
>      struct kvm_create_device xics_create_device = {
> @@ -441,14 +438,6 @@ static void xics_kvm_realize(DeviceState *dev, Error **errp)
>  
>      xicskvm->kernel_xics_fd = xics_create_device.fd;
>  
> -    QLIST_FOREACH(ics, &xics->ics, list) {
> -        object_property_set_bool(OBJECT(ics), true, "realized", &error);
> -        if (error) {
> -            error_propagate(errp, error);
> -            goto fail;
> -        }
> -    }
> -
>      assert(xics->nr_servers);
>      for (i = 0; i < xics->nr_servers; i++) {
>          object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
> @@ -472,17 +461,6 @@ fail:
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
>  }
>  
> -static void xics_kvm_initfn(Object *obj)
> -{
> -    XICSState *xics = XICS_COMMON(obj);
> -    ICSState *ics;
> -
> -    ics = ICS_SIMPLE(object_new(TYPE_ICS_KVM));
> -    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
> -    ics->xics = xics;
> -    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> -}
> -
>  static void xics_kvm_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -490,7 +468,6 @@ static void xics_kvm_class_init(ObjectClass *oc, void *data)
>  
>      dc->realize = xics_kvm_realize;
>      xsc->cpu_setup = xics_kvm_cpu_setup;
> -    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
>      xsc->set_nr_servers = xics_kvm_set_nr_servers;
>  }
>  
> @@ -499,7 +476,6 @@ static const TypeInfo xics_spapr_kvm_info = {
>      .parent        = TYPE_XICS_COMMON,
>      .instance_size = sizeof(KVMXICSState),
>      .class_init    = xics_kvm_class_init,
> -    .instance_init = xics_kvm_initfn,
>  };
>  
>  static void xics_kvm_register_types(void)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e3f1c5e95b2..03e42a866603 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -239,18 +239,6 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -static void xics_spapr_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
> -                                   Error **errp)
> -{
> -    ICSState *ics = QLIST_FIRST(&xics->ics);
> -
> -    /* This needs to be deprecated ... */
> -    xics->nr_irqs = nr_irqs;
> -    if (ics) {
> -        ics->nr_irqs = nr_irqs;
> -    }
> -}
> -
>  static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>                                        Error **errp)
>  {
> @@ -260,7 +248,6 @@ static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>  static void xics_spapr_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *xics = XICS_SPAPR(dev);
> -    ICSState *ics;
>      Error *error = NULL;
>      int i;
>  
> @@ -282,14 +269,6 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
>  
> -    QLIST_FOREACH(ics, &xics->ics, list) {
> -        object_property_set_bool(OBJECT(ics), true, "realized", &error);
> -        if (error) {
> -            error_propagate(errp, error);
> -            return;
> -        }
> -    }
> -
>      for (i = 0; i < xics->nr_servers; i++) {
>          object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>                                   &error);
> @@ -300,24 +279,12 @@ static void xics_spapr_realize(DeviceState *dev, Error **errp)
>      }
>  }
>  
> -static void xics_spapr_initfn(Object *obj)
> -{
> -    XICSState *xics = XICS_SPAPR(obj);
> -    ICSState *ics;
> -
> -    ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
> -    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
> -    ics->xics = xics;
> -    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> -}
> -
>  static void xics_spapr_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      XICSStateClass *xsc = XICS_SPAPR_CLASS(oc);
>  
>      dc->realize = xics_spapr_realize;
> -    xsc->set_nr_irqs = xics_spapr_set_nr_irqs;
>      xsc->set_nr_servers = xics_spapr_set_nr_servers;
>  }
>  
> @@ -327,7 +294,6 @@ static const TypeInfo xics_spapr_info = {
>      .instance_size = sizeof(XICSState),
>      .class_size = sizeof(XICSStateClass),
>      .class_init    = xics_spapr_class_init,
> -    .instance_init = xics_spapr_initfn,
>  };
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9b6ad0f2fd77..94b1e8e3227a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -95,22 +95,42 @@
>  
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>  
> -static XICSState *try_create_xics(const char *type, int nr_servers,
> -                                  int nr_irqs, Error **errp)
> -{
> -    Error *err = NULL;
> -    DeviceState *dev;
> +static XICSState *try_create_xics(const char *type, const char *type_ics,
> +                                  int nr_servers, int nr_irqs, Error **errp)
> +{
> +    Error *err = NULL, *local_err = NULL;
> +    XICSState *xics;
> +    ICSState *ics = NULL;
> +
> +    xics = XICS_COMMON(object_new(type));
> +    qdev_set_parent_bus(DEVICE(xics), sysbus_get_default());
> +    object_property_set_int(OBJECT(xics), nr_servers, "nr_servers", &err);
> +    object_property_set_bool(OBJECT(xics), true, "realized", &local_err);
> +    error_propagate(&err, local_err);
> +    if (err) {
> +        goto error;
> +    }
>  
> -    dev = DEVICE(object_new(type));
> -    qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
> -    qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
> -    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    ics = ICS_SIMPLE(object_new(type_ics));
> +    object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
> +    object_property_set_int(OBJECT(ics), nr_irqs, "nr-irqs", &err);
> +    object_property_add_const_link(OBJECT(ics), "xics", OBJECT(xics), NULL);
> +    object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
> +    error_propagate(&err, local_err);
>      if (err) {
> -        error_propagate(errp, err);
> -        object_unparent(OBJECT(dev));
> -        return NULL;
> +        goto error;
>      }
> -    return XICS_COMMON(dev);
> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> +
> +    return xics;
> +
> +error:
> +    error_propagate(errp, err);
> +    if (ics) {
> +        object_unparent(OBJECT(ics));
> +    }
> +    object_unparent(OBJECT(xics));
> +    return NULL;
>  }
>  
>  static XICSState *xics_system_init(MachineState *machine,
> @@ -122,8 +142,8 @@ static XICSState *xics_system_init(MachineState *machine,
>          Error *err = NULL;
>  
>          if (machine_kernel_irqchip_allowed(machine)) {
> -            xics = try_create_xics(TYPE_XICS_SPAPR_KVM, nr_servers, nr_irqs,
> -                                   &err);
> +            xics = try_create_xics(TYPE_XICS_SPAPR_KVM, TYPE_ICS_KVM,
> +                                   nr_servers, nr_irqs, &err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !xics) {
>              error_reportf_err(err,
> @@ -134,7 +154,8 @@ static XICSState *xics_system_init(MachineState *machine,
>      }
>  
>      if (!xics) {
> -        xics = try_create_xics(TYPE_XICS_SPAPR, nr_servers, nr_irqs, errp);
> +        xics = try_create_xics(TYPE_XICS_SPAPR, TYPE_ICS_SIMPLE, nr_servers,
> +                               nr_irqs, errp);
>      }
>  
>      return xics;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 3f0c31610aa4..8fe3c4d2ceab 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -74,7 +74,6 @@ struct XICSStateClass {
>      DeviceClass parent_class;
>  
>      void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
> -    void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
>      void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error **errp);
>  };
>  
> @@ -83,7 +82,6 @@ struct XICSState {
>      SysBusDevice parent_obj;
>      /*< public >*/
>      uint32_t nr_servers;
> -    uint32_t nr_irqs;
>      ICPState *ss;
>      QLIST_HEAD(, ICSState) ics;
>  };

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]