[Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Aleksandar Markovic]

From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

This patch enables QMP-based querying of the available CPU types for
MIPS and MIPS64 platforms.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---

v1->v2:

  - rebased to the latest QAPI code

 qapi/target.json     |  4 ++--
 target/mips/helper.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/qapi/target.json b/qapi/target.json
index da7b4be..1d4d54b 100644
--- a/qapi/target.json
+++ b/qapi/target.json
@@ -499,7 +499,7 @@
             'static': 'bool',
             '*unavailable-features': [ 'str' ],
             'typename': 'str' },
-  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
+  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
 
 ##
 # @query-cpu-definitions:
@@ -511,4 +511,4 @@
 # Since: 1.2.0
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
-  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
+  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
diff --git a/target/mips/helper.c b/target/mips/helper.c
index 944f094..c44cdca 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -24,6 +24,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/log.h"
 #include "hw/mips/cpudevs.h"
+#include "qapi/qapi-commands-target.h"
 
 enum {
     TLBRET_XI = -6,
@@ -1470,3 +1471,35 @@ void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
 
     cpu_loop_exit_restore(cs, pc);
 }
+
+static void mips_cpu_add_definition(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    CpuDefinitionInfoList **cpu_list = user_data;
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
+    const char *typename;
+
+    typename = object_class_get_name(oc);
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strndup(typename,
+                           strlen(typename) - strlen("-" TYPE_MIPS_CPU));
+    info->q_typename = g_strdup(typename);
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+    entry->next = *cpu_list;
+    *cpu_list = entry;
+}
+
+CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+{
+    CpuDefinitionInfoList *cpu_list = NULL;
+    GSList *list;
+
+    list = object_class_get_list(TYPE_MIPS_CPU, false);
+    g_slist_foreach(list, mips_cpu_add_definition, &cpu_list);
+    g_slist_free(list);
+
+    return cpu_list;
+}
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Philippe Mathieu-Daudé]

On 2/19/19 8:15 PM, Aleksandar Markovic wrote:
> From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> 
> This patch enables QMP-based querying of the available CPU types for
> MIPS and MIPS64 platforms.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Aleksandar, FYI I tested v1 and your v2 that way:

$ mips64el-softmmu/qemu-system-mips64el -S \
    -qmp tcp:localhost:4444,server,nowait

$ telnet localhost 4444
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 3},
"package": "v3.1.0-1924-gc7b700fafb"}, "capabilities": ["oob"]}}

{ "execute": "qmp_capabilities" }
{"return": {}}

{ "execute": "query-cpu-definitions" }
{"return": [{"name": "24KEc", "typename": "24KEc-mips64-cpu", "static":
false}, {"name": "MIPS64R2-generic", "typename":
"MIPS64R2-generic-mips64-cpu", "static": false}, {"name": "VR5432",
"typename": "VR5432-mips64-cpu", "static": false}, {"name": "M14Kc",
"typename": "M14Kc-mips64-cpu", "static": false}, {"name": "20Kc",
"typename": "20Kc-mips64-cpu", "static": false}, {"name": "4Km",
"typename": "4Km-mips64-cpu", "static": false}, {"name": "mips64dspr2",
"typename": "mips64dspr2-mips64-cpu", "static": false}, {"name":
"I6400", "typename": "I6400-mips64-cpu", "static": false}, {"name":
"Loongson-2E", "typename": "Loongson-2E-mips64-cpu", "static": false},
{"name": "I6500", "typename": "I6500-mips64-cpu", "static": false},
{"name": "4KEcR1", "typename": "4KEcR1-mips64-cpu", "static": false},
{"name": "74Kf", "typename": "74Kf-mips64-cpu", "static": false},
{"name": "24Kf", "typename": "24Kf-mips64-cpu", "static": false},
{"name": "5KEc", "typename": "5KEc-mips64-cpu", "static": false},
{"name": "P5600", "typename": "P5600-mips64-cpu", "static": false},
{"name": "I7200", "typename": "I7200-mips64-cpu", "static": false},
{"name": "M14K", "typename": "M14K-mips64-cpu", "static": false},
{"name": "5KEf", "typename": "5KEf-mips64-cpu", "static": false},
{"name": "24Kc", "typename": "24Kc-mips64-cpu", "static": false},
{"name": "4KEm", "typename": "4KEm-mips64-cpu", "static": false},
{"name": "4Kc", "typename": "4Kc-mips64-cpu", "static": false}, {"name":
"5Kc", "typename": "5Kc-mips64-cpu", "static": false}, {"name":
"4KEmR1", "typename": "4KEmR1-mips64-cpu", "static": false}, {"name":
"R4000", "typename": "R4000-mips64-cpu", "static": false}, {"name":
"5Kf", "typename": "5Kf-mips64-cpu", "static": false}, {"name":
"mips32r6-generic", "typename": "mips32r6-generic-mips64-cpu", "static":
false}, {"name": "4KEc", "typename": "4KEc-mips64-cpu", "static":
false}, {"name": "34Kf", "typename": "34Kf-mips64-cpu", "static":
false}, {"name": "Loongson-2F", "typename": "Loongson-2F-mips64-cpu",
"static": false}]}

See: https://wiki.qemu.org/Documentation/QMP

Regards,

Phil.

> ---
> 
> v1->v2:
> 
>   - rebased to the latest QAPI code
> 
>  qapi/target.json     |  4 ++--
>  target/mips/helper.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/target.json b/qapi/target.json
> index da7b4be..1d4d54b 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -499,7 +499,7 @@
>              'static': 'bool',
>              '*unavailable-features': [ 'str' ],
>              'typename': 'str' },
> -  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
> +  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
>  
>  ##
>  # @query-cpu-definitions:
> @@ -511,4 +511,4 @@
>  # Since: 1.2.0
>  ##
>  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> -  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
> +  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index 944f094..c44cdca 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -24,6 +24,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/log.h"
>  #include "hw/mips/cpudevs.h"
> +#include "qapi/qapi-commands-target.h"
>  
>  enum {
>      TLBRET_XI = -6,
> @@ -1470,3 +1471,35 @@ void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
>  
>      cpu_loop_exit_restore(cs, pc);
>  }
> +
> +static void mips_cpu_add_definition(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CpuDefinitionInfoList **cpu_list = user_data;
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +    const char *typename;
> +
> +    typename = object_class_get_name(oc);
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strndup(typename,
> +                           strlen(typename) - strlen("-" TYPE_MIPS_CPU));
> +    info->q_typename = g_strdup(typename);
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;
> +    entry->next = *cpu_list;
> +    *cpu_list = entry;
> +}
> +
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +{
> +    CpuDefinitionInfoList *cpu_list = NULL;
> +    GSList *list;
> +
> +    list = object_class_get_list(TYPE_MIPS_CPU, false);
> +    g_slist_foreach(list, mips_cpu_add_definition, &cpu_list);
> +    g_slist_free(list);
> +
> +    return cpu_list;
> +}
> 

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Aleksandar Markovic]

> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> Subject: Re: [PATCH v2] target/mips: implement QMP query-cpu-definitions command
> 
> On 2/19/19 8:15 PM, Aleksandar Markovic wrote:
> > From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> >
> > This patch enables QMP-based querying of the available CPU types for
> > MIPS and MIPS64 platforms.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Aleksandar, FYI I tested v1 and your v2 that way:
> 
> $ mips64el-softmmu/qemu-system-mips64el -S \
>     -qmp tcp:localhost:4444,server,nowait
> 
> $ telnet localhost 4444
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 3},
> "package": "v3.1.0-1924-gc7b700fafb"}, "capabilities": ["oob"]}}

...

That's fabulous! Thanks, Philippe!

Aleksandar

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Eric Blake]

On 2/19/19 1:15 PM, Aleksandar Markovic wrote:
> From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> 
> This patch enables QMP-based querying of the available CPU types for
> MIPS and MIPS64 platforms.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

> +++ b/qapi/target.json
> @@ -499,7 +499,7 @@
>              'static': 'bool',
>              '*unavailable-features': [ 'str' ],
>              'typename': 'str' },
> -  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
> +  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }

Hmm. Long line; however, the argument to 'if' is pasted literally to an
#if directive, which would break if we added newlines in the middle.
And we can't use literal newlines in the middle of a JSON string.  About
the only thing I could think of that might allow for more manageable
line lengths would be permitting:

'if': [ 'defined(TARGET_PPC)',
        'defined(TARGET_ARM)' ...]

where the QAPI generator would in turn form the disjunction of supplying
the || between each term when the 'if' is an array of strings. But that
feels like a lot of effort for little gain compared to just living with
the long lines.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Philippe Mathieu-Daudé]

Hi Eric,

On 2/19/19 9:12 PM, Eric Blake wrote:
> On 2/19/19 1:15 PM, Aleksandar Markovic wrote:
>> From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>
>> This patch enables QMP-based querying of the available CPU types for
>> MIPS and MIPS64 platforms.
>>
>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
> 
>> +++ b/qapi/target.json
>> @@ -499,7 +499,7 @@
>>              'static': 'bool',
>>              '*unavailable-features': [ 'str' ],
>>              'typename': 'str' },
>> -  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
>> +  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> 
> Hmm. Long line; however, the argument to 'if' is pasted literally to an
> #if directive, which would break if we added newlines in the middle.
> And we can't use literal newlines in the middle of a JSON string.  About
> the only thing I could think of that might allow for more manageable
> line lengths would be permitting:
> 
> 'if': [ 'defined(TARGET_PPC)',
>         'defined(TARGET_ARM)' ...]

Can this be:

'if': [
        'defined(TARGET_PPC)',
        'defined(TARGET_ARM)',
        ...
      ]

?

> where the QAPI generator would in turn form the disjunction of supplying
> the || between each term when the 'if' is an array of strings. But that
> feels like a lot of effort for little gain compared to just living with
> the long lines.

Single line looks easier thus less bug prone.

Multiline improves git diff/blame.

The former looks safer.

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 2/19/19 1:15 PM, Aleksandar Markovic wrote:
>> From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>> 
>> This patch enables QMP-based querying of the available CPU types for
>> MIPS and MIPS64 platforms.
>> 
>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>
>> +++ b/qapi/target.json
>> @@ -499,7 +499,7 @@
>>              'static': 'bool',
>>              '*unavailable-features': [ 'str' ],
>>              'typename': 'str' },
>> -  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X)' }
>> +  'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
>
> Hmm. Long line; however, the argument to 'if' is pasted literally to an
> #if directive, which would break if we added newlines in the middle.
> And we can't use literal newlines in the middle of a JSON string.  About
> the only thing I could think of that might allow for more manageable
> line lengths would be permitting:
>
> 'if': [ 'defined(TARGET_PPC)',
>         'defined(TARGET_ARM)' ...]
>
> where the QAPI generator would in turn form the disjunction of supplying
> the || between each term when the 'if' is an array of strings. But that
> feels like a lot of effort for little gain compared to just living with
> the long lines.

I hate long lines as much as anyone, but we already burned the list
syntax on conjunction:

docs/devel/qapi-code-gen.txt:

    === Configuring the schema ===

    The 'struct', 'enum', 'union', 'alternate', 'command' and 'event'
    top-level expressions can take an 'if' key.  Its value must be a string
    or a list of strings.  A string is shorthand for a list containing just
    that string.  The code generated for the top-level expression will then
    be guarded by #if COND for each COND in the list.

    Example: a conditional struct

     { 'struct': 'IfStruct', 'data': { 'foo': 'int' },
       'if': ['defined(CONFIG_FOO)', 'defined(HAVE_BAR)'] }

    gets its generated code guarded like this:

     #if defined(CONFIG_FOO)
     #if defined(HAVE_BAR)
     ... generated code ...
     #endif /* defined(HAVE_BAR) */
     #endif /* defined(CONFIG_FOO) */

The problem of long strings requiring long lines is not limited to 'if'.
The common solution in programming languages is to concatenate adjacent
string literals.  A more user-friendly QAPI language would support
something like that.  By choosing JSON, a language designed "for the
serialization of structured data" (RFC 4627, 7159, 8259), we opted
against user-friendly.  We then admitted our regrets halfheartedly by
making our version of JSON incompatible enough to defeat tools like
javascript-mode.  Correcting our fundamental mistake now would be a lot
of churn.

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Markus Armbruster]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>
> This patch enables QMP-based querying of the available CPU types for
> MIPS and MIPS64 platforms.
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Almost identical to the ARM code.  Factoring out the common core doesn't
seem worth the bother for just two copies.  Perhaps if we grow more
copies.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Aleksandar Markovic]

> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

...

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Unless there is somebody's objection, I am going to accept this patch into the next
MIPS pull request, planned to be sent in next few days.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Markus Armbruster]

Aleksandar Markovic <amarkovic@wavecomp.com> writes:

>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command
>
> ...
>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Unless there is somebody's objection, I am going to accept this patch into the next
> MIPS pull request, planned to be sent in next few days.

As far as I'm concerned: go right ahead.

Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command

[Aleksandar Markovic]

> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command
> 
> Aleksandar Markovic <amarkovic@wavecomp.com> writes:
> 
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Subject: Re: [Qemu-devel] [PATCH v2] target/mips: implement QMP query-cpu-definitions command
> >
> > ...
> >
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> > Unless there is somebody's objection, I am going to accept this patch into the next
> > MIPS pull request, planned to be sent in next few days.
> 
> As far as I'm concerned: go right ahead.

This patch was selected to be in the MIPS queue, and was subsequently integrated in the main tree today.

Thanks to all folks involved!

Aleksandar