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