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

[Eduardo Habkost]

On Thu, Sep 21, 2017 at 04:41:50PM -0700, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> 
> 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..abebfabdb8 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 (machine_class->valid_cpu_types && machine->cpu_type) {
> +        int i;
> +
> +        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> +            ObjectClass *class = object_class_by_name(machine->cpu_type);
> +
> +            if (!class) {
> +                break;
> +            }
> +
> +            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.
> +                 */
> +                goto done;

I would move the object_class_by_name() call outside the for loop
and remove the "goto", like this:

    if (machine->cpu_type && machine_class->valid_cpu_types) {
        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])) {
                /* Valid CPU type, we're good to go */
                break;
            }
        }
        if (!machine_class->valid_cpu_types[i]) {
            error_report(...);
            ...
        }
    }

    machine_class->init(machine);

> +            }
> +        }
> +
> +        /* The user specified CPU must not be a valid CPU, print a sane
> +         * error
> +         */
> +        error_report("Invalid CPU: %s", machine->cpu_type);
> +        error_printf("The valid options 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");

I would still like to make this share code with
query-cpu-definitions one day, but I'm OK with this
implementation.

I would just rewrite the message as "valid types are:" instead of
"valid options are:" and "Invalid CPU type:" instead of "Invalid
CPU:", because the -cpu option doesn't need to match a string in
valid_cpu_types exactly, it just needs to resolve to a type that
implements a valid type.


> +
> +        exit(1);
> +    }
> +
> +done:
>      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