Re: [Qemu-ppc] [PATCH v2 02/22] ppc/xics: remove set_nr_servers() handler from XICSStateClass

[David Gibson]

On Thu, Feb 16, 2017 at 02:47:25PM +0100, Cédric Le Goater wrote:
> Today, the ICP (Interrupt Controller Presenter) objects are created by
> the 'nr_servers' property handler of the XICS object and a class
> handler. They are realized in the XICS object realize routine.
> 
> Let's simplify the process by creating the ICP objects along with the
> XICS object at the machine level.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>

Reviewed-by: David Gibson <address@hidden>

> ---
> 
>  Changes since v1:
> 
>  - added a XICS link to the ICS object
>  - removed xics_set_nr_servers() routine
>  - removed set_nr_servers() class handler
> 
>  hw/intc/xics.c        | 74 
> +++++++++++++--------------------------------------
>  hw/intc/xics_kvm.c    | 21 +--------------
>  hw/intc/xics_spapr.c  | 26 ------------------
>  hw/ppc/spapr.c        | 30 ++++++++++++++++-----
>  include/hw/ppc/xics.h |  3 ---
>  5 files changed, 42 insertions(+), 112 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e70d3b8b1095..9f22814815c9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -151,67 +151,11 @@ static void xics_common_reset(DeviceState *d)
>      }
>  }
>  
> -void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> -                         const char *typename, Error **errp)
> -{
> -    int i;
> -
> -    xics->nr_servers = nr_servers;
> -
> -    xics->ss = g_malloc0(xics->nr_servers * sizeof(ICPState));
> -    for (i = 0; i < xics->nr_servers; i++) {
> -        char name[32];
> -        ICPState *icp = &xics->ss[i];
> -
> -        object_initialize(icp, sizeof(*icp), typename);
> -        snprintf(name, sizeof(name), "icp[%d]", i);
> -        object_property_add_child(OBJECT(xics), name, OBJECT(icp), errp);
> -        icp->xics = xics;
> -    }
> -}
> -
> -static void xics_prop_get_nr_servers(Object *obj, Visitor *v,
> -                                     const char *name, void *opaque,
> -                                     Error **errp)
> -{
> -    XICSState *xics = XICS_COMMON(obj);
> -    int64_t value = xics->nr_servers;
> -
> -    visit_type_int(v, name, &value, errp);
> -}
> -
> -static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
> -                                     const char *name, void *opaque,
> -                                     Error **errp)
> -{
> -    XICSState *xics = XICS_COMMON(obj);
> -    XICSStateClass *xsc = 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_servers) {
> -        error_setg(errp, "Number of servers is already set to %u",
> -                   xics->nr_servers);
> -        return;
> -    }
> -
> -    assert(xsc->set_nr_servers);
> -    xsc->set_nr_servers(xics, value, errp);
> -}
> -
>  static void xics_common_initfn(Object *obj)
>  {
>      XICSState *xics = XICS_COMMON(obj);
>  
>      QLIST_INIT(&xics->ics);
> -    object_property_add(obj, "nr_servers", "int",
> -                        xics_prop_get_nr_servers, xics_prop_set_nr_servers,
> -                        NULL, NULL, NULL);
>  }
>  
>  static void xics_common_class_init(ObjectClass *oc, void *data)
> @@ -450,12 +394,30 @@ static void icp_reset(DeviceState *dev)
>      qemu_set_irq(icp->output, 0);
>  }
>  
> +static void icp_realize(DeviceState *dev, Error **errp)
> +{
> +    ICPState *icp = ICP(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;
> +    }
> +
> +    icp->xics = XICS_COMMON(obj);
> +}
> +
> +
>  static void icp_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->reset = icp_reset;
>      dc->vmsd = &vmstate_icp_server;
> +    dc->realize = icp_realize;
>  }
>  
>  static const TypeInfo icp_info = {
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4a6c0522feb6..6cabc11f6be1 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -368,12 +368,6 @@ static void xics_kvm_cpu_setup(XICSState *xics, 
> PowerPCCPU *cpu)
>      ss->cap_irq_xics_enabled = true;
>  }
>  
> -static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> -                                    Error **errp)
> -{
> -    xics_set_nr_servers(xics, nr_servers, TYPE_KVM_ICP, errp);
> -}
> -
>  static void rtas_dummy(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                         uint32_t token,
>                         uint32_t nargs, target_ulong args,
> @@ -386,9 +380,7 @@ static void rtas_dummy(PowerPCCPU *cpu, sPAPRMachineState 
> *spapr,
>  static void xics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
> -    XICSState *xics = XICS_COMMON(dev);
> -    int i, rc;
> -    Error *error = NULL;
> +    int rc;
>      struct kvm_create_device xics_create_device = {
>          .type = KVM_DEV_TYPE_XICS,
>          .flags = 0,
> @@ -438,16 +430,6 @@ static void xics_kvm_realize(DeviceState *dev, Error 
> **errp)
>  
>      xicskvm->kernel_xics_fd = xics_create_device.fd;
>  
> -    assert(xics->nr_servers);
> -    for (i = 0; i < xics->nr_servers; i++) {
> -        object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
> -                                 &error);
> -        if (error) {
> -            error_propagate(errp, error);
> -            goto fail;
> -        }
> -    }
> -
>      kvm_kernel_irqchip = true;
>      kvm_msi_via_irqfd_allowed = true;
>      kvm_gsi_direct_mapping = true;
> @@ -468,7 +450,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_servers = xics_kvm_set_nr_servers;
>  }
>  
>  static const TypeInfo xics_spapr_kvm_info = {
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 03e42a866603..859b5675e175 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -239,23 +239,8 @@ static void rtas_int_on(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
>  
> -static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> -                                      Error **errp)
> -{
> -    xics_set_nr_servers(xics, nr_servers, TYPE_ICP, errp);
> -}
> -
>  static void xics_spapr_realize(DeviceState *dev, Error **errp)
>  {
> -    XICSState *xics = XICS_SPAPR(dev);
> -    Error *error = NULL;
> -    int i;
> -
> -    if (!xics->nr_servers) {
> -        error_setg(errp, "Number of servers needs to be greater 0");
> -        return;
> -    }
> -
>      /* Registration of global state belongs into realize */
>      spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
>      spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive);
> @@ -268,24 +253,13 @@ static void xics_spapr_realize(DeviceState *dev, Error 
> **errp)
>      spapr_register_hypercall(H_XIRR_X, h_xirr_x);
>      spapr_register_hypercall(H_EOI, h_eoi);
>      spapr_register_hypercall(H_IPOLL, h_ipoll);
> -
> -    for (i = 0; i < xics->nr_servers; i++) {
> -        object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
> -                                 &error);
> -        if (error) {
> -            error_propagate(errp, error);
> -            return;
> -        }
> -    }
>  }
>  
>  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_servers = xics_spapr_set_nr_servers;
>  }
>  
>  static const TypeInfo xics_spapr_info = {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 94b1e8e3227a..5d7c35de8cd9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -96,17 +96,17 @@
>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>  
>  static XICSState *try_create_xics(const char *type, const char *type_ics,
> -                                  int nr_servers, int nr_irqs, Error **errp)
> +                                  const char *type_icp, int nr_servers,
> +                                  int nr_irqs, Error **errp)
>  {
>      Error *err = NULL, *local_err = NULL;
>      XICSState *xics;
>      ICSState *ics = NULL;
> +    int i;
>  
>      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);
> +    object_property_set_bool(OBJECT(xics), true, "realized", &err);
>      if (err) {
>          goto error;
>      }
> @@ -122,6 +122,22 @@ static XICSState *try_create_xics(const char *type, 
> const char *type_ics,
>      }
>      QLIST_INSERT_HEAD(&xics->ics, ics, list);
>  
> +    xics->ss = g_malloc0(nr_servers * sizeof(ICPState));
> +    xics->nr_servers = nr_servers;
> +
> +    for (i = 0; i < nr_servers; i++) {
> +        ICPState *icp = &xics->ss[i];
> +
> +        object_initialize(icp, sizeof(*icp), type_icp);
> +        object_property_add_child(OBJECT(xics), "icp[*]", OBJECT(icp), NULL);
> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(xics), 
> NULL);
> +        object_property_set_bool(OBJECT(icp), true, "realized", &err);
> +        if (err) {
> +            goto error;
> +        }
> +        object_unref(OBJECT(icp));
> +    }
> +
>      return xics;
>  
>  error:
> @@ -143,7 +159,7 @@ static XICSState *xics_system_init(MachineState *machine,
>  
>          if (machine_kernel_irqchip_allowed(machine)) {
>              xics = try_create_xics(TYPE_XICS_SPAPR_KVM, TYPE_ICS_KVM,
> -                                   nr_servers, nr_irqs, &err);
> +                                   TYPE_KVM_ICP, nr_servers, nr_irqs, &err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !xics) {
>              error_reportf_err(err,
> @@ -154,8 +170,8 @@ static XICSState *xics_system_init(MachineState *machine,
>      }
>  
>      if (!xics) {
> -        xics = try_create_xics(TYPE_XICS_SPAPR, TYPE_ICS_SIMPLE, nr_servers,
> -                               nr_irqs, errp);
> +        xics = try_create_xics(TYPE_XICS_SPAPR, TYPE_ICS_SIMPLE, TYPE_ICP,
> +                               nr_servers, nr_irqs, errp);
>      }
>  
>      return xics;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 8fe3c4d2ceab..fc4abcd4e796 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_servers)(XICSState *icp, uint32_t nr_servers, Error 
> **errp);
>  };
>  
>  struct XICSState {
> @@ -189,8 +188,6 @@ void spapr_dt_xics(XICSState *xics, void *fdt, uint32_t 
> phandle);
>  
>  void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
>  void xics_cpu_destroy(XICSState *icp, PowerPCCPU *cpu);
> -void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
> -                         const char *typename, Error **errp);
>  
>  /* Internal XICS interfaces */
>  int xics_get_cpu_index_by_dt_id(int cpu_dt_id);

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

signature.asc
Description: PGP signature