| Message ID | 1358942867-8612-5-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote: > Consolidate model checking into a new arm_cpu_class_by_name(). > > If the name matches an existing type, also check whether that type is > actually (a sub-type of) TYPE_ARM_CPU. > > This fixes, e.g., -cpu tmp105 asserting. > > Cc: qemu-stable <qemu-stable@nongnu.org> > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > target-arm/cpu.c | 17 +++++++++++++++++ > target-arm/helper.c | 6 ++++-- > 2 Dateien geändert, 21 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 07588a1..d85f251 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -201,6 +201,21 @@ void arm_cpu_realize(ARMCPU *cpu) > > /* CPU models */ > > +static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) > +{ > + ObjectClass *oc; > + > + if (cpu_model == NULL) { > + return NULL; > + } explicit "== NULL" is kind of ugly; established style in target-arm/ is "if (!cpu_model)..." > + > + oc = object_class_by_name(cpu_model); I note that the object_class_by_name() implementation returns NULL for NULL input, though the documentation doesn't guarantee it will... > + if (oc == NULL || object_class_dynamic_cast(oc, TYPE_ARM_CPU) == NULL) { > + return NULL; > + } > + return oc; > +} > + > static void arm926_initfn(Object *obj) > { > ARMCPU *cpu = ARM_CPU(obj); > @@ -766,6 +781,8 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) > > acc->parent_reset = cc->reset; > cc->reset = arm_cpu_reset; > + > + cc->class_by_name = arm_cpu_class_by_name; Is this a class method because the plan is that eventually the code that instantiates the CPU object will become generic rather than target specific? > } > > static void cpu_register(const ARMCPUInfo *info) > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 37c34a1..4c29117 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -1262,12 +1262,14 @@ ARMCPU *cpu_arm_init(const char *cpu_model) > { > ARMCPU *cpu; > CPUARMState *env; > + ObjectClass *oc; > static int inited = 0; > > - if (!object_class_by_name(cpu_model)) { > + oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); > + if (oc == NULL) { > return NULL; > } > - cpu = ARM_CPU(object_new(cpu_model)); > + cpu = ARM_CPU(object_new(object_class_get_name(oc))); Do we really have to convert back to the char* type name in order to instantiate an object given the class? > env = &cpu->env; > env->cpu_model_str = cpu_model; > arm_cpu_realize(cpu); > -- > 1.7.10.4 thanks -- PMM
Am 23.01.2013 14:03, schrieb Peter Maydell: > On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote: >> Consolidate model checking into a new arm_cpu_class_by_name(). >> >> If the name matches an existing type, also check whether that type is >> actually (a sub-type of) TYPE_ARM_CPU. >> >> This fixes, e.g., -cpu tmp105 asserting. >> >> Cc: qemu-stable <qemu-stable@nongnu.org> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> target-arm/cpu.c | 17 +++++++++++++++++ >> target-arm/helper.c | 6 ++++-- >> 2 Dateien geändert, 21 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) >> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c >> index 07588a1..d85f251 100644 >> --- a/target-arm/cpu.c >> +++ b/target-arm/cpu.c >> @@ -201,6 +201,21 @@ void arm_cpu_realize(ARMCPU *cpu) >> >> /* CPU models */ >> >> +static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) >> +{ >> + ObjectClass *oc; >> + >> + if (cpu_model == NULL) { >> + return NULL; >> + } > > explicit "== NULL" is kind of ugly; established style in > target-arm/ is "if (!cpu_model)..." I consistently use !foo only if foo is bool. Any decent compiler will optimize this appropriately. It not being that way in helper.c most likely is a symptom of you replacing my patch with your initfn approach. ;) > >> + >> + oc = object_class_by_name(cpu_model); > > I note that the object_class_by_name() implementation returns > NULL for NULL input, though the documentation doesn't guarantee > it will... > >> + if (oc == NULL || object_class_dynamic_cast(oc, TYPE_ARM_CPU) == NULL) { >> + return NULL; >> + } >> + return oc; >> +} >> + >> static void arm926_initfn(Object *obj) >> { >> ARMCPU *cpu = ARM_CPU(obj); >> @@ -766,6 +781,8 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) >> >> acc->parent_reset = cc->reset; >> cc->reset = arm_cpu_reset; >> + >> + cc->class_by_name = arm_cpu_class_by_name; > > Is this a class method because the plan is that eventually > the code that instantiates the CPU object will become > generic rather than target specific? Yes, the plan as indicated in the CPUState realizefn series is to generalize cpu_init() so that it only needs to know which base type to operate on. I'm not yet sure how to handle CPU properties in a generic way, but said series got three or four targets into a generic QOM'ish form already. > >> } >> >> static void cpu_register(const ARMCPUInfo *info) >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 37c34a1..4c29117 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -1262,12 +1262,14 @@ ARMCPU *cpu_arm_init(const char *cpu_model) >> { >> ARMCPU *cpu; >> CPUARMState *env; >> + ObjectClass *oc; >> static int inited = 0; >> >> - if (!object_class_by_name(cpu_model)) { >> + oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); >> + if (oc == NULL) { >> return NULL; >> } >> - cpu = ARM_CPU(object_new(cpu_model)); >> + cpu = ARM_CPU(object_new(object_class_get_name(oc))); > > Do we really have to convert back to the char* type > name in order to instantiate an object given the class? Unless someone adds a new function, I fear so... internally TypeImpl is used as alternative but that's not really exposed so far. CC'ing Anthony. Cheers, Andreas > >> env = &cpu->env; >> env->cpu_model_str = cpu_model; >> arm_cpu_realize(cpu); >> -- >> 1.7.10.4 > > thanks > -- PMM >
On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote: > Am 23.01.2013 14:03, schrieb Peter Maydell: >> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote: >>> + if (cpu_model == NULL) { >>> + return NULL; >>> + } >> >> explicit "== NULL" is kind of ugly; established style in >> target-arm/ is "if (!cpu_model)..." > > I consistently use !foo only if foo is bool. Any decent compiler will > optimize this appropriately. Yes -- I prefer (!ptr) not because I think the code will be different but because I think it is better style (and consistent with the current code -- there are no instances of "== NULL" in target-arm today). >> Is this a class method because the plan is that eventually >> the code that instantiates the CPU object will become >> generic rather than target specific? > > Yes, the plan as indicated in the CPUState realizefn series is to > generalize cpu_init() so that it only needs to know which base type to > operate on. I'm not yet sure how to handle CPU properties in a generic > way, but said series got three or four targets into a generic QOM'ish > form already. Cool. -- PMM
Am 23.01.2013 14:41, schrieb Peter Maydell: > On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote: >> Am 23.01.2013 14:03, schrieb Peter Maydell: >>> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote: >>>> + if (cpu_model == NULL) { >>>> + return NULL; >>>> + } >>> >>> explicit "== NULL" is kind of ugly; established style in >>> target-arm/ is "if (!cpu_model)..." >> >> I consistently use !foo only if foo is bool. Any decent compiler will >> optimize this appropriately. > > Yes -- I prefer (!ptr) not because I think the code will be different > but because I think it is better style (and consistent with the > current code -- there are no instances of "== NULL" in target-arm > today). Please see style-changed version here: https://github.com/afaerber/qemu-cpu/commits/qom-cpu-types https://github.com/afaerber/qemu-cpu/commit/726554290fa69425d0e94e2e4fd2fdfeeb54e00c >>> Is this a class method because the plan is that eventually >>> the code that instantiates the CPU object will become >>> generic rather than target specific? >> >> Yes, the plan as indicated in the CPUState realizefn series is to >> generalize cpu_init() so that it only needs to know which base type to >> operate on. I'm not yet sure how to handle CPU properties in a generic >> way, but said series got three or four targets into a generic QOM'ish >> form already. https://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg03606.html > Cool. Preview of rebased CPU realizefn here: https://github.com/afaerber/qemu-cpu/commits/qom-cpu-realize Andreas
On 23 January 2013 15:25, Andreas Färber <afaerber@suse.de> wrote: > Am 23.01.2013 14:41, schrieb Peter Maydell: >> On 23 January 2013 13:38, Andreas Färber <afaerber@suse.de> wrote: >>> Am 23.01.2013 14:03, schrieb Peter Maydell: >>>> On 23 January 2013 12:07, Andreas Färber <afaerber@suse.de> wrote: >>>>> + if (cpu_model == NULL) { >>>>> + return NULL; >>>>> + } >>>> >>>> explicit "== NULL" is kind of ugly; established style in >>>> target-arm/ is "if (!cpu_model)..." >>> >>> I consistently use !foo only if foo is bool. Any decent compiler will >>> optimize this appropriately. >> >> Yes -- I prefer (!ptr) not because I think the code will be different >> but because I think it is better style (and consistent with the >> current code -- there are no instances of "== NULL" in target-arm >> today). > > Please see style-changed version here: > https://github.com/afaerber/qemu-cpu/commits/qom-cpu-types > https://github.com/afaerber/qemu-cpu/commit/726554290fa69425d0e94e2e4fd2fdfeeb54e00c Thanks. Acked-by: Peter Maydell <peter.maydell@linaro.org> for the arm related patches in this series. -- PMM
diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 07588a1..d85f251 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -201,6 +201,21 @@ void arm_cpu_realize(ARMCPU *cpu) /* CPU models */ +static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) +{ + ObjectClass *oc; + + if (cpu_model == NULL) { + return NULL; + } + + oc = object_class_by_name(cpu_model); + if (oc == NULL || object_class_dynamic_cast(oc, TYPE_ARM_CPU) == NULL) { + return NULL; + } + return oc; +} + static void arm926_initfn(Object *obj) { ARMCPU *cpu = ARM_CPU(obj); @@ -766,6 +781,8 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data) acc->parent_reset = cc->reset; cc->reset = arm_cpu_reset; + + cc->class_by_name = arm_cpu_class_by_name; } static void cpu_register(const ARMCPUInfo *info) diff --git a/target-arm/helper.c b/target-arm/helper.c index 37c34a1..4c29117 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -1262,12 +1262,14 @@ ARMCPU *cpu_arm_init(const char *cpu_model) { ARMCPU *cpu; CPUARMState *env; + ObjectClass *oc; static int inited = 0; - if (!object_class_by_name(cpu_model)) { + oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model); + if (oc == NULL) { return NULL; } - cpu = ARM_CPU(object_new(cpu_model)); + cpu = ARM_CPU(object_new(object_class_get_name(oc))); env = &cpu->env; env->cpu_model_str = cpu_model; arm_cpu_realize(cpu);
Consolidate model checking into a new arm_cpu_class_by_name(). If the name matches an existing type, also check whether that type is actually (a sub-type of) TYPE_ARM_CPU. This fixes, e.g., -cpu tmp105 asserting. Cc: qemu-stable <qemu-stable@nongnu.org> Signed-off-by: Andreas Färber <afaerber@suse.de> --- target-arm/cpu.c | 17 +++++++++++++++++ target-arm/helper.c | 6 ++++-- 2 Dateien geändert, 21 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)