[Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property

[Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property

[Alistair Francis]

There are numorous QEMU machines that only have a single or a handful of
valid CPU options. To simplyfy the management of specificying which CPU
is/isn't valid let's create a property that can be set in the machine
init. We can then check to see if the user supplied CPU is in that list
or not.

This is just a quick setup, if this method is agreed apon I can add a
nice macro to add the valid CPU options (similar to SET_MACHINE_COMPAT)
and improve the error messages. I just wanted to get some input before I
spent too much time on that.

Alistair Francis (2):
  machine: Add a valid_cpu_types property
  netduino2: Specify the valid CPUs

 hw/arm/netduino2.c  |  5 +++++
 hw/core/machine.c   | 27 +++++++++++++++++++++++++++
 include/hw/boards.h |  1 +
 3 files changed, 33 insertions(+)

-- 
2.11.0

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

[Alistair Francis]

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;
+
+        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;
+            }
+        }
+        /* 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");
+        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;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 
-- 
2.11.0

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

[Eduardo Habkost]

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

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

[Igor Mammedov]

On Sat, 9 Sep 2017 17:16:57 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

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

in addition check should run before
  machine_class->init(machine);
it should be possible after cpu_model -> cpu_type refactoring is complete

[...]

> > 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?
wile working on cpu_model, I've seen only static checks
used in some machines. So +1 to static null terminated array
[
  TYPE_FOO1,
  TYPE_FOO2,
  NULL
]

> 
> >      void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
> >                                   int nb_nodes, ram_addr_t size);
> >  
> > -- 
> > 2.11.0
> >   
> 

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

[Alistair Francis]

On Sat, Sep 9, 2017 at 1:16 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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.

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

[Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/arm/netduino2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 3cfe332dd1..accca344fd 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -43,8 +43,13 @@ static void netduino2_init(MachineState *machine)
 
 static void netduino2_machine_init(MachineClass *mc)
 {
+    const char *val = "cortex-m3";
+
     mc->desc = "Netduino 2 Machine";
     mc->init = netduino2_init;
+
+    mc->valid_cpu_types = g_array_new(false, false, sizeof(char *));
+    g_array_append_val(mc->valid_cpu_types, val);
 }
 
 DEFINE_MACHINE("netduino2", netduino2_machine_init)
-- 
2.11.0

Re: [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs

[KONRAD Frederic]

On 09/06/2017 02:12 AM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>   hw/arm/netduino2.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index 3cfe332dd1..accca344fd 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -43,8 +43,13 @@ static void netduino2_init(MachineState *machine)
>   
>   static void netduino2_machine_init(MachineClass *mc)
>   {
> +    const char *val = "cortex-m3";
> +
>       mc->desc = "Netduino 2 Machine";
>       mc->init = netduino2_init;
> +
> +    mc->valid_cpu_types = g_array_new(false, false, sizeof(char *));
> +    g_array_append_val(mc->valid_cpu_types, val);
>   }

Hi Alistair,

Nice idea! Why not adding an helper function which take a const
char** in argument?

Fred

>   
>   DEFINE_MACHINE("netduino2", netduino2_machine_init)
> 

Re: [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs

[Alistair Francis]

On Mon, Sep 11, 2017 at 4:56 AM, KONRAD Frederic
<frederic.konrad@adacore.com> wrote:
>
>
> On 09/06/2017 02:12 AM, Alistair Francis wrote:
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>   hw/arm/netduino2.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>> index 3cfe332dd1..accca344fd 100644
>> --- a/hw/arm/netduino2.c
>> +++ b/hw/arm/netduino2.c
>> @@ -43,8 +43,13 @@ static void netduino2_init(MachineState *machine)
>>     static void netduino2_machine_init(MachineClass *mc)
>>   {
>> +    const char *val = "cortex-m3";
>> +
>>       mc->desc = "Netduino 2 Machine";
>>       mc->init = netduino2_init;
>> +
>> +    mc->valid_cpu_types = g_array_new(false, false, sizeof(char *));
>> +    g_array_append_val(mc->valid_cpu_types, val);
>>   }
>
>
> Hi Alistair,
>
> Nice idea! Why not adding an helper function which take a const
> char** in argument?

Thanks Fred.

I'm going to send a v2 with just that :)

Thanks,
Alistair

>
> Fred
>
>>     DEFINE_MACHINE("netduino2", netduino2_machine_init)
>>
>