[Qemu-devel] [PATCH 0/2] ppc/xics: rework the ICS classes inheritance tree

[Qemu-devel] [PATCH 0/2] ppc/xics: rework the ICS classes inheritance tree

[Cédric Le Goater]

Hello,

First patch moves the common ICS code the ICS_BASE class. It makes the
class hierarchy much cleaner and removes duplicated code. Second patch
simply introduces ICP DeviceRealize and DeviceReset handler.

As we are touching the location of the objects states, migration
compatibilty needs to be checked. The following tests were performed
under KVM :

  qemu-3.0 (pseries-3.0)   -> qemu-3.0  (pseries-3.0)   OK
  qemu-3.0 (pseries-2.12)  -> qemu-2.12 (pseries-2.12)  OK
  qemu-3.0 (pseries-2.11)  -> qemu-2.11 (pseries-2.11)  OK
  qemu-3.0 (pseries-2.10)  -> qemu-2.10 (pseries-2.10)  OK
  qemu-3.0 (pseries-2.9)   -> qemu-2.9  (pseries-2.9)   OK
  qemu-3.0 (pseries-2.8)   -> qemu-2.8  (pseries-2.8)   OK
  qemu-3.0 (pseries-2.7)   -> qemu-2.7  (pseries-2.7)   FAIL

and back :

  qemu-3.0 (pseries-3.0)  <-  qemu-3.0  (pseries-3.0)   OK
  qemu-3.0 (pseries-2.12) <-  qemu-2.12 (pseries-2.12)  OK
  qemu-3.0 (pseries-2.11) <-  qemu-2.11 (pseries-2.11)  OK
  qemu-3.0 (pseries-2.10) <-  qemu-2.10 (pseries-2.10)  OK
  qemu-3.0 (pseries-2.9)  <-  qemu-2.9  (pseries-2.9)   OK
  qemu-3.0 (pseries-2.8)  <-  qemu-2.8  (pseries-2.8)   OK
  qemu-3.0 (pseries-2.7)  <-  qemu-2.7  (pseries-2.7)   OK

under TCG, same scenarios were run but up to 2.10 only, in which
case the migration fails for other reasons.

I wouldn't mind some extra cross checking from someone else.

Thanks,

C.


Cédric Le Goater (2):
  ppc/xics: rework the ICS classes inheritance tree
  ppc/xics: introduce ICP DeviceRealize and DeviceReset handlers

 include/hw/ppc/xics.h |   9 ++-
 hw/intc/xics.c        | 176 +++++++++++++++++++++++++++-----------------------
 hw/intc/xics_kvm.c    |  81 ++++++++++++++---------
 hw/intc/xics_pnv.c    |  15 ++++-
 hw/ppc/spapr.c        |   2 +-
 5 files changed, 166 insertions(+), 117 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/2] ppc/xics: rework the ICS classes inheritance tree

[Cédric Le Goater]

This moves the common ICS code of the realize and reset handlers of
the ICS_SIMPLE class under the ICS_BASE class. The vmstate is also
moved down one class.

The benefits are that the ICS_KVM class can directly inherit from
ICS_BASE class and not from the intermediate ICS_SIMPLE. It makes the
class hierarchy much cleaner and removes duplicated code. DeviceRealize
and DeviceReset handlers are introduce so that parent handlers are
called from the inheriting classes.

What is left in the top classes is the low level interface to access
the KVM XICS device in ICS_KVM and the XICS emulating handlers in
ICS_SIMPLE.

This should not break migration compatibility.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xics.h |   4 +-
 hw/intc/xics.c        | 166 ++++++++++++++++++++++++++++----------------------
 hw/intc/xics_kvm.c    |  47 +++++++-------
 hw/ppc/spapr.c        |   2 +-
 4 files changed, 123 insertions(+), 96 deletions(-)

diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6cebff47a7d4..adc5f437b118 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -114,7 +114,9 @@ struct PnvICPState {
 struct ICSStateClass {
     DeviceClass parent_class;
 
-    void (*realize)(ICSState *s, Error **errp);
+    DeviceRealize parent_realize;
+    DeviceReset parent_reset;
+
     void (*pre_save)(ICSState *s);
     int (*post_load)(ICSState *s, int version_id);
     void (*reject)(ICSState *s, uint32_t irq);
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e73e623e3b53..b351262d1db9 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -547,9 +547,61 @@ static void ics_simple_eoi(ICSState *ics, uint32_t nr)
     }
 }
 
-static void ics_simple_reset(void *dev)
+static void ics_simple_reset(DeviceState *dev)
 {
-    ICSState *ics = ICS_SIMPLE(dev);
+    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
+
+    icsc->parent_reset(dev);
+}
+
+static void ics_simple_reset_handler(void *dev)
+{
+    ics_simple_reset(dev);
+}
+
+static void ics_simple_realize(DeviceState *dev, Error **errp)
+{
+    ICSState *ics = ICS_BASE(dev);
+    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
+    Error *local_err = NULL;
+
+    icsc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
+
+    qemu_register_reset(ics_simple_reset_handler, ics);
+}
+
+static void ics_simple_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ICSStateClass *isc = ICS_BASE_CLASS(klass);
+
+    device_class_set_parent_realize(dc, ics_simple_realize,
+                                    &isc->parent_realize);
+    device_class_set_parent_reset(dc, ics_simple_reset,
+                                  &isc->parent_reset);
+
+    isc->reject = ics_simple_reject;
+    isc->resend = ics_simple_resend;
+    isc->eoi = ics_simple_eoi;
+}
+
+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_reset(DeviceState *dev)
+{
+    ICSState *ics = ICS_BASE(dev);
     int i;
     uint8_t flags[ics->nr_irqs];
 
@@ -566,7 +618,35 @@ static void ics_simple_reset(void *dev)
     }
 }
 
-static int ics_simple_dispatch_pre_save(void *opaque)
+static void ics_base_realize(DeviceState *dev, Error **errp)
+{
+    ICSState *ics = ICS_BASE(dev);
+    Object *obj;
+    Error *err = NULL;
+
+    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
+    if (!obj) {
+        error_propagate(errp, err);
+        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
+        return;
+    }
+    ics->xics = XICS_FABRIC(obj);
+
+    if (!ics->nr_irqs) {
+        error_setg(errp, "Number of interrupts needs to be greater 0");
+        return;
+    }
+    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
+}
+
+static void ics_base_instance_init(Object *obj)
+{
+    ICSState *ics = ICS_BASE(obj);
+
+    ics->offset = XICS_IRQ_BASE;
+}
+
+static int ics_base_dispatch_pre_save(void *opaque)
 {
     ICSState *ics = opaque;
     ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
@@ -578,7 +658,7 @@ static int ics_simple_dispatch_pre_save(void *opaque)
     return 0;
 }
 
-static int ics_simple_dispatch_post_load(void *opaque, int version_id)
+static int ics_base_dispatch_post_load(void *opaque, int version_id)
 {
     ICSState *ics = opaque;
     ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
@@ -590,7 +670,7 @@ static int ics_simple_dispatch_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static const VMStateDescription vmstate_ics_simple_irq = {
+static const VMStateDescription vmstate_ics_base_irq = {
     .name = "ics/irq",
     .version_id = 2,
     .minimum_version_id = 1,
@@ -604,95 +684,36 @@ static const VMStateDescription vmstate_ics_simple_irq = {
     },
 };
 
-static const VMStateDescription vmstate_ics_simple = {
+static const VMStateDescription vmstate_ics_base = {
     .name = "ics",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_save = ics_simple_dispatch_pre_save,
-    .post_load = ics_simple_dispatch_post_load,
+    .pre_save = ics_base_dispatch_pre_save,
+    .post_load = ics_base_dispatch_post_load,
     .fields = (VMStateField[]) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
 
         VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
-                                             vmstate_ics_simple_irq,
+                                             vmstate_ics_base_irq,
                                              ICSIRQState),
         VMSTATE_END_OF_LIST()
     },
 };
 
-static void ics_simple_initfn(Object *obj)
-{
-    ICSState *ics = ICS_SIMPLE(obj);
-
-    ics->offset = XICS_IRQ_BASE;
-}
-
-static void ics_simple_realize(ICSState *ics, Error **errp)
-{
-    if (!ics->nr_irqs) {
-        error_setg(errp, "Number of interrupts needs to be greater 0");
-        return;
-    }
-    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
-    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
-
-    qemu_register_reset(ics_simple_reset, ics);
-}
-
-static Property ics_simple_properties[] = {
+static Property ics_base_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);
-
-    isc->realize = ics_simple_realize;
-    dc->props = ics_simple_properties;
-    dc->vmsd = &vmstate_ics_simple;
-    isc->reject = ics_simple_reject;
-    isc->resend = ics_simple_resend;
-    isc->eoi = ics_simple_eoi;
-}
-
-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),
-    .instance_init = ics_simple_initfn,
-};
-
-static void ics_base_realize(DeviceState *dev, Error **errp)
-{
-    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
-    ICSState *ics = ICS_BASE(dev);
-    Object *obj;
-    Error *err = NULL;
-
-    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
-    if (!obj) {
-        error_propagate(errp, err);
-        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
-        return;
-    }
-    ics->xics = XICS_FABRIC(obj);
-
-
-    if (icsc->realize) {
-        icsc->realize(ics, errp);
-    }
-}
-
 static void ics_base_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = ics_base_realize;
+    dc->reset = ics_base_reset;
+    dc->props = ics_base_properties;
+    dc->vmsd = &vmstate_ics_base;
 }
 
 static const TypeInfo ics_base_info = {
@@ -700,6 +721,7 @@ static const TypeInfo ics_base_info = {
     .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),
 };
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 8dba2f84e71e..57d0ebbfaa8a 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -304,44 +304,47 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
     }
 }
 
-static void ics_kvm_reset(void *dev)
+static void ics_kvm_reset(DeviceState *dev)
 {
-    ICSState *ics = ICS_SIMPLE(dev);
-    int i;
-    uint8_t flags[ics->nr_irqs];
-
-    for (i = 0; i < ics->nr_irqs; i++) {
-        flags[i] = ics->irqs[i].flags;
-    }
+    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
 
-    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
+    icsc->parent_reset(dev);
 
-    for (i = 0; i < ics->nr_irqs; i++) {
-        ics->irqs[i].priority = 0xff;
-        ics->irqs[i].saved_priority = 0xff;
-        ics->irqs[i].flags = flags[i];
-    }
+    ics_set_kvm_state(ICS_KVM(dev), 1);
+}
 
-    ics_set_kvm_state(ics, 1);
+static void ics_kvm_reset_handler(void *dev)
+{
+    ics_kvm_reset(dev);
 }
 
-static void ics_kvm_realize(ICSState *ics, Error **errp)
+static void ics_kvm_realize(DeviceState *dev, Error **errp)
 {
-    if (!ics->nr_irqs) {
-        error_setg(errp, "Number of interrupts needs to be greater 0");
+    ICSState *ics = ICS_BASE(dev);
+    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
+    Error *local_err = NULL;
+
+    icsc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
-    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
+
     ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
 
-    qemu_register_reset(ics_kvm_reset, ics);
+    qemu_register_reset(ics_kvm_reset_handler, ics);
 }
 
 static void ics_kvm_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     ICSStateClass *icsc = ICS_BASE_CLASS(klass);
 
-    icsc->realize = ics_kvm_realize;
+    device_class_set_parent_realize(dc, ics_kvm_realize,
+                                    &icsc->parent_realize);
+    device_class_set_parent_reset(dc, ics_kvm_reset,
+                                  &icsc->parent_reset);
+
     icsc->pre_save = ics_get_kvm_state;
     icsc->post_load = ics_set_kvm_state;
     icsc->synchronize_state = ics_synchronize_state;
@@ -349,7 +352,7 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo ics_kvm_info = {
     .name = TYPE_ICS_KVM,
-    .parent = TYPE_ICS_SIMPLE,
+    .parent = TYPE_ICS_BASE,
     .instance_size = sizeof(ICSState),
     .class_init = ics_kvm_class_init,
 };
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 78186500e917..468539100327 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -136,7 +136,7 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
         goto error;
     }
 
-    return ICS_SIMPLE(obj);
+    return ICS_BASE(obj);
 
 error:
     error_propagate(errp, local_err);
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: rework the ICS classes inheritance tree

[David Gibson]

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

On Wed, Jun 20, 2018 at 12:24:12PM +0200, Cédric Le Goater wrote:
> This moves the common ICS code of the realize and reset handlers of
> the ICS_SIMPLE class under the ICS_BASE class. The vmstate is also
> moved down one class.
> 
> The benefits are that the ICS_KVM class can directly inherit from
> ICS_BASE class and not from the intermediate ICS_SIMPLE. It makes the
> class hierarchy much cleaner and removes duplicated code. DeviceRealize
> and DeviceReset handlers are introduce so that parent handlers are
> called from the inheriting classes.
> 
> What is left in the top classes is the low level interface to access
> the KVM XICS device in ICS_KVM and the XICS emulating handlers in
> ICS_SIMPLE.
> 
> This should not break migration compatibility.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

I like the idea here, but I'm finding it pretty hard to follow the
patch and convince myself its really safe.  How difficult would it be
to rework to separate out the change from base calls derived
->realize (and reset), to the more normal device_set_parent whatnot
from the actual consolidate of the classes?

> ---
>  include/hw/ppc/xics.h |   4 +-
>  hw/intc/xics.c        | 166 ++++++++++++++++++++++++++++----------------------
>  hw/intc/xics_kvm.c    |  47 +++++++-------
>  hw/ppc/spapr.c        |   2 +-
>  4 files changed, 123 insertions(+), 96 deletions(-)
> 
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7d4..adc5f437b118 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -114,7 +114,9 @@ struct PnvICPState {
>  struct ICSStateClass {
>      DeviceClass parent_class;
>  
> -    void (*realize)(ICSState *s, Error **errp);
> +    DeviceRealize parent_realize;
> +    DeviceReset parent_reset;
> +
>      void (*pre_save)(ICSState *s);
>      int (*post_load)(ICSState *s, int version_id);
>      void (*reject)(ICSState *s, uint32_t irq);
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b53..b351262d1db9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -547,9 +547,61 @@ static void ics_simple_eoi(ICSState *ics, uint32_t nr)
>      }
>  }
>  
> -static void ics_simple_reset(void *dev)
> +static void ics_simple_reset(DeviceState *dev)
>  {
> -    ICSState *ics = ICS_SIMPLE(dev);
> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> +
> +    icsc->parent_reset(dev);
> +}
> +
> +static void ics_simple_reset_handler(void *dev)
> +{
> +    ics_simple_reset(dev);
> +}
> +
> +static void ics_simple_realize(DeviceState *dev, Error **errp)
> +{
> +    ICSState *ics = ICS_BASE(dev);
> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
> +    Error *local_err = NULL;
> +
> +    icsc->parent_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> +
> +    qemu_register_reset(ics_simple_reset_handler, ics);
> +}
> +
> +static void ics_simple_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *isc = ICS_BASE_CLASS(klass);
> +
> +    device_class_set_parent_realize(dc, ics_simple_realize,
> +                                    &isc->parent_realize);
> +    device_class_set_parent_reset(dc, ics_simple_reset,
> +                                  &isc->parent_reset);
> +
> +    isc->reject = ics_simple_reject;
> +    isc->resend = ics_simple_resend;
> +    isc->eoi = ics_simple_eoi;
> +}
> +
> +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_reset(DeviceState *dev)
> +{
> +    ICSState *ics = ICS_BASE(dev);
>      int i;
>      uint8_t flags[ics->nr_irqs];
>  
> @@ -566,7 +618,35 @@ static void ics_simple_reset(void *dev)
>      }
>  }
>  
> -static int ics_simple_dispatch_pre_save(void *opaque)
> +static void ics_base_realize(DeviceState *dev, Error **errp)
> +{
> +    ICSState *ics = ICS_BASE(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +
> +    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
> +        return;
> +    }
> +    ics->xics = XICS_FABRIC(obj);
> +
> +    if (!ics->nr_irqs) {
> +        error_setg(errp, "Number of interrupts needs to be greater 0");
> +        return;
> +    }
> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +}
> +
> +static void ics_base_instance_init(Object *obj)
> +{
> +    ICSState *ics = ICS_BASE(obj);
> +
> +    ics->offset = XICS_IRQ_BASE;
> +}
> +
> +static int ics_base_dispatch_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
>      ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
> @@ -578,7 +658,7 @@ static int ics_simple_dispatch_pre_save(void *opaque)
>      return 0;
>  }
>  
> -static int ics_simple_dispatch_post_load(void *opaque, int version_id)
> +static int ics_base_dispatch_post_load(void *opaque, int version_id)
>  {
>      ICSState *ics = opaque;
>      ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
> @@ -590,7 +670,7 @@ static int ics_simple_dispatch_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static const VMStateDescription vmstate_ics_simple_irq = {
> +static const VMStateDescription vmstate_ics_base_irq = {
>      .name = "ics/irq",
>      .version_id = 2,
>      .minimum_version_id = 1,
> @@ -604,95 +684,36 @@ static const VMStateDescription vmstate_ics_simple_irq = {
>      },
>  };
>  
> -static const VMStateDescription vmstate_ics_simple = {
> +static const VMStateDescription vmstate_ics_base = {
>      .name = "ics",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = ics_simple_dispatch_pre_save,
> -    .post_load = ics_simple_dispatch_post_load,
> +    .pre_save = ics_base_dispatch_pre_save,
> +    .post_load = ics_base_dispatch_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
>  
>          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
> -                                             vmstate_ics_simple_irq,
> +                                             vmstate_ics_base_irq,
>                                               ICSIRQState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
>  
> -static void ics_simple_initfn(Object *obj)
> -{
> -    ICSState *ics = ICS_SIMPLE(obj);
> -
> -    ics->offset = XICS_IRQ_BASE;
> -}
> -
> -static void ics_simple_realize(ICSState *ics, Error **errp)
> -{
> -    if (!ics->nr_irqs) {
> -        error_setg(errp, "Number of interrupts needs to be greater 0");
> -        return;
> -    }
> -    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> -    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> -
> -    qemu_register_reset(ics_simple_reset, ics);
> -}
> -
> -static Property ics_simple_properties[] = {
> +static Property ics_base_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);
> -
> -    isc->realize = ics_simple_realize;
> -    dc->props = ics_simple_properties;
> -    dc->vmsd = &vmstate_ics_simple;
> -    isc->reject = ics_simple_reject;
> -    isc->resend = ics_simple_resend;
> -    isc->eoi = ics_simple_eoi;
> -}
> -
> -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),
> -    .instance_init = ics_simple_initfn,
> -};
> -
> -static void ics_base_realize(DeviceState *dev, Error **errp)
> -{
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> -    ICSState *ics = ICS_BASE(dev);
> -    Object *obj;
> -    Error *err = NULL;
> -
> -    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> -    if (!obj) {
> -        error_propagate(errp, err);
> -        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
> -        return;
> -    }
> -    ics->xics = XICS_FABRIC(obj);
> -
> -
> -    if (icsc->realize) {
> -        icsc->realize(ics, errp);
> -    }
> -}
> -
>  static void ics_base_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = ics_base_realize;
> +    dc->reset = ics_base_reset;
> +    dc->props = ics_base_properties;
> +    dc->vmsd = &vmstate_ics_base;
>  }
>  
>  static const TypeInfo ics_base_info = {
> @@ -700,6 +721,7 @@ static const TypeInfo ics_base_info = {
>      .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),
>  };
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 8dba2f84e71e..57d0ebbfaa8a 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -304,44 +304,47 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
>      }
>  }
>  
> -static void ics_kvm_reset(void *dev)
> +static void ics_kvm_reset(DeviceState *dev)
>  {
> -    ICSState *ics = ICS_SIMPLE(dev);
> -    int i;
> -    uint8_t flags[ics->nr_irqs];
> -
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        flags[i] = ics->irqs[i].flags;
> -    }
> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>  
> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> +    icsc->parent_reset(dev);
>  
> -    for (i = 0; i < ics->nr_irqs; i++) {
> -        ics->irqs[i].priority = 0xff;
> -        ics->irqs[i].saved_priority = 0xff;
> -        ics->irqs[i].flags = flags[i];
> -    }
> +    ics_set_kvm_state(ICS_KVM(dev), 1);
> +}
>  
> -    ics_set_kvm_state(ics, 1);
> +static void ics_kvm_reset_handler(void *dev)
> +{
> +    ics_kvm_reset(dev);
>  }
>  
> -static void ics_kvm_realize(ICSState *ics, Error **errp)
> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
>  {
> -    if (!ics->nr_irqs) {
> -        error_setg(errp, "Number of interrupts needs to be greater 0");
> +    ICSState *ics = ICS_BASE(dev);
> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
> +    Error *local_err = NULL;
> +
> +    icsc->parent_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          return;
>      }
> -    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +
>      ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>  
> -    qemu_register_reset(ics_kvm_reset, ics);
> +    qemu_register_reset(ics_kvm_reset_handler, ics);
>  }
>  
>  static void ics_kvm_class_init(ObjectClass *klass, void *data)
>  {
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>      ICSStateClass *icsc = ICS_BASE_CLASS(klass);
>  
> -    icsc->realize = ics_kvm_realize;
> +    device_class_set_parent_realize(dc, ics_kvm_realize,
> +                                    &icsc->parent_realize);
> +    device_class_set_parent_reset(dc, ics_kvm_reset,
> +                                  &icsc->parent_reset);
> +
>      icsc->pre_save = ics_get_kvm_state;
>      icsc->post_load = ics_set_kvm_state;
>      icsc->synchronize_state = ics_synchronize_state;
> @@ -349,7 +352,7 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>  
>  static const TypeInfo ics_kvm_info = {
>      .name = TYPE_ICS_KVM,
> -    .parent = TYPE_ICS_SIMPLE,
> +    .parent = TYPE_ICS_BASE,
>      .instance_size = sizeof(ICSState),
>      .class_init = ics_kvm_class_init,
>  };
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 78186500e917..468539100327 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -136,7 +136,7 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>          goto error;
>      }
>  
> -    return ICS_SIMPLE(obj);
> +    return ICS_BASE(obj);
>  
>  error:
>      error_propagate(errp, local_err);

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/2] ppc/xics: rework the ICS classes inheritance tree

[Cédric Le Goater]

On 06/25/2018 08:27 AM, David Gibson wrote:
> On Wed, Jun 20, 2018 at 12:24:12PM +0200, Cédric Le Goater wrote:
>> This moves the common ICS code of the realize and reset handlers of
>> the ICS_SIMPLE class under the ICS_BASE class. The vmstate is also
>> moved down one class.
>>
>> The benefits are that the ICS_KVM class can directly inherit from
>> ICS_BASE class and not from the intermediate ICS_SIMPLE. It makes the
>> class hierarchy much cleaner and removes duplicated code. DeviceRealize
>> and DeviceReset handlers are introduce so that parent handlers are
>> called from the inheriting classes.
>>
>> What is left in the top classes is the low level interface to access
>> the KVM XICS device in ICS_KVM and the XICS emulating handlers in
>> ICS_SIMPLE.
>>
>> This should not break migration compatibility.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> I like the idea here, but I'm finding it pretty hard to follow the
> patch and convince myself its really safe.

I understand. The diff brings a lot of fuzziness. I tried to convince 
you with the migration tests.

> How difficult would it be
> to rework to separate out the change from base calls derived
> ->realize (and reset), to the more normal device_set_parent whatnot
> from the actual consolidate of the classes?

I will try that.

Thanks,

C.  

>> ---
>>  include/hw/ppc/xics.h |   4 +-
>>  hw/intc/xics.c        | 166 ++++++++++++++++++++++++++++----------------------
>>  hw/intc/xics_kvm.c    |  47 +++++++-------
>>  hw/ppc/spapr.c        |   2 +-
>>  4 files changed, 123 insertions(+), 96 deletions(-)
>>
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 6cebff47a7d4..adc5f437b118 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -114,7 +114,9 @@ struct PnvICPState {
>>  struct ICSStateClass {
>>      DeviceClass parent_class;
>>  
>> -    void (*realize)(ICSState *s, Error **errp);
>> +    DeviceRealize parent_realize;
>> +    DeviceReset parent_reset;
>> +
>>      void (*pre_save)(ICSState *s);
>>      int (*post_load)(ICSState *s, int version_id);
>>      void (*reject)(ICSState *s, uint32_t irq);
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index e73e623e3b53..b351262d1db9 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -547,9 +547,61 @@ static void ics_simple_eoi(ICSState *ics, uint32_t nr)
>>      }
>>  }
>>  
>> -static void ics_simple_reset(void *dev)
>> +static void ics_simple_reset(DeviceState *dev)
>>  {
>> -    ICSState *ics = ICS_SIMPLE(dev);
>> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>> +
>> +    icsc->parent_reset(dev);
>> +}
>> +
>> +static void ics_simple_reset_handler(void *dev)
>> +{
>> +    ics_simple_reset(dev);
>> +}
>> +
>> +static void ics_simple_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ICSState *ics = ICS_BASE(dev);
>> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
>> +    Error *local_err = NULL;
>> +
>> +    icsc->parent_realize(dev, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>> +
>> +    qemu_register_reset(ics_simple_reset_handler, ics);
>> +}
>> +
>> +static void ics_simple_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ICSStateClass *isc = ICS_BASE_CLASS(klass);
>> +
>> +    device_class_set_parent_realize(dc, ics_simple_realize,
>> +                                    &isc->parent_realize);
>> +    device_class_set_parent_reset(dc, ics_simple_reset,
>> +                                  &isc->parent_reset);
>> +
>> +    isc->reject = ics_simple_reject;
>> +    isc->resend = ics_simple_resend;
>> +    isc->eoi = ics_simple_eoi;
>> +}
>> +
>> +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_reset(DeviceState *dev)
>> +{
>> +    ICSState *ics = ICS_BASE(dev);
>>      int i;
>>      uint8_t flags[ics->nr_irqs];
>>  
>> @@ -566,7 +618,35 @@ static void ics_simple_reset(void *dev)
>>      }
>>  }
>>  
>> -static int ics_simple_dispatch_pre_save(void *opaque)
>> +static void ics_base_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ICSState *ics = ICS_BASE(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
>> +    if (!obj) {
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
>> +        return;
>> +    }
>> +    ics->xics = XICS_FABRIC(obj);
>> +
>> +    if (!ics->nr_irqs) {
>> +        error_setg(errp, "Number of interrupts needs to be greater 0");
>> +        return;
>> +    }
>> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> +}
>> +
>> +static void ics_base_instance_init(Object *obj)
>> +{
>> +    ICSState *ics = ICS_BASE(obj);
>> +
>> +    ics->offset = XICS_IRQ_BASE;
>> +}
>> +
>> +static int ics_base_dispatch_pre_save(void *opaque)
>>  {
>>      ICSState *ics = opaque;
>>      ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>> @@ -578,7 +658,7 @@ static int ics_simple_dispatch_pre_save(void *opaque)
>>      return 0;
>>  }
>>  
>> -static int ics_simple_dispatch_post_load(void *opaque, int version_id)
>> +static int ics_base_dispatch_post_load(void *opaque, int version_id)
>>  {
>>      ICSState *ics = opaque;
>>      ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>> @@ -590,7 +670,7 @@ static int ics_simple_dispatch_post_load(void *opaque, int version_id)
>>      return 0;
>>  }
>>  
>> -static const VMStateDescription vmstate_ics_simple_irq = {
>> +static const VMStateDescription vmstate_ics_base_irq = {
>>      .name = "ics/irq",
>>      .version_id = 2,
>>      .minimum_version_id = 1,
>> @@ -604,95 +684,36 @@ static const VMStateDescription vmstate_ics_simple_irq = {
>>      },
>>  };
>>  
>> -static const VMStateDescription vmstate_ics_simple = {
>> +static const VMStateDescription vmstate_ics_base = {
>>      .name = "ics",
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>> -    .pre_save = ics_simple_dispatch_pre_save,
>> -    .post_load = ics_simple_dispatch_post_load,
>> +    .pre_save = ics_base_dispatch_pre_save,
>> +    .post_load = ics_base_dispatch_post_load,
>>      .fields = (VMStateField[]) {
>>          /* Sanity check */
>>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
>>  
>>          VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
>> -                                             vmstate_ics_simple_irq,
>> +                                             vmstate_ics_base_irq,
>>                                               ICSIRQState),
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
>>  
>> -static void ics_simple_initfn(Object *obj)
>> -{
>> -    ICSState *ics = ICS_SIMPLE(obj);
>> -
>> -    ics->offset = XICS_IRQ_BASE;
>> -}
>> -
>> -static void ics_simple_realize(ICSState *ics, Error **errp)
>> -{
>> -    if (!ics->nr_irqs) {
>> -        error_setg(errp, "Number of interrupts needs to be greater 0");
>> -        return;
>> -    }
>> -    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> -    ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>> -
>> -    qemu_register_reset(ics_simple_reset, ics);
>> -}
>> -
>> -static Property ics_simple_properties[] = {
>> +static Property ics_base_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);
>> -
>> -    isc->realize = ics_simple_realize;
>> -    dc->props = ics_simple_properties;
>> -    dc->vmsd = &vmstate_ics_simple;
>> -    isc->reject = ics_simple_reject;
>> -    isc->resend = ics_simple_resend;
>> -    isc->eoi = ics_simple_eoi;
>> -}
>> -
>> -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),
>> -    .instance_init = ics_simple_initfn,
>> -};
>> -
>> -static void ics_base_realize(DeviceState *dev, Error **errp)
>> -{
>> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>> -    ICSState *ics = ICS_BASE(dev);
>> -    Object *obj;
>> -    Error *err = NULL;
>> -
>> -    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
>> -    if (!obj) {
>> -        error_propagate(errp, err);
>> -        error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
>> -        return;
>> -    }
>> -    ics->xics = XICS_FABRIC(obj);
>> -
>> -
>> -    if (icsc->realize) {
>> -        icsc->realize(ics, errp);
>> -    }
>> -}
>> -
>>  static void ics_base_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>>      dc->realize = ics_base_realize;
>> +    dc->reset = ics_base_reset;
>> +    dc->props = ics_base_properties;
>> +    dc->vmsd = &vmstate_ics_base;
>>  }
>>  
>>  static const TypeInfo ics_base_info = {
>> @@ -700,6 +721,7 @@ static const TypeInfo ics_base_info = {
>>      .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),
>>  };
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 8dba2f84e71e..57d0ebbfaa8a 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -304,44 +304,47 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
>>      }
>>  }
>>  
>> -static void ics_kvm_reset(void *dev)
>> +static void ics_kvm_reset(DeviceState *dev)
>>  {
>> -    ICSState *ics = ICS_SIMPLE(dev);
>> -    int i;
>> -    uint8_t flags[ics->nr_irqs];
>> -
>> -    for (i = 0; i < ics->nr_irqs; i++) {
>> -        flags[i] = ics->irqs[i].flags;
>> -    }
>> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>>  
>> -    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
>> +    icsc->parent_reset(dev);
>>  
>> -    for (i = 0; i < ics->nr_irqs; i++) {
>> -        ics->irqs[i].priority = 0xff;
>> -        ics->irqs[i].saved_priority = 0xff;
>> -        ics->irqs[i].flags = flags[i];
>> -    }
>> +    ics_set_kvm_state(ICS_KVM(dev), 1);
>> +}
>>  
>> -    ics_set_kvm_state(ics, 1);
>> +static void ics_kvm_reset_handler(void *dev)
>> +{
>> +    ics_kvm_reset(dev);
>>  }
>>  
>> -static void ics_kvm_realize(ICSState *ics, Error **errp)
>> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
>>  {
>> -    if (!ics->nr_irqs) {
>> -        error_setg(errp, "Number of interrupts needs to be greater 0");
>> +    ICSState *ics = ICS_BASE(dev);
>> +    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
>> +    Error *local_err = NULL;
>> +
>> +    icsc->parent_realize(dev, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>          return;
>>      }
>> -    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> +
>>      ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>>  
>> -    qemu_register_reset(ics_kvm_reset, ics);
>> +    qemu_register_reset(ics_kvm_reset_handler, ics);
>>  }
>>  
>>  static void ics_kvm_class_init(ObjectClass *klass, void *data)
>>  {
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>      ICSStateClass *icsc = ICS_BASE_CLASS(klass);
>>  
>> -    icsc->realize = ics_kvm_realize;
>> +    device_class_set_parent_realize(dc, ics_kvm_realize,
>> +                                    &icsc->parent_realize);
>> +    device_class_set_parent_reset(dc, ics_kvm_reset,
>> +                                  &icsc->parent_reset);
>> +
>>      icsc->pre_save = ics_get_kvm_state;
>>      icsc->post_load = ics_set_kvm_state;
>>      icsc->synchronize_state = ics_synchronize_state;
>> @@ -349,7 +352,7 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>>  
>>  static const TypeInfo ics_kvm_info = {
>>      .name = TYPE_ICS_KVM,
>> -    .parent = TYPE_ICS_SIMPLE,
>> +    .parent = TYPE_ICS_BASE,
>>      .instance_size = sizeof(ICSState),
>>      .class_init = ics_kvm_class_init,
>>  };
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 78186500e917..468539100327 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -136,7 +136,7 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>>          goto error;
>>      }
>>  
>> -    return ICS_SIMPLE(obj);
>> +    return ICS_BASE(obj);
>>  
>>  error:
>>      error_propagate(errp, local_err);
> 

[Qemu-devel] [PATCH 2/2] ppc/xics: introduce ICP DeviceRealize and DeviceReset handlers

[Cédric Le Goater]

This changes the IPC realize and reset handlers in DeviceRealize and
DeviceReset handlers. parent handlers are now called from the
inheriting classes which is a cleaner object pattern.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xics.h |  5 +++--
 hw/intc/xics.c        | 10 ----------
 hw/intc/xics_kvm.c    | 34 +++++++++++++++++++++++++++-------
 hw/intc/xics_pnv.c    | 15 +++++++++++++--
 4 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index adc5f437b118..6ac8a9392da6 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -65,10 +65,11 @@ typedef struct XICSFabric XICSFabric;
 struct ICPStateClass {
     DeviceClass parent_class;
 
-    void (*realize)(ICPState *icp, Error **errp);
+    DeviceRealize parent_realize;
+    DeviceReset parent_reset;
+
     void (*pre_save)(ICPState *icp);
     int (*post_load)(ICPState *icp, int version_id);
-    void (*reset)(ICPState *icp);
     void (*synchronize_state)(ICPState *icp);
 };
 
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index b351262d1db9..ef5c612dc711 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -294,7 +294,6 @@ static const VMStateDescription vmstate_icp_server = {
 static void icp_reset(void *dev)
 {
     ICPState *icp = ICP(dev);
-    ICPStateClass *icpc = ICP_GET_CLASS(icp);
 
     icp->xirr = 0;
     icp->pending_priority = 0xff;
@@ -302,16 +301,11 @@ static void icp_reset(void *dev)
 
     /* Make all outputs are deasserted */
     qemu_set_irq(icp->output, 0);
-
-    if (icpc->reset) {
-        icpc->reset(icp);
-    }
 }
 
 static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
-    ICPStateClass *icpc = ICP_GET_CLASS(dev);
     PowerPCCPU *cpu;
     CPUPPCState *env;
     Object *obj;
@@ -351,10 +345,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (icpc->realize) {
-        icpc->realize(icp, errp);
-    }
-
     qemu_register_reset(icp_reset, dev);
     vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 57d0ebbfaa8a..50d7457abd34 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -114,22 +114,38 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
     return 0;
 }
 
-static void icp_kvm_reset(ICPState *icp)
+static void icp_kvm_reset(DeviceState *dev)
 {
-    icp_set_kvm_state(icp, 1);
+    ICPStateClass *icpc = ICP_GET_CLASS(dev);
+
+    icpc->parent_reset(dev);
+
+    icp_set_kvm_state(ICP(dev), 1);
 }
 
-static void icp_kvm_realize(ICPState *icp, Error **errp)
+static void icp_kvm_realize(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = icp->cs;
+    ICPState *icp = ICP(dev);
+    ICPStateClass *icpc = ICP_GET_CLASS(icp);
+    Error *local_err = NULL;
+    CPUState *cs;
     KVMEnabledICP *enabled_icp;
-    unsigned long vcpu_id = kvm_arch_vcpu_id(cs);
+    unsigned long vcpu_id;
     int ret;
 
     if (kernel_xics_fd == -1) {
         abort();
     }
 
+    icpc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    cs = icp->cs;
+    vcpu_id = kvm_arch_vcpu_id(cs);
+
     /*
      * If we are reusing a parked vCPU fd corresponding to the CPU
      * which was hot-removed earlier we don't have to renable
@@ -154,12 +170,16 @@ static void icp_kvm_realize(ICPState *icp, Error **errp)
 
 static void icp_kvm_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     ICPStateClass *icpc = ICP_CLASS(klass);
 
+    device_class_set_parent_realize(dc, icp_kvm_realize,
+                                    &icpc->parent_realize);
+    device_class_set_parent_reset(dc, icp_kvm_reset,
+                                  &icpc->parent_reset);
+
     icpc->pre_save = icp_get_kvm_state;
     icpc->post_load = icp_set_kvm_state;
-    icpc->realize = icp_kvm_realize;
-    icpc->reset = icp_kvm_reset;
     icpc->synchronize_state = icp_synchronize_state;
 }
 
diff --git a/hw/intc/xics_pnv.c b/hw/intc/xics_pnv.c
index c87de2189cf7..fa48505f365e 100644
--- a/hw/intc/xics_pnv.c
+++ b/hw/intc/xics_pnv.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "qemu/log.h"
 #include "hw/ppc/xics.h"
@@ -158,9 +159,18 @@ static const MemoryRegionOps pnv_icp_ops = {
     },
 };
 
-static void pnv_icp_realize(ICPState *icp, Error **errp)
+static void pnv_icp_realize(DeviceState *dev, Error **errp)
 {
+    ICPState *icp = ICP(dev);
     PnvICPState *pnv_icp = PNV_ICP(icp);
+    ICPStateClass *icpc = ICP_GET_CLASS(icp);
+    Error *local_err = NULL;
+
+    icpc->parent_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     memory_region_init_io(&pnv_icp->mmio, OBJECT(icp), &pnv_icp_ops,
                           icp, "icp-thread", 0x1000);
@@ -171,7 +181,8 @@ static void pnv_icp_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     ICPStateClass *icpc = ICP_CLASS(klass);
 
-    icpc->realize = pnv_icp_realize;
+    device_class_set_parent_realize(dc, pnv_icp_realize,
+                                    &icpc->parent_realize);
     dc->desc = "PowerNV ICP";
 }
 
-- 
2.13.6

Re: [Qemu-devel] [PATCH 2/2] ppc/xics: introduce ICP DeviceRealize and DeviceReset handlers

[David Gibson]

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

On Wed, Jun 20, 2018 at 12:24:13PM +0200, Cédric Le Goater wrote:
> This changes the IPC realize and reset handlers in DeviceRealize and
> DeviceReset handlers. parent handlers are now called from the
> inheriting classes which is a cleaner object pattern.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  include/hw/ppc/xics.h |  5 +++--
>  hw/intc/xics.c        | 10 ----------
>  hw/intc/xics_kvm.c    | 34 +++++++++++++++++++++++++++-------
>  hw/intc/xics_pnv.c    | 15 +++++++++++++--
>  4 files changed, 43 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index adc5f437b118..6ac8a9392da6 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -65,10 +65,11 @@ typedef struct XICSFabric XICSFabric;
>  struct ICPStateClass {
>      DeviceClass parent_class;
>  
> -    void (*realize)(ICPState *icp, Error **errp);
> +    DeviceRealize parent_realize;
> +    DeviceReset parent_reset;
> +
>      void (*pre_save)(ICPState *icp);
>      int (*post_load)(ICPState *icp, int version_id);
> -    void (*reset)(ICPState *icp);
>      void (*synchronize_state)(ICPState *icp);
>  };
>  
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index b351262d1db9..ef5c612dc711 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -294,7 +294,6 @@ static const VMStateDescription vmstate_icp_server = {
>  static void icp_reset(void *dev)
>  {
>      ICPState *icp = ICP(dev);
> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>  
>      icp->xirr = 0;
>      icp->pending_priority = 0xff;
> @@ -302,16 +301,11 @@ static void icp_reset(void *dev)
>  
>      /* Make all outputs are deasserted */
>      qemu_set_irq(icp->output, 0);
> -
> -    if (icpc->reset) {
> -        icpc->reset(icp);
> -    }
>  }
>  
>  static void icp_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> -    ICPStateClass *icpc = ICP_GET_CLASS(dev);
>      PowerPCCPU *cpu;
>      CPUPPCState *env;
>      Object *obj;
> @@ -351,10 +345,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    if (icpc->realize) {
> -        icpc->realize(icp, errp);
> -    }
> -
>      qemu_register_reset(icp_reset, dev);
>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 57d0ebbfaa8a..50d7457abd34 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -114,22 +114,38 @@ static int icp_set_kvm_state(ICPState *icp, int version_id)
>      return 0;
>  }
>  
> -static void icp_kvm_reset(ICPState *icp)
> +static void icp_kvm_reset(DeviceState *dev)
>  {
> -    icp_set_kvm_state(icp, 1);
> +    ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +
> +    icpc->parent_reset(dev);
> +
> +    icp_set_kvm_state(ICP(dev), 1);
>  }
>  
> -static void icp_kvm_realize(ICPState *icp, Error **errp)
> +static void icp_kvm_realize(DeviceState *dev, Error **errp)
>  {
> -    CPUState *cs = icp->cs;
> +    ICPState *icp = ICP(dev);
> +    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> +    Error *local_err = NULL;
> +    CPUState *cs;
>      KVMEnabledICP *enabled_icp;
> -    unsigned long vcpu_id = kvm_arch_vcpu_id(cs);
> +    unsigned long vcpu_id;
>      int ret;
>  
>      if (kernel_xics_fd == -1) {
>          abort();
>      }
>  
> +    icpc->parent_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    cs = icp->cs;
> +    vcpu_id = kvm_arch_vcpu_id(cs);
> +
>      /*
>       * If we are reusing a parked vCPU fd corresponding to the CPU
>       * which was hot-removed earlier we don't have to renable
> @@ -154,12 +170,16 @@ static void icp_kvm_realize(ICPState *icp, Error **errp)
>  
>  static void icp_kvm_class_init(ObjectClass *klass, void *data)
>  {
> +    DeviceClass *dc = DEVICE_CLASS(klass);
>      ICPStateClass *icpc = ICP_CLASS(klass);
>  
> +    device_class_set_parent_realize(dc, icp_kvm_realize,
> +                                    &icpc->parent_realize);
> +    device_class_set_parent_reset(dc, icp_kvm_reset,
> +                                  &icpc->parent_reset);
> +
>      icpc->pre_save = icp_get_kvm_state;
>      icpc->post_load = icp_set_kvm_state;
> -    icpc->realize = icp_kvm_realize;
> -    icpc->reset = icp_kvm_reset;
>      icpc->synchronize_state = icp_synchronize_state;
>  }
>  
> diff --git a/hw/intc/xics_pnv.c b/hw/intc/xics_pnv.c
> index c87de2189cf7..fa48505f365e 100644
> --- a/hw/intc/xics_pnv.c
> +++ b/hw/intc/xics_pnv.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "sysemu/sysemu.h"
>  #include "qemu/log.h"
>  #include "hw/ppc/xics.h"
> @@ -158,9 +159,18 @@ static const MemoryRegionOps pnv_icp_ops = {
>      },
>  };
>  
> -static void pnv_icp_realize(ICPState *icp, Error **errp)
> +static void pnv_icp_realize(DeviceState *dev, Error **errp)
>  {
> +    ICPState *icp = ICP(dev);
>      PnvICPState *pnv_icp = PNV_ICP(icp);
> +    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> +    Error *local_err = NULL;
> +
> +    icpc->parent_realize(dev, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      memory_region_init_io(&pnv_icp->mmio, OBJECT(icp), &pnv_icp_ops,
>                            icp, "icp-thread", 0x1000);
> @@ -171,7 +181,8 @@ static void pnv_icp_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      ICPStateClass *icpc = ICP_CLASS(klass);
>  
> -    icpc->realize = pnv_icp_realize;
> +    device_class_set_parent_realize(dc, pnv_icp_realize,
> +                                    &icpc->parent_realize);
>      dc->desc = "PowerNV ICP";
>  }
>  

-- 
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: 833 bytes --]