* [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property @ 2018-02-02 0:42 Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis ` (6 more replies) 0 siblings, 7 replies; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell There are numorous QEMU machines that only have a single or a handful of valid CPU options. To simplyfy the management of specificying which CPU is/isn't valid let's create a property that can be set in the machine init. We can then check to see if the user supplied CPU is in that list or not. I have added the valid_cpu_types for some ARM machines only at the moment. Here is what specifying the CPUs looks like now: $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S QEMU 2.10.50 monitor - type 'help' for more information (qemu) info cpus * CPU #0: thread_id=24175 (qemu) q $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S QEMU 2.10.50 monitor - type 'help' for more information (qemu) q $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S qemu-system-aarch64: unable to find CPU model 'cortex-m5' $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu The valid models are: cortex-m3, cortex-m4 V5: - Use cpu_model instead of cpu_type V4: - Rebase - Remove spaces V3: - Make the varialbes static V2: - Rebase - Reorder patches - Add a Raspberry Pi 2 CPU fix V1: - Small fixes to prepare a series instead of RFC - Add commit messages for the commits - Expand the machine support to ARM machines RFC v2: - Rebase on Igor's work - Use more QEMUisms inside the code - List the supported machines in a NULL terminated array Alistair Francis (6): machine: Convert the valid cpu types to use cpu_model netduino2: Specify the valid CPUs bcm2836: Use the Cortex-A7 instead of Cortex-A15 raspi: Specify the valid CPUs xlnx-zcu102: Specify the valid CPUs xilinx_zynq: Specify the valid CPUs hw/arm/bcm2836.c | 2 +- hw/arm/netduino2.c | 10 +++++++++- hw/arm/raspi.c | 7 +++++++ hw/arm/xilinx_zynq.c | 6 ++++++ hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++ hw/core/machine.c | 11 +++++------ 6 files changed, 45 insertions(+), 8 deletions(-) -- 2.14.1 ^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-02-02 18:23 ` Eduardo Habkost 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis ` (5 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell 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.francis@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])) { /* The user specificed CPU is in the valid field, we are * good to go. */ @@ -792,8 +791,8 @@ void machine_run_board_init(MachineState *machine) if (!machine_class->valid_cpu_types[i]) { /* The user specified CPU is not valid */ - error_report("Invalid CPU type: %s", machine->cpu_type); - error_printf("The valid types are: %s", + error_report("Invalid CPU model: %s", machine->cpu_model); + error_printf("The valid models are: %s", machine_class->valid_cpu_types[0]); for (i = 1; machine_class->valid_cpu_types[i]; i++) { error_printf(", %s", machine_class->valid_cpu_types[i]); -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis @ 2018-02-02 18:23 ` Eduardo Habkost 2018-02-05 11:22 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2018-02-02 18:23 UTC (permalink / raw) To: Alistair Francis Cc: qemu-devel, peter.maydell, f4bug, imammedo, marcel, alistair23 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.francis@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. > /* The user specificed CPU is in the valid field, we are > * good to go. > */ > @@ -792,8 +791,8 @@ void machine_run_board_init(MachineState *machine) > > if (!machine_class->valid_cpu_types[i]) { > /* The user specified CPU is not valid */ > - error_report("Invalid CPU type: %s", machine->cpu_type); > - error_printf("The valid types are: %s", > + error_report("Invalid CPU model: %s", machine->cpu_model); > + error_printf("The valid models are: %s", > machine_class->valid_cpu_types[0]); > for (i = 1; machine_class->valid_cpu_types[i]; i++) { > error_printf(", %s", machine_class->valid_cpu_types[i]); > -- > 2.14.1 > > -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-02 18:23 ` Eduardo Habkost @ 2018-02-05 11:22 ` Igor Mammedov 2018-02-05 13:54 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2018-02-05 11:22 UTC (permalink / raw) To: Eduardo Habkost Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23 On Fri, 2 Feb 2018 16:23:26 -0200 Eduardo Habkost <ehabkost@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.francis@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: } DEFINE_MACHINE("netduino2", netduino2_machine_init) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index cc1856c..dacc3cc 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -928,6 +928,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) acc->parent_realize(dev, errp); } +char *arm_cpu_name_by_type_name(const char *typename) +{ + return g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU)); +} + static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) { ObjectClass *oc; diff --git a/target/arm/helper.c b/target/arm/helper.c index c83c901..e27b7f0 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5373,7 +5373,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data) char *name; typename = object_class_get_name(oc); - name = g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU)); + name = arm_cpu_name_by_type_name(typename); (*s->cpu_fprintf)(s->file, " %s\n", name); g_free(name); @@ -5410,8 +5410,7 @@ static void arm_cpu_add_definition(gpointer data, gpointer user_data) typename = object_class_get_name(oc); info = g_malloc0(sizeof(*info)); - info->name = g_strndup(typename, - strlen(typename) - strlen("-" TYPE_ARM_CPU)); + info->name = arm_cpu_name_by_type_name(typename); info->q_typename = g_strdup(typename); entry = g_malloc0(sizeof(*entry)); diff --git a/vl.c b/vl.c index c10b0f4..fda1cb2 100644 --- a/vl.c +++ b/vl.c @@ -4646,6 +4646,11 @@ int main(int argc, char **argv, char **envp) if (cpu_model) { current_machine->cpu_type = cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model); + + if !is_valid_cpu(current_machine->cpu_type) + print("valid cpu types: ") + for_each_valid_type + print(machine_class->cpu_name_by_type_name()) } } ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-05 11:22 ` Igor Mammedov @ 2018-02-05 13:54 ` Eduardo Habkost 2018-02-05 14:42 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2018-02-05 13:54 UTC (permalink / raw) To: Igor Mammedov Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23 On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote: > On Fri, 2 Feb 2018 16:23:26 -0200 > Eduardo Habkost <ehabkost@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.francis@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. 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. -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-05 13:54 ` Eduardo Habkost @ 2018-02-05 14:42 ` Igor Mammedov 2018-02-05 22:42 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2018-02-05 14:42 UTC (permalink / raw) To: Eduardo Habkost Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23 On Mon, 5 Feb 2018 11:54:01 -0200 Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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. 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 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) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-05 14:42 ` Igor Mammedov @ 2018-02-05 22:42 ` Eduardo Habkost 2018-02-06 14:43 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2018-02-05 22:42 UTC (permalink / raw) To: Igor Mammedov Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23 On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: > On Mon, 5 Feb 2018 11:54:01 -0200 > Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-05 22:42 ` Eduardo Habkost @ 2018-02-06 14:43 ` Igor Mammedov 2019-06-17 5:09 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2018-02-06 14:43 UTC (permalink / raw) To: Eduardo Habkost Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23 On Mon, 5 Feb 2018 20:42:05 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: > > On Mon, 5 Feb 2018 11:54:01 -0200 > > Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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]. True, some of them could use generic hook and reduce code duplication greatly, we should do it regardless of whether table or target specific func approach is used. > 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. Table based approach is possible but it won't be as simple as you've just pictured it. From what I recall from cpu_class_by_name cleanups table should be able to describe cases like (sometimes combination of them): * 1:1 mapping - where cpu_model == cpu_type * cpu_model <==> cpu_model + suffix - most common usecase * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types * NULL => some_fixed type * case (in) sensitive flag * garbage => some_fixed type * substitutions * aliases (sometimes dynamic depending on --enable-kvm (PPC)) Maybe something else. We can think about it at leisure but I can't say if new approach complexity it's worth of the effort. It would be nice see impl, but it's a lot of refactoring that's clearly out of scope of this series. I'd prefer small incremental refactoring (if possible) that won't scare people of and easy to review vs a huge one. > > 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. it could be generic helper if target does the same plus not mandatory at that (in case target/board doesn't care about valid cpus). > > > 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. If we use MachineClass hook then it might be done per target on demand, so no one would require that every target should implement it. Also there could be a generic helper for targets that do the same. Machine which needs to enable valid_cpus, will have to use generic hook impl or provide target specific if it's special case. Though I do see harm in adding cpu_model tables in boards code vs target specific hooks on demand as that will be copy-pasted in other boards afterwards (number of which is bigger compared to targets count) and ultimately it would duplicate cpu_name strings in every board vs hook approach where cpu_model could be calculated from cpu_type by a function (generic or target specific). Good thing about hook is that it's non intrusive and isolates(consolidates) existing cpu_type -> cpu_model conversion in multiple places into one place. Then later it would be easier to generalize if someone decides to do it. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2018-02-06 14:43 ` Igor Mammedov @ 2019-06-17 5:09 ` Philippe Mathieu-Daudé 2019-06-17 14:43 ` Eduardo Habkost 2019-06-17 15:01 ` Igor Mammedov 0 siblings, 2 replies; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2019-06-17 5:09 UTC (permalink / raw) To: Igor Mammedov, Eduardo Habkost Cc: marcel, peter.maydell, qemu-devel, alistair23 Hi Igor, Eduardo, On 2/6/18 3:43 PM, Igor Mammedov wrote: > On Mon, 5 Feb 2018 20:42:05 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: >>> On Mon, 5 Feb 2018 11:54:01 -0200 >>> Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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]. > True, some of them could use generic hook and reduce > code duplication greatly, we should do it regardless of whether > table or target specific func approach is used. > >> 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. > > Table based approach is possible but it won't be as simple > as you've just pictured it. > > From what I recall from cpu_class_by_name cleanups table should be able > to describe cases like (sometimes combination of them): > * 1:1 mapping - where cpu_model == cpu_type > * cpu_model <==> cpu_model + suffix - most common usecase > * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types > * NULL => some_fixed type > * case (in) sensitive flag > * garbage => some_fixed type > * substitutions > * aliases (sometimes dynamic depending on --enable-kvm (PPC)) > Maybe something else. > > We can think about it at leisure but I can't say if new approach > complexity it's worth of the effort. > > It would be nice see impl, but it's a lot of refactoring that's > clearly out of scope of this series. > I'd prefer small incremental refactoring (if possible) that > won't scare people of and easy to review vs a huge one. > >>> 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. > it could be generic helper if target does the same plus > not mandatory at that (in case target/board doesn't care > about valid cpus). > >>>> 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. > If we use MachineClass hook then it might be done per target > on demand, so no one would require that every target should > implement it. > Also there could be a generic helper for targets that do the same. > Machine which needs to enable valid_cpus, will have to use generic > hook impl or provide target specific if it's special case. > > Though I do see harm in adding cpu_model tables in boards code > vs target specific hooks on demand as that will be copy-pasted > in other boards afterwards (number of which is bigger compared > to targets count) and ultimately it would duplicate cpu_name > strings in every board vs hook approach where cpu_model could > be calculated from cpu_type by a function (generic or > target specific). > > Good thing about hook is that it's non intrusive and > isolates(consolidates) existing cpu_type -> cpu_model > conversion in multiple places into one place. > Then later it would be easier to generalize if someone > decides to do it. I wonder how you want to proceed with this series, the first patch got merged (c9cf636d48f) but after your "CPU model name" rework, this commit seems now not very complete/usable. Rebasing this series, i.e. with this snippet: -- >8 -- diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c index f57fc38f92..cca4ec6648 100644 --- a/hw/arm/netduino2.c +++ b/hw/arm/netduino2.c @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine) DeviceState *dev; dev = qdev_create(NULL, TYPE_STM32F205_SOC); - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type); object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal); armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine) static void netduino2_machine_init(MachineClass *mc) { + static const char *valid_cpus[] = { + ARM_CPU_TYPE_NAME("cortex-m3"), + ARM_CPU_TYPE_NAME("cortex-m4"), + NULL + }; mc->desc = "Netduino 2 Machine"; mc->init = netduino2_init; + mc->valid_cpu_types = valid_cpus; mc->ignore_memory_transaction_failures = true; } --- We get cpu names with suffix: $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926 qemu-system-arm: Invalid CPU type: arm926-arm-cpu The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu I understand you won't want a global cpu_name_by_type_name, how do you want to do then? Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX? Then we could have generic machine code to parse the names. Thanks, Phil. ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 5:09 ` Philippe Mathieu-Daudé @ 2019-06-17 14:43 ` Eduardo Habkost 2019-06-17 15:01 ` Igor Mammedov 1 sibling, 0 replies; 33+ messages in thread From: Eduardo Habkost @ 2019-06-17 14:43 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: marcel, Igor Mammedov, qemu-devel, alistair23, peter.maydell On Mon, Jun 17, 2019 at 07:09:59AM +0200, Philippe Mathieu-Daudé wrote: [...] > > We get cpu names with suffix: > > $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926 > qemu-system-arm: Invalid CPU type: arm926-arm-cpu > The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > I understand you won't want a global cpu_name_by_type_name, how do you > want to do then? I would like having a global cpu_name_by_type_name() function. I wouldn't want to force each architecture to provide yet another string conversion/parsing function, though. > > Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX? > Then we could have generic machine code to parse the names. I think we could do that, but with a CPUClass field instead of a preprocessor macro. For a reference on existing corner cases of CPU model name parsing that might get in the way, see this series: https://www.mail-archive.com/qemu-devel@nongnu.org/msg611813.html If you are interested, I have work in progress for a generic CPU model listing function at: https://github.com/ehabkost/qemu-hacks/commits/work/cpu-generic-list https://github.com/ehabkost/qemu-hacks/commit/df122e7e47476d7e2b7b809a4b4120537f50c137 -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 5:09 ` Philippe Mathieu-Daudé 2019-06-17 14:43 ` Eduardo Habkost @ 2019-06-17 15:01 ` Igor Mammedov 2019-06-17 15:15 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2019-06-17 15:01 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel On Mon, 17 Jun 2019 07:09:59 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Igor, Eduardo, > > On 2/6/18 3:43 PM, Igor Mammedov wrote: > > On Mon, 5 Feb 2018 20:42:05 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > >> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: > >>> On Mon, 5 Feb 2018 11:54:01 -0200 > >>> Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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]. > > True, some of them could use generic hook and reduce > > code duplication greatly, we should do it regardless of whether > > table or target specific func approach is used. > > > >> 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. > > > > Table based approach is possible but it won't be as simple > > as you've just pictured it. > > > > From what I recall from cpu_class_by_name cleanups table should be able > > to describe cases like (sometimes combination of them): > > * 1:1 mapping - where cpu_model == cpu_type > > * cpu_model <==> cpu_model + suffix - most common usecase > > * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types > > * NULL => some_fixed type > > * case (in) sensitive flag > > * garbage => some_fixed type > > * substitutions > > * aliases (sometimes dynamic depending on --enable-kvm (PPC)) > > Maybe something else. > > > > We can think about it at leisure but I can't say if new approach > > complexity it's worth of the effort. > > > > It would be nice see impl, but it's a lot of refactoring that's > > clearly out of scope of this series. > > I'd prefer small incremental refactoring (if possible) that > > won't scare people of and easy to review vs a huge one. > > > >>> 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. > > it could be generic helper if target does the same plus > > not mandatory at that (in case target/board doesn't care > > about valid cpus). > > > >>>> 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. > > If we use MachineClass hook then it might be done per target > > on demand, so no one would require that every target should > > implement it. > > Also there could be a generic helper for targets that do the same. > > Machine which needs to enable valid_cpus, will have to use generic > > hook impl or provide target specific if it's special case. > > > > Though I do see harm in adding cpu_model tables in boards code > > vs target specific hooks on demand as that will be copy-pasted > > in other boards afterwards (number of which is bigger compared > > to targets count) and ultimately it would duplicate cpu_name > > strings in every board vs hook approach where cpu_model could > > be calculated from cpu_type by a function (generic or > > target specific). > > > > Good thing about hook is that it's non intrusive and > > isolates(consolidates) existing cpu_type -> cpu_model > > conversion in multiple places into one place. > > Then later it would be easier to generalize if someone > > decides to do it. > > I wonder how you want to proceed with this series, the first patch got > merged (c9cf636d48f) but after your "CPU model name" rework, this commit > seems now not very complete/usable. > > Rebasing this series, i.e. with this snippet: > > -- >8 -- > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c > index f57fc38f92..cca4ec6648 100644 > --- a/hw/arm/netduino2.c > +++ b/hw/arm/netduino2.c > @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine) > DeviceState *dev; > > dev = qdev_create(NULL, TYPE_STM32F205_SOC); > - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); > + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type); > object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal); > > armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, > @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine) > > static void netduino2_machine_init(MachineClass *mc) > { > + static const char *valid_cpus[] = { > + ARM_CPU_TYPE_NAME("cortex-m3"), > + ARM_CPU_TYPE_NAME("cortex-m4"), > + NULL > + }; > mc->desc = "Netduino 2 Machine"; > mc->init = netduino2_init; > + mc->valid_cpu_types = valid_cpus; > mc->ignore_memory_transaction_failures = true; > } > --- > > We get cpu names with suffix: > > $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926 > qemu-system-arm: Invalid CPU type: arm926-arm-cpu > The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > > I understand you won't want a global cpu_name_by_type_name, how do you > want to do then? > > Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX? > Then we could have generic machine code to parse the names. It would work only for some cases, problem is that we have a zoo of naming schemes. Considering that cpus models are used widely we probably can't deprecate it outright (for versioned machine types). Instead of wasting resources on translating cpu-type => 'cpu-name', (hook or lookup tables) how about simplifying code and making all boards accept full typenames? It could be handled in generic way and then printing error with full type names would be acceptable since user would be able to feed it to -cpu. '-cpu help' - would need some work to display types as well (also could be generic) Perhaps with it we could deprecate cpu_models for non versioned machines/targets, which in most cases would allow us to drop special suffix/prefix/nonsense/case-sensitive/substitutions and whatever else is already existing and keep exiting translation routines (hooks) only for versioned machine types as necessary evil. > Thanks, > > Phil. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 15:01 ` Igor Mammedov @ 2019-06-17 15:15 ` Philippe Mathieu-Daudé 2019-06-17 15:33 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2019-06-17 15:15 UTC (permalink / raw) To: Igor Mammedov Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel On 6/17/19 5:01 PM, Igor Mammedov wrote: > On Mon, 17 Jun 2019 07:09:59 +0200 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> Hi Igor, Eduardo, >> >> On 2/6/18 3:43 PM, Igor Mammedov wrote: >>> On Mon, 5 Feb 2018 20:42:05 -0200 >>> Eduardo Habkost <ehabkost@redhat.com> wrote: >>> >>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: >>>>> On Mon, 5 Feb 2018 11:54:01 -0200 >>>>> Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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]. >>> True, some of them could use generic hook and reduce >>> code duplication greatly, we should do it regardless of whether >>> table or target specific func approach is used. >>> >>>> 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. >>> >>> Table based approach is possible but it won't be as simple >>> as you've just pictured it. >>> >>> From what I recall from cpu_class_by_name cleanups table should be able >>> to describe cases like (sometimes combination of them): >>> * 1:1 mapping - where cpu_model == cpu_type >>> * cpu_model <==> cpu_model + suffix - most common usecase >>> * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types >>> * NULL => some_fixed type >>> * case (in) sensitive flag >>> * garbage => some_fixed type >>> * substitutions >>> * aliases (sometimes dynamic depending on --enable-kvm (PPC)) >>> Maybe something else. >>> >>> We can think about it at leisure but I can't say if new approach >>> complexity it's worth of the effort. >>> >>> It would be nice see impl, but it's a lot of refactoring that's >>> clearly out of scope of this series. >>> I'd prefer small incremental refactoring (if possible) that >>> won't scare people of and easy to review vs a huge one. >>> >>>>> 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. >>> it could be generic helper if target does the same plus >>> not mandatory at that (in case target/board doesn't care >>> about valid cpus). >>> >>>>>> 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. >>> If we use MachineClass hook then it might be done per target >>> on demand, so no one would require that every target should >>> implement it. >>> Also there could be a generic helper for targets that do the same. >>> Machine which needs to enable valid_cpus, will have to use generic >>> hook impl or provide target specific if it's special case. >>> >>> Though I do see harm in adding cpu_model tables in boards code >>> vs target specific hooks on demand as that will be copy-pasted >>> in other boards afterwards (number of which is bigger compared >>> to targets count) and ultimately it would duplicate cpu_name >>> strings in every board vs hook approach where cpu_model could >>> be calculated from cpu_type by a function (generic or >>> target specific). >>> >>> Good thing about hook is that it's non intrusive and >>> isolates(consolidates) existing cpu_type -> cpu_model >>> conversion in multiple places into one place. >>> Then later it would be easier to generalize if someone >>> decides to do it. >> >> I wonder how you want to proceed with this series, the first patch got >> merged (c9cf636d48f) but after your "CPU model name" rework, this commit >> seems now not very complete/usable. >> >> Rebasing this series, i.e. with this snippet: >> >> -- >8 -- >> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c >> index f57fc38f92..cca4ec6648 100644 >> --- a/hw/arm/netduino2.c >> +++ b/hw/arm/netduino2.c >> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine) >> DeviceState *dev; >> >> dev = qdev_create(NULL, TYPE_STM32F205_SOC); >> - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); >> + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type); >> object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal); >> >> armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, >> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine) >> >> static void netduino2_machine_init(MachineClass *mc) >> { >> + static const char *valid_cpus[] = { >> + ARM_CPU_TYPE_NAME("cortex-m3"), >> + ARM_CPU_TYPE_NAME("cortex-m4"), >> + NULL >> + }; >> mc->desc = "Netduino 2 Machine"; >> mc->init = netduino2_init; >> + mc->valid_cpu_types = valid_cpus; >> mc->ignore_memory_transaction_failures = true; >> } >> --- >> >> We get cpu names with suffix: >> >> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926 >> qemu-system-arm: Invalid CPU type: arm926-arm-cpu >> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu >> >> I understand you won't want a global cpu_name_by_type_name, how do you >> want to do then? >> >> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX? >> Then we could have generic machine code to parse the names. > It would work only for some cases, > problem is that we have a zoo of naming schemes. > Considering that cpus models are used widely we probably can't > deprecate it outright (for versioned machine types). > > Instead of wasting resources on translating cpu-type => 'cpu-name', > (hook or lookup tables) how about simplifying code and making all > boards accept full typenames? > > It could be handled in generic way and then printing error with > full type names would be acceptable since user would be able to feed it > to -cpu. > > '-cpu help' - would need some work to display types as well (also could be generic) > > Perhaps with it we could deprecate cpu_models for non versioned > machines/targets, which in most cases would allow us to drop special > suffix/prefix/nonsense/case-sensitive/substitutions and whatever else > is already existing and keep exiting translation routines (hooks) only > for versioned machine types as necessary evil. Yes. Eduardo and you should write some lines to explain this, and then we will follow :) I feel concerned because: 1/ Alistair series is very helpful to new users, and 2/ As the RX architecture series showed, today it is not very clear how to correctly use cpu_models. Eduardo is working on a series, I'll wait for his work. Regards, Phil. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 15:15 ` Philippe Mathieu-Daudé @ 2019-06-17 15:33 ` Igor Mammedov 2019-06-17 16:27 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2019-06-17 15:33 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel On Mon, 17 Jun 2019 17:15:21 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 6/17/19 5:01 PM, Igor Mammedov wrote: > > On Mon, 17 Jun 2019 07:09:59 +0200 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > >> Hi Igor, Eduardo, > >> > >> On 2/6/18 3:43 PM, Igor Mammedov wrote: > >>> On Mon, 5 Feb 2018 20:42:05 -0200 > >>> Eduardo Habkost <ehabkost@redhat.com> wrote: > >>> > >>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote: > >>>>> On Mon, 5 Feb 2018 11:54:01 -0200 > >>>>> Eduardo Habkost <ehabkost@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 <ehabkost@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.francis@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]. > >>> True, some of them could use generic hook and reduce > >>> code duplication greatly, we should do it regardless of whether > >>> table or target specific func approach is used. > >>> > >>>> 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. > >>> > >>> Table based approach is possible but it won't be as simple > >>> as you've just pictured it. > >>> > >>> From what I recall from cpu_class_by_name cleanups table should be able > >>> to describe cases like (sometimes combination of them): > >>> * 1:1 mapping - where cpu_model == cpu_type > >>> * cpu_model <==> cpu_model + suffix - most common usecase > >>> * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types > >>> * NULL => some_fixed type > >>> * case (in) sensitive flag > >>> * garbage => some_fixed type > >>> * substitutions > >>> * aliases (sometimes dynamic depending on --enable-kvm (PPC)) > >>> Maybe something else. > >>> > >>> We can think about it at leisure but I can't say if new approach > >>> complexity it's worth of the effort. > >>> > >>> It would be nice see impl, but it's a lot of refactoring that's > >>> clearly out of scope of this series. > >>> I'd prefer small incremental refactoring (if possible) that > >>> won't scare people of and easy to review vs a huge one. > >>> > >>>>> 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. > >>> it could be generic helper if target does the same plus > >>> not mandatory at that (in case target/board doesn't care > >>> about valid cpus). > >>> > >>>>>> 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. > >>> If we use MachineClass hook then it might be done per target > >>> on demand, so no one would require that every target should > >>> implement it. > >>> Also there could be a generic helper for targets that do the same. > >>> Machine which needs to enable valid_cpus, will have to use generic > >>> hook impl or provide target specific if it's special case. > >>> > >>> Though I do see harm in adding cpu_model tables in boards code > >>> vs target specific hooks on demand as that will be copy-pasted > >>> in other boards afterwards (number of which is bigger compared > >>> to targets count) and ultimately it would duplicate cpu_name > >>> strings in every board vs hook approach where cpu_model could > >>> be calculated from cpu_type by a function (generic or > >>> target specific). > >>> > >>> Good thing about hook is that it's non intrusive and > >>> isolates(consolidates) existing cpu_type -> cpu_model > >>> conversion in multiple places into one place. > >>> Then later it would be easier to generalize if someone > >>> decides to do it. > >> > >> I wonder how you want to proceed with this series, the first patch got > >> merged (c9cf636d48f) but after your "CPU model name" rework, this commit > >> seems now not very complete/usable. > >> > >> Rebasing this series, i.e. with this snippet: > >> > >> -- >8 -- > >> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c > >> index f57fc38f92..cca4ec6648 100644 > >> --- a/hw/arm/netduino2.c > >> +++ b/hw/arm/netduino2.c > >> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine) > >> DeviceState *dev; > >> > >> dev = qdev_create(NULL, TYPE_STM32F205_SOC); > >> - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); > >> + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type); > >> object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal); > >> > >> armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, > >> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine) > >> > >> static void netduino2_machine_init(MachineClass *mc) > >> { > >> + static const char *valid_cpus[] = { > >> + ARM_CPU_TYPE_NAME("cortex-m3"), > >> + ARM_CPU_TYPE_NAME("cortex-m4"), > >> + NULL > >> + }; > >> mc->desc = "Netduino 2 Machine"; > >> mc->init = netduino2_init; > >> + mc->valid_cpu_types = valid_cpus; > >> mc->ignore_memory_transaction_failures = true; > >> } > >> --- > >> > >> We get cpu names with suffix: > >> > >> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926 > >> qemu-system-arm: Invalid CPU type: arm926-arm-cpu > >> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu > >> > >> I understand you won't want a global cpu_name_by_type_name, how do you > >> want to do then? > >> > >> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX? > >> Then we could have generic machine code to parse the names. > > It would work only for some cases, > > problem is that we have a zoo of naming schemes. > > Considering that cpus models are used widely we probably can't > > deprecate it outright (for versioned machine types). > > > > Instead of wasting resources on translating cpu-type => 'cpu-name', > > (hook or lookup tables) how about simplifying code and making all > > boards accept full typenames? > > > > It could be handled in generic way and then printing error with > > full type names would be acceptable since user would be able to feed it > > to -cpu. > > > > '-cpu help' - would need some work to display types as well (also could be generic) > > > > Perhaps with it we could deprecate cpu_models for non versioned > > machines/targets, which in most cases would allow us to drop special > > suffix/prefix/nonsense/case-sensitive/substitutions and whatever else > > is already existing and keep exiting translation routines (hooks) only > > for versioned machine types as necessary evil. Maybe we don't even need to keep it versioned machine types, considering change mapping is static and doesn't influence ABI/migration. Management interface could just translate cpu_model to type name for new QEMU. That would allow us to remove quite a bit target specific code that deals with all permutations of CPU model and replace a bunch of '-cpu help' target specific handlers with a generic enumeration of built-in CPU types. > Yes. Eduardo and you should write some lines to explain this, and then > we will follow :) Unfortunately I don't recall details anymore. One could check out all implementations of class_by_name callbacks to find out current state. > I feel concerned because: > 1/ Alistair series is very helpful to new users, and > 2/ As the RX architecture series showed, today it is not very clear how > to correctly use cpu_models. That's why I'm pushing for supporting only type names whenever possible so there won't be confusion anymore. Using types is consistent with -device and some QMP interfaces, enforces strict error checking so user would get error on nonsense input. > Eduardo is working on a series, I'll wait for his work. > > Regards, > > Phil. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 15:33 ` Igor Mammedov @ 2019-06-17 16:27 ` Eduardo Habkost 2019-06-18 11:34 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2019-06-17 16:27 UTC (permalink / raw) To: Igor Mammedov Cc: marcel, peter.maydell, Philippe Mathieu-Daudé, qemu-devel, alistair23 On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > On Mon, 17 Jun 2019 17:15:21 +0200 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: [...] > > Yes. Eduardo and you should write some lines to explain this, and then > > we will follow :) > Unfortunately I don't recall details anymore. One could check out all > implementations of class_by_name callbacks to find out current state. See this message for a summary of existing class_by_name quirks: https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html Date: Wed, 08 May 2019 10:34:44 +0200 Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions I'm unsure about Igor's suggestion to get rid of CPU model names and use only QOM type names in external interfaces. In either case, we can still simplify the rules rules and reduce the amount of arch-specific code. -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-17 16:27 ` Eduardo Habkost @ 2019-06-18 11:34 ` Igor Mammedov 2019-06-18 13:55 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2019-06-18 11:34 UTC (permalink / raw) To: Eduardo Habkost Cc: marcel, peter.maydell, Philippe Mathieu-Daudé, qemu-devel, alistair23 On Mon, 17 Jun 2019 13:27:00 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > > On Mon, 17 Jun 2019 17:15:21 +0200 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > [...] > > > Yes. Eduardo and you should write some lines to explain this, and then > > > we will follow :) > > Unfortunately I don't recall details anymore. One could check out all > > implementations of class_by_name callbacks to find out current state. > > See this message for a summary of existing class_by_name quirks: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html > Date: Wed, 08 May 2019 10:34:44 +0200 > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions > > I'm unsure about Igor's suggestion to get rid of CPU model names > and use only QOM type names in external interfaces. In either > case, we can still simplify the rules rules and reduce the amount > of arch-specific code. as far as we have cpu_class_by_name, we have to watch over that new patches/targets won't add some custom handling/fallbac/whatnot. On contrary -device works just with type names for all devices, applying the same to -cpu which is basically translator model->type[,-global type.foo,...] would be consistent with -device and less confusing for everyone (not counting significant code reduction). It would certainly simplify contributing new targets as contributor won't have to care about cpu model naming and do something about it. This option wasn't considered before because we didn't have deprecation back then, but now it opens possibility to simplify qemu and consolidate naming. (we probably would be able to fold '-cpu help' into '-device help' as well). ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-18 11:34 ` Igor Mammedov @ 2019-06-18 13:55 ` Eduardo Habkost 2019-06-20 9:02 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2019-06-18 13:55 UTC (permalink / raw) To: Igor Mammedov Cc: marcel, peter.maydell, Philippe Mathieu-Daudé, qemu-devel, alistair23 On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote: > On Mon, 17 Jun 2019 13:27:00 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > > > On Mon, 17 Jun 2019 17:15:21 +0200 > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > [...] > > > > Yes. Eduardo and you should write some lines to explain this, and then > > > > we will follow :) > > > Unfortunately I don't recall details anymore. One could check out all > > > implementations of class_by_name callbacks to find out current state. > > > > See this message for a summary of existing class_by_name quirks: > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html > > Date: Wed, 08 May 2019 10:34:44 +0200 > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions > > > > I'm unsure about Igor's suggestion to get rid of CPU model names > > and use only QOM type names in external interfaces. In either > > case, we can still simplify the rules rules and reduce the amount > > of arch-specific code. > as far as we have cpu_class_by_name, we have to watch over that > new patches/targets won't add some custom handling/fallbac/whatnot. We can get rid of CPUClass::cpu_class_by_name() without changing the external interfaces provided by QEMU. I don't have a strong opinion about using only QOM types at -cpu, yet. But first we need to get rid of the arch-specific CPU model name exceptions enumerated at the URL above (which would be very welcome). > > On contrary -device works just with type names for all devices, > applying the same to -cpu which is basically translator > model->type[,-global type.foo,...] > would be consistent with -device and less confusing for everyone > (not counting significant code reduction). > It would certainly simplify contributing new targets as contributor > won't have to care about cpu model naming and do something about it. > > This option wasn't considered before because we didn't have deprecation > back then, but now it opens possibility to simplify qemu and consolidate > naming. (we probably would be able to fold '-cpu help' into '-device help' > as well). > -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-18 13:55 ` Eduardo Habkost @ 2019-06-20 9:02 ` Igor Mammedov 2019-06-20 14:43 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Igor Mammedov @ 2019-06-20 9:02 UTC (permalink / raw) To: Eduardo Habkost Cc: marcel, peter.maydell, Philippe Mathieu-Daudé, qemu-devel, alistair23 On Tue, 18 Jun 2019 10:55:16 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote: > > On Mon, 17 Jun 2019 13:27:00 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > > > > On Mon, 17 Jun 2019 17:15:21 +0200 > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > [...] > > > > > Yes. Eduardo and you should write some lines to explain this, and then > > > > > we will follow :) > > > > Unfortunately I don't recall details anymore. One could check out all > > > > implementations of class_by_name callbacks to find out current state. > > > > > > See this message for a summary of existing class_by_name quirks: > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html > > > Date: Wed, 08 May 2019 10:34:44 +0200 > > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> > > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions > > > > > > I'm unsure about Igor's suggestion to get rid of CPU model names > > > and use only QOM type names in external interfaces. In either > > > case, we can still simplify the rules rules and reduce the amount > > > of arch-specific code. > > as far as we have cpu_class_by_name, we have to watch over that > > new patches/targets won't add some custom handling/fallbac/whatnot. > > We can get rid of CPUClass::cpu_class_by_name() without changing > the external interfaces provided by QEMU. if you mean QMP, than it is possible to keep model there where it already exists. Based on experiment [1](x86) I did, it's local to affected target and doesn't pollute other code. > I don't have a strong opinion about using only QOM types at -cpu, > yet. But first we need to get rid of the arch-specific CPU model > name exceptions enumerated at the URL above (which would be very > welcome). One way to get rid of them is to deprecate them in favor of strict match (no fallback/substitutions/aliases) to typename and when we drop it make switch type based naming only. 1) I've just took a quick look at how much of duplicated/confusing code we could get rid off if we drop cpu_class_by_name/*_cpu_list. It amounts to >800LOC of trivial removal (not counting ppc/s390 that depend on model naming heavily and in need of some non trivial refactoring) > > > > > On contrary -device works just with type names for all devices, > > applying the same to -cpu which is basically translator > > model->type[,-global type.foo,...] > > would be consistent with -device and less confusing for everyone > > (not counting significant code reduction). > > It would certainly simplify contributing new targets as contributor > > won't have to care about cpu model naming and do something about it. > > > > This option wasn't considered before because we didn't have deprecation > > back then, but now it opens possibility to simplify qemu and consolidate > > naming. (we probably would be able to fold '-cpu help' into '-device help' > > as well). > > > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-20 9:02 ` Igor Mammedov @ 2019-06-20 14:43 ` Eduardo Habkost 2020-01-23 18:48 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 33+ messages in thread From: Eduardo Habkost @ 2019-06-20 14:43 UTC (permalink / raw) To: Igor Mammedov Cc: marcel, peter.maydell, Philippe Mathieu-Daudé, qemu-devel, alistair23 On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote: > On Tue, 18 Jun 2019 10:55:16 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote: > > > On Mon, 17 Jun 2019 13:27:00 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > > > > > On Mon, 17 Jun 2019 17:15:21 +0200 > > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > [...] > > > > > > Yes. Eduardo and you should write some lines to explain this, and then > > > > > > we will follow :) > > > > > Unfortunately I don't recall details anymore. One could check out all > > > > > implementations of class_by_name callbacks to find out current state. > > > > > > > > See this message for a summary of existing class_by_name quirks: > > > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html > > > > Date: Wed, 08 May 2019 10:34:44 +0200 > > > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> > > > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions > > > > > > > > I'm unsure about Igor's suggestion to get rid of CPU model names > > > > and use only QOM type names in external interfaces. In either > > > > case, we can still simplify the rules rules and reduce the amount > > > > of arch-specific code. > > > as far as we have cpu_class_by_name, we have to watch over that > > > new patches/targets won't add some custom handling/fallbac/whatnot. > > > > We can get rid of CPUClass::cpu_class_by_name() without changing > > the external interfaces provided by QEMU. > if you mean QMP, than it is possible to keep model there where > it already exists. Based on experiment [1](x86) I did, it's local to > affected target and doesn't pollute other code. I mean both command line and QMP. > > > I don't have a strong opinion about using only QOM types at -cpu, > > yet. But first we need to get rid of the arch-specific CPU model > > name exceptions enumerated at the URL above (which would be very > > welcome). > One way to get rid of them is to deprecate them in favor of strict > match (no fallback/substitutions/aliases) to typename and when we > drop it make switch type based naming only. This is true, but is it desirable? Aren't we just moving complexity elsewhere? Management software would still need to translate CPU models from existing VM configurations to QOM type names. > > 1) I've just took a quick look at how much of duplicated/confusing > code we could get rid off if we drop cpu_class_by_name/*_cpu_list. > It amounts to >800LOC of trivial removal (not counting ppc/s390 > that depend on model naming heavily and in need of some non > trivial refactoring) Removing the code might be trivial. Coordinating with the maintainers of all architectures to confirm this is really something everybody wants is the difficult part. If you really want to do it, please make sure all the architecture maintainers (and libvirt developers) are involved in the discussion. > > > > > > > > > On contrary -device works just with type names for all devices, > > > applying the same to -cpu which is basically translator > > > model->type[,-global type.foo,...] > > > would be consistent with -device and less confusing for everyone > > > (not counting significant code reduction). > > > It would certainly simplify contributing new targets as contributor > > > won't have to care about cpu model naming and do something about it. > > > > > > This option wasn't considered before because we didn't have deprecation > > > back then, but now it opens possibility to simplify qemu and consolidate > > > naming. (we probably would be able to fold '-cpu help' into '-device help' > > > as well). > > > > > > -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2019-06-20 14:43 ` Eduardo Habkost @ 2020-01-23 18:48 ` Philippe Mathieu-Daudé 2020-02-06 20:59 ` Eduardo Habkost 0 siblings, 1 reply; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2020-01-23 18:48 UTC (permalink / raw) To: Eduardo Habkost, Igor Mammedov, Markus Armbruster Cc: marcel, peter.maydell, qemu-devel, alistair23 On 6/20/19 4:43 PM, Eduardo Habkost wrote: > On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote: >> On Tue, 18 Jun 2019 10:55:16 -0300 >> Eduardo Habkost <ehabkost@redhat.com> wrote: >> >>> On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote: >>>> On Mon, 17 Jun 2019 13:27:00 -0300 >>>> Eduardo Habkost <ehabkost@redhat.com> wrote: >>>> >>>>> On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: >>>>>> On Mon, 17 Jun 2019 17:15:21 +0200 >>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>>>> [...] >>>>>>> Yes. Eduardo and you should write some lines to explain this, and then >>>>>>> we will follow :) >>>>>> Unfortunately I don't recall details anymore. One could check out all >>>>>> implementations of class_by_name callbacks to find out current state. >>>>> >>>>> See this message for a summary of existing class_by_name quirks: >>>>> >>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html >>>>> Date: Wed, 08 May 2019 10:34:44 +0200 >>>>> Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> >>>>> Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions >>>>> >>>>> I'm unsure about Igor's suggestion to get rid of CPU model names >>>>> and use only QOM type names in external interfaces. In either >>>>> case, we can still simplify the rules rules and reduce the amount >>>>> of arch-specific code. >>>> as far as we have cpu_class_by_name, we have to watch over that >>>> new patches/targets won't add some custom handling/fallbac/whatnot. >>> >>> We can get rid of CPUClass::cpu_class_by_name() without changing >>> the external interfaces provided by QEMU. >> if you mean QMP, than it is possible to keep model there where >> it already exists. Based on experiment [1](x86) I did, it's local to >> affected target and doesn't pollute other code. > > I mean both command line and QMP. > >> >>> I don't have a strong opinion about using only QOM types at -cpu, >>> yet. But first we need to get rid of the arch-specific CPU model >>> name exceptions enumerated at the URL above (which would be very >>> welcome). >> One way to get rid of them is to deprecate them in favor of strict >> match (no fallback/substitutions/aliases) to typename and when we >> drop it make switch type based naming only. > > This is true, but is it desirable? Aren't we just moving > complexity elsewhere? Management software would still need to > translate CPU models from existing VM configurations to QOM type > names. > >> >> 1) I've just took a quick look at how much of duplicated/confusing >> code we could get rid off if we drop cpu_class_by_name/*_cpu_list. >> It amounts to >800LOC of trivial removal (not counting ppc/s390 >> that depend on model naming heavily and in need of some non >> trivial refactoring) > > Removing the code might be trivial. Coordinating with the > maintainers of all architectures to confirm this is really > something everybody wants is the difficult part. > > If you really want to do it, please make sure all the > architecture maintainers (and libvirt developers) are involved in > the discussion. From the previous link (summary of existing class_by_name quirks): https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html "Deprecating unwanted stuff now is likely to make a later cleanup so much easier." This was 8 months ago :/ IIUC we need to restart this topic addressing the libvirt community first, then see with the QEMU one? > >> >>> >>>> >>>> On contrary -device works just with type names for all devices, >>>> applying the same to -cpu which is basically translator >>>> model->type[,-global type.foo,...] >>>> would be consistent with -device and less confusing for everyone >>>> (not counting significant code reduction). >>>> It would certainly simplify contributing new targets as contributor >>>> won't have to care about cpu model naming and do something about it. >>>> >>>> This option wasn't considered before because we didn't have deprecation >>>> back then, but now it opens possibility to simplify qemu and consolidate >>>> naming. (we probably would be able to fold '-cpu help' into '-device help' >>>> as well). >>>> >>> >> > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model 2020-01-23 18:48 ` Philippe Mathieu-Daudé @ 2020-02-06 20:59 ` Eduardo Habkost 0 siblings, 0 replies; 33+ messages in thread From: Eduardo Habkost @ 2020-02-06 20:59 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, Markus Armbruster, qemu-devel, alistair23, marcel, Igor Mammedov On Thu, Jan 23, 2020 at 07:48:21PM +0100, Philippe Mathieu-Daudé wrote: > On 6/20/19 4:43 PM, Eduardo Habkost wrote: > > On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote: > > > On Tue, 18 Jun 2019 10:55:16 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote: > > > > > On Mon, 17 Jun 2019 13:27:00 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote: > > > > > > > On Mon, 17 Jun 2019 17:15:21 +0200 > > > > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > > > [...] > > > > > > > > Yes. Eduardo and you should write some lines to explain this, and then > > > > > > > > we will follow :) > > > > > > > Unfortunately I don't recall details anymore. One could check out all > > > > > > > implementations of class_by_name callbacks to find out current state. > > > > > > > > > > > > See this message for a summary of existing class_by_name quirks: > > > > > > > > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html > > > > > > Date: Wed, 08 May 2019 10:34:44 +0200 > > > > > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org> > > > > > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions > > > > > > > > > > > > I'm unsure about Igor's suggestion to get rid of CPU model names > > > > > > and use only QOM type names in external interfaces. In either > > > > > > case, we can still simplify the rules rules and reduce the amount > > > > > > of arch-specific code. > > > > > as far as we have cpu_class_by_name, we have to watch over that > > > > > new patches/targets won't add some custom handling/fallbac/whatnot. > > > > > > > > We can get rid of CPUClass::cpu_class_by_name() without changing > > > > the external interfaces provided by QEMU. > > > if you mean QMP, than it is possible to keep model there where > > > it already exists. Based on experiment [1](x86) I did, it's local to > > > affected target and doesn't pollute other code. > > > > I mean both command line and QMP. > > > > > > > > > I don't have a strong opinion about using only QOM types at -cpu, > > > > yet. But first we need to get rid of the arch-specific CPU model > > > > name exceptions enumerated at the URL above (which would be very > > > > welcome). > > > One way to get rid of them is to deprecate them in favor of strict > > > match (no fallback/substitutions/aliases) to typename and when we > > > drop it make switch type based naming only. > > > > This is true, but is it desirable? Aren't we just moving > > complexity elsewhere? Management software would still need to > > translate CPU models from existing VM configurations to QOM type > > names. > > > > > > > > 1) I've just took a quick look at how much of duplicated/confusing > > > code we could get rid off if we drop cpu_class_by_name/*_cpu_list. > > > It amounts to >800LOC of trivial removal (not counting ppc/s390 > > > that depend on model naming heavily and in need of some non > > > trivial refactoring) > > > > Removing the code might be trivial. Coordinating with the > > maintainers of all architectures to confirm this is really > > something everybody wants is the difficult part. > > > > If you really want to do it, please make sure all the > > architecture maintainers (and libvirt developers) are involved in > > the discussion. > > From the previous link (summary of existing class_by_name quirks): > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html > > "Deprecating unwanted stuff now is likely to make a > later cleanup so much easier." > > This was 8 months ago :/ > > IIUC we need to restart this topic addressing the libvirt community first, > then see with the QEMU one? The next step is to deprecate the weird and pointless exceptions listed at the URL you mentioned. Not all exceptions will affect libvirt (maybe most won't). In either case, we can just use qemu-deprecated.texi to communicate this to libvirt developers. MAINTAINERS already has a "R: libvir-list" line for qemu-deprecated.texi. -- Eduardo ^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis ` (4 subsequent siblings) 6 siblings, 0 replies; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell List all possible valid CPU options. Although the board only ever has a Cortex-M3 we mark the Cortex-M4 as supported because the Netduino2 Plus supports the Cortex-M4 and the Netduino2 Plus is similar to the Netduino2. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- V5: - Use cpu_model names V3: - Add static property V2: - Fixup allignment RFC v2: - Use a NULL terminated list - Add the Cortex-M4 for testing hw/arm/netduino2.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c index f936017d4a..91c925a56c 100644 --- a/hw/arm/netduino2.c +++ b/hw/arm/netduino2.c @@ -34,18 +34,26 @@ static void netduino2_init(MachineState *machine) DeviceState *dev; dev = qdev_create(NULL, TYPE_STM32F205_SOC); - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3")); + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type); object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal); armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, FLASH_SIZE); } +static const char *netduino_valid_cpus[] = { + "cortex-m3", + "cortex-m4", + NULL +}; + static void netduino2_machine_init(MachineClass *mc) { mc->desc = "Netduino 2 Machine"; mc->init = netduino2_init; mc->ignore_memory_transaction_failures = true; + mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3"); + mc->valid_cpu_types = netduino_valid_cpus; } DEFINE_MACHINE("netduino2", netduino2_machine_init) -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-02-15 13:23 ` Philippe Mathieu-Daudé 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis ` (3 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use the correct CPU. https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Igor Mammedov <imammedo@redhat.com> --- V3: - Use ARM_CPU_TYPE_NAME() macro V2: - Fix the BCM2836 CPU hw/arm/bcm2836.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 8c43291112..c477772484 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj) for (n = 0; n < BCM2836_NCPUS; n++) { object_initialize(&s->cpus[n], sizeof(s->cpus[n]), - "cortex-a15-" TYPE_ARM_CPU); + ARM_CPU_TYPE_NAME("cortex-a7")); object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), &error_abort); } -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis @ 2018-02-15 13:23 ` Philippe Mathieu-Daudé 2018-02-15 22:41 ` Alistair Francis 0 siblings, 1 reply; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2018-02-15 13:23 UTC (permalink / raw) To: peter.maydell Cc: Alistair Francis, qemu-devel, alistair23, ehabkost, marcel, imammedo Hi Peter, On 02/01/2018 09:42 PM, Alistair Francis wrote: > The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use > the correct CPU. > https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf Can you add these lines with reference to the commits? When the BCM2836 was introduced (bad5623690b) the Cortex-A7 was not available, so the very similar Cortex-A15 was used. Since dcf578ed8ce we can model the correct core. Thanks! Phil. > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > V3: > - Use ARM_CPU_TYPE_NAME() macro > V2: > - Fix the BCM2836 CPU > > hw/arm/bcm2836.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 8c43291112..c477772484 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj) > > for (n = 0; n < BCM2836_NCPUS; n++) { > object_initialize(&s->cpus[n], sizeof(s->cpus[n]), > - "cortex-a15-" TYPE_ARM_CPU); > + ARM_CPU_TYPE_NAME("cortex-a7")); > object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), > &error_abort); > } > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 2018-02-15 13:23 ` Philippe Mathieu-Daudé @ 2018-02-15 22:41 ` Alistair Francis 0 siblings, 0 replies; 33+ messages in thread From: Alistair Francis @ 2018-02-15 22:41 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, Alistair Francis, qemu-devel@nongnu.org Developers, Eduardo Habkost, Marcel Apfelbaum, Igor Mammedov On Thu, Feb 15, 2018 at 5:23 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Peter, > > On 02/01/2018 09:42 PM, Alistair Francis wrote: >> The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use >> the correct CPU. >> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf > > Can you add these lines with reference to the commits? > > When the BCM2836 was introduced (bad5623690b) the Cortex-A7 was not > available, so the very similar Cortex-A15 was used. > > Since dcf578ed8ce we can model the correct core. > > Thanks! Added! I'm not sure when I'll send the next version out though. Alistair > > Phil. > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >> --- >> V3: >> - Use ARM_CPU_TYPE_NAME() macro >> V2: >> - Fix the BCM2836 CPU >> >> hw/arm/bcm2836.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >> index 8c43291112..c477772484 100644 >> --- a/hw/arm/bcm2836.c >> +++ b/hw/arm/bcm2836.c >> @@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj) >> >> for (n = 0; n < BCM2836_NCPUS; n++) { >> object_initialize(&s->cpus[n], sizeof(s->cpus[n]), >> - "cortex-a15-" TYPE_ARM_CPU); >> + ARM_CPU_TYPE_NAME("cortex-a7")); >> object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]), >> &error_abort); >> } >> ^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis ` (2 preceding siblings ...) 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-02-15 11:29 ` Peter Maydell 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis ` (2 subsequent siblings) 6 siblings, 1 reply; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell List all possible valid CPU options. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- V5: - Use cpu_model names V4: - Remove spaces V3: - Add static property V2: - Fix the indentation hw/arm/raspi.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index cd5fa8c3dc..745a880726 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine) setup_boot(machine, 2, machine->ram_size - vcram_size); } +static const char *raspi2_valid_cpus[] = { + "cortex-a7", + NULL +}; + static void raspi2_machine_init(MachineClass *mc) { mc->desc = "Raspberry Pi 2"; @@ -171,5 +176,7 @@ static void raspi2_machine_init(MachineClass *mc) mc->default_cpus = BCM2836_NCPUS; mc->default_ram_size = 1024 * 1024 * 1024; mc->ignore_memory_transaction_failures = true; + mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7"); + mc->valid_cpu_types = raspi2_valid_cpus; }; DEFINE_MACHINE("raspi2", raspi2_machine_init) -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis @ 2018-02-15 11:29 ` Peter Maydell 2018-02-15 13:04 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 33+ messages in thread From: Peter Maydell @ 2018-02-15 11:29 UTC (permalink / raw) To: Alistair Francis Cc: QEMU Developers, Alistair Francis, Eduardo Habkost, Marcel Apfelbaum, Igor Mammedov, Philippe Mathieu-Daudé On 2 February 2018 at 00:42, Alistair Francis <alistair.francis@xilinx.com> wrote: > List all possible valid CPU options. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > > V5: > - Use cpu_model names > V4: > - Remove spaces > V3: > - Add static property > V2: > - Fix the indentation > > hw/arm/raspi.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index cd5fa8c3dc..745a880726 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine) > setup_boot(machine, 2, machine->ram_size - vcram_size); > } > > +static const char *raspi2_valid_cpus[] = { > + "cortex-a7", > + NULL > +}; Is this definitely right? Looking at the code, the raspi2 board creates a TYPE_BCM2836, and that creates cortex-a15 CPUs... thanks -- PMM ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs 2018-02-15 11:29 ` Peter Maydell @ 2018-02-15 13:04 ` Philippe Mathieu-Daudé 2018-02-15 13:17 ` Peter Maydell 0 siblings, 1 reply; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2018-02-15 13:04 UTC (permalink / raw) To: Peter Maydell, Alistair Francis Cc: QEMU Developers, Alistair Francis, Eduardo Habkost, Marcel Apfelbaum, Igor Mammedov Hi Peter, On 02/15/2018 08:29 AM, Peter Maydell wrote: > On 2 February 2018 at 00:42, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> List all possible valid CPU options. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> >> V5: >> - Use cpu_model names >> V4: >> - Remove spaces >> V3: >> - Add static property >> V2: >> - Fix the indentation >> >> hw/arm/raspi.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >> index cd5fa8c3dc..745a880726 100644 >> --- a/hw/arm/raspi.c >> +++ b/hw/arm/raspi.c >> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine) >> setup_boot(machine, 2, machine->ram_size - vcram_size); >> } >> >> +static const char *raspi2_valid_cpus[] = { >> + "cortex-a7", >> + NULL >> +}; > > Is this definitely right? Looking at the code, the raspi2 board > creates a TYPE_BCM2836, and that creates cortex-a15 CPUs... The BCM2836 use a cortex-a7 but this cpu was not available at the time of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7 is very similar to the A15." I can prepare a patch for it to apply before this series. > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs 2018-02-15 13:04 ` Philippe Mathieu-Daudé @ 2018-02-15 13:17 ` Peter Maydell 2018-02-15 17:08 ` Igor Mammedov 0 siblings, 1 reply; 33+ messages in thread From: Peter Maydell @ 2018-02-15 13:17 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Alistair Francis, QEMU Developers, Alistair Francis, Eduardo Habkost, Marcel Apfelbaum, Igor Mammedov On 15 February 2018 at 13:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Peter, > > On 02/15/2018 08:29 AM, Peter Maydell wrote: >> On 2 February 2018 at 00:42, Alistair Francis >> <alistair.francis@xilinx.com> wrote: >>> List all possible valid CPU options. >>> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> >>> V5: >>> - Use cpu_model names >>> V4: >>> - Remove spaces >>> V3: >>> - Add static property >>> V2: >>> - Fix the indentation >>> >>> hw/arm/raspi.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >>> index cd5fa8c3dc..745a880726 100644 >>> --- a/hw/arm/raspi.c >>> +++ b/hw/arm/raspi.c >>> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine) >>> setup_boot(machine, 2, machine->ram_size - vcram_size); >>> } >>> >>> +static const char *raspi2_valid_cpus[] = { >>> + "cortex-a7", >>> + NULL >>> +}; >> >> Is this definitely right? Looking at the code, the raspi2 board >> creates a TYPE_BCM2836, and that creates cortex-a15 CPUs... > > The BCM2836 use a cortex-a7 but this cpu was not available at the time > of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7 > is very similar to the A15." > > I can prepare a patch for it to apply before this series. I noticed after I'd written that comment that patch 3 in this series does exactly the change to cortex-a7... thanks -- PMM ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs 2018-02-15 13:17 ` Peter Maydell @ 2018-02-15 17:08 ` Igor Mammedov 0 siblings, 0 replies; 33+ messages in thread From: Igor Mammedov @ 2018-02-15 17:08 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Alistair Francis, QEMU Developers, Alistair Francis, Eduardo Habkost, Marcel Apfelbaum On Thu, 15 Feb 2018 13:17:50 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 15 February 2018 at 13:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Hi Peter, > > > > On 02/15/2018 08:29 AM, Peter Maydell wrote: > >> On 2 February 2018 at 00:42, Alistair Francis > >> <alistair.francis@xilinx.com> wrote: > >>> List all possible valid CPU options. > >>> > >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >>> --- > >>> > >>> V5: > >>> - Use cpu_model names > >>> V4: > >>> - Remove spaces > >>> V3: > >>> - Add static property > >>> V2: > >>> - Fix the indentation > >>> > >>> hw/arm/raspi.c | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> > >>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > >>> index cd5fa8c3dc..745a880726 100644 > >>> --- a/hw/arm/raspi.c > >>> +++ b/hw/arm/raspi.c > >>> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine) > >>> setup_boot(machine, 2, machine->ram_size - vcram_size); > >>> } > >>> > >>> +static const char *raspi2_valid_cpus[] = { > >>> + "cortex-a7", > >>> + NULL > >>> +}; > >> > >> Is this definitely right? Looking at the code, the raspi2 board > >> creates a TYPE_BCM2836, and that creates cortex-a15 CPUs... > > > > The BCM2836 use a cortex-a7 but this cpu was not available at the time > > of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7 > > is very similar to the A15." > > > > I can prepare a patch for it to apply before this series. > > I noticed after I'd written that comment that patch 3 in this > series does exactly the change to cortex-a7... anyways this series, needs not to depend MachineState::cpu_model, removal of which is queued in Eduardo's tree. It seems we can't find consensus on how to deal with cpu_model names in boards code [1/6]. > > thanks > -- PMM ^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: Specify the valid CPUs 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis ` (3 preceding siblings ...) 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis 2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé 6 siblings, 0 replies; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell List all possible valid CPU options. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- An implementation for single CPU machines is still being discussed. A solution proposed by Eduardo is this: 1) Change the default on TYPE_MACHINE to: mc->valid_cpu_types = { TYPE_CPU, NULL }; This will keep the existing behavior for all boards. 2) mc->valid_cpu_types=NULL be interpreted as "no CPU model except the default is accepted" or "-cpu is not accepted" in machine_run_board_init() (I prefer the former, but both options would be correct) 3) Boards like xlnx_zynqmp could then just do this: static void xxx_class_init(...) { mc->default_cpu_type = MY_CPU_TYPE; /* Reason: XXX_init() is hardcoded to MY_CPU_TYPE */ mc->valid_cpu_types = NULL; } V5: - Use cpu_model names V4: - Remove spaces V3: - Make variable static V2: - Don't use the users -cpu - Fixup allignment hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c index b126cf148b..994b19a36f 100644 --- a/hw/arm/xlnx-zcu102.c +++ b/hw/arm/xlnx-zcu102.c @@ -184,6 +184,11 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine) arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo); } +static const char *xlnx_zynqmp_valid_cpus[] = { + "cortex-a53", + NULL +}; + static void xlnx_ep108_init(MachineState *machine) { XlnxZCU102 *s = EP108_MACHINE(machine); @@ -216,6 +221,12 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data) mc->ignore_memory_transaction_failures = true; mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"); + /* The ZynqMP SoC is always a Cortex-A53. We add this here to give + * users a sane error if they specify a different CPU, but we never + * use their CPU choice. + */ + mc->valid_cpu_types = xlnx_zynqmp_valid_cpus; } static const TypeInfo xlnx_ep108_machine_init_typeinfo = { @@ -274,6 +285,12 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data) mc->ignore_memory_transaction_failures = true; mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS; mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS; + mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53"); + /* The ZynqMP SoC is always a Cortex-A53. We add this here to give + * users a sane error if they specify a different CPU, but we never + * use their CPU choice. + */ + mc->valid_cpu_types = xlnx_zynqmp_valid_cpus; } static const TypeInfo xlnx_zcu102_machine_init_typeinfo = { -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: Specify the valid CPUs 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis ` (4 preceding siblings ...) 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis @ 2018-02-02 0:42 ` Alistair Francis 2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé 6 siblings, 0 replies; 33+ messages in thread From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw) To: qemu-devel Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug, peter.maydell List all possible valid CPU options. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- V5: - Use cpu_model names V4: - Remove spaces V3: - Make variable static V2: - Fixup alignment hw/arm/xilinx_zynq.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c index 1836a4ed45..822e1889a5 100644 --- a/hw/arm/xilinx_zynq.c +++ b/hw/arm/xilinx_zynq.c @@ -313,6 +313,11 @@ static void zynq_init(MachineState *machine) arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo); } +static const char *xlnx_zynq_7000_valid_cpus[] = { + "cortex-a9", + NULL +}; + static void zynq_machine_init(MachineClass *mc) { mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9"; @@ -321,6 +326,7 @@ static void zynq_machine_init(MachineClass *mc) mc->no_sdcard = 1; mc->ignore_memory_transaction_failures = true; mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9"); + mc->valid_cpu_types = xlnx_zynq_7000_valid_cpus; } DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init) -- 2.14.1 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis ` (5 preceding siblings ...) 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis @ 2018-03-13 23:13 ` Philippe Mathieu-Daudé 2018-03-21 14:33 ` Igor Mammedov 6 siblings, 1 reply; 33+ messages in thread From: Philippe Mathieu-Daudé @ 2018-03-13 23:13 UTC (permalink / raw) To: Peter Maydell, alistair23, Andrew Baumann Cc: qemu-devel, ehabkost, marcel, imammedo On 02/02/2018 01:42 AM, Alistair Francis wrote: > > There are numorous QEMU machines that only have a single or a handful of > valid CPU options. To simplyfy the management of specificying which CPU > is/isn't valid let's create a property that can be set in the machine > init. We can then check to see if the user supplied CPU is in that list > or not. Sadly this series missed the 2.12 train. > > I have added the valid_cpu_types for some ARM machines only at the > moment. > > Here is what specifying the CPUs looks like now: > > $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) info cpus > * CPU #0: thread_id=24175 > (qemu) q > > $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S > QEMU 2.10.50 monitor - type 'help' for more information > (qemu) q > > $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S > qemu-system-aarch64: unable to find CPU model 'cortex-m5' > > $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S > qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu > The valid models are: cortex-m3, cortex-m4 > > V5: > - Use cpu_model instead of cpu_type > V4: > - Rebase > - Remove spaces > V3: > - Make the varialbes static > V2: > - Rebase > - Reorder patches > - Add a Raspberry Pi 2 CPU fix > V1: > - Small fixes to prepare a series instead of RFC > - Add commit messages for the commits > - Expand the machine support to ARM machines > RFC v2: > - Rebase on Igor's work > - Use more QEMUisms inside the code > - List the supported machines in a NULL terminated array > > > > Alistair Francis (6): > machine: Convert the valid cpu types to use cpu_model > netduino2: Specify the valid CPUs > bcm2836: Use the Cortex-A7 instead of Cortex-A15 > raspi: Specify the valid CPUs > xlnx-zcu102: Specify the valid CPUs > xilinx_zynq: Specify the valid CPUs > > hw/arm/bcm2836.c | 2 +- > hw/arm/netduino2.c | 10 +++++++++- > hw/arm/raspi.c | 7 +++++++ > hw/arm/xilinx_zynq.c | 6 ++++++ > hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++ > hw/core/machine.c | 11 +++++------ > 6 files changed, 45 insertions(+), 8 deletions(-) > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property 2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé @ 2018-03-21 14:33 ` Igor Mammedov 0 siblings, 0 replies; 33+ messages in thread From: Igor Mammedov @ 2018-03-21 14:33 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Peter Maydell, alistair23, Andrew Baumann, qemu-devel, ehabkost, marcel On Wed, 14 Mar 2018 00:13:40 +0100 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 02/02/2018 01:42 AM, Alistair Francis wrote: > > > > There are numorous QEMU machines that only have a single or a handful of > > valid CPU options. To simplyfy the management of specificying which CPU > > is/isn't valid let's create a property that can be set in the machine > > init. We can then check to see if the user supplied CPU is in that list > > or not. > > Sadly this series missed the 2.12 train. problematic part here is 'cpu_type -> cpu_model' conversion without introducing back cpu model names in boards code. Each target sort of already has this code 'cpu_list' for printing supported CPU models. What we need is to generalize it as most of the targets implement it the same way. And then reuse generic code for 'cpu_type -> cpu_model' so that boards won't have to be involved in it. So currently cpu_list needs a volunteer who would be able to generalize it and cleanup all targets across the the tree. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-02-06 21:01 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis 2018-02-02 18:23 ` Eduardo Habkost 2018-02-05 11:22 ` Igor Mammedov 2018-02-05 13:54 ` Eduardo Habkost 2018-02-05 14:42 ` Igor Mammedov 2018-02-05 22:42 ` Eduardo Habkost 2018-02-06 14:43 ` Igor Mammedov 2019-06-17 5:09 ` Philippe Mathieu-Daudé 2019-06-17 14:43 ` Eduardo Habkost 2019-06-17 15:01 ` Igor Mammedov 2019-06-17 15:15 ` Philippe Mathieu-Daudé 2019-06-17 15:33 ` Igor Mammedov 2019-06-17 16:27 ` Eduardo Habkost 2019-06-18 11:34 ` Igor Mammedov 2019-06-18 13:55 ` Eduardo Habkost 2019-06-20 9:02 ` Igor Mammedov 2019-06-20 14:43 ` Eduardo Habkost 2020-01-23 18:48 ` Philippe Mathieu-Daudé 2020-02-06 20:59 ` Eduardo Habkost 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis 2018-02-15 13:23 ` Philippe Mathieu-Daudé 2018-02-15 22:41 ` Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis 2018-02-15 11:29 ` Peter Maydell 2018-02-15 13:04 ` Philippe Mathieu-Daudé 2018-02-15 13:17 ` Peter Maydell 2018-02-15 17:08 ` Igor Mammedov 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis 2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis 2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé 2018-03-21 14:33 ` Igor Mammedov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).