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

[David Gibson]

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 <address@hidden>

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

signature.asc
Description: PGP signature