| Message ID | b8474e9d2e0a219d9bac901342f983b13d009301.1507059418.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a valid_cpu_types property | expand |
On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote: > This patch add a MachineClass element that can be set in the machine C > code to specify a list of supported CPU types. If the supported CPU > types are specified the user enter CPU (by -cpu at runtime) is checked > against the supported types and QEMU exits if they aren't supported. > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- Thanks! Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> However, I will squash the following changes before queueing, because: * object_class_dynamic_cast() is safe even if class is NULL, so there's no need to validate cpu_type here. * "must not be valid" sounds like the CPU is not allowed to be a valid CPU, so I rewrote the comment. diff --git a/hw/core/machine.c b/hw/core/machine.c index 3afc6a7b5b..36c2fb069c 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine) ObjectClass *class = object_class_by_name(machine->cpu_type); int i; - /* machine->cpu_type is supposed to be always a valid QOM type */ - assert(class); - for (i = 0; machine_class->valid_cpu_types[i]; i++) { if (object_class_dynamic_cast(class, machine_class->valid_cpu_types[i])) { @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine) } if (!machine_class->valid_cpu_types[i]) { - /* The user specified CPU must not be a valid CPU */ + /* The user specified CPU is not valid */ error_report("Invalid CPU type: %s", machine->cpu_type); error_printf("The valid types are: %s", machine_class->valid_cpu_types[0]); > > V1: > - Use 'type' in the error message > - Move a uneeded operation out of the for loop > - Remove the goto > RFC v2: > - Rebase on Igor's cpu_type work > - Use object_class_dynamic_cast() > - Use a NULL terminated cahr** list > - Do the check before the machine_class init() is called > > > hw/core/machine.c | 35 +++++++++++++++++++++++++++++++++++ > include/hw/boards.h | 1 + > 2 files changed, 36 insertions(+) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 80647edc2a..3afc6a7b5b 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -758,6 +758,41 @@ void machine_run_board_init(MachineState *machine) > if (nb_numa_nodes) { > machine_numa_finish_init(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); > + int i; > + > + /* machine->cpu_type is supposed to be always a valid QOM type */ > + assert(class); > + > + for (i = 0; machine_class->valid_cpu_types[i]; i++) { > + if (object_class_dynamic_cast(class, > + machine_class->valid_cpu_types[i])) { > + /* The user specificed CPU is in the valid field, we are > + * good to go. > + */ > + break; > + } > + } > + > + if (!machine_class->valid_cpu_types[i]) { > + /* The user specified CPU must not be a valid CPU */ > + error_report("Invalid CPU type: %s", machine->cpu_type); > + error_printf("The valid types 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]); > + } > + error_printf("\n"); > + > + exit(1); > + } > + } > + > machine_class->init(machine); > } > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 156e0a5701..191a5b3cd8 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -191,6 +191,7 @@ struct MachineClass { > bool has_hotpluggable_cpus; > bool ignore_memory_transaction_failures; > int numa_mem_align_shift; > + const char **valid_cpu_types; > void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, > int nb_nodes, ram_addr_t size); > > -- > 2.11.0 >
On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote: >> This patch add a MachineClass element that can be set in the machine C >> code to specify a list of supported CPU types. If the supported CPU >> types are specified the user enter CPU (by -cpu at runtime) is checked >> against the supported types and QEMU exits if they aren't supported. >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- > > Thanks! > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > However, I will squash the following changes before queueing, > because: > > * object_class_dynamic_cast() is safe even if class is NULL, > so there's no need to validate cpu_type here. > * "must not be valid" sounds like the CPU is not allowed to be a > valid CPU, so I rewrote the comment. > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 3afc6a7b5b..36c2fb069c 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine) > ObjectClass *class = object_class_by_name(machine->cpu_type); > int i; > > - /* machine->cpu_type is supposed to be always a valid QOM type */ > - assert(class); > - > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > if (object_class_dynamic_cast(class, > machine_class->valid_cpu_types[i])) { > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine) > } > > if (!machine_class->valid_cpu_types[i]) { > - /* The user specified CPU must not be a valid CPU */ > + /* The user specified CPU is not valid */ > error_report("Invalid CPU type: %s", machine->cpu_type); > error_printf("The valid types are: %s", > machine_class->valid_cpu_types[0]); Looks good to me. Does that mean you are taking the whole series now? Thanks, Alistair >> >> V1: >> - Use 'type' in the error message >> - Move a uneeded operation out of the for loop >> - Remove the goto >> RFC v2: >> - Rebase on Igor's cpu_type work >> - Use object_class_dynamic_cast() >> - Use a NULL terminated cahr** list >> - Do the check before the machine_class init() is called >> >> >> hw/core/machine.c | 35 +++++++++++++++++++++++++++++++++++ >> include/hw/boards.h | 1 + >> 2 files changed, 36 insertions(+) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index 80647edc2a..3afc6a7b5b 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -758,6 +758,41 @@ void machine_run_board_init(MachineState *machine) >> if (nb_numa_nodes) { >> machine_numa_finish_init(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); >> + int i; >> + >> + /* machine->cpu_type is supposed to be always a valid QOM type */ >> + assert(class); >> + >> + for (i = 0; machine_class->valid_cpu_types[i]; i++) { >> + if (object_class_dynamic_cast(class, >> + machine_class->valid_cpu_types[i])) { >> + /* The user specificed CPU is in the valid field, we are >> + * good to go. >> + */ >> + break; >> + } >> + } >> + >> + if (!machine_class->valid_cpu_types[i]) { >> + /* The user specified CPU must not be a valid CPU */ >> + error_report("Invalid CPU type: %s", machine->cpu_type); >> + error_printf("The valid types 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]); >> + } >> + error_printf("\n"); >> + >> + exit(1); >> + } >> + } >> + >> machine_class->init(machine); >> } >> >> diff --git a/include/hw/boards.h b/include/hw/boards.h >> index 156e0a5701..191a5b3cd8 100644 >> --- a/include/hw/boards.h >> +++ b/include/hw/boards.h >> @@ -191,6 +191,7 @@ struct MachineClass { >> bool has_hotpluggable_cpus; >> bool ignore_memory_transaction_failures; >> int numa_mem_align_shift; >> + const char **valid_cpu_types; >> void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, >> int nb_nodes, ram_addr_t size); >> >> -- >> 2.11.0 >> > > -- > Eduardo
On Tue, Oct 03, 2017 at 01:26:53PM -0700, Alistair Francis wrote: > On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote: > >> This patch add a MachineClass element that can be set in the machine C > >> code to specify a list of supported CPU types. If the supported CPU > >> types are specified the user enter CPU (by -cpu at runtime) is checked > >> against the supported types and QEMU exits if they aren't supported. > >> > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > >> --- > > > > Thanks! > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > However, I will squash the following changes before queueing, > > because: > > > > * object_class_dynamic_cast() is safe even if class is NULL, > > so there's no need to validate cpu_type here. > > * "must not be valid" sounds like the CPU is not allowed to be a > > valid CPU, so I rewrote the comment. > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 3afc6a7b5b..36c2fb069c 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine) > > ObjectClass *class = object_class_by_name(machine->cpu_type); > > int i; > > > > - /* machine->cpu_type is supposed to be always a valid QOM type */ > > - assert(class); > > - > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > > if (object_class_dynamic_cast(class, > > machine_class->valid_cpu_types[i])) { > > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine) > > } > > > > if (!machine_class->valid_cpu_types[i]) { > > - /* The user specified CPU must not be a valid CPU */ > > + /* The user specified CPU is not valid */ > > error_report("Invalid CPU type: %s", machine->cpu_type); > > error_printf("The valid types are: %s", > > machine_class->valid_cpu_types[0]); > > Looks good to me. > > Does that mean you are taking the whole series now? I was planning to tacke only patch 1/5, but I can take the whole series if I get an Acked-by from the corresponding maintainers.
On Tue, Oct 3, 2017 at 1:33 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Oct 03, 2017 at 01:26:53PM -0700, Alistair Francis wrote: >> On Tue, Oct 3, 2017 at 1:23 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Tue, Oct 03, 2017 at 01:05:09PM -0700, Alistair Francis wrote: >> >> This patch add a MachineClass element that can be set in the machine C >> >> code to specify a list of supported CPU types. If the supported CPU >> >> types are specified the user enter CPU (by -cpu at runtime) is checked >> >> against the supported types and QEMU exits if they aren't supported. >> >> >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> >> --- >> > >> > Thanks! >> > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> > >> > However, I will squash the following changes before queueing, >> > because: >> > >> > * object_class_dynamic_cast() is safe even if class is NULL, >> > so there's no need to validate cpu_type here. >> > * "must not be valid" sounds like the CPU is not allowed to be a >> > valid CPU, so I rewrote the comment. >> > >> > >> > diff --git a/hw/core/machine.c b/hw/core/machine.c >> > index 3afc6a7b5b..36c2fb069c 100644 >> > --- a/hw/core/machine.c >> > +++ b/hw/core/machine.c >> > @@ -766,9 +766,6 @@ void machine_run_board_init(MachineState *machine) >> > ObjectClass *class = object_class_by_name(machine->cpu_type); >> > int i; >> > >> > - /* machine->cpu_type is supposed to be always a valid QOM type */ >> > - assert(class); >> > - >> > for (i = 0; machine_class->valid_cpu_types[i]; i++) { >> > if (object_class_dynamic_cast(class, >> > machine_class->valid_cpu_types[i])) { >> > @@ -780,7 +777,7 @@ void machine_run_board_init(MachineState *machine) >> > } >> > >> > if (!machine_class->valid_cpu_types[i]) { >> > - /* The user specified CPU must not be a valid CPU */ >> > + /* The user specified CPU is not valid */ >> > error_report("Invalid CPU type: %s", machine->cpu_type); >> > error_printf("The valid types are: %s", >> > machine_class->valid_cpu_types[0]); >> >> Looks good to me. >> >> Does that mean you are taking the whole series now? > > I was planning to tacke only patch 1/5, but I can take the whole > series if I get an Acked-by from the corresponding maintainers. Most of them are maintained by me. Just getting patch 1 in is the most important part though (then others can use it). So if you want to just take it by itself that's fine with me. Thanks, Alistair > > -- > Eduardo >
diff --git a/hw/core/machine.c b/hw/core/machine.c index 80647edc2a..3afc6a7b5b 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -758,6 +758,41 @@ void machine_run_board_init(MachineState *machine) if (nb_numa_nodes) { machine_numa_finish_init(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); + int i; + + /* machine->cpu_type is supposed to be always a valid QOM type */ + assert(class); + + for (i = 0; machine_class->valid_cpu_types[i]; i++) { + if (object_class_dynamic_cast(class, + machine_class->valid_cpu_types[i])) { + /* The user specificed CPU is in the valid field, we are + * good to go. + */ + break; + } + } + + if (!machine_class->valid_cpu_types[i]) { + /* The user specified CPU must not be a valid CPU */ + error_report("Invalid CPU type: %s", machine->cpu_type); + error_printf("The valid types 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]); + } + error_printf("\n"); + + exit(1); + } + } + machine_class->init(machine); } diff --git a/include/hw/boards.h b/include/hw/boards.h index 156e0a5701..191a5b3cd8 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -191,6 +191,7 @@ struct MachineClass { bool has_hotpluggable_cpus; bool ignore_memory_transaction_failures; int numa_mem_align_shift; + const char **valid_cpu_types; void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes, int nb_nodes, ram_addr_t size);
This patch add a MachineClass element that can be set in the machine C code to specify a list of supported CPU types. If the supported CPU types are specified the user enter CPU (by -cpu at runtime) is checked against the supported types and QEMU exits if they aren't supported. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- V1: - Use 'type' in the error message - Move a uneeded operation out of the for loop - Remove the goto RFC v2: - Rebase on Igor's cpu_type work - Use object_class_dynamic_cast() - Use a NULL terminated cahr** list - Do the check before the machine_class init() is called hw/core/machine.c | 35 +++++++++++++++++++++++++++++++++++ include/hw/boards.h | 1 + 2 files changed, 36 insertions(+)