Re: [Qemu-devel] [PATCH qom-next v2 3/5] target-arm: Use parent classes for reset + realize

[Igor Mammedov]

On Thu, 11 Jul 2013 13:31:15 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote:
> > On Thu, 11 Jul 2013 11:47:16 +1000
> > address@hidden wrote:
> > 
> > > From: Peter Crosthwaite <address@hidden>
> > > 
> > > ARMCPUClass is only needed for parent-class abstract function access.
> > > Just use parent classes for reset and realize access and remove
> > > ARMCPUClass completely.
> > > 
> > > Signed-off-by: Peter Crosthwaite <address@hidden>
> > > ---
> > > 
> > >  target-arm/cpu-qom.h | 20 --------------------
> > >  target-arm/cpu.c     | 16 +++++++---------
> > >  2 files changed, 7 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> > > index ef6261f..bdad93a 100644
> > > --- a/target-arm/cpu-qom.h
> > > +++ b/target-arm/cpu-qom.h
> > > @@ -24,28 +24,8 @@
> > >  
> > >  #define TYPE_ARM_CPU "arm-cpu"
> > >  
> > > -#define ARM_CPU_CLASS(klass) \
> > > -    OBJECT_CLASS_CHECK(ARMCPUClass, (klass), TYPE_ARM_CPU)
> > >  #define ARM_CPU(obj) \
> > >      OBJECT_CHECK(ARMCPU, (obj), TYPE_ARM_CPU)
> > > -#define ARM_CPU_GET_CLASS(obj) \
> > > -    OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
> > > -
> > > -/**
> > > - * ARMCPUClass:
> > > - * @parent_realize: The parent class' realize handler.
> > > - * @parent_reset: The parent class' reset handler.
> > > - *
> > > - * An ARM CPU model.
> > > - */
> > > -typedef struct ARMCPUClass {
> > > -    /*< private >*/
> > > -    CPUClass parent_class;
> > > -    /*< public >*/
> > > -
> > > -    DeviceRealize parent_realize;
> > > -    void (*parent_reset)(CPUState *cpu);
> > > -} ARMCPUClass;
> > >  
> > >  /**
> > >   * ARMCPU:
> > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > > index ed53df8..ad5ec7b 100644
> > > --- a/target-arm/cpu.c
> > > +++ b/target-arm/cpu.c
> > > @@ -60,7 +60,8 @@ static void cp_reg_reset(gpointer key, gpointer value, 
> > > gpointer opaque)
> > >  static void arm_cpu_reset(CPUState *s)
> > >  {
> > >      ARMCPU *cpu = ARM_CPU(s);
> > > -    ARMCPUClass *acc = ARM_CPU_GET_CLASS(cpu);
> > > +    CPUClass *cc_parent =
> > > +            CPU_CLASS(object_class_get_parent_by_name(TYPE_ARM_CPU));
> > Maybe object_class_get_parent_of_type() would be less confusing?
> > 
> > This usage assumes that parent of TYPE_ARM_CPU is TYPE_CPU and if
> > another TYPE_X added between them, it might break if TYPE_X doesn't
> > re-implement this logic in its reset.
> 
> If what's needed is TYPE_CPU, you can just look up the class by name.

As a quick way to reduce amount of current code /save+override+call/ it will
work too, but adding intermediate TYPE bears the same risk, i.e. children
must be amended to call its reset if it has one.

> 
> > Could reset be modeled like DEVICE.instance_init() instead? Then
> > no explicit access to parent from child would be needed and it
> > still leaves possibility to override resets if parent->child
> > propagation order is not desirable for a particular device.
> > 
> > >      CPUARMState *env = &cpu->env;
> > >  
> > >      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> > > @@ -68,7 +69,7 @@ static void arm_cpu_reset(CPUState *s)
> > >          log_cpu_state(env, 0);
> > >      }
> > >  
> > > -    acc->parent_reset(s);
> > > +    cc_parent->reset(s);
> > >  
> > >      memset(env, 0, offsetof(CPUARMState, breakpoints));
> > >      g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu);
> > > @@ -158,7 +159,8 @@ static void arm_cpu_finalizefn(Object *obj)
> > >  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> > >  {
> > >      ARMCPU *cpu = ARM_CPU(dev);
> > > -    ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
> > > +    DeviceClass *dc_parent =
> > > +            DEVICE_CLASS(object_class_get_parent_by_name(TYPE_ARM_CPU));
> > >      CPUARMState *env = &cpu->env;
> > >  
> > >      /* Some features automatically imply others: */
> > > @@ -209,7 +211,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > > **errp)
> > >  
> > >      cpu_reset(CPU(cpu));
> > >  
> > > -    acc->parent_realize(dev, errp);
> > > +    dc_parent->realize(dev, errp);
> > >  }
> > >  
> > >  /* CPU models */
> > > @@ -803,14 +805,11 @@ static const ARMCPUInfo arm_cpus[] = {
> > >  
> > >  static void arm_cpu_class_init(ObjectClass *oc, void *data)
> > >  {
> > > -    ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> > > -    CPUClass *cc = CPU_CLASS(acc);
> > > +    CPUClass *cc = CPU_CLASS(oc);
> > >      DeviceClass *dc = DEVICE_CLASS(oc);
> > >  
> > > -    acc->parent_realize = dc->realize;
> > >      dc->realize = arm_cpu_realizefn;
> > >  
> > > -    acc->parent_reset = cc->reset;
> > >      cc->reset = arm_cpu_reset;
> > >  
> > >      cc->class_by_name = arm_cpu_class_by_name;
> > > @@ -839,7 +838,6 @@ static const TypeInfo arm_cpu_type_info = {
> > >      .instance_init = arm_cpu_initfn,
> > >      .instance_finalize = arm_cpu_finalizefn,
> > >      .abstract = true,
> > > -    .class_size = sizeof(ARMCPUClass),
> > >      .class_init = arm_cpu_class_init,
> > >  };
> > >  
>