Re: [PATCH 2/4] xics: Merge reset and realize hooks

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi David,

On 9/24/19 6:59 AM, David Gibson wrote:
> Currently TYPE_XICS_BASE and TYPE_XICS_SIMPLE have their own reset and
> realize methods, using the standard technique for having the subtype
> call the supertype's methods before doing its own thing.
> 
> But TYPE_XICS_SIMPLE is the only subtype of TYPE_XICS_BASE ever
> instantiated, so there's no point having the split here.  Merge them
> together into just ics_reset() and ics_realize() functions.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/xics.c        | 97 ++++++++++++++++---------------------------
>  include/hw/ppc/xics.h |  3 --
>  2 files changed, 35 insertions(+), 65 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 93139b0189..db0e532bd9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -548,68 +548,13 @@ static void ics_eoi(ICSState *ics, uint32_t nr)
>      }
>  }
>  
> -static void ics_simple_reset(DeviceState *dev)
> -{
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> -
> -    icsc->parent_reset(dev);
> -
> -    if (kvm_irqchip_in_kernel()) {
> -        Error *local_err = NULL;
> -
> -        ics_set_kvm_state(ICS_BASE(dev), &local_err);
> -        if (local_err) {
> -            error_report_err(local_err);
> -        }
> -    }
> -}
> -
> -static void ics_simple_reset_handler(void *dev)
> -{
> -    ics_simple_reset(dev);
> -}
> -
> -static void ics_simple_realize(DeviceState *dev, Error **errp)
> -{
> -    ICSState *ics = ICS_SIMPLE(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;
> -    }
> -
> -    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);
> -}
> -
> -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_reset_irq(ICSIRQState *irq)
>  {
>      irq->priority = 0xff;
>      irq->saved_priority = 0xff;
>  }
>  
> -static void ics_base_reset(DeviceState *dev)
> +static void ics_reset(DeviceState *dev)
>  {
>      ICSState *ics = ICS_BASE(dev);
>      int i;
> @@ -625,17 +570,31 @@ static void ics_base_reset(DeviceState *dev)
>          ics_reset_irq(ics->irqs + i);
>          ics->irqs[i].flags = flags[i];
>      }
> +
> +    if (kvm_irqchip_in_kernel()) {
> +        Error *local_err = NULL;
> +
> +        ics_set_kvm_state(ICS_BASE(dev), &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }
> +    }
> +}
> +
> +static void ics_reset_handler(void *dev)
> +{
> +    ics_reset(dev);
>  }
>  
> -static void ics_base_realize(DeviceState *dev, Error **errp)
> +static void ics_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_BASE(dev);
> +    Error *local_err = NULL;

Nit: This variable renaming is confusing, maybe another patch?

>      Object *obj;
> -    Error *err = NULL;
>  
> -    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> +    obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &local_err);
>      if (!obj) {
> -        error_propagate_prepend(errp, err,
> +        error_propagate_prepend(errp, local_err,
>                                  "required link '" ICS_PROP_XICS
>                                  "' not found: ");
>          return;
> @@ -647,8 +606,22 @@ static void ics_base_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +
> +    qemu_register_reset(ics_reset_handler, ics);

Can you change this call by ...

> +}
> +
> +static void ics_simple_class_init(ObjectClass *klass, void *data)
> +{

... this?

    DeviceClass *dc = DEVICE_CLASS(klass);

    dc->reset = ics_reset;

>  }
>  
> +static const TypeInfo ics_simple_info = {
> +    .name = TYPE_ICS_SIMPLE,
> +    .parent = TYPE_ICS_BASE,

But now reading here, why keep TYPE_ICS_BASE?
It seems you can simplify further using directly:

       .parent = TYPE_DEVICE,

> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_simple_class_init,
> +    .class_size = sizeof(ICSStateClass),
> +};
> +
>  static void ics_base_instance_init(Object *obj)
>  {
>      ICSState *ics = ICS_BASE(obj);
> @@ -725,9 +698,9 @@ static void ics_base_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    dc->realize = ics_base_realize;
> +    dc->realize = ics_realize;
>      dc->props = ics_base_properties;
> -    dc->reset = ics_base_reset;
> +    dc->reset = ics_reset;
>      dc->vmsd = &vmstate_ics_base;
>  }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 34d7985b7c..0eb39c2561 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -103,9 +103,6 @@ struct PnvICPState {
>  
>  struct ICSStateClass {
>      DeviceClass parent_class;
> -
> -    DeviceRealize parent_realize;
> -    DeviceReset parent_reset;
>  };
>  
>  struct ICSState {
>