Message ID | f821cf6fc855bf99b177916e693373fbf84ca69d.1373440128.git.peter.crosthwaite@xilinx.com |
---|---|
State | New |
Headers | show |
On Thu, 11 Jul 2013 11:47:16 +1000 peter.crosthwaite@xilinx.com wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > 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 <peter.crosthwaite@xilinx.com> > --- > > 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. 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, > }; >
On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote: > On Thu, 11 Jul 2013 11:47:16 +1000 > peter.crosthwaite@xilinx.com wrote: > > > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > > 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 <peter.crosthwaite@xilinx.com> > > --- > > > > 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. > 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, > > }; > >
Am 11.07.2013 12:31, schrieb Michael S. Tsirkin: > On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote: >> On Thu, 11 Jul 2013 11:47:16 +1000 >> peter.crosthwaite@xilinx.com wrote: >> >>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> >>> 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 <peter.crosthwaite@xilinx.com> >>> --- >>> >>> 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. My suggestion was to hide the implementation details behind an ARM_CPU_GET_PARENT_CLASS(obj) macro. That would at the same time allow to later switch to an iterative approach as seen in v1 without a whole lot of refactoring. Andreas > >> 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, >>> }; >>>
On Thu, 11 Jul 2013 13:31:15 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote: > > On Thu, 11 Jul 2013 11:47:16 +1000 > > peter.crosthwaite@xilinx.com wrote: > > > > > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > > > > 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 <peter.crosthwaite@xilinx.com> > > > --- > > > > > > 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, > > > }; > > > >
Hi Igor, On Thu, Jul 11, 2013 at 7:47 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 11 Jul 2013 11:47:16 +1000 > peter.crosthwaite@xilinx.com wrote: > >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> 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 <peter.crosthwaite@xilinx.com> >> --- >> >> 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 It assumes that a parent (could be a grandparent on any level) is TYPE_CPU not necessarily the direct parent. > and if > another TYPE_X added between them, it might break if TYPE_X doesn't > re-implement this logic in its reset. That's not really an issue. Assuming your example inheritance heirachy: TYPE_ARM_CPU -> TYPE_X -> TYPE_CPU If TYPE_X does not override reset, then TYPE_X inherits TYPE_CPU::reset as its reset. When TYPE_ARM_CPU does object_class_get_parent_by_name(TYPE_ARM_CPU), TYPE_X will be returned, but it will call TYPE_CPU::reset through inheritance anyway. If TYPE_X does implement a reset, then its TYPE_X's job to call parent reset as well but that is no different to the status quo anyway. > > 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. > I don't think thats a good idea. Then reset is inconsistent with other deviceClass::foo APIs. Leave calling of the parent open coded for the child to determine. Also the parent should not be allowed to force the child to call its reset. The child should be able override an API throwing the original away entirely. The child should also be able to call the parent version when it wants, rather than us having to worry about the need for multiple "post or pre" versions. Regards, Peter >> 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, >> }; >> > >
On Thu, Jul 11, 2013 at 8:36 PM, Andreas Färber <afaerber@suse.de> wrote: > Am 11.07.2013 12:31, schrieb Michael S. Tsirkin: >> On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote: >>> On Thu, 11 Jul 2013 11:47:16 +1000 >>> peter.crosthwaite@xilinx.com wrote: >>> >>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>> >>>> 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 <peter.crosthwaite@xilinx.com> >>>> --- >>>> >>>> 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. > That means the function has knowedge of who the direct parent is which I am trying to avoid. The goal is the Device::realize function knows it has to call the parent version. As for who that direct parent is, that ideally remain abstracted away from here. If you directly reference a parent class by name you also make it hard to change an objects parent (which is currently only defined in one place. > My suggestion was to hide the implementation details behind an > ARM_CPU_GET_PARENT_CLASS(obj) macro. > Respinning. as such. I think this is a good idea. Looks more consistent with neighbouring code as well. Regards, Peter
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)); 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, };