On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: > On Mon, 5 Feb 2018 11:54:01 -0200 > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote: > > > On Fri, 2 Feb 2018 16:23:26 -0200 > > > Eduardo Habkost <ehabk...@redhat.com> wrote: > > > > > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote: > > > > > As cpu_type is not a user visible string let's convert the > > > > > valid_cpu_types to compare against cpu_model instead. This way we > > > > > have a > > > > > user friendly string to report back. > > > > > > > > > > Once we have a cpu_type to cpu_model conversion this patch should be > > > > > reverted and we should use cpu_type instead. > > > > > > > > > > Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com> > > > > > --- > > > > > > > > > > hw/core/machine.c | 11 +++++------ > > > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > > > index cdc1163dc6..de5bac1c84 100644 > > > > > --- a/hw/core/machine.c > > > > > +++ b/hw/core/machine.c > > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState > > > > > *machine) > > > > > /* If the machine supports the valid_cpu_types check and the user > > > > > * specified a CPU with -cpu check here that the user CPU is > > > > > supported. > > > > > */ > > > > > - if (machine_class->valid_cpu_types && machine->cpu_type) { > > > > > - ObjectClass *class = object_class_by_name(machine->cpu_type); > > > > > + if (machine_class->valid_cpu_types && machine->cpu_model) { > > > > > int i; > > > > > > > > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > > > > > - if (object_class_dynamic_cast(class, > > > > > - > > > > > machine_class->valid_cpu_types[i])) { > > > > > + if (!strcmp(machine->cpu_model, > > > > > + machine_class->valid_cpu_types[i])) { > > > > > > > > I would rename valid_cpu_types to valid_cpu_models to make the > > > > new semantics clearer. > > > > > > > > Anyway, I have bad and good news: > > > > > > > > The bad news is Igor already sent patches last week that remove > > > > MachineState::cpu_model, so this conflicts with his series. Now > > > > parse_cpu_model() will be the only place where the original CPU model > > > > name is > > > > available, but the function needs to work on *-user too. See: > > > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)". > > > > > > > > The good news is that I think we can fix this very easily if > > > > validation is done at the same place where parse_cpu_model() is > > > > called. e.g.: > > > > > > > > current_machine->cpu_type = machine_class->default_cpu_type; > > > > if (cpu_model) { > > > > current_machine->cpu_type = parse_cpu_model(cpu_model); > > > > > > > > if (machine_class->valid_cpu_models) { > > > > ObjectClass *class = > > > > object_class_by_name(machine->cpu_type); > > > > int i; > > > > > > > > for (i = 0; machine_class->valid_cpu_models[i]; i++) { > > > > const char *valid_model = > > > > machine_class->valid_cpu_models[i]; > > > > ObjectClass *valid_class = > > > > cpu_class_by_name(machine->cpu_type, valid_model); > > > > if (object_class_dynamic_cast(class, > > > > > > > > object_class_get_name(valid_class))) { > > > > /* Valid CPU type, we're good to go */ > > > > break; > > > > } > > > > } > > > > if (!machine_class->valid_cpu_models[i]) { > > > > error_report("Invalid CPU model: %s", cpu_model); > > > > error_printf("The valid CPU models are: %s", > > > > machine_class->valid_cpu_models[0]); > > > > for (i = 1; machine_class->valid_cpu_models[i]; i++) { > > > > error_printf(", %s", > > > > machine_class->valid_cpu_models[i]); > > > > } > > > > error_printf("\n"); > > > > exit(1); > > > > } > > > > } > > > > } > > > > > > > > This can be done inside main(), or moved inside > > > > machine_run_board_init() if main() pass cpu_model as argument to > > > > the function. > > > > > > > > On either case, I think it's a good idea to do validation and > > > > printing of error messages closer to the code that parses the > > > > command-line options. This way we separate parsing/validation > > > > from initialization. > > > I agree it's better like you suggest as at least it prevents > > > ms->cpu_model creeping back into boards code. > > > > > > But I still dislike (hate) an idea of new code adding non > > > canonized cpu_model strings back in the boards code. > > > It's just a matter of time when someone would use them > > > and cpu_model parsing will creep back into boards. > > > > > > It would be much better to if we add > > > char *MachineClass::cpu_name_by_type_name(char *cpu_type) > > > callback and let machines in this patchset to set it, > > > something along following lines which is not much of > > > refactoring and allows for gradual conversion: > > > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > > index 9631670..85cca84 100644 > > > --- a/target/arm/cpu.h > > > +++ b/target/arm/cpu.h > > > @@ -2885,4 +2885,6 @@ static inline void > > > *arm_get_el_change_hook_opaque(ARMCPU *cpu) > > > return cpu->el_change_hook_opaque; > > > } > > > > > > +char *arm_cpu_name_by_type_name(const char *typename); > > > + > > > #endif > > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c > > > index f936017..ae6adb7 100644 > > > --- a/hw/arm/netduino2.c > > > +++ b/hw/arm/netduino2.c > > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc) > > > mc->desc = "Netduino 2 Machine"; > > > mc->init = netduino2_init; > > > mc->ignore_memory_transaction_failures = true; > > > + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name: > > > > I really don't want to introduce a new arch-specific hook just > > for that. We should move CPU type lookup logic to common code > > and make it unnecessary to write new hooks. > unfortunately cpu_model (cpu name part) is target specific > and it's translation to type and back is target specific mayhem.
Why can't the model<->type translation be represented as data? We could have simple cpu_type_name_suffix + an alias table. We have at least 4 arches that return a constant at class_by_name. We have at least 10 arches that simply add a suffix to the CPU model name. We must make them use common code instead of requiring them to implement yet another hook[1]. In addition to the ones above, we have 3 that seem to just need an alias table (cris, superh, alpha). ppc can probably also use an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just needs whitespaces translated to '-' (sparc), which can be done using an alias table. In the end I couldn't find any example that can't be represented by a suffix + alias table. > > So I'd prefer having both back and forth functions together in > one place. And common code to call them when necessary. > > We could do global cpu_name_by_type_name() instead of hook, > which I'd prefer even more but then conversion can't be done > only for one target but rather for all targets at once. I don't mind letting a few targets override default behavior with a hook if really necessary, but I have a problem with requiring all targets to implement what's basically the same boilerplate code to add/remove a string suffix and translating aliases. > > > I agree it would be better if we had a cpu_name_by_type_name() > > function, but I would like to have it implemented cleanly. > In some cases(targets) it can be common helper, but in other > cases it's not so. > My suggestion though allows to do gradual conversion and > avoid putting cpu_model names back in board's code (which I just manged to > remove). > Once all targets converted and relevant code is isolated > we can attempt to generalize it if it's possible or at least > make of it global per target helper to get rid of > temporary machine hook. > > (seeing this series reposted with cpu_model names in boards code, > it doesn't looks like author would like to implement tree-wide > generalization first) Well, if nobody is willing to generalize all target-specific code right now, I don't see the harm in having cpu_model-based tables in a few boards in the meantime (as this patch series does). But I do see harm in requiring all our 20 targets to implement yet another hook and increasing the costs of cleaning up the mess later. --- [1] Really, this amount of duplication is insane: static char *mips_cpu_type_name(const char *cpu_model) { return g_strdup_printf(MIPS_CPU_TYPE_NAME("%s"), cpu_model); } static ObjectClass *mips_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = mips_cpu_type_name(cpu_model); oc = object_class_by_name(typename); g_free(typename); return oc; } static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU) || object_class_is_abstract(oc)) { return NULL; } return oc; } static ObjectClass *uc32_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(UNICORE32_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_UNICORE32_CPU) || object_class_is_abstract(oc))) { oc = NULL; } return oc; } static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU) || object_class_is_abstract(oc)) { return NULL; } return oc; } static char *x86_cpu_type_name(const char *model_name) { return g_strdup_printf(X86_CPU_TYPE_NAME("%s"), model_name); } static ObjectClass *x86_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; if (cpu_model == NULL) { return NULL; } typename = x86_cpu_type_name(cpu_model); oc = object_class_by_name(typename); g_free(typename); return oc; } static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; char **cpuname; cpuname = g_strsplit(cpu_model, ",", 1); typename = g_strdup_printf(ARM_CPU_TYPE_NAME("%s"), cpuname[0]); oc = object_class_by_name(typename); g_strfreev(cpuname); g_free(typename); if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) || object_class_is_abstract(oc)) { return NULL; } return oc; } static ObjectClass *lm32_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(LM32_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_LM32_CPU) || object_class_is_abstract(oc))) { oc = NULL; } return oc; } static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc != NULL && (object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL || object_class_is_abstract(oc))) { return NULL; } return oc; } static ObjectClass *moxie_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(MOXIE_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_MOXIE_CPU) || object_class_is_abstract(oc))) { return NULL; } return oc; } static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; char *typename; typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model); oc = object_class_by_name(typename); g_free(typename); if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) || object_class_is_abstract(oc))) { return NULL; } return oc; } -- Eduardo