On Sat, Sep 9, 2017 at 1:16 PM, Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Tue, Sep 05, 2017 at 05:12:01PM -0700, Alistair Francis wrote:
>> Signed-off-by: Alistair Francis <alistair.fran...@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
