| Message ID | b2bfe1ee20bd9764572015df0d167d3015036ef0.1504656490.git.alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 05, 2017 at 05:12:01PM -0700, Alistair Francis wrote: > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > > hw/core/machine.c | 27 +++++++++++++++++++++++++++ > include/hw/boards.h | 1 + > 2 files changed, 28 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 41b53a17ad..de0f127d27 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -758,6 +758,33 @@ void machine_run_board_init(MachineState *machine) > machine_numa_finish_init(machine); > } > machine_class->init(machine); > + > + if (machine_class->valid_cpu_types && machine->cpu_model) { > + const char *temp; > + int i, len = machine_class->valid_cpu_types->len; I suggest doing this after Igor's series that replaces the cpu_model field (full -cpu string) with cpu_type (only the CPU type name). > + > + for (i = 0; i < len; i++) { > + temp = g_array_index(machine_class->valid_cpu_types, char *, i); > + if (!strcmp(machine->cpu_model, temp)) { > + /* The user specificed CPU is in the valid field, we are > + * good to go. > + */ > + g_array_free(machine_class->valid_cpu_types, true); > + return; I suggest checking for: object_class_dynamic_cast(object_class_get_name(machine->cpu_type), type) instead. This way machines could just enumerate a common parent type to all supported CPU models. > + } > + } > + /* The user specified CPU must not be a valid CPU, print a sane error */ > + temp = g_array_index(machine_class->valid_cpu_types, char *, 0); > + error_report("Invalid CPU: %s", machine->cpu_model); > + error_printf("The valid options are: %s", temp); > + for (i = 1; i < len; i++) { > + temp = g_array_index(machine_class->valid_cpu_types, char *, i); > + error_printf(", %s", temp); > + } > + error_printf("\n"); Now we have a completely new method to list the valid CPU types in addition to arch_query_cpu_definitions() and list_cpus() (which are already a bit messy and need to share more code). I think this should share code with "-cpu help"/query-cpu-definitions instead. This means arch_query_cpu_definitions() will need a MachineClass* argument, if the user wants only the CPU types supported by a specific machine type, but I think it would be an interesting improvement to query-cpu-definitions. > + g_array_free(machine_class->valid_cpu_types, true); > + exit(1); > + } > } > > static void machine_class_finalize(ObjectClass *klass, void *data) > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 3363dd19fd..78678f84a9 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -172,6 +172,7 @@ struct MachineClass { > int minimum_page_bits; > bool has_hotpluggable_cpus; > int numa_mem_align_shift; > + GArray *valid_cpu_types; The list of CPU types for a machine are very likely to be statically defined at build time. Any specific reason to not make it a simple char** pointer? > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > int nb_nodes, ram_addr_t size); > > -- > 2.11.0 >
On Sat, 9 Sep 2017 17:16:57 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Sep 05, 2017 at 05:12:01PM -0700, Alistair Francis wrote: > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > > --- > > > > hw/core/machine.c | 27 +++++++++++++++++++++++++++ > > include/hw/boards.h | 1 + > > 2 files changed, 28 insertions(+) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 41b53a17ad..de0f127d27 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -758,6 +758,33 @@ void machine_run_board_init(MachineState *machine) > > machine_numa_finish_init(machine); > > } > > machine_class->init(machine); > > + > > + if (machine_class->valid_cpu_types && machine->cpu_model) { > > + const char *temp; > > + int i, len = machine_class->valid_cpu_types->len; > > I suggest doing this after Igor's series that replaces the > cpu_model field (full -cpu string) with cpu_type (only the CPU > type name). in addition check should run before machine_class->init(machine); it should be possible after cpu_model -> cpu_type refactoring is complete [...] > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 3363dd19fd..78678f84a9 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -172,6 +172,7 @@ struct MachineClass { > > int minimum_page_bits; > > bool has_hotpluggable_cpus; > > int numa_mem_align_shift; > > + GArray *valid_cpu_types; > > The list of CPU types for a machine are very likely to be > statically defined at build time. Any specific reason to not > make it a simple char** pointer? wile working on cpu_model, I've seen only static checks used in some machines. So +1 to static null terminated array [ TYPE_FOO1, TYPE_FOO2, NULL ] > > > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > > int nb_nodes, ram_addr_t size); > > > > -- > > 2.11.0 > > >
On Sat, Sep 9, 2017 at 1:16 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Sep 05, 2017 at 05:12:01PM -0700, Alistair Francis wrote: >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> hw/core/machine.c | 27 +++++++++++++++++++++++++++ >> include/hw/boards.h | 1 + >> 2 files changed, 28 insertions(+) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 41b53a17ad..de0f127d27 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -758,6 +758,33 @@ void machine_run_board_init(MachineState *machine) >> machine_numa_finish_init(machine); >> } >> machine_class->init(machine); >> + >> + if (machine_class->valid_cpu_types && machine->cpu_model) { >> + const char *temp; >> + int i, len = machine_class->valid_cpu_types->len; > > I suggest doing this after Igor's series that replaces the > cpu_model field (full -cpu string) with cpu_type (only the CPU > type name). > >> + >> + for (i = 0; i < len; i++) { >> + temp = g_array_index(machine_class->valid_cpu_types, char *, i); >> + if (!strcmp(machine->cpu_model, temp)) { >> + /* The user specificed CPU is in the valid field, we are >> + * good to go. >> + */ >> + g_array_free(machine_class->valid_cpu_types, true); >> + return; > > I suggest checking for: > object_class_dynamic_cast(object_class_get_name(machine->cpu_type), type) > instead. This way machines could just enumerate a common parent > type to all supported CPU models. > >> + } >> + } >> + /* The user specified CPU must not be a valid CPU, print a sane error */ >> + temp = g_array_index(machine_class->valid_cpu_types, char *, 0); >> + error_report("Invalid CPU: %s", machine->cpu_model); >> + error_printf("The valid options are: %s", temp); >> + for (i = 1; i < len; i++) { >> + temp = g_array_index(machine_class->valid_cpu_types, char *, i); >> + error_printf(", %s", temp); >> + } >> + error_printf("\n"); > > Now we have a completely new method to list the valid CPU types > in addition to arch_query_cpu_definitions() and list_cpus() > (which are already a bit messy and need to share more code). > > I think this should share code with "-cpu > help"/query-cpu-definitions instead. This means > arch_query_cpu_definitions() will need a MachineClass* argument, > if the user wants only the CPU types supported by a specific > machine type, but I think it would be an interesting improvement > to query-cpu-definitions. I don't see the improvement here. arch_query_cpu_definitions() is just a pretty simple list of that arch's CPUs. I don't really see how it helps in this case. We don't re-use much code at all. Ideally I would like to see less arch dependent code in QEMU, and relying on arch_query_cpu_definitions() is moving in the opposite direction. It looks like arch_query_cpu_definitions() isn't supported by every architecture as well. I only see ARM, x86, PPC and S390x support. I have addressed all the other comments so I'm going to send an RFCv2 out later today. The code looks a lot nicer now. I'm happy to keep discussing this though, just want to keep the ball rolling. Thanks, Alistair > >> + g_array_free(machine_class->valid_cpu_types, true); >> + exit(1); >> + } >> } >> >> static void machine_class_finalize(ObjectClass *klass, void *data) >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 3363dd19fd..78678f84a9 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -172,6 +172,7 @@ struct MachineClass { >> int minimum_page_bits; >> bool has_hotpluggable_cpus; >> int numa_mem_align_shift; >> + GArray *valid_cpu_types; > > The list of CPU types for a machine are very likely to be > statically defined at build time. Any specific reason to not > make it a simple char** pointer? > >> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, >> int nb_nodes, ram_addr_t size); >> >> -- >> 2.11.0 >> > > -- > Eduardo
diff --git a/hw/core/machine.c b/hw/core/machine.c index 41b53a17ad..de0f127d27 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -758,6 +758,33 @@ void machine_run_board_init(MachineState *machine) machine_numa_finish_init(machine); } machine_class->init(machine); + + if (machine_class->valid_cpu_types && machine->cpu_model) { + const char *temp; + int i, len = machine_class->valid_cpu_types->len; + + for (i = 0; i < len; i++) { + temp = g_array_index(machine_class->valid_cpu_types, char *, i); + if (!strcmp(machine->cpu_model, temp)) { + /* The user specificed CPU is in the valid field, we are + * good to go. + */ + g_array_free(machine_class->valid_cpu_types, true); + return; + } + } + /* The user specified CPU must not be a valid CPU, print a sane error */ + temp = g_array_index(machine_class->valid_cpu_types, char *, 0); + error_report("Invalid CPU: %s", machine->cpu_model); + error_printf("The valid options are: %s", temp); + for (i = 1; i < len; i++) { + temp = g_array_index(machine_class->valid_cpu_types, char *, i); + error_printf(", %s", temp); + } + error_printf("\n"); + g_array_free(machine_class->valid_cpu_types, true); + exit(1); + } } static void machine_class_finalize(ObjectClass *klass, void *data) diff --git a/include/hw/boards.h b/include/hw/boards.h index 3363dd19fd..78678f84a9 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -172,6 +172,7 @@ struct MachineClass { int minimum_page_bits; bool has_hotpluggable_cpus; int numa_mem_align_shift; + GArray *valid_cpu_types; void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, int nb_nodes, ram_addr_t size);
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- hw/core/machine.c | 27 +++++++++++++++++++++++++++ include/hw/boards.h | 1 + 2 files changed, 28 insertions(+)