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(+)

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 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
> 

-- 
Eduardo

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.

-- 
Eduardo

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
>