* [Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property
@ 2017-09-06 0:11 Alistair Francis
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 1/2] machine: " Alistair Francis
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs Alistair Francis
0 siblings, 2 replies; 8+ messages in thread
From: Alistair Francis @ 2017-09-06 0:11 UTC (permalink / raw)
To: qemu-devel; +Cc: alistair.francis, alistair23, ehabkost, marcel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [RFC v1 1/2] machine: Add a valid_cpu_types property
2017-09-06 0:11 [Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property Alistair Francis
@ 2017-09-06 0:12 ` Alistair Francis
2017-09-09 20:16 ` Eduardo Habkost
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs Alistair Francis
1 sibling, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2017-09-06 0:12 UTC (permalink / raw)
To: qemu-devel; +Cc: alistair.francis, alistair23, ehabkost, marcel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs
2017-09-06 0:11 [Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property Alistair Francis
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 1/2] machine: " Alistair Francis
@ 2017-09-06 0:12 ` Alistair Francis
2017-09-11 11:56 ` KONRAD Frederic
1 sibling, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2017-09-06 0:12 UTC (permalink / raw)
To: qemu-devel; +Cc: alistair.francis, alistair23, ehabkost, marcel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [RFC v1 1/2] machine: Add a valid_cpu_types property
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 1/2] machine: " Alistair Francis
@ 2017-09-09 20:16 ` Eduardo Habkost
2017-09-11 11:31 ` Igor Mammedov
2017-09-21 23:06 ` Alistair Francis
0 siblings, 2 replies; 8+ messages in thread
From: Eduardo Habkost @ 2017-09-09 20:16 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, alistair23, marcel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [RFC v1 1/2] machine: Add a valid_cpu_types property
2017-09-09 20:16 ` Eduardo Habkost
@ 2017-09-11 11:31 ` Igor Mammedov
2017-09-21 23:06 ` Alistair Francis
1 sibling, 0 replies; 8+ messages in thread
From: Igor Mammedov @ 2017-09-11 11:31 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Alistair Francis, marcel, alistair23, qemu-devel
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
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs Alistair Francis
@ 2017-09-11 11:56 ` KONRAD Frederic
2017-09-21 23:07 ` Alistair Francis
0 siblings, 1 reply; 8+ messages in thread
From: KONRAD Frederic @ 2017-09-11 11:56 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, marcel, alistair23, ehabkost
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)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [RFC v1 1/2] machine: Add a valid_cpu_types property
2017-09-09 20:16 ` Eduardo Habkost
2017-09-11 11:31 ` Igor Mammedov
@ 2017-09-21 23:06 ` Alistair Francis
1 sibling, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2017-09-21 23:06 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, Marcel Apfelbaum
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs
2017-09-11 11:56 ` KONRAD Frederic
@ 2017-09-21 23:07 ` Alistair Francis
0 siblings, 0 replies; 8+ messages in thread
From: Alistair Francis @ 2017-09-21 23:07 UTC (permalink / raw)
To: KONRAD Frederic
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
Marcel Apfelbaum, Eduardo Habkost
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)
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-21 23:08 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 0:11 [Qemu-devel] [RFC v1 0/2] Add a valid_cpu_types property Alistair Francis
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 1/2] machine: " Alistair Francis
2017-09-09 20:16 ` Eduardo Habkost
2017-09-11 11:31 ` Igor Mammedov
2017-09-21 23:06 ` Alistair Francis
2017-09-06 0:12 ` [Qemu-devel] [RFC v1 2/2] netduino2: Specify the valid CPUs Alistair Francis
2017-09-11 11:56 ` KONRAD Frederic
2017-09-21 23:07 ` Alistair Francis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.