[PATCH for-6.0 0/6] Add HMP/QMP commands to query accelerator

[PATCH for-6.0 0/6] Add HMP/QMP commands to query accelerator

[Roman Bolshakov]

Hi,

Management applications have no way to determine if certain accelerator
is available. That complicates discovery of non-KVM accelerators.

To address the issue, the series adds two commands:

  'query-accel' for QMP to be used by management apps, and

  'info accel' for HMP to replace 'info kvm' in future.

Thanks,
Roman

Roman Bolshakov (6):
  qapi: Add query-accel command
  qapi: Rename KvmInfo to AccelInfo
  qapi: Use qmp_query_accel() in qmp_query_kvm()
  softmmu: Remove kvm_available()
  hmp: Add 'info accel' command
  qapi: Deprecate 'query-kvm'

 hmp-commands-info.hx       | 13 +++++++++++++
 include/monitor/hmp.h      |  1 +
 include/sysemu/arch_init.h |  1 -
 monitor/hmp-cmds.c         | 36 ++++++++++++++++++++++++++++++++++--
 monitor/qmp-cmds.c         | 18 ++++++++++++++----
 qapi/machine.json          | 37 ++++++++++++++++++++++++++++++-------
 softmmu/arch_init.c        |  9 ---------
 7 files changed, 92 insertions(+), 23 deletions(-)

-- 
2.29.2

[PATCH for-6.0 1/6] qapi: Add query-accel command

[Roman Bolshakov]

There's a problem for management applications to determine if certain
accelerators available. Generic QMP command should help with that.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 monitor/qmp-cmds.c | 15 +++++++++++++++
 qapi/machine.json  | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index a08143b323..0454394e76 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -62,6 +62,21 @@ KvmInfo *qmp_query_kvm(Error **errp)
     return info;
 }
 
+KvmInfo *qmp_query_accel(const char *name, Error **errp)
+{
+    KvmInfo *info = g_malloc0(sizeof(*info));
+
+    AccelClass *ac = accel_find(name);
+
+    if (ac) {
+        info->enabled = *ac->allowed;
+        info->present = true;
+    }
+
+    return info;
+}
+
+
 UuidInfo *qmp_query_uuid(Error **errp)
 {
     UuidInfo *info = g_malloc0(sizeof(*info));
diff --git a/qapi/machine.json b/qapi/machine.json
index 7c9a263778..11f364fab4 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -591,6 +591,25 @@
 ##
 { 'command': 'query-kvm', 'returns': 'KvmInfo' }
 
+##
+# @query-accel:
+#
+# Returns information about an accelerator
+#
+# Returns: @KvmInfo
+#
+# Since: 6.0.0
+#
+# Example:
+#
+# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
+# <- { "return": { "enabled": true, "present": true } }
+#
+##
+{ 'command': 'query-accel',
+  'data': { 'name': 'str' },
+  'returns': 'KvmInfo' }
+
 ##
 # @NumaOptionsType:
 #
-- 
2.29.2

[PATCH for-6.0 2/6] qapi: Rename KvmInfo to AccelInfo

[Roman Bolshakov]

There's nothing specific to KVM in the structure. A more generic name
would be more appropriate.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 monitor/hmp-cmds.c |  4 ++--
 monitor/qmp-cmds.c |  8 ++++----
 qapi/machine.json  | 18 +++++++++---------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..ea86289fe8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -120,7 +120,7 @@ void hmp_info_version(Monitor *mon, const QDict *qdict)
 
 void hmp_info_kvm(Monitor *mon, const QDict *qdict)
 {
-    KvmInfo *info;
+    AccelInfo *info;
 
     info = qmp_query_kvm(NULL);
     monitor_printf(mon, "kvm support: ");
@@ -130,7 +130,7 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "not compiled\n");
     }
 
-    qapi_free_KvmInfo(info);
+    qapi_free_AccelInfo(info);
 }
 
 void hmp_info_status(Monitor *mon, const QDict *qdict)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 0454394e76..f5d50afa9c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -52,9 +52,9 @@ NameInfo *qmp_query_name(Error **errp)
     return info;
 }
 
-KvmInfo *qmp_query_kvm(Error **errp)
+AccelInfo *qmp_query_kvm(Error **errp)
 {
-    KvmInfo *info = g_malloc0(sizeof(*info));
+    AccelInfo *info = g_malloc0(sizeof(*info));
 
     info->enabled = kvm_enabled();
     info->present = kvm_available();
@@ -62,9 +62,9 @@ KvmInfo *qmp_query_kvm(Error **errp)
     return info;
 }
 
-KvmInfo *qmp_query_accel(const char *name, Error **errp)
+AccelInfo *qmp_query_accel(const char *name, Error **errp)
 {
-    KvmInfo *info = g_malloc0(sizeof(*info));
+    AccelInfo *info = g_malloc0(sizeof(*info));
 
     AccelClass *ac = accel_find(name);
 
diff --git a/qapi/machine.json b/qapi/machine.json
index 11f364fab4..5648d8d24d 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -562,24 +562,24 @@
 { 'command': 'inject-nmi' }
 
 ##
-# @KvmInfo:
+# @AccelInfo:
 #
-# Information about support for KVM acceleration
+# Information about support for an acceleration
 #
-# @enabled: true if KVM acceleration is active
+# @enabled: true if an acceleration is active
 #
-# @present: true if KVM acceleration is built into this executable
+# @present: true if an acceleration is built into this executable
 #
 # Since: 0.14.0
 ##
-{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
+{ 'struct': 'AccelInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
 
 ##
 # @query-kvm:
 #
 # Returns information about KVM acceleration
 #
-# Returns: @KvmInfo
+# Returns: @AccelInfo
 #
 # Since: 0.14.0
 #
@@ -589,14 +589,14 @@
 # <- { "return": { "enabled": true, "present": true } }
 #
 ##
-{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
+{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
 
 ##
 # @query-accel:
 #
 # Returns information about an accelerator
 #
-# Returns: @KvmInfo
+# Returns: @AccelInfo
 #
 # Since: 6.0.0
 #
@@ -608,7 +608,7 @@
 ##
 { 'command': 'query-accel',
   'data': { 'name': 'str' },
-  'returns': 'KvmInfo' }
+  'returns': 'AccelInfo' }
 
 ##
 # @NumaOptionsType:
-- 
2.29.2

[PATCH for-6.0 3/6] qapi: Use qmp_query_accel() in qmp_query_kvm()

[Roman Bolshakov]

qmp_query_accel() is generic and can be used instead of open-coding
qmp_query_kvm().

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 monitor/qmp-cmds.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index f5d50afa9c..5a5f3a65f4 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -54,12 +54,7 @@ NameInfo *qmp_query_name(Error **errp)
 
 AccelInfo *qmp_query_kvm(Error **errp)
 {
-    AccelInfo *info = g_malloc0(sizeof(*info));
-
-    info->enabled = kvm_enabled();
-    info->present = kvm_available();
-
-    return info;
+    return qmp_query_accel("kvm", errp);
 }
 
 AccelInfo *qmp_query_accel(const char *name, Error **errp)
-- 
2.29.2

[PATCH for-6.0 4/6] softmmu: Remove kvm_available()

[Roman Bolshakov]

The function isn't used anywhere after qmp_query_kvm() has been switched
to invoke qmp_query_accel().

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 include/sysemu/arch_init.h | 1 -
 softmmu/arch_init.c        | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 54f069d491..b32ce1afa9 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -32,7 +32,6 @@ enum {
 
 extern const uint32_t arch_type;
 
-int kvm_available(void);
 int xen_available(void);
 
 #endif
diff --git a/softmmu/arch_init.c b/softmmu/arch_init.c
index 7fd5c09b2b..56cbe0d3b5 100644
--- a/softmmu/arch_init.c
+++ b/softmmu/arch_init.c
@@ -96,15 +96,6 @@ int graphic_depth = 32;
 
 const uint32_t arch_type = QEMU_ARCH;
 
-int kvm_available(void)
-{
-#ifdef CONFIG_KVM
-    return 1;
-#else
-    return 0;
-#endif
-}
-
 int xen_available(void)
 {
 #ifdef CONFIG_XEN
-- 
2.29.2

[PATCH for-6.0 5/6] hmp: Add 'info accel' command

[Roman Bolshakov]

The command is made after 'info kvm' and aims to replace it as more
generic one.

If used without parameters, the command can used to get current
accelerator. Otherwise, it may be used to determine if an accelerator is
available. Here's an example if a VM with hvf accel is started:

  (qemu) info accel
  hvf support: enabled
  (qemu) info accel kvm
  kvm support: not compiled
  (qemu) info accel tcg
  tcg support: disabled

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 hmp-commands-info.hx  | 13 +++++++++++++
 include/monitor/hmp.h |  1 +
 monitor/hmp-cmds.c    | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 117ba25f91..e9da6b52e4 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -337,6 +337,19 @@ SRST
     Show KVM information.
 ERST
 
+    {
+        .name       = "accel",
+        .args_type  = "name:s?",
+        .params     = "[name]",
+        .help       = "show accelerator information",
+        .cmd        = hmp_info_accel,
+    },
+
+SRST
+  ``info accel``` [*name*]
+    Show accelerator information.
+ERST
+
     {
         .name       = "numa",
         .args_type  = "",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index ed2913fd18..03f22957d9 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -21,6 +21,7 @@ void hmp_handle_error(Monitor *mon, Error *err);
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
 void hmp_info_kvm(Monitor *mon, const QDict *qdict);
+void hmp_info_accel(Monitor *mon, const QDict *qdict);
 void hmp_info_status(Monitor *mon, const QDict *qdict);
 void hmp_info_uuid(Monitor *mon, const QDict *qdict);
 void hmp_info_chardev(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ea86289fe8..00db791aa3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -20,6 +20,7 @@
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/runstate.h"
+#include "sysemu/accel.h"
 #include "qemu/config-file.h"
 #include "qemu/option.h"
 #include "qemu/timer.h"
@@ -133,6 +134,37 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
     qapi_free_AccelInfo(info);
 }
 
+void hmp_info_accel(Monitor *mon, const QDict *qdict)
+{
+    AccelInfo *info;
+    AccelClass *acc;
+    const char *name, *typename;
+    char *current_name;
+    int len;
+
+    /* Figure out current accelerator */
+    acc = ACCEL_GET_CLASS(current_accel());
+    typename = object_class_get_name(OBJECT_CLASS(acc));
+    len = strlen(typename) - strlen(ACCEL_CLASS_SUFFIX);
+    current_name = g_strndup(typename, len);
+
+    name = qdict_get_try_str(qdict, "name");
+    if (!name) {
+        name = current_name;
+    }
+
+    info = qmp_query_accel(name, NULL);
+    monitor_printf(mon, "%s support: ", name);
+    if (info->present) {
+        monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled");
+    } else {
+        monitor_printf(mon, "not compiled\n");
+    }
+
+    qapi_free_AccelInfo(info);
+    g_free(current_name);
+}
+
 void hmp_info_status(Monitor *mon, const QDict *qdict)
 {
     StatusInfo *info;
-- 
2.29.2

[PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Roman Bolshakov]

'query-accel' QMP command should be used instead.

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 qapi/machine.json | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 5648d8d24d..130b0dbebc 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -579,6 +579,9 @@
 #
 # Returns information about KVM acceleration
 #
+# Features:
+# @deprecated: This command is deprecated, use 'query-accel' instead.
+#
 # Returns: @AccelInfo
 #
 # Since: 0.14.0
@@ -589,7 +592,8 @@
 # <- { "return": { "enabled": true, "present": true } }
 #
 ##
-{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
+{ 'command': 'query-kvm', 'returns': 'AccelInfo',
+  'features': [ 'deprecated' ] }
 
 ##
 # @query-accel:
-- 
2.29.2

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Eric Blake]

On 11/16/20 7:10 AM, Roman Bolshakov wrote:
> There's a problem for management applications to determine if certain
> accelerators available. Generic QMP command should help with that.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  monitor/qmp-cmds.c | 15 +++++++++++++++
>  qapi/machine.json  | 19 +++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 

> +++ b/qapi/machine.json
> @@ -591,6 +591,25 @@
>  ##
>  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
>  
> +##
> +# @query-accel:
> +#
> +# Returns information about an accelerator
> +#
> +# Returns: @KvmInfo
> +#
> +# Since: 6.0.0

We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
although I prefer the shorter form.  Maybe Markus has an opnion on that.

> +#
> +# Example:
> +#
> +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> +# <- { "return": { "enabled": true, "present": true } }
> +#
> +##
> +{ 'command': 'query-accel',
> +  'data': { 'name': 'str' },
> +  'returns': 'KvmInfo' }

'@name' is undocumented and an open-coded string.  Better would be
requiring 'name' to be one of an enum type.  Even better would be
returning an array of KvmInfo with information on all supported
accelerators at once, rather than making the user call this command once
per name.

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

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Roman Bolshakov]

On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
> > There's a problem for management applications to determine if certain
> > accelerators available. Generic QMP command should help with that.
> > 
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  monitor/qmp-cmds.c | 15 +++++++++++++++
> >  qapi/machine.json  | 19 +++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> > 
> 
> > +++ b/qapi/machine.json
> > @@ -591,6 +591,25 @@
> >  ##
> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
> >  
> > +##
> > +# @query-accel:
> > +#
> > +# Returns information about an accelerator
> > +#
> > +# Returns: @KvmInfo
> > +#
> > +# Since: 6.0.0
> 
> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
> although I prefer the shorter form.  Maybe Markus has an opnion on that.
> 

Sure, please let me know which one is better.

> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> > +# <- { "return": { "enabled": true, "present": true } }
> > +#
> > +##
> > +{ 'command': 'query-accel',
> > +  'data': { 'name': 'str' },
> > +  'returns': 'KvmInfo' }
> 
> '@name' is undocumented and an open-coded string.
>

Thanks for catching that! I'll add documentation for the field.

> Better would be requiring 'name' to be one of an enum type.

I haven't found any enums available, that's why I used accel_find that
looks up accel from string in QOM.

> Even better would be returning an array of KvmInfo with information on
> all supported accelerators at once, rather than making the user call
> this command once per name.
> 

I considered that, but wasn't sure if it's right or wrong. I'd prefer it
over the first option with enums. Likely, we can do that by iterating
all concerete accelerators:

  object_class_get_list(TYPE_ACCEL, false);

name parameter can be then dropped and query-accel would be renamed to
query-accels.

The approach has a drawback - there's no way to return accelerators that
aren't compiled, i.e. kvm on macOS or hvf on Linux. I don't know if it's
an issue or not.

query-accels would only return all available accelerators registered via
QOM and one of them would be enabled.

I think I'd try to use query-accel in libvirt before proceeding with
query-accels. If it'll be apparent that query-accels is superior, then'd
go with it.

Thanks,
Roman

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Eduardo Habkost]

On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
> > There's a problem for management applications to determine if certain
> > accelerators available. Generic QMP command should help with that.
> > 
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  monitor/qmp-cmds.c | 15 +++++++++++++++
> >  qapi/machine.json  | 19 +++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> > 
> 
> > +++ b/qapi/machine.json
> > @@ -591,6 +591,25 @@
> >  ##
> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
> >  
> > +##
> > +# @query-accel:
> > +#
> > +# Returns information about an accelerator
> > +#
> > +# Returns: @KvmInfo
> > +#
> > +# Since: 6.0.0
> 
> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
> although I prefer the shorter form.  Maybe Markus has an opnion on that.
> 
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> > +# <- { "return": { "enabled": true, "present": true } }
> > +#
> > +##
> > +{ 'command': 'query-accel',
> > +  'data': { 'name': 'str' },
> > +  'returns': 'KvmInfo' }
> 
> '@name' is undocumented and an open-coded string.  Better would be
> requiring 'name' to be one of an enum type.  [...]

This seem similar to CPU models, machine types, device types, and
backend object types: the set of valid values is derived from the
list of subtypes of a QOM type.  We don't duplicate lists of QOM
types in the QAPI schema, today.

Do we want to duplicate the list of accelerators in the QAPI
schema, or should we wait for a generic solution that works for
any QOM type?

>                                       [...]  Even better would be
> returning an array of KvmInfo with information on all supported
> accelerators at once, rather than making the user call this command once
> per name.

Maybe.  It would save us the work of answering the question
above, but is this (querying information for all accelerators at
once) going to be a common use case?

-- 
Eduardo

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
>> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
>> > There's a problem for management applications to determine if certain
>> > accelerators available. Generic QMP command should help with that.
>> > 
>> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> > ---
>> >  monitor/qmp-cmds.c | 15 +++++++++++++++
>> >  qapi/machine.json  | 19 +++++++++++++++++++
>> >  2 files changed, 34 insertions(+)
>> > 
>> 
>> > +++ b/qapi/machine.json
>> > @@ -591,6 +591,25 @@
>> >  ##
>> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
>> >  
>> > +##
>> > +# @query-accel:
>> > +#
>> > +# Returns information about an accelerator
>> > +#
>> > +# Returns: @KvmInfo

Is reusing KvmInfo here is good idea?  Recall:

    ##
    # @KvmInfo:
    #
    # Information about support for KVM acceleration
    #
    # @enabled: true if KVM acceleration is active
    #
    # @present: true if KVM acceleration is built into this executable
    #
    # Since: 0.14.0
    ##
    { 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }

I figure @present will always be true with query-accel.  In other words,
it's useless noise.

If we return information on all accelerators, KvmInfo won't do, because
it lacks the accelerator name.

If we choose to reuse KvmInfo regardless, it needs to be renamed to
something like AccelInfo, and the doc comment generalized beyond KVM.

>> > +#
>> > +# Since: 6.0.0
>> 
>> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
>> although I prefer the shorter form.  Maybe Markus has an opnion on that.

Yes: use the shorter form, unless .z != .0.

The shorter form is much more common:

    $ sed -En 's/.*since:? *([0-9][0-9.]*).*/\1/pi' qapi/*json | sed 's/[^.]//g' | sort | uniq -c
       1065 .
        129 ..

.z != 0 should be rare: the stable branch is for bug fixes, and bug
fixes rarely require schema changes.  It is: there is just one instance,
from commit ab9ba614556 (v3.0.0) and 0779afdc897 (v2.12.1).

>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
>> > +# <- { "return": { "enabled": true, "present": true } }
>> > +#
>> > +##
>> > +{ 'command': 'query-accel',
>> > +  'data': { 'name': 'str' },
>> > +  'returns': 'KvmInfo' }
>> 
>> '@name' is undocumented and an open-coded string.  Better would be
>> requiring 'name' to be one of an enum type.  [...]
>
> This seem similar to CPU models, machine types, device types, and
> backend object types: the set of valid values is derived from the
> list of subtypes of a QOM type.  We don't duplicate lists of QOM
> types in the QAPI schema, today.

Yes.

> Do we want to duplicate the list of accelerators in the QAPI
> schema, or should we wait for a generic solution that works for
> any QOM type?

There are only a few accelerators, so duplicating them wouldn't be too
bad.  Still, we need a better reason than "because we can".

QAPI enum vs. 'str' doesn't affect the wire format: both use JSON
string.

With a QAPI enum, the values available in this QEMU build are visible in
query-qmp-schema.

Without a QAPI enum, we need a separate, ad hoc query.  For QOM types,
we have qom-list-types.

If you're familiar with qom-list-types, you may want to skip to
"Conclusion:" now.

Ad hoc queries can take additional arguments.  qom-list-types does:
"implements" and "abstract" limit output.  Default is "all
non-abstract".

This provides a way to find accelerators: use "implements": "accel" to
return only concrete subtypes of "accel":

   ---> {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
   <--- {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": "tcg-accel", "parent": "accel"}, {"name": "xen-accel", "parent": "accel"}, {"name": "kvm-accel", "parent": "accel"}, {"name": "accel", "parent": "object"}]}

Aside: the reply includes "accel", because it's not abstract.  Bug?

Same for CPU models ("implements": "cpu"), machine types ("implements":
"machine"), device types ("implements": "device").  Not for backend
object types, because they don't have a common base type.  Certain kinds
of backend object types do, e.g. character device backends are subtypes
of "chardev".

Ad hoc queries can provide additional information.  qom-list-types does:
the parent type.

This provides a second way to find subtypes, which might be more
efficient when you need to know the subtypes of T for many T:

   Get *all* QOM types in one go:

   ---> {"execute": "qom-list-types", "arguments": {"abstract": false}}
   <--- lots of output

   Use output member "parent" to construct the parent tree.  The
   sub-tree rooted at QOM type "accel" has the subtypes of "accel".

   Awkward: since output lacks an "abstract" member, we have to
   determine it indirectly, by getting just the concrete types:

   ---> {"execute": "qom-list-types", "arguments": {"abstract": true}}
   <--- slightly less output

   We can add "abstract" to the output if we care.

   Unlike the first way, this one works *only* for "is subtype of",
   *not* for "implements interface".

   We can add interface information to the output if we care.

Likewise, QOM properties are not in the QAPI schema, and we need ad hoc
queries instead of query-qmp-schema: qom-list-properties, qom-list,
device-list-properties.  Flaws include inadequate type information and
difficulties around dynamic properties.

Conclusion: as is, QOM is designed to defeat our general QAPI/QMP
introspection mechanism.  It provides its own instead.  Proper
integration of QOM and QAPI/QMP would be great.  Integrating small parts
in ways that do not get us closer to complete integration does not seem
worthwhile.

For some QOM types, we have additional ad hoc queries that provide more
information, e.g. query-machines, query-cpu-definitions, and now the
proposed query-accel.

query-accel is *only* necessary if its additional information is.

I unde

>>                                       [...]  Even better would be
>> returning an array of KvmInfo with information on all supported
>> accelerators at once, rather than making the user call this command once
>> per name.
>
> Maybe.  It would save us the work of answering the question
> above, but is this (querying information for all accelerators at
> once) going to be a common use case?

I recommend to scratch the query-accel parameter, and return information
on all accelerators in this build of QEMU.  Simple, and consistent with
similar ad hoc queries.  If a client is interested in just one, fishing
it out of the list is easy enough.

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Roman Bolshakov]

On Tue, Nov 17, 2020 at 09:51:58AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
> >> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
> >> > There's a problem for management applications to determine if certain
> >> > accelerators available. Generic QMP command should help with that.
> >> > 
> >> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> >> > ---
> >> >  monitor/qmp-cmds.c | 15 +++++++++++++++
> >> >  qapi/machine.json  | 19 +++++++++++++++++++
> >> >  2 files changed, 34 insertions(+)
> >> > 
> >> 
> >> > +++ b/qapi/machine.json
> >> > @@ -591,6 +591,25 @@
> >> >  ##
> >> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
> >> >  
> >> > +##
> >> > +# @query-accel:
> >> > +#
> >> > +# Returns information about an accelerator
> >> > +#
> >> > +# Returns: @KvmInfo
> 
> Is reusing KvmInfo here is good idea?  Recall:
> 
>     ##
>     # @KvmInfo:
>     #
>     # Information about support for KVM acceleration
>     #
>     # @enabled: true if KVM acceleration is active
>     #
>     # @present: true if KVM acceleration is built into this executable
>     #
>     # Since: 0.14.0
>     ##
>     { 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
> 
> I figure @present will always be true with query-accel.  In other words,
> it's useless noise.
> 

Hi Markus,

Actually, no. Provided implementation returns present = true only if we
can find the accel in QOM, i.e. on macOS we get present = false for kvm.
And on Linux we get present = false for hvf, e.g.:

C: {"execute": "query-accel", "arguments": {"name": "hvf"}}
S: {"return": {"enabled": true, "present": true}}
C: {"execute": "query-accel", "arguments": {"name": "kvm"}}
S: {"return": {"enabled": false, "present": false}}
C: {"execute": "query-accel", "arguments": {"name": "tcg"}}
S: {"return": {"enabled": false, "present": true}}

> If we return information on all accelerators, KvmInfo won't do, because
> it lacks the accelerator name.
> 
> If we choose to reuse KvmInfo regardless, it needs to be renamed to
> something like AccelInfo, and the doc comment generalized beyond KVM.
> 

I have renamed it to AccelInfo in another patch in the series :)

> >> > +#
> >> > +# Since: 6.0.0
> >> 
> >> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
> >> although I prefer the shorter form.  Maybe Markus has an opnion on that.
> 
> Yes: use the shorter form, unless .z != .0.
> 
> The shorter form is much more common:
> 
>     $ sed -En 's/.*since:? *([0-9][0-9.]*).*/\1/pi' qapi/*json | sed 's/[^.]//g' | sort | uniq -c
>        1065 .
>         129 ..
> 
> .z != 0 should be rare: the stable branch is for bug fixes, and bug
> fixes rarely require schema changes.  It is: there is just one instance,
> from commit ab9ba614556 (v3.0.0) and 0779afdc897 (v2.12.1).
> 

Thanks, I'll use 6.0 then.

> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> >> > +# <- { "return": { "enabled": true, "present": true } }
> >> > +#
> >> > +##
> >> > +{ 'command': 'query-accel',
> >> > +  'data': { 'name': 'str' },
> >> > +  'returns': 'KvmInfo' }
> >> 
> >> '@name' is undocumented and an open-coded string.  Better would be
> >> requiring 'name' to be one of an enum type.  [...]
> >
> > This seem similar to CPU models, machine types, device types, and
> > backend object types: the set of valid values is derived from the
> > list of subtypes of a QOM type.  We don't duplicate lists of QOM
> > types in the QAPI schema, today.
> 
> Yes.
> 
> > Do we want to duplicate the list of accelerators in the QAPI
> > schema, or should we wait for a generic solution that works for
> > any QOM type?
> 
> There are only a few accelerators, so duplicating them wouldn't be too
> bad.  Still, we need a better reason than "because we can".
> 
> QAPI enum vs. 'str' doesn't affect the wire format: both use JSON
> string.
> 

'str' is quite flexible. If we remove an accel, provided QOM command
won't complain. It'll just reply that the accel is not present :)

> With a QAPI enum, the values available in this QEMU build are visible in
> query-qmp-schema.
> 
> Without a QAPI enum, we need a separate, ad hoc query.  For QOM types,
> we have qom-list-types.
> 
> If you're familiar with qom-list-types, you may want to skip to
> "Conclusion:" now.
> 
> Ad hoc queries can take additional arguments.  qom-list-types does:
> "implements" and "abstract" limit output.  Default is "all
> non-abstract".
> 
> This provides a way to find accelerators: use "implements": "accel" to
> return only concrete subtypes of "accel":
> 
>    ---> {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
>    <--- {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": "tcg-accel", "parent": "accel"}, {"name": "xen-accel", "parent": "accel"}, {"name": "kvm-accel", "parent": "accel"}, {"name": "accel", "parent": "object"}]}
> 
> Aside: the reply includes "accel", because it's not abstract.  Bug?
> 
> Same for CPU models ("implements": "cpu"), machine types ("implements":
> "machine"), device types ("implements": "device").  Not for backend
> object types, because they don't have a common base type.  Certain kinds
> of backend object types do, e.g. character device backends are subtypes
> of "chardev".
> 
> Ad hoc queries can provide additional information.  qom-list-types does:
> the parent type.
> 
> This provides a second way to find subtypes, which might be more
> efficient when you need to know the subtypes of T for many T:
> 
>    Get *all* QOM types in one go:
> 
>    ---> {"execute": "qom-list-types", "arguments": {"abstract": false}}
>    <--- lots of output
> 
>    Use output member "parent" to construct the parent tree.  The
>    sub-tree rooted at QOM type "accel" has the subtypes of "accel".
> 
>    Awkward: since output lacks an "abstract" member, we have to
>    determine it indirectly, by getting just the concrete types:
> 
>    ---> {"execute": "qom-list-types", "arguments": {"abstract": true}}
>    <--- slightly less output
> 
>    We can add "abstract" to the output if we care.
> 
>    Unlike the first way, this one works *only* for "is subtype of",
>    *not* for "implements interface".
> 
>    We can add interface information to the output if we care.
> 
> Likewise, QOM properties are not in the QAPI schema, and we need ad hoc
> queries instead of query-qmp-schema: qom-list-properties, qom-list,
> device-list-properties.  Flaws include inadequate type information and
> difficulties around dynamic properties.
> 
> Conclusion: as is, QOM is designed to defeat our general QAPI/QMP
> introspection mechanism.  It provides its own instead.  Proper
> integration of QOM and QAPI/QMP would be great.  Integrating small parts
> in ways that do not get us closer to complete integration does not seem
> worthwhile.
> 
> For some QOM types, we have additional ad hoc queries that provide more
> information, e.g. query-machines, query-cpu-definitions, and now the
> proposed query-accel.
> 
> query-accel is *only* necessary if its additional information is.
> 

Thanks for the profound answer!

I'm not an expert of QOM/QAPI. Please correct me if my understanding is
wrong.

1. We can use QOM capabilities in QMP to get all accelerators:

C: {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
S: {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": "tcg-accel", "parent": "accel"}, {"name": "hax-accel", "parent": "accel"}, {"name": "hvf-accel", "parent": "accel"}, {"name": "accel", "parent": "object"}]}

The response answers if an accelerator was compiled with current QEMU
binary. All accelerators outside of the list aren't present.

2. We can't figure out if an accel is actually enabled without relying
on ad-hoc query-accel because there are no means for that in QOM.
I might be wrong here but I couldn't find a way to list fields of an
accel class using qom-list and qom-get.

I have no particular preference of query-accel vs wiring current accel
to QOM to be able to find it unless the latter one takes x10 time to
implement and rework everything. But I need some understanding of what
would be preferred way for everyone :)

> I unde
> 
> >>                                       [...]  Even better would be
> >> returning an array of KvmInfo with information on all supported
> >> accelerators at once, rather than making the user call this command once
> >> per name.
> >
> > Maybe.  It would save us the work of answering the question
> > above, but is this (querying information for all accelerators at
> > once) going to be a common use case?
> 
> I recommend to scratch the query-accel parameter, and return information
> on all accelerators in this build of QEMU.  Simple, and consistent with
> similar ad hoc queries.  If a client is interested in just one, fishing
> it out of the list is easy enough.
> 

Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
with two fields: name and enabled. enabled will be true only for
currently active accelerator.

Thanks,
Roman

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Paolo, there's a question for you further down, marked with your name.

Roman Bolshakov <r.bolshakov@yadro.com> writes:

> On Tue, Nov 17, 2020 at 09:51:58AM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Mon, Nov 16, 2020 at 10:20:04AM -0600, Eric Blake wrote:
>> >> On 11/16/20 7:10 AM, Roman Bolshakov wrote:
>> >> > There's a problem for management applications to determine if certain
>> >> > accelerators available. Generic QMP command should help with that.
>> >> > 
>> >> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> >> > ---
>> >> >  monitor/qmp-cmds.c | 15 +++++++++++++++
>> >> >  qapi/machine.json  | 19 +++++++++++++++++++
>> >> >  2 files changed, 34 insertions(+)
>> >> > 
>> >> 
>> >> > +++ b/qapi/machine.json
>> >> > @@ -591,6 +591,25 @@
>> >> >  ##
>> >> >  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
>> >> >  
>> >> > +##
>> >> > +# @query-accel:
>> >> > +#
>> >> > +# Returns information about an accelerator
>> >> > +#
>> >> > +# Returns: @KvmInfo
>> 
>> Is reusing KvmInfo here is good idea?  Recall:
>> 
>>     ##
>>     # @KvmInfo:
>>     #
>>     # Information about support for KVM acceleration
>>     #
>>     # @enabled: true if KVM acceleration is active
>>     #
>>     # @present: true if KVM acceleration is built into this executable
>>     #
>>     # Since: 0.14.0
>>     ##
>>     { 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>> 
>> I figure @present will always be true with query-accel.  In other words,
>> it's useless noise.
>> 
>
> Hi Markus,
>
> Actually, no. Provided implementation returns present = true only if we
> can find the accel in QOM, i.e. on macOS we get present = false for kvm.
> And on Linux we get present = false for hvf, e.g.:
>
> C: {"execute": "query-accel", "arguments": {"name": "hvf"}}
> S: {"return": {"enabled": true, "present": true}}
> C: {"execute": "query-accel", "arguments": {"name": "kvm"}}
> S: {"return": {"enabled": false, "present": false}}
> C: {"execute": "query-accel", "arguments": {"name": "tcg"}}
> S: {"return": {"enabled": false, "present": true}}

Aha.  I expected query-accel to fail when the named accelerator does not
exist.  Has two advantages:

* Same behavior for "does not exist because it's not compiled in" and
  for "does not exist because it was added only to a future version of
  QEMU'".  Easier for QMP clients: they get to handle one kind of
  failure rather than two.

* If we later change 'name' from 'str' to an enum, behavior stays the
  same.

Compelling enough, don't you think?

>> If we return information on all accelerators, KvmInfo won't do, because
>> it lacks the accelerator name.
>> 
>> If we choose to reuse KvmInfo regardless, it needs to be renamed to
>> something like AccelInfo, and the doc comment generalized beyond KVM.
>> 
>
> I have renamed it to AccelInfo in another patch in the series :)

I noticed only after I sent my reply :)

>> >> > +#
>> >> > +# Since: 6.0.0
>> >> 
>> >> We're inconsistent on whether we have 'Since: x.y' or 'Since: x.y.z',
>> >> although I prefer the shorter form.  Maybe Markus has an opnion on that.
>> 
>> Yes: use the shorter form, unless .z != .0.
>> 
>> The shorter form is much more common:
>> 
>>     $ sed -En 's/.*since:? *([0-9][0-9.]*).*/\1/pi' qapi/*json | sed 's/[^.]//g' | sort | uniq -c
>>        1065 .
>>         129 ..
>> 
>> .z != 0 should be rare: the stable branch is for bug fixes, and bug
>> fixes rarely require schema changes.  It is: there is just one instance,
>> from commit ab9ba614556 (v3.0.0) and 0779afdc897 (v2.12.1).
>> 
>
> Thanks, I'll use 6.0 then.
>
>> >> > +#
>> >> > +# Example:
>> >> > +#
>> >> > +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
>> >> > +# <- { "return": { "enabled": true, "present": true } }
>> >> > +#
>> >> > +##
>> >> > +{ 'command': 'query-accel',
>> >> > +  'data': { 'name': 'str' },
>> >> > +  'returns': 'KvmInfo' }
>> >> 
>> >> '@name' is undocumented and an open-coded string.  Better would be
>> >> requiring 'name' to be one of an enum type.  [...]
>> >
>> > This seem similar to CPU models, machine types, device types, and
>> > backend object types: the set of valid values is derived from the
>> > list of subtypes of a QOM type.  We don't duplicate lists of QOM
>> > types in the QAPI schema, today.
>> 
>> Yes.
>> 
>> > Do we want to duplicate the list of accelerators in the QAPI
>> > schema, or should we wait for a generic solution that works for
>> > any QOM type?
>> 
>> There are only a few accelerators, so duplicating them wouldn't be too
>> bad.  Still, we need a better reason than "because we can".
>> 
>> QAPI enum vs. 'str' doesn't affect the wire format: both use JSON
>> string.
>> 
>
> 'str' is quite flexible. If we remove an accel, provided QOM command
> won't complain. It'll just reply that the accel is not present :)
>
>> With a QAPI enum, the values available in this QEMU build are visible in
>> query-qmp-schema.
>> 
>> Without a QAPI enum, we need a separate, ad hoc query.  For QOM types,
>> we have qom-list-types.
>> 
>> If you're familiar with qom-list-types, you may want to skip to
>> "Conclusion:" now.
>> 
>> Ad hoc queries can take additional arguments.  qom-list-types does:
>> "implements" and "abstract" limit output.  Default is "all
>> non-abstract".
>> 
>> This provides a way to find accelerators: use "implements": "accel" to
>> return only concrete subtypes of "accel":
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
>>    <--- {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": "tcg-accel", "parent": "accel"}, {"name": "xen-accel", "parent": "accel"}, {"name": "kvm-accel", "parent": "accel"}, {"name": "accel", "parent": "object"}]}
>> 
>> Aside: the reply includes "accel", because it's not abstract.  Bug?
>> 
>> Same for CPU models ("implements": "cpu"), machine types ("implements":
>> "machine"), device types ("implements": "device").  Not for backend
>> object types, because they don't have a common base type.  Certain kinds
>> of backend object types do, e.g. character device backends are subtypes
>> of "chardev".
>> 
>> Ad hoc queries can provide additional information.  qom-list-types does:
>> the parent type.
>> 
>> This provides a second way to find subtypes, which might be more
>> efficient when you need to know the subtypes of T for many T:
>> 
>>    Get *all* QOM types in one go:
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"abstract": false}}
>>    <--- lots of output
>> 
>>    Use output member "parent" to construct the parent tree.  The
>>    sub-tree rooted at QOM type "accel" has the subtypes of "accel".
>> 
>>    Awkward: since output lacks an "abstract" member, we have to
>>    determine it indirectly, by getting just the concrete types:
>> 
>>    ---> {"execute": "qom-list-types", "arguments": {"abstract": true}}
>>    <--- slightly less output
>> 
>>    We can add "abstract" to the output if we care.
>> 
>>    Unlike the first way, this one works *only* for "is subtype of",
>>    *not* for "implements interface".
>> 
>>    We can add interface information to the output if we care.
>> 
>> Likewise, QOM properties are not in the QAPI schema, and we need ad hoc
>> queries instead of query-qmp-schema: qom-list-properties, qom-list,
>> device-list-properties.  Flaws include inadequate type information and
>> difficulties around dynamic properties.
>> 
>> Conclusion: as is, QOM is designed to defeat our general QAPI/QMP
>> introspection mechanism.  It provides its own instead.  Proper
>> integration of QOM and QAPI/QMP would be great.  Integrating small parts
>> in ways that do not get us closer to complete integration does not seem
>> worthwhile.
>> 
>> For some QOM types, we have additional ad hoc queries that provide more
>> information, e.g. query-machines, query-cpu-definitions, and now the
>> proposed query-accel.
>> 
>> query-accel is *only* necessary if its additional information is.
>> 
>
> Thanks for the profound answer!

You're welcome!

> I'm not an expert of QOM/QAPI. Please correct me if my understanding is
> wrong.
>
> 1. We can use QOM capabilities in QMP to get all accelerators:
>
> C: {"execute": "qom-list-types", "arguments": {"implements": "accel"}}
> S: {"return": [{"name": "qtest-accel", "parent": "accel"}, {"name": "tcg-accel", "parent": "accel"}, {"name": "hax-accel", "parent": "accel"}, {"name": "hvf-accel", "parent": "accel"}, {"name": "accel", "parent": "object"}]}
>
> The response answers if an accelerator was compiled with current QEMU
> binary. All accelerators outside of the list aren't present.

Correct.

> 2. We can't figure out if an accel is actually enabled without relying
> on ad-hoc query-accel because there are no means for that in QOM.
> I might be wrong here but I couldn't find a way to list fields of an
> accel class using qom-list and qom-get.

I have to confess I find the code confusing.

The part that counts is do_configure_accelerator().  I works as follows:

1. Look up the chosen accelerator's QOM type (can fail)
2. Instantiate it (can't fail)
3. Set properties (can fail)
4. Connect the accelerator to the current machine (can fail)

Aside: step 3 uses &error_fatal, unlike the other failures.  Fishy.

Failure in step 1 is "accelerator not compiled in".  Predictable with
qom-list-types.

Failure in step 3 doesn't look predictable.

Failure in step 4 can be due to kernel lacking (a workable version of)
KVM, but the current machine gets a say, too.  Also doesn't look
predictable.

Aside: kvm_init() reports errors with fprintf(), tsk, tsk, tsk.

Existing query-kvm doesn't really predict failure, either.  'present' is
true if CONFIG_KVM.  Should be equivalent to "QOM type exists",
i.e. step 1.  I guess 'enabled' is true when the KVM accelerator is in
use.

While figuring this out, I noticed that the TYPE_ACCEL instance we
create doesn't get its parent set.  It's therefore not in the QOM
composition tree, and inaccessible with qom-get.  Paolo, is this as it
should be?

> I have no particular preference of query-accel vs wiring current accel
> to QOM to be able to find it unless the latter one takes x10 time to
> implement and rework everything. But I need some understanding of what
> would be preferred way for everyone :)

If all you want is figuring out which accelerators this particular QEMU
build has, use qom-list-types.

If you have a compelling use case for predicting which accelerators can
actually work, and nobody has clever ideas on how to do that with
existing commands, then an ad hoc query-accel seems the way to go.

[...]
>> >>                                       [...]  Even better would be
>> >> returning an array of KvmInfo with information on all supported
>> >> accelerators at once, rather than making the user call this command once
>> >> per name.
>> >
>> > Maybe.  It would save us the work of answering the question
>> > above, but is this (querying information for all accelerators at
>> > once) going to be a common use case?
>> 
>> I recommend to scratch the query-accel parameter, and return information
>> on all accelerators in this build of QEMU.  Simple, and consistent with
>> similar ad hoc queries.  If a client is interested in just one, fishing
>> it out of the list is easy enough.
>> 
>
> Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
> with two fields: name and enabled. enabled will be true only for
> currently active accelerator.

Please document that at most on result can have 'enabled': true.

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Paolo Bonzini]

On 18/11/20 09:36, Markus Armbruster wrote:
> 
> The part that counts is do_configure_accelerator().  I works as follows:
> 
> 1. Look up the chosen accelerator's QOM type (can fail)
> 2. Instantiate it (can't fail)
> 3. Set properties (can fail)
> 4. Connect the accelerator to the current machine (can fail)
> 
> Aside: step 3 uses &error_fatal, unlike the other failures.  Fishy.

That's because step 3 is a user error, unlike (in the usual case) the 
others:

1. You get it from "-accel whpx" if whpx is not available; this is not a 
user error if there is a fallback.  You also get it from misspellings 
such as "-accel kvmm", which is presumably a user error, but it's hard 
to distinguish the two especially if a future version of QEMU ends up 
adding a "kvmm" accelerator

3. You get it from "-accel tcg,misspeled-property=off".  This is a user 
error.  You also get it from "-accel tcg,absent-property=on" and "-accel 
tcg,invalid-value=42".  This may be a property that the user wants to 
set but was only added in a future version of QEMU.  Either way, it's 
quite likely that the user does _not_ want a fallback here.

4. Here the accelerator exists but the machine does not support it.  The 
choice is to fallback to the next accelerato.

This means there is no way for the user to distinguish "the accelerator 
doesn't exist" from "the accelerator exists but is not supported".  This 
was done for no particular reason other than to keep the code simple.

> Failure in step 1 is "accelerator not compiled in".  Predictable with
> qom-list-types.
> 
> Failure in step 3 doesn't look predictable.

It's more or less predictable with qom-list-properties, though of course 
not the "invalid value for the property" case.

> Failure in step 4 can be due to kernel lacking (a workable version of)
> KVM, but the current machine gets a say, too.  Also doesn't look
> predictable.
> 
> Aside: kvm_init() reports errors with fprintf(), tsk, tsk, tsk.
> 
> Existing query-kvm doesn't really predict failure, either.  'present' is
> true if CONFIG_KVM.  Should be equivalent to "QOM type exists",
> i.e. step 1.  I guess 'enabled' is true when the KVM accelerator is in
> use.
> 
> While figuring this out, I noticed that the TYPE_ACCEL instance we
> create doesn't get its parent set.  It's therefore not in the QOM
> composition tree, and inaccessible with qom-get.  Paolo, is this as it
> should be?

It should be added, I agree, perhaps as /machine/accel.

Paolo

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Kevin Wolf]

Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben:
> >> >>                                       [...]  Even better would be
> >> >> returning an array of KvmInfo with information on all supported
> >> >> accelerators at once, rather than making the user call this command once
> >> >> per name.
> >> >
> >> > Maybe.  It would save us the work of answering the question
> >> > above, but is this (querying information for all accelerators at
> >> > once) going to be a common use case?
> >> 
> >> I recommend to scratch the query-accel parameter, and return information
> >> on all accelerators in this build of QEMU.  Simple, and consistent with
> >> similar ad hoc queries.  If a client is interested in just one, fishing
> >> it out of the list is easy enough.
> >> 
> >
> > Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
> > with two fields: name and enabled. enabled will be true only for
> > currently active accelerator.
> 
> Please document that at most on result can have 'enabled': true.

This doesn't feel right.

If I understand right, the proposal is to get a result like this:

    [ { 'name': 'kvm', 'enabled': true },
      { 'name': 'tcg', 'enabled': false },
      { 'name': 'xen', 'enabled': false } ]

The condition that only one field can be enabled would only be in the
documentation instead of being enforced.

Instead, consider a response like this:

    { 'available': [ 'kvm', 'tcg', 'xen' ],
      'active': 'kvm' }

This is not only shorter, but also makes sure that only one accelerator
can be reported as active.

Kevin

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Daniel P. Berrangé]

On Wed, Nov 18, 2020 at 12:28:45PM +0100, Kevin Wolf wrote:
> Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben:
> > >> >>                                       [...]  Even better would be
> > >> >> returning an array of KvmInfo with information on all supported
> > >> >> accelerators at once, rather than making the user call this command once
> > >> >> per name.
> > >> >
> > >> > Maybe.  It would save us the work of answering the question
> > >> > above, but is this (querying information for all accelerators at
> > >> > once) going to be a common use case?
> > >> 
> > >> I recommend to scratch the query-accel parameter, and return information
> > >> on all accelerators in this build of QEMU.  Simple, and consistent with
> > >> similar ad hoc queries.  If a client is interested in just one, fishing
> > >> it out of the list is easy enough.
> > >> 
> > >
> > > Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
> > > with two fields: name and enabled. enabled will be true only for
> > > currently active accelerator.
> > 
> > Please document that at most on result can have 'enabled': true.
> 
> This doesn't feel right.
> 
> If I understand right, the proposal is to get a result like this:
> 
>     [ { 'name': 'kvm', 'enabled': true },
>       { 'name': 'tcg', 'enabled': false },
>       { 'name': 'xen', 'enabled': false } ]
> 
> The condition that only one field can be enabled would only be in the
> documentation instead of being enforced.
> 
> Instead, consider a response like this:
> 
>     { 'available': [ 'kvm', 'tcg', 'xen' ],
>       'active': 'kvm' }
> 
> This is not only shorter, but also makes sure that only one accelerator
> can be reported as active.

I guess this can be extended with a union to report extra props for the
accelerator, discriminated on the 'active' field eg

     { 'available': [ 'kvm', 'tcg', 'xen' ],
       'active': 'kvm',
       'data': {
           "allow-nested": true,
       }
    }


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/11/20 09:36, Markus Armbruster wrote:
>> 
>> The part that counts is do_configure_accelerator().  I works as follows:
>> 
>> 1. Look up the chosen accelerator's QOM type (can fail)
>> 2. Instantiate it (can't fail)
>> 3. Set properties (can fail)
>> 4. Connect the accelerator to the current machine (can fail)
>> 
>> Aside: step 3 uses &error_fatal, unlike the other failures.  Fishy.
>
> That's because step 3 is a user error, unlike (in the usual case) the 
> others:
>
> 1. You get it from "-accel whpx" if whpx is not available; this is not a 
> user error if there is a fallback.  You also get it from misspellings 
> such as "-accel kvmm", which is presumably a user error, but it's hard 
> to distinguish the two especially if a future version of QEMU ends up 
> adding a "kvmm" accelerator
>
> 3. You get it from "-accel tcg,misspeled-property=off".  This is a user 
> error.  You also get it from "-accel tcg,absent-property=on" and "-accel 
> tcg,invalid-value=42".  This may be a property that the user wants to 
> set but was only added in a future version of QEMU.  Either way, it's 
> quite likely that the user does _not_ want a fallback here.
>
> 4. Here the accelerator exists but the machine does not support it.  The 
> choice is to fallback to the next accelerato.
>
> This means there is no way for the user to distinguish "the accelerator 
> doesn't exist" from "the accelerator exists but is not supported".  This 
> was done for no particular reason other than to keep the code simple.

The resulting error reporting is perhaps not as clear as it could be.

We report several kinds of errors:

(a) Accelerator misconfiguration, immediately fatal

(b) Accelerator doesn't work, not fatal (we fall back to the next one)

(c) "no accelerator found", fatal

(d) "falling back to", not fatal (we carry on)

Reporting (b) as error is questionable, because it need not be an actual
error.

We report (d) when an accelerator works after other(s) didn't.  This is
not actually an error.

Example:

    $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm
    xencall: error: Could not obtain handle on privileged command interface: No such file or directory
    xen be core: xen be core: can't open xen interface
    can't open xen interface

So far, this is just libxen* and accel/xen/xen-all.c being loquacious.

    qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted
    qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent
    qemu-system-x86_64: falling back to KVM

These look like errors, but aren't; things are working exactly as
intended, and QEMU runs.  If we want to be chatty about it, we should
make them info, not error.

Note that a nonsensical accelerator is treated just like an accelerator
that exists, but happens to be unavailable.  Trap for less than perfect
typists.

Same with /dev/kvm made inaccessible:

    $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm
    [Xen chatter...]
    qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted
    qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent
    Could not access KVM kernel module: Permission denied
    qemu-system-x86_64: -accel kvm: failed to initialize kvm: Permission denied

Here, we do have a fatal error.  We report it as four errors.
Tolerable.

If we turn the maybe-not-really-errors into info to make the first
example less confusing, we need to report a "no accelerator works" error
at the end.

>> Failure in step 1 is "accelerator not compiled in".  Predictable with
>> qom-list-types.
>> 
>> Failure in step 3 doesn't look predictable.
>
> It's more or less predictable with qom-list-properties, though of course 
> not the "invalid value for the property" case.
>
>> Failure in step 4 can be due to kernel lacking (a workable version of)
>> KVM, but the current machine gets a say, too.  Also doesn't look
>> predictable.
>> 
>> Aside: kvm_init() reports errors with fprintf(), tsk, tsk, tsk.
>> 
>> Existing query-kvm doesn't really predict failure, either.  'present' is
>> true if CONFIG_KVM.  Should be equivalent to "QOM type exists",
>> i.e. step 1.  I guess 'enabled' is true when the KVM accelerator is in
>> use.
>> 
>> While figuring this out, I noticed that the TYPE_ACCEL instance we
>> create doesn't get its parent set.  It's therefore not in the QOM
>> composition tree, and inaccessible with qom-get.  Paolo, is this as it
>> should be?
>
> It should be added, I agree, perhaps as /machine/accel.

Makes sense.

Thanks!

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Paolo Bonzini]

On 18/11/20 14:08, Markus Armbruster wrote:
> These look like errors, but aren't; things are working exactly as
> intended, and QEMU runs.  If we want to be chatty about it, we should
> make them info, not error.

If there were an info_report, I would have sent a patch already. :)  In 
general, these are probably not the only cases where error_report is 
used as a fancy fprintf(stderr), rather than to report actual errors.

Paolo

> Same with /dev/kvm made inaccessible:
> 
>      $ qemu-system-x86_64 -accel xen -S -accel nonexistent -accel kvm
>      [Xen chatter...]
>      qemu-system-x86_64: -accel xen: failed to initialize xen: Operation not permitted
>      qemu-system-x86_64: -accel nonexistent: invalid accelerator nonexistent
>      Could not access KVM kernel module: Permission denied
>      qemu-system-x86_64: -accel kvm: failed to initialize kvm: Permission denied
> 
> Here, we do have a fatal error.  We report it as four errors.
> Tolerable.
> 
> If we turn the maybe-not-really-errors into info to make the first
> example less confusing, we need to report a "no accelerator works" error
> at the end.

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Nov 18, 2020 at 12:28:45PM +0100, Kevin Wolf wrote:
>> Am 18.11.2020 um 09:36 hat Markus Armbruster geschrieben:
>> > >> >>                                       [...]  Even better would be
>> > >> >> returning an array of KvmInfo with information on all supported
>> > >> >> accelerators at once, rather than making the user call this command once
>> > >> >> per name.
>> > >> >
>> > >> > Maybe.  It would save us the work of answering the question
>> > >> > above, but is this (querying information for all accelerators at
>> > >> > once) going to be a common use case?
>> > >> 
>> > >> I recommend to scratch the query-accel parameter, and return information
>> > >> on all accelerators in this build of QEMU.  Simple, and consistent with
>> > >> similar ad hoc queries.  If a client is interested in just one, fishing
>> > >> it out of the list is easy enough.
>> > >> 
>> > >
>> > > Works for me. I'll then leave KvmInfo as is and will introduce AccelInfo
>> > > with two fields: name and enabled. enabled will be true only for
>> > > currently active accelerator.
>> > 
>> > Please document that at most on result can have 'enabled': true.
>> 
>> This doesn't feel right.
>> 
>> If I understand right, the proposal is to get a result like this:
>> 
>>     [ { 'name': 'kvm', 'enabled': true },
>>       { 'name': 'tcg', 'enabled': false },
>>       { 'name': 'xen', 'enabled': false } ]
>> 
>> The condition that only one field can be enabled would only be in the
>> documentation instead of being enforced.
>> 
>> Instead, consider a response like this:
>> 
>>     { 'available': [ 'kvm', 'tcg', 'xen' ],
>>       'active': 'kvm' }
>> 
>> This is not only shorter, but also makes sure that only one accelerator
>> can be reported as active.

Better.

Documentation only, not enforced: no duplicate array elements.  We got
that elsewhere already (arrays used as sets, basically).

If we want to provide for reporting additional information on available
accelerators, tweak it to

      {"available": [{"name": "kvm"}, {"name": "tcg"}, {"name": "xen"}],
       "active": "kvm"}

If accelerator-specific extra information is needed, the array elements
can compatibly evolve into flat unions.

Another way to skin this cat:

      {"available": {"kvm": { extra properties... },
                     "tcg": ...,
                     "xen": ...},
       "active": "kvm"}

No need for unions then.  "No dupes" is enforced.

We could inline "available":

      {"kvm": { extra properties... },
       "tcg": ...,
       "xen": ...,
       "active": "kvm"}

Future accelerators can't be named "active" then.

> I guess this can be extended with a union to report extra props for the
> accelerator, discriminated on the 'active' field eg
>
>      { 'available': [ 'kvm', 'tcg', 'xen' ],
>        'active': 'kvm',
>        'data': {
>            "allow-nested": true,
>        }
>     }

Correct.

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Roman Bolshakov]

On Wed, Nov 18, 2020 at 02:08:21PM +0100, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> > On 18/11/20 09:36, Markus Armbruster wrote:
> >> While figuring this out, I noticed that the TYPE_ACCEL instance we
> >> create doesn't get its parent set.  It's therefore not in the QOM
> >> composition tree, and inaccessible with qom-get.  Paolo, is this as it
> >> should be?
> >
> > It should be added, I agree, perhaps as /machine/accel.
> 
> Makes sense.
> 

I also tried to find it there when traversed QOM :)

So, I'll then add it instead of ad-hoc queries in v2?

That'd allow us to use standard QOM queries to determine all built-in
accelerators and query actually enabled one.

Thanks,
Roman

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/11/20 14:08, Markus Armbruster wrote:
>> These look like errors, but aren't; things are working exactly as
>> intended, and QEMU runs.  If we want to be chatty about it, we should
>> make them info, not error.
>
> If there were an info_report, I would have sent a patch already. :)

Commit 97f40301f1 "error: Functions to report warnings and informational
messages", 2017-07-13 :)

> In general, these are probably not the only cases where error_report
> is used as a fancy fprintf(stderr), rather than to report actual
> errors.

True!

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Paolo Bonzini]

On 18/11/20 15:45, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 18/11/20 14:08, Markus Armbruster wrote:
>>> These look like errors, but aren't; things are working exactly as
>>> intended, and QEMU runs.  If we want to be chatty about it, we should
>>> make them info, not error.
>>
>> If there were an info_report, I would have sent a patch already. :)
> 
> Commit 97f40301f1 "error: Functions to report warnings and informational
> messages", 2017-07-13 :)

Doh, I just learnt about info_report.  It never occurred to me until now 
that without a warning or info marker it would be an error.  I can see 
though why you didn't add "error" automatically for REPORT_TYPE_ERROR, 
while leaving REPORT_TYPE_INFO unadorned.  Between the 
incorrectly-marked errors and probably some "error: error: " it would be 
awful.

Paolo

>> In general, these are probably not the only cases where error_report
>> is used as a fancy fprintf(stderr), rather than to report actual
>> errors.
> 
> True!
> 

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Eduardo Habkost]

On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
[...]
> Another way to skin this cat:
> 
>       {"available": {"kvm": { extra properties... },
>                      "tcg": ...,
>                      "xen": ...},
>        "active": "kvm"}

How would this structure be represented in the QAPI schema?

In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?

> 
> No need for unions then.  "No dupes" is enforced.
> 
> We could inline "available":
> 
>       {"kvm": { extra properties... },
>        "tcg": ...,
>        "xen": ...,
>        "active": "kvm"}
> 
> Future accelerators can't be named "active" then.
> 
> > I guess this can be extended with a union to report extra props for the
> > accelerator, discriminated on the 'active' field eg
> >
> >      { 'available': [ 'kvm', 'tcg', 'xen' ],
> >        'active': 'kvm',
> >        'data': {
> >            "allow-nested": true,
> >        }
> >     }
> 
> Correct.

-- 
Eduardo

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Eric Blake]

On 11/18/20 9:45 AM, Eduardo Habkost wrote:
> On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
> [...]
>> Another way to skin this cat:
>>
>>       {"available": {"kvm": { extra properties... },
>>                      "tcg": ...,
>>                      "xen": ...},
>>        "active": "kvm"}
> 
> How would this structure be represented in the QAPI schema?
> 
> In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?

{'type':'AvailAccel', 'data': {
  '*kvm': 'KvmExtraProps',
  '*tcg': 'TcgExtraProps',
  '*xen': 'XenExtraProps',
  '*hax': 'HaxExtraProps' } }
{'command':'query-accel', 'returns': {
   'available': 'AvailAccel', 'active': 'strOrEnum' } }

where adding a new accelerator then adds a new optional member to
AvailAccel as well as possibly a new enum member if 'active' is driving
by an enum instead of 'str'.

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

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Eduardo Habkost]

On Wed, Nov 18, 2020 at 09:56:28AM -0600, Eric Blake wrote:
> On 11/18/20 9:45 AM, Eduardo Habkost wrote:
> > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
> > [...]
> >> Another way to skin this cat:
> >>
> >>       {"available": {"kvm": { extra properties... },
> >>                      "tcg": ...,
> >>                      "xen": ...},
> >>        "active": "kvm"}
> > 
> > How would this structure be represented in the QAPI schema?
> > 
> > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?
> 
> {'type':'AvailAccel', 'data': {
>   '*kvm': 'KvmExtraProps',
>   '*tcg': 'TcgExtraProps',
>   '*xen': 'XenExtraProps',
>   '*hax': 'HaxExtraProps' } }
> {'command':'query-accel', 'returns': {
>    'available': 'AvailAccel', 'active': 'strOrEnum' } }
> 
> where adding a new accelerator then adds a new optional member to
> AvailAccel as well as possibly a new enum member if 'active' is driving
> by an enum instead of 'str'.

Is it possible to represent this if we don't enumerate all
possible dictionary keys in advance?  (I'm not suggesting we
should/shouldn't do that, just wondering if it's possible)

-- 
Eduardo

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Nov 18, 2020 at 09:56:28AM -0600, Eric Blake wrote:
>> On 11/18/20 9:45 AM, Eduardo Habkost wrote:
>> > On Wed, Nov 18, 2020 at 02:53:26PM +0100, Markus Armbruster wrote:
>> > [...]
>> >> Another way to skin this cat:
>> >>
>> >>       {"available": {"kvm": { extra properties... },
>> >>                      "tcg": ...,
>> >>                      "xen": ...},
>> >>        "active": "kvm"}
>> > 
>> > How would this structure be represented in the QAPI schema?
>> > 
>> > In other words, how do I say "Dict[str, AccelInfo]" in QAPIese?
>> 
>> {'type':'AvailAccel', 'data': {
>>   '*kvm': 'KvmExtraProps',
>>   '*tcg': 'TcgExtraProps',
>>   '*xen': 'XenExtraProps',
>>   '*hax': 'HaxExtraProps' } }
>> {'command':'query-accel', 'returns': {
>>    'available': 'AvailAccel', 'active': 'strOrEnum' } }
>> 
>> where adding a new accelerator then adds a new optional member to
>> AvailAccel as well as possibly a new enum member if 'active' is driving
>> by an enum instead of 'str'.
>
> Is it possible to represent this if we don't enumerate all
> possible dictionary keys in advance?  (I'm not suggesting we
> should/shouldn't do that, just wondering if it's possible)

Mostly no.

The definition of a complex type (struct or union) specifies all
members.  There is no way to say "and whatever else may be there".

We actually have such types anyway.  Consider command device_add: it
takes arguments 'driver', 'bus', 'str', and properties.  Its arguments
type is "struct of driver, bus, str, and whatever else may be there".

Since the schema language can't express this, we cheat:

    { 'command': 'device_add',
      'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
      'gen': false } # so we can get the additional arguments

With 'gen': false, 'data' is at best a statement of intent.  In this
case, it's correct, just incomplete[*].

Introspection takes 'data' at face value.  It's exactly as accurate as
'data' is.

We could extend the schema language so we can say

    { 'command': 'device_add',
      'data': {'driver': 'str', '*bus': 'str', '*id': 'str',
               '**props': 'dict'}

where 'props' receives any remaining arguments.  Fairly common language
feature, e.g. &rest in Lisp, ** in Python, ...

Removed the need for 'gen': false, and enables more accurate
introspection.

Type 'dict' doesn't exist, yet.  I think it could.  We got 'any'
already.


[*] There have been uses of 'gen': false where 'data' was actually
wrong.  For an example, see commit b8a98326d5 "qapi-schema: Fix up
misleading specification of netdev_add".

Re: [PATCH for-6.0 0/6] Add HMP/QMP commands to query accelerator

[Claudio Fontana]

Ciao Roman,

On 11/16/20 2:10 PM, Roman Bolshakov wrote:
> Hi,
> 
> Management applications have no way to determine if certain accelerator
> is available. That complicates discovery of non-KVM accelerators.

are we thinking about how to make this future-proof when it comes to modularization efforts,
ie, when we get to fully modularized accelerator plugins?

Maybe too soon to consider, but still worth mentioning on my side I think.

Ciao,

Claudio

> 
> To address the issue, the series adds two commands:
> 
>   'query-accel' for QMP to be used by management apps, and
> 
>   'info accel' for HMP to replace 'info kvm' in future.
> 
> Thanks,
> Roman
> 
> Roman Bolshakov (6):
>   qapi: Add query-accel command
>   qapi: Rename KvmInfo to AccelInfo
>   qapi: Use qmp_query_accel() in qmp_query_kvm()
>   softmmu: Remove kvm_available()
>   hmp: Add 'info accel' command
>   qapi: Deprecate 'query-kvm'
> 
>  hmp-commands-info.hx       | 13 +++++++++++++
>  include/monitor/hmp.h      |  1 +
>  include/sysemu/arch_init.h |  1 -
>  monitor/hmp-cmds.c         | 36 ++++++++++++++++++++++++++++++++++--
>  monitor/qmp-cmds.c         | 18 ++++++++++++++----
>  qapi/machine.json          | 37 ++++++++++++++++++++++++++++++-------
>  softmmu/arch_init.c        |  9 ---------
>  7 files changed, 92 insertions(+), 23 deletions(-)
> 

Re: [PATCH for-6.0 0/6] Add HMP/QMP commands to query accelerator

[Roman Bolshakov]

On Thu, Nov 19, 2020 at 03:41:53PM +0100, Claudio Fontana wrote:
> On 11/16/20 2:10 PM, Roman Bolshakov wrote:
> > Management applications have no way to determine if certain accelerator
> > is available. That complicates discovery of non-KVM accelerators.
> 
> are we thinking about how to make this future-proof when it comes to
> modularization efforts, ie, when we get to fully modularized
> accelerator plugins?
> 
> Maybe too soon to consider, but still worth mentioning on my side I think.
> 

Hi Claudio,

I'd be happy to do it future-proof if you have something on the mind.
As far as I understand from the discussion, if we have /machine/accel
container, we can use QOM to query properties of the container including
accel name:
qom-get /machine/accel/type

Thanks,
Roman

Re: [PATCH for-6.0 0/6] Add HMP/QMP commands to query accelerator

[Claudio Fontana]

On 11/19/20 4:46 PM, Roman Bolshakov wrote:
> On Thu, Nov 19, 2020 at 03:41:53PM +0100, Claudio Fontana wrote:
>> On 11/16/20 2:10 PM, Roman Bolshakov wrote:
>>> Management applications have no way to determine if certain accelerator
>>> is available. That complicates discovery of non-KVM accelerators.
>>
>> are we thinking about how to make this future-proof when it comes to
>> modularization efforts, ie, when we get to fully modularized
>> accelerator plugins?
>>
>> Maybe too soon to consider, but still worth mentioning on my side I think.
>>
> 
> Hi Claudio,
> 
> I'd be happy to do it future-proof if you have something on the mind.
> As far as I understand from the discussion, if we have /machine/accel
> container, we can use QOM to query properties of the container including
> accel name:
> qom-get /machine/accel/type
> 
> Thanks,
> Roman
> 

My understanding is very limited here, especially when it comes then to how libvirt f.e. uses this,

I wonder how to make sure that libvirt does not query the currently selected accelerator "too early",
ie before it has been finally selected (what in the other series I called the INIT_ACCEL_CPU time),

and how to query "available/supported" accelerators in a way that accounts for the fact that the plugin might not be loaded or available.

I guess this is a larger question about how libvirt detects modularized features in QEMU, when those features are (or are not) present only in an external binary plugin.

Ciao,

Claudio

Re: [PATCH for-6.0 5/6] hmp: Add 'info accel' command

[Dr. David Alan Gilbert]

* Roman Bolshakov (r.bolshakov@yadro.com) wrote:
> The command is made after 'info kvm' and aims to replace it as more
> generic one.
> 
> If used without parameters, the command can used to get current
> accelerator. Otherwise, it may be used to determine if an accelerator is
> available. Here's an example if a VM with hvf accel is started:
> 
>   (qemu) info accel
>   hvf support: enabled
>   (qemu) info accel kvm
>   kvm support: not compiled
>   (qemu) info accel tcg
>   tcg support: disabled
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  hmp-commands-info.hx  | 13 +++++++++++++
>  include/monitor/hmp.h |  1 +
>  monitor/hmp-cmds.c    | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 117ba25f91..e9da6b52e4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -337,6 +337,19 @@ SRST
>      Show KVM information.
>  ERST
>  
> +    {
> +        .name       = "accel",
> +        .args_type  = "name:s?",
> +        .params     = "[name]",
> +        .help       = "show accelerator information",
> +        .cmd        = hmp_info_accel,
> +    },
> +
> +SRST
> +  ``info accel``` [*name*]
> +    Show accelerator information.
> +ERST
> +
>      {
>          .name       = "numa",
>          .args_type  = "",
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index ed2913fd18..03f22957d9 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -21,6 +21,7 @@ void hmp_handle_error(Monitor *mon, Error *err);
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>  void hmp_info_kvm(Monitor *mon, const QDict *qdict);
> +void hmp_info_accel(Monitor *mon, const QDict *qdict);
>  void hmp_info_status(Monitor *mon, const QDict *qdict);
>  void hmp_info_uuid(Monitor *mon, const QDict *qdict);
>  void hmp_info_chardev(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ea86289fe8..00db791aa3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -20,6 +20,7 @@
>  #include "chardev/char.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/runstate.h"
> +#include "sysemu/accel.h"
>  #include "qemu/config-file.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
> @@ -133,6 +134,37 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>      qapi_free_AccelInfo(info);
>  }
>  
> +void hmp_info_accel(Monitor *mon, const QDict *qdict)
> +{
> +    AccelInfo *info;
> +    AccelClass *acc;
> +    const char *name, *typename;
> +    char *current_name;
> +    int len;
> +
> +    /* Figure out current accelerator */
> +    acc = ACCEL_GET_CLASS(current_accel());

Is that always guaranteed non-null?

> +    typename = object_class_get_name(OBJECT_CLASS(acc));
> +    len = strlen(typename) - strlen(ACCEL_CLASS_SUFFIX);
> +    current_name = g_strndup(typename, len);
> +
> +    name = qdict_get_try_str(qdict, "name");
> +    if (!name) {
> +        name = current_name;
> +    }
> +
> +    info = qmp_query_accel(name, NULL);
> +    monitor_printf(mon, "%s support: ", name);
> +    if (info->present) {
> +        monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled");
> +    } else {
> +        monitor_printf(mon, "not compiled\n");
> +    }
> +
> +    qapi_free_AccelInfo(info);
> +    g_free(current_name);
> +}

I think that's fine, since HMP is not guaranteed stable, I'd say it's
fine to kill off 'info kvm' and replace it with this.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Dave

> +
>  void hmp_info_status(Monitor *mon, const QDict *qdict)
>  {
>      StatusInfo *info;
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-6.0 2/6] qapi: Rename KvmInfo to AccelInfo

[Dr. David Alan Gilbert]

* Roman Bolshakov (r.bolshakov@yadro.com) wrote:
> There's nothing specific to KVM in the structure. A more generic name
> would be more appropriate.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>

For HMP:

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Markus/Eric: Is it OK from QMP to change the type like that?

Dave

> ---
>  monitor/hmp-cmds.c |  4 ++--
>  monitor/qmp-cmds.c |  8 ++++----
>  qapi/machine.json  | 18 +++++++++---------
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..ea86289fe8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -120,7 +120,7 @@ void hmp_info_version(Monitor *mon, const QDict *qdict)
>  
>  void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>  {
> -    KvmInfo *info;
> +    AccelInfo *info;
>  
>      info = qmp_query_kvm(NULL);
>      monitor_printf(mon, "kvm support: ");
> @@ -130,7 +130,7 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "not compiled\n");
>      }
>  
> -    qapi_free_KvmInfo(info);
> +    qapi_free_AccelInfo(info);
>  }
>  
>  void hmp_info_status(Monitor *mon, const QDict *qdict)
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 0454394e76..f5d50afa9c 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -52,9 +52,9 @@ NameInfo *qmp_query_name(Error **errp)
>      return info;
>  }
>  
> -KvmInfo *qmp_query_kvm(Error **errp)
> +AccelInfo *qmp_query_kvm(Error **errp)
>  {
> -    KvmInfo *info = g_malloc0(sizeof(*info));
> +    AccelInfo *info = g_malloc0(sizeof(*info));
>  
>      info->enabled = kvm_enabled();
>      info->present = kvm_available();
> @@ -62,9 +62,9 @@ KvmInfo *qmp_query_kvm(Error **errp)
>      return info;
>  }
>  
> -KvmInfo *qmp_query_accel(const char *name, Error **errp)
> +AccelInfo *qmp_query_accel(const char *name, Error **errp)
>  {
> -    KvmInfo *info = g_malloc0(sizeof(*info));
> +    AccelInfo *info = g_malloc0(sizeof(*info));
>  
>      AccelClass *ac = accel_find(name);
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 11f364fab4..5648d8d24d 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -562,24 +562,24 @@
>  { 'command': 'inject-nmi' }
>  
>  ##
> -# @KvmInfo:
> +# @AccelInfo:
>  #
> -# Information about support for KVM acceleration
> +# Information about support for an acceleration
>  #
> -# @enabled: true if KVM acceleration is active
> +# @enabled: true if an acceleration is active
>  #
> -# @present: true if KVM acceleration is built into this executable
> +# @present: true if an acceleration is built into this executable
>  #
>  # Since: 0.14.0
>  ##
> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
> +{ 'struct': 'AccelInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>  
>  ##
>  # @query-kvm:
>  #
>  # Returns information about KVM acceleration
>  #
> -# Returns: @KvmInfo
> +# Returns: @AccelInfo
>  #
>  # Since: 0.14.0
>  #
> @@ -589,14 +589,14 @@
>  # <- { "return": { "enabled": true, "present": true } }
>  #
>  ##
> -{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
> +{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
>  
>  ##
>  # @query-accel:
>  #
>  # Returns information about an accelerator
>  #
> -# Returns: @KvmInfo
> +# Returns: @AccelInfo
>  #
>  # Since: 6.0.0
>  #
> @@ -608,7 +608,7 @@
>  ##
>  { 'command': 'query-accel',
>    'data': { 'name': 'str' },
> -  'returns': 'KvmInfo' }
> +  'returns': 'AccelInfo' }
>  
>  ##
>  # @NumaOptionsType:
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Daniel P. Berrangé]

Copying libvir-list for the deprecation warning.


On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> 'query-accel' QMP command should be used instead.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  qapi/machine.json | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

docs/system/deprecated.rst needs to be updated for any deprecation
to be visible to consumers of QEMU.


> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 5648d8d24d..130b0dbebc 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -579,6 +579,9 @@
>  #
>  # Returns information about KVM acceleration
>  #
> +# Features:
> +# @deprecated: This command is deprecated, use 'query-accel' instead.
> +#
>  # Returns: @AccelInfo
>  #
>  # Since: 0.14.0
> @@ -589,7 +592,8 @@
>  # <- { "return": { "enabled": true, "present": true } }
>  #
>  ##
> -{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
> +{ 'command': 'query-kvm', 'returns': 'AccelInfo',
> +  'features': [ 'deprecated' ] }
>  
>  ##
>  # @query-accel:
> -- 
> 2.29.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Peter Krempa]

On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote:
> Copying libvir-list for the deprecation warning.
> 
> 
> On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> > 'query-accel' QMP command should be used instead.
> > 
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> >  qapi/machine.json | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> docs/system/deprecated.rst needs to be updated for any deprecation
> to be visible to consumers of QEMU.

On behalf of libvirt I'd like to ask to consider cases where the
replacement for a deprecated feature is added in the same release as the
deprecation happens, to treat the replacement as API stable at merge
time.

Any changes to the command after the series is merged with a deprecation
of the old should be consulted with the libvirt team as we might
actually have already added support for the new approach. Considering it
as "it wasn't released so we can change it" may introduce breakage given
that the relase cycles of libvirt and qemu are not in sync.

We try hard to stay on top of such changes by using the new interface as
soon as possible, but that is very hard if the replacement changes
during the dev cycle.

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Roman Bolshakov]

On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
> On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote:
> > Copying libvir-list for the deprecation warning.
> > 
> > 
> > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> > > 'query-accel' QMP command should be used instead.
> > > 
> > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > ---
> > >  qapi/machine.json | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > docs/system/deprecated.rst needs to be updated for any deprecation
> > to be visible to consumers of QEMU.
> 
> On behalf of libvirt I'd like to ask to consider cases where the
> replacement for a deprecated feature is added in the same release as the
> deprecation happens, to treat the replacement as API stable at merge
> time.
> 
> Any changes to the command after the series is merged with a deprecation
> of the old should be consulted with the libvirt team as we might
> actually have already added support for the new approach. Considering it
> as "it wasn't released so we can change it" may introduce breakage given
> that the relase cycles of libvirt and qemu are not in sync.
> 
> We try hard to stay on top of such changes by using the new interface as
> soon as possible, but that is very hard if the replacement changes
> during the dev cycle.
> 

I see, thanks for the explanation! Perhaps I'll drop deprecation from
the series to avoid the issue.

Then as soon as libvirt gets support for queyring accels we might
consider deprecation again.

Best Regards,
Roman

Re: [PATCH for-6.0 2/6] qapi: Rename KvmInfo to AccelInfo

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Roman Bolshakov (r.bolshakov@yadro.com) wrote:
>> There's nothing specific to KVM in the structure. A more generic name
>> would be more appropriate.
>> 
>> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>
> For HMP:
>
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> Markus/Eric: Is it OK from QMP to change the type like that?

Type names are not part of the external interface, and can therefore be
changed freely.

>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 11f364fab4..5648d8d24d 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -562,24 +562,24 @@
>>  { 'command': 'inject-nmi' }
>>  
>>  ##
>> -# @KvmInfo:
>> +# @AccelInfo:
>>  #
>> -# Information about support for KVM acceleration
>> +# Information about support for an acceleration

We might add accelerator information that isn't about "is this one
supported?", and then the description becomes misleading.

Suggest

    # Accelerator information

>>  #
>> -# @enabled: true if KVM acceleration is active
>> +# @enabled: true if an acceleration is active

Well, "an acceleration" is always active.  The flag tells us that *this*
accelerator is active.  Suggest

    # @enabled: whether this accelerator is active

>>  #
>> -# @present: true if KVM acceleration is built into this executable
>> +# @present: true if an acceleration is built into this executable

Suggest

    # @present: whether this accelerator is ...


>>  #
>>  # Since: 0.14.0
>>  ##
>> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>> +{ 'struct': 'AccelInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>>  
>>  ##
>>  # @query-kvm:
>>  #
>>  # Returns information about KVM acceleration
>>  #
>> -# Returns: @KvmInfo
>> +# Returns: @AccelInfo
>>  #
>>  # Since: 0.14.0
>>  #
>> @@ -589,14 +589,14 @@
>>  # <- { "return": { "enabled": true, "present": true } }
>>  #
>>  ##
>> -{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
>> +{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
>>  
>>  ##
>>  # @query-accel:
>>  #
>>  # Returns information about an accelerator
>>  #
>> -# Returns: @KvmInfo
>> +# Returns: @AccelInfo
>>  #
>>  # Since: 6.0.0
>>  #
>> @@ -608,7 +608,7 @@
>>  ##
>>  { 'command': 'query-accel',
>>    'data': { 'name': 'str' },
>> -  'returns': 'KvmInfo' }
>> +  'returns': 'AccelInfo' }
>>  
>>  ##
>>  # @NumaOptionsType:
>> -- 
>> 2.29.2
>> 

Adjust the comments, and you may add
Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Peter Krempa]

On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
> > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote:
> > > Copying libvir-list for the deprecation warning.
> > > 
> > > 
> > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> > > > 'query-accel' QMP command should be used instead.
> > > > 
> > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > ---

[...]

> > We try hard to stay on top of such changes by using the new interface as
> > soon as possible, but that is very hard if the replacement changes
> > during the dev cycle.
> > 
> 
> I see, thanks for the explanation! Perhaps I'll drop deprecation from
> the series to avoid the issue.
> 
> Then as soon as libvirt gets support for queyring accels we might
> consider deprecation again.

I don't want to imply that it's entirely necessary to postpone it, but
in such cases the new API which was added to replace the old one needs
to be considered a bit more strongly until the release.

The main reason for this is that libvirt has tests whether it uses any
deprecated interface. If anything is marked as deprecated and our tests
flag it, we add an override. Usually the override is added in the same
patchset which actually implements the new approach.

We obviously can add an override and then wait for the supported
interface, but once the override is added there's nothing to remind us
later on, so I generally like to have everything in one series.

As you can see this has an issue when we have to add support for a
unreleased interface, which may change during the dev cycle or plainly
forget that something got deprecated as we've already added an override.

This mainly comes from libvirt trying to keep on top of the changes so
we refresh the QMP schema during qemu's dev cycle multiple times.

Obviously the argument that we try to depend on unreleased functionality
can be used, but that would be to detrement of progress IMO.

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
>> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
>> > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote:
>> > > Copying libvir-list for the deprecation warning.
>> > > 
>> > > 
>> > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
>> > > > 'query-accel' QMP command should be used instead.
>> > > > 
>> > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> > > > ---
>
> [...]
>
>> > We try hard to stay on top of such changes by using the new interface as
>> > soon as possible, but that is very hard if the replacement changes
>> > during the dev cycle.
>> > 
>> 
>> I see, thanks for the explanation! Perhaps I'll drop deprecation from
>> the series to avoid the issue.
>> 
>> Then as soon as libvirt gets support for queyring accels we might
>> consider deprecation again.
>
> I don't want to imply that it's entirely necessary to postpone it, but
> in such cases the new API which was added to replace the old one needs
> to be considered a bit more strongly until the release.
>
> The main reason for this is that libvirt has tests whether it uses any
> deprecated interface. If anything is marked as deprecated and our tests
> flag it, we add an override. Usually the override is added in the same
> patchset which actually implements the new approach.
>
> We obviously can add an override and then wait for the supported
> interface, but once the override is added there's nothing to remind us
> later on, so I generally like to have everything in one series.
>
> As you can see this has an issue when we have to add support for a
> unreleased interface, which may change during the dev cycle or plainly
> forget that something got deprecated as we've already added an override.
>
> This mainly comes from libvirt trying to keep on top of the changes so
> we refresh the QMP schema during qemu's dev cycle multiple times.
>
> Obviously the argument that we try to depend on unreleased functionality
> can be used, but that would be to detrement of progress IMO.

I understand your concerns.

We have a somewhat similar problem in QEMU: there's nothing to remind us
later on that the old interface still needs to be deprecated.

Here's an idea.  Keep a list of things to deprecate in the repository.
Instead of deprecating right away, we add to the list.  When soft freeze
comes, we go through the list and decide: either deprecate now (and
delete from the list), or postpone deprecation to the next release (and
keep it on the list).

Would that work for libvirt?

There's still a risk of us forgetting about the list.  Perhaps keeping a
reminder on the Planning/x.y wiki page could help.  Peter (Maydell), do
you have a check list for the various milestones?

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Daniel P. Berrangé]

On Fri, Nov 27, 2020 at 01:18:09PM +0100, Peter Krempa wrote:
> On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
> > On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
> > > On Fri, Nov 27, 2020 at 10:50:59 +0000, Daniel Berrange wrote:
> > > > Copying libvir-list for the deprecation warning.
> > > > 
> > > > 
> > > > On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> > > > > 'query-accel' QMP command should be used instead.
> > > > > 
> > > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > > ---
> 
> [...]
> 
> > > We try hard to stay on top of such changes by using the new interface as
> > > soon as possible, but that is very hard if the replacement changes
> > > during the dev cycle.
> > > 
> > 
> > I see, thanks for the explanation! Perhaps I'll drop deprecation from
> > the series to avoid the issue.
> > 
> > Then as soon as libvirt gets support for queyring accels we might
> > consider deprecation again.
> 
> I don't want to imply that it's entirely necessary to postpone it, but
> in such cases the new API which was added to replace the old one needs
> to be considered a bit more strongly until the release.
> 
> The main reason for this is that libvirt has tests whether it uses any
> deprecated interface. If anything is marked as deprecated and our tests
> flag it, we add an override. Usually the override is added in the same
> patchset which actually implements the new approach.
> 
> We obviously can add an override and then wait for the supported
> interface, but once the override is added there's nothing to remind us
> later on, so I generally like to have everything in one series.

IIUC, this all relies on us importing a dump of the latest QEMU
capabilities into the libvirt test suite.

Most of the capabilities we import are the release version, but
we also periodically import the git snapshot capabilities and
will refresh them until GA of QEMU.

Could we arrange it so that libvirt only reports an error for use
of deprecated interfaces when testing against the GA capabilities
dump. If testing against QEMU git snapshot capabilities, we can
emit a warning only.

That way, we'll have a grace period in which libvirt can see the
warning from tests, and we would only need to add an override
to silence it once we import the GA capabilities, at which time
it is safe to implement the new solution too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Peter Krempa]

On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
> >> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:

 [...]

> > As you can see this has an issue when we have to add support for a
> > unreleased interface, which may change during the dev cycle or plainly
> > forget that something got deprecated as we've already added an override.
> >
> > This mainly comes from libvirt trying to keep on top of the changes so
> > we refresh the QMP schema during qemu's dev cycle multiple times.
> >
> > Obviously the argument that we try to depend on unreleased functionality
> > can be used, but that would be to detrement of progress IMO.
> 
> I understand your concerns.
> 
> We have a somewhat similar problem in QEMU: there's nothing to remind us
> later on that the old interface still needs to be deprecated.

Oh, yes. That's basically the same thing.

The thing is that changes to the new interface are very rare, but very
real. Since I don't really want to increase the burden for any normal
scenario.

I'd also very much like to keep libvirt pulling in snapshots of qemu's
capabilities/qapi schema etc throughout the development cycle. It allows
us to adapt faster and develop features simultaneously.

This unfortunately undermines my own arguments partially as libvirt
regularly develops features on top of unreleased qemu features so we are
regularly risking very similar circumstances. The small but important
difference though is, that releasing a broken feature is not as bad as
breaking an existing feature.

As a conclusion of the above I'd basically prefer a sort of gentleman's
agreement, that new APIs become 'somewhat' stable at the moment the
commit deprecating the old interface hits upstream.

The 'somewhat'-stable API would mean that any changes to the new API
should be consulted with libvirt so that we can either give a go-ahead
that we didn't adapt yet, disable the adaptation until the changes can
be done, or another compromise depending on what's the state.

I know it's hard to enforce, but probably the cheapest in terms of
drawbacks any other solution would be.

I'll probably keep notifying patchsets which implement and deprecate old
api at the same time to keep in mind that we need to be kept in touch,
but I really don't want to impose any kind of extra process to
development on either side.

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Peter Krempa]

On Fri, Nov 27, 2020 at 15:53:16 +0000, Daniel Berrange wrote:
> On Fri, Nov 27, 2020 at 01:18:09PM +0100, Peter Krempa wrote:
> > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:

[...]

> IIUC, this all relies on us importing a dump of the latest QEMU
> capabilities into the libvirt test suite.
> 
> Most of the capabilities we import are the release version, but
> we also periodically import the git snapshot capabilities and
> will refresh them until GA of QEMU.
> 
> Could we arrange it so that libvirt only reports an error for use
> of deprecated interfaces when testing against the GA capabilities
> dump. If testing against QEMU git snapshot capabilities, we can
> emit a warning only.
> 
> That way, we'll have a grace period in which libvirt can see the
> warning from tests, and we would only need to add an override
> to silence it once we import the GA capabilities, at which time
> it is safe to implement the new solution too.

As I've noted in my reply to Markus, in many cases we develop features
on top of unreleased qemu features. I don't really want to throw a
spanner into that process.

While deprecating old interfaces which are changed would lead to
regression in behaviour rather than releasing a broken feature if
changes are done in the API of a new feature the situation is slightly
worse, but it's also a very rare occurence.

I want to basically just an agreement that after such a thing is done
libvirt will be notified. We'll still try to develop replacements sooner
which can actually catch problems in the new features sooner than they
are released in qemu.

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> > On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
>> >> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
>
>  [...]
>
>> > As you can see this has an issue when we have to add support for a
>> > unreleased interface, which may change during the dev cycle or plainly
>> > forget that something got deprecated as we've already added an override.
>> >
>> > This mainly comes from libvirt trying to keep on top of the changes so
>> > we refresh the QMP schema during qemu's dev cycle multiple times.
>> >
>> > Obviously the argument that we try to depend on unreleased functionality
>> > can be used, but that would be to detrement of progress IMO.
>> 
>> I understand your concerns.
>> 
>> We have a somewhat similar problem in QEMU: there's nothing to remind us
>> later on that the old interface still needs to be deprecated.
>
> Oh, yes. That's basically the same thing.
>
> The thing is that changes to the new interface are very rare, but very
> real. Since I don't really want to increase the burden for any normal
> scenario.
>
> I'd also very much like to keep libvirt pulling in snapshots of qemu's
> capabilities/qapi schema etc throughout the development cycle. It allows
> us to adapt faster and develop features simultaneously.
>
> This unfortunately undermines my own arguments partially as libvirt
> regularly develops features on top of unreleased qemu features so we are
> regularly risking very similar circumstances. The small but important
> difference though is, that releasing a broken feature is not as bad as
> breaking an existing feature.
>
> As a conclusion of the above I'd basically prefer a sort of gentleman's
> agreement, that new APIs become 'somewhat' stable at the moment the
> commit deprecating the old interface hits upstream.
>
> The 'somewhat'-stable API would mean that any changes to the new API
> should be consulted with libvirt so that we can either give a go-ahead
> that we didn't adapt yet, disable the adaptation until the changes can
> be done, or another compromise depending on what's the state.
>
> I know it's hard to enforce, but probably the cheapest in terms of
> drawbacks any other solution would be.

We can and should try.  

Can we flag problematic interface changes automatically?  Semantic
changes, no.  What about changes visible in query-qmp-schema?

> I'll probably keep notifying patchsets which implement and deprecate old
> api at the same time to keep in mind that we need to be kept in touch,
> but I really don't want to impose any kind of extra process to
> development on either side.

Thanks!

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Peter Krempa]

On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote:
> >> Peter Krempa <pkrempa@redhat.com> writes:

[...]

> > I know it's hard to enforce, but probably the cheapest in terms of
> > drawbacks any other solution would be.
> 
> We can and should try.  
> 
> Can we flag problematic interface changes automatically?  Semantic
> changes, no.  What about changes visible in query-qmp-schema?

I don't know the internals of qemu good enough, but from the perspective
of an user of 'query-qmp-schema' it might be possible but not without
additional tooling.

The output of query-qmp-schema is basically unreviewable when we are
updating it. A small change in the schema may trigger a re-numbering of
the internal type names so the result is a giant messy piece of JSON
where it's impossible to differentiate changes from churn.

I've played with generating/expanding all the possibilites and thus
stripping the internal numbering for a tool which would simplify writing
the query strings (a libvirtism for querying whether the QMP schema has
something [1]). This tool could be used in this case to generate a fully
expanded and sorted list which should in most cases be append only when
new stuff is added. This could be then used to see whether something
changed when we'd store the output and compare it against the new one.

Unfortunately that would just make query-qmp-schema dumps more
reviewable in libvirt though. A change in an interface would be noticed
only after it hits qemu upstream.

[1] https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392
    https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Eric Blake]

On 11/27/20 10:30 AM, Peter Krempa wrote:
> On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>>
>>> On Fri, Nov 27, 2020 at 14:45:12 +0300, Roman Bolshakov wrote:
>>>> On Fri, Nov 27, 2020 at 12:21:54PM +0100, Peter Krempa wrote:
> 
>  [...]
> 
>>> As you can see this has an issue when we have to add support for a
>>> unreleased interface, which may change during the dev cycle or plainly
>>> forget that something got deprecated as we've already added an override.
>>>
>>> This mainly comes from libvirt trying to keep on top of the changes so
>>> we refresh the QMP schema during qemu's dev cycle multiple times.
>>>
>>> Obviously the argument that we try to depend on unreleased functionality
>>> can be used, but that would be to detrement of progress IMO.
>>
>> I understand your concerns.
>>
>> We have a somewhat similar problem in QEMU: there's nothing to remind us
>> later on that the old interface still needs to be deprecated.
> 
> Oh, yes. That's basically the same thing.
> 
> The thing is that changes to the new interface are very rare, but very
> real. Since I don't really want to increase the burden for any normal
> scenario.

Case in point: our last-minute changes to block-export-add in qemu
commit cbad81ce.  The original deprecation of nbd-server-add occurred
much earlier in the 5.2 devel cycle, in qemu 443127e8, and also forgot
to tell libvirt
(https://www.redhat.com/archives/libvir-list/2020-October/msg00855.html);
then in my efforts to improve qemu-nbd, I made more changes to
block-export-add, but didn't cc libvirt on v1.  We eventually got
everything coordinated with libvirt in cc, but it did lead to some last
minute churn in libvirt to avoid a parity mismatch between versions
(https://www.redhat.com/archives/libvir-list/2020-October/msg01369.html).

> 
> I'd also very much like to keep libvirt pulling in snapshots of qemu's
> capabilities/qapi schema etc throughout the development cycle. It allows
> us to adapt faster and develop features simultaneously.
> 
> This unfortunately undermines my own arguments partially as libvirt
> regularly develops features on top of unreleased qemu features so we are
> regularly risking very similar circumstances. The small but important
> difference though is, that releasing a broken feature is not as bad as
> breaking an existing feature.
> 
> As a conclusion of the above I'd basically prefer a sort of gentleman's
> agreement, that new APIs become 'somewhat' stable at the moment the
> commit deprecating the old interface hits upstream.

Yes, moving towards this goal makes sense.  And because we've called
attention to the fact, I'll try harder to remember in my qapi reviews
any time where a new interface exists _because_ it has replaced an
interface we already marked as deprecated.

> 
> The 'somewhat'-stable API would mean that any changes to the new API
> should be consulted with libvirt so that we can either give a go-ahead
> that we didn't adapt yet, disable the adaptation until the changes can
> be done, or another compromise depending on what's the state.
> 
> I know it's hard to enforce, but probably the cheapest in terms of
> drawbacks any other solution would be.
> 
> I'll probably keep notifying patchsets which implement and deprecate old
> api at the same time to keep in mind that we need to be kept in touch,
> but I really don't want to impose any kind of extra process to
> development on either side.
> 

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

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Mon, Nov 30, 2020 at 10:21:08 +0100, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> > On Fri, Nov 27, 2020 at 16:44:05 +0100, Markus Armbruster wrote:
>> >> Peter Krempa <pkrempa@redhat.com> writes:
>
> [...]
>
>> > I know it's hard to enforce, but probably the cheapest in terms of
>> > drawbacks any other solution would be.
>> 
>> We can and should try.  
>> 
>> Can we flag problematic interface changes automatically?  Semantic
>> changes, no.  What about changes visible in query-qmp-schema?
>
> I don't know the internals of qemu good enough, but from the perspective
> of an user of 'query-qmp-schema' it might be possible but not without
> additional tooling.
>
> The output of query-qmp-schema is basically unreviewable when we are
> updating it. A small change in the schema may trigger a re-numbering of
> the internal type names so the result is a giant messy piece of JSON
> where it's impossible to differentiate changes from churn.

A structural comparison could fare better.

qapi-gen option -u might help:

  -u, --unmask-non-abi-names
                        expose non-ABI names in introspection

> I've played with generating/expanding all the possibilites and thus
> stripping the internal numbering for a tool which would simplify writing
> the query strings (a libvirtism for querying whether the QMP schema has
> something [1]). This tool could be used in this case to generate a fully
> expanded and sorted list which should in most cases be append only when
> new stuff is added. This could be then used to see whether something
> changed when we'd store the output and compare it against the new one.

Such an expansion is one way to compare structurally.  It reports
differences for "command C, argument A.B...".  Mapping to the QAPI
schema is straightforward.

> Unfortunately that would just make query-qmp-schema dumps more
> reviewable in libvirt though. A change in an interface would be noticed
> only after it hits qemu upstream.

Yes, implementing the comparison in the QEMU repository would be more
useful.

> [1] https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_qapi.c#L392
>     https://gitlab.com/libvirt/libvirt/-/blob/08ae9e5f40f8bae0c3cf48f84181884ddd310fa0/src/qemu/qemu_capabilities.c#L1512

Re: [PATCH for-6.0 1/6] qapi: Add query-accel command

[Philippe Mathieu-Daudé]

On 11/16/20 2:10 PM, Roman Bolshakov wrote:
> There's a problem for management applications to determine if certain
> accelerators available. Generic QMP command should help with that.
> 
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  monitor/qmp-cmds.c | 15 +++++++++++++++
>  qapi/machine.json  | 19 +++++++++++++++++++
>  2 files changed, 34 insertions(+)
...
> +##
> +# @query-accel:
> +#
> +# Returns information about an accelerator
> +#
> +# Returns: @KvmInfo
> +#
> +# Since: 6.0.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accel", "arguments": { "name": "kvm" } }
> +# <- { "return": { "enabled": true, "present": true } }

FWIW you can use 'qom-list-types' for that:

{ "execute": "qom-list-types", "arguments": { "implements": "accel" } }
{
    "return": [
        {
            "name": "qtest-accel",
            "parent": "accel"
        },
        {
            "name": "tcg-accel",
            "parent": "accel"
        },
        {
            "name": "xen-accel",
            "parent": "accel"
        },
        {
            "name": "kvm-accel",
            "parent": "accel"
        },
        {
            "name": "accel",
            "parent": "object"
        }
    ]
}

Which is what I use for integration tests:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg675079.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg675085.html