[Qemu-devel] [PATCH 0/2] ppc/xics: simplify ICS and ICP creation

[Qemu-devel] [PATCH 0/2] ppc/xics: simplify ICS and ICP creation

[Cédric Le Goater]

Hello,

This series moves the creation of the ICS and ICP objects from the
XICS object to the sPAPR machine. This simplifies the code
significantly and prepares ground for future changes on the XICS object.

The goal is to reduce XICS to a set of Interfaces of the machine
providing accessors to the ICS and ICPs objects. 

Thanks,

C.

Cédric Le Goater (2):
  ppc/xics: remove set_nr_irqs() handler from XICSStateClass
  ppc/xics: remove set_nr_servers() handler from XICSStateClass

 hw/intc/xics.c        | 78 ++++-----------------------------------------------
 hw/intc/xics_kvm.c    | 55 +-----------------------------------
 hw/intc/xics_spapr.c  | 60 ---------------------------------------
 hw/ppc/spapr.c        | 60 ++++++++++++++++++++++++++++++---------
 include/hw/ppc/xics.h |  2 --
 5 files changed, 53 insertions(+), 202 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[Cédric Le Goater]

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>
---
 hw/intc/xics.c        | 41 ++++++--------------------------------
 hw/intc/xics_kvm.c    | 34 --------------------------------
 hw/intc/xics_spapr.c  | 34 --------------------------------
 hw/ppc/spapr.c        | 54 ++++++++++++++++++++++++++++++++++++---------------
 include/hw/ppc/xics.h |  2 --
 5 files changed, 44 insertions(+), 121 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 095c16a30082..58e7f2f86ed4 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);
@@ -746,12 +711,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..6bb8c6d14865 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -358,18 +358,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 +377,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 +428,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 +451,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 +458,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 +466,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 6f37288a7f66..c84b2f4f938e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -95,22 +95,43 @@
 
 #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 = qdev_create(NULL, 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_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;
+    }
+
+    ics->xics = xics;
+    QLIST_INSERT_HEAD(&xics->ics, ics, list);
+
+    return xics;
+
+error:
+    error_propagate(errp, err);
+    if (ics) {
+        object_unparent(OBJECT(ics));
     }
-    return XICS_COMMON(dev);
+    object_unparent(OBJECT(xics));
+    return NULL;
 }
 
 static XICSState *xics_system_init(MachineState *machine,
@@ -122,8 +143,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 +155,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 e8794aa5cba8..8f60f65dd464 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -74,7 +74,6 @@ struct XICSStateClass {
     SysBusDeviceClass 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;
 };
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[David Gibson]

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

On Mon, Feb 13, 2017 at 03:09:16PM +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>

I like the basic idea here: while the ics and icp objects are pretty
straightforward, the "xics" object has always been a bit of a hack,
with logic that really belongs in the machine.

But.. I don't think the approach here really works.  Specifically..

[snip]
> -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 = qdev_create(NULL, 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_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;
> +    }
> +
> +    ics->xics = xics;
> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);

Poking into the ics and xics objects directly from the machine here
violates abstraction even worse than the existing xics device does.
In fact, avoiding that is basically why the xics device exists in the
first place.

I've thought about this a bit more, and I think I know how to solve
this better now.

I think that "xics" shouldn't be a concrete object at all, instead it
should be a QOM interface, implemented by the machine type.  Both ICS
and ICP would take a link property to find their xics.  The xics
interface would provide methods to return an ics object given an irq
number and to return an icp object given a server number. This gives
full control of the irq and server number spaces back to the machine
type, which is really where it belongs.

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

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[Cédric Le Goater]

On 02/14/2017 06:02 AM, David Gibson wrote:
> On Mon, Feb 13, 2017 at 03:09:16PM +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>
> 
> I like the basic idea here: while the ics and icp objects are pretty
> straightforward, the "xics" object has always been a bit of a hack,
> with logic that really belongs in the machine.
> 
> But.. I don't think the approach here really works.  Specifically..
> 
> [snip]
>> -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 = qdev_create(NULL, 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_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;
>> +    }
>> +
>> +    ics->xics = xics;
>> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> 
> Poking into the ics and xics objects directly from the machine here
> violates abstraction even worse than the existing xics device does.
> In fact, avoiding that is basically why the xics device exists in the
> first place.

Well, currently, xics_set_nr_servers() also does a :

	icp->xics = xics;

So, I think we can live with it until we move the ICS and ICP objects 
out of XICS ?

if this is the only worrisome problem in the patch, I can start the
series with a QOM Interface handler implemented at the machine level, 
something like : 

	void (*ics_insert)(ICSState *ics);

doing the insert above to hide the temporary hideousness. Is that ok ? 

And, as you propose below, I think we can add the 'xics' link property 
right now to get rid of :

	ic[ps]->xics = xics;

> I've thought about this a bit more, and I think I know how to solve
> this better now.
>
> I think that "xics" shouldn't be a concrete object at all, instead it
> should be a QOM interface, implemented by the machine type.  Both ICS
> and ICP would take a link property to find their xics. 

yes.

> The xics
> interface would provide methods to return an ics object given an irq
> number and to return an icp object given a server number. This gives
> full control of the irq and server number spaces back to the machine
> type, which is really where it belongs.

Yes, I agree, we started talking about it a while ago :

    http://lists.nongnu.org/archive/html/qemu-ppc/2016-11/msg00056.html

and this is what this first series is trying to do. It prepares ground 
by emptying the XICS object. 

The next step is to move out :

    ICPState *ss;
    QLIST_HEAD(, ICSState) ics;

from XICSState to sPAPRMachineState I think. This is when the QOM 
interfaces will come in play but we need to untangle the code slowly, 
to understand what is being done (honestly, it is not that obvious).  

As for xics->nr_servers, the only place where I still have doubts is 
ics_simple_post_load(). It should be easier after more cleanups.

This is like mahjong, you need to start from the right piece :)

Thanks,
C.

 

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[Cédric Le Goater]

On 02/14/2017 08:04 AM, Cédric Le Goater wrote:
> On 02/14/2017 06:02 AM, David Gibson wrote:
>> On Mon, Feb 13, 2017 at 03:09:16PM +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>
>>
>> I like the basic idea here: while the ics and icp objects are pretty
>> straightforward, the "xics" object has always been a bit of a hack,
>> with logic that really belongs in the machine.
>>
>> But.. I don't think the approach here really works.  Specifically..
>>
>> [snip]
>>> -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 = qdev_create(NULL, 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_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;
>>> +    }
>>> +
>>> +    ics->xics = xics;
>>> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>>
>> Poking into the ics and xics objects directly from the machine here
>> violates abstraction even worse than the existing xics device does.
>> In fact, avoiding that is basically why the xics device exists in the
>> first place.
> 
> Well, currently, xics_set_nr_servers() also does a :
> 
> 	icp->xics = xics;
> 
> So, I think we can live with it until we move the ICS and ICP objects 
> out of XICS ?
> 
> if this is the only worrisome problem in the patch, I can start the
> series with a QOM Interface handler implemented at the machine level, 
> something like : 
> 
> 	void (*ics_insert)(ICSState *ics);
> 
> doing the insert above to hide the temporary hideousness. Is that ok ? 

After some thought, I don't think this one is important. At the 
machine level, it seems OK to me to insert an ICS in the XICS list. 
I agree that XICS should disappear, but it is still there for the 
moment. 

> And, as you propose below, I think we can add the 'xics' link property 
> right now to get rid of :
> 
> 	ic[ps]->xics = xics;

I have a v2 ready adding the 'xics' link property but keeping the 
QLIST_INSERT_HEAD() call in try_create_xics(). Do you think it is 
worth sending as an initial cleanup :

 5 files changed, 96 insertions(+), 225 deletions(-)

or do you want the full picture to be addressed ? 

Thanks,

C. 

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[David Gibson]

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

On Tue, Feb 14, 2017 at 03:52:09PM +0100, Cédric Le Goater wrote:
> On 02/14/2017 08:04 AM, Cédric Le Goater wrote:
> > On 02/14/2017 06:02 AM, David Gibson wrote:
> >> On Mon, Feb 13, 2017 at 03:09:16PM +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>
> >>
> >> I like the basic idea here: while the ics and icp objects are pretty
> >> straightforward, the "xics" object has always been a bit of a hack,
> >> with logic that really belongs in the machine.
> >>
> >> But.. I don't think the approach here really works.  Specifically..
> >>
> >> [snip]
> >>> -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 = qdev_create(NULL, 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_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;
> >>> +    }
> >>> +
> >>> +    ics->xics = xics;
> >>> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> >>
> >> Poking into the ics and xics objects directly from the machine here
> >> violates abstraction even worse than the existing xics device does.
> >> In fact, avoiding that is basically why the xics device exists in the
> >> first place.
> > 
> > Well, currently, xics_set_nr_servers() also does a :
> > 
> > 	icp->xics = xics;
> > 
> > So, I think we can live with it until we move the ICS and ICP objects 
> > out of XICS ?
> > 
> > if this is the only worrisome problem in the patch, I can start the
> > series with a QOM Interface handler implemented at the machine level, 
> > something like : 
> > 
> > 	void (*ics_insert)(ICSState *ics);
> > 
> > doing the insert above to hide the temporary hideousness. Is that ok ? 
> 
> After some thought, I don't think this one is important. At the 
> machine level, it seems OK to me to insert an ICS in the XICS list. 
> I agree that XICS should disappear, but it is still there for the 
> moment. 
> 
> > And, as you propose below, I think we can add the 'xics' link property 
> > right now to get rid of :
> > 
> > 	ic[ps]->xics = xics;
> 
> I have a v2 ready adding the 'xics' link property but keeping the 
> QLIST_INSERT_HEAD() call in try_create_xics(). Do you think it is 
> worth sending as an initial cleanup :
> 
>  5 files changed, 96 insertions(+), 225 deletions(-)
> 
> or do you want the full picture to be addressed ? 

I don't think I'll want to merge until the full picture is addressed.
However, I'm fine with posting incomplete versions for earlier review
- just label them RFC and note in the 0/N comment that there's more
work to be done to complete the picture.

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

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[Cédric Le Goater]

On 02/15/2017 02:59 AM, David Gibson wrote:
> I don't think I'll want to merge until the full picture is addressed.
> However, I'm fine with posting incomplete versions for earlier review
> - just label them RFC and note in the 0/N comment that there's more
> work to be done to complete the picture.

OK. This is a good way to discuss. I will do my best.

Thanks,

C.

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: remove set_nr_irqs() handler from XICSStateClass

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:04:42AM +0100, Cédric Le Goater wrote:
> On 02/14/2017 06:02 AM, David Gibson wrote:
> > On Mon, Feb 13, 2017 at 03:09:16PM +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>
> > 
> > I like the basic idea here: while the ics and icp objects are pretty
> > straightforward, the "xics" object has always been a bit of a hack,
> > with logic that really belongs in the machine.
> > 
> > But.. I don't think the approach here really works.  Specifically..
> > 
> > [snip]
> >> -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 = qdev_create(NULL, 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_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;
> >> +    }
> >> +
> >> +    ics->xics = xics;
> >> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
> > 
> > Poking into the ics and xics objects directly from the machine here
> > violates abstraction even worse than the existing xics device does.
> > In fact, avoiding that is basically why the xics device exists in the
> > first place.
> 
> Well, currently, xics_set_nr_servers() also does a :
> 
> 	icp->xics = xics;

Well.. yes, but that's at least from code within the xics files,
moving it into the machine makes the abstraction breakage worse.  (You
could think of it at the moment as treating XICS as a "friend" class
of ICS and ICP).

> So, I think we can live with it until we move the ICS and ICP objects 
> out of XICS ?

As an interim step it might be acceptable, yes.  I wouldn't want to
apply the change except as part of a series which completes the
transition, though.

> if this is the only worrisome problem in the patch, I can start the
> series with a QOM Interface handler implemented at the machine level, 
> something like : 

The QLIST_INSERT_HEAD() is also a problem.  Inside the XICS object it
altered ICS internal state, which is bad.  Inside the machine it
alters both XICS and ICS internal state, which is worse.

> 
> 	void (*ics_insert)(ICSState *ics);
> 
> doing the insert above to hide the temporary hideousness. Is that ok ? 

I don't 100% follow how you intend to use that, but it sounds like it
mightg work.

> And, as you propose below, I think we can add the 'xics' link property 
> right now to get rid of :
> 
> 	ic[ps]->xics = xics;

Yes, I think that's a better option.

> > I've thought about this a bit more, and I think I know how to solve
> > this better now.
> >
> > I think that "xics" shouldn't be a concrete object at all, instead it
> > should be a QOM interface, implemented by the machine type.  Both ICS
> > and ICP would take a link property to find their xics. 
> 
> yes.
> 
> > The xics
> > interface would provide methods to return an ics object given an irq
> > number and to return an icp object given a server number. This gives
> > full control of the irq and server number spaces back to the machine
> > type, which is really where it belongs.
> 
> Yes, I agree, we started talking about it a while ago :
> 
>     http://lists.nongnu.org/archive/html/qemu-ppc/2016-11/msg00056.html

Ah, sorry, I'd forgotten that.

> and this is what this first series is trying to do. It prepares ground 
> by emptying the XICS object. 
> 
> The next step is to move out :
> 
>     ICPState *ss;
>     QLIST_HEAD(, ICSState) ics;
> 
> from XICSState to sPAPRMachineState I think. This is when the QOM 
> interfaces will come in play but we need to untangle the code slowly, 
> to understand what is being done (honestly, it is not that obvious).  
> 
> As for xics->nr_servers, the only place where I still have doubts is 
> ics_simple_post_load(). It should be easier after more cleanups.
> 
> This is like mahjong, you need to start from the right piece :)

Ah, ok.  Sorry, I'd forgotten the context of that discussion.  As
initial steps to a more thorough cleanup, these look fine.  However, I
don't want to commit them, except as part of a series which ends up
with less abstration violations than we started with, rather than
more.

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

[Qemu-devel] [PATCH 2/2] ppc/xics: remove set_nr_servers() handler from XICSStateClass

[Cédric Le Goater]

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 <clg@kaod.org>
---
 hw/intc/xics.c       | 37 -------------------------------------
 hw/intc/xics_kvm.c   | 21 +--------------------
 hw/intc/xics_spapr.c | 26 --------------------------
 hw/ppc/spapr.c       | 24 +++++++++++++++++-------
 4 files changed, 18 insertions(+), 90 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 58e7f2f86ed4..002d142a4792 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -170,48 +170,11 @@ void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
     }
 }
 
-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)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 6bb8c6d14865..9ec33c9b0d94 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -358,12 +358,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,
@@ -376,9 +370,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,
@@ -428,16 +420,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;
@@ -458,7 +440,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 c84b2f4f938e..d7e484f75c55 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;
     }
@@ -123,6 +123,16 @@ static XICSState *try_create_xics(const char *type, const char *type_ics,
     ics->xics = xics;
     QLIST_INSERT_HEAD(&xics->ics, ics, list);
 
+    xics_set_nr_servers(xics, nr_servers, type_icp, NULL);
+
+    for (i = 0; i < nr_servers; i++) {
+        object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
+                                 &err);
+        if (err) {
+            goto error;
+        }
+    }
+
     return xics;
 
 error:
@@ -144,7 +154,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,
@@ -155,8 +165,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;
-- 
2.7.4