Re: [Qemu-devel] [RFC v1 1/2] machine: Add a valid_cpu_types property

[Eduardo Habkost <ehabkost@redhat.com>]

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
> 

-- 
Eduardo