[Qemu-devel] [PATCH v2 0/3] ARM: add query-gic-capability SMP command

[Qemu-devel] [PATCH v2 0/3] ARM: add query-gic-capability SMP command

[Peter Xu]

This is a re-work of v1 patch:

https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg05270.html

This patch is to add ARM-specific command "query-gic-capability".

The new command can report which kind of GIC device the host/QEMU
support. The returned result is in the form of array.

Changes from v1:

- result layout change: use array and dict for the capability bits
  rather than a single array of strings [Andrea/Markus]
- spelling out what GIC is in doc [Eric]

Sample command and output:

{"execute": "query-gic-capability"}
{"return": [{"emulated": false, "version": 3, "kernel": false},
            {"emulated": true, "version": 2, "kernel": true}]}

Testing:

Smoke tests on both x86 (emulated) and another moonshot ARM server.

Please review!

--peterx

Peter Xu (3):
  arm: qmp: add GICCapability struct
  arm: qmp: add query-gic-capability interface
  arm: implement query-gic-capability

 monitor.c            |  8 ++++++++
 qapi-schema.json     | 33 ++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 26 ++++++++++++++++++++++++++
 scripts/qapi.py      |  1 +
 target-arm/machine.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 121 insertions(+)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/3] arm: qmp: add GICCapability struct

[Peter Xu]

Define new struct to describe whether we support specific GIC version.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi-schema.json | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 7b8f2a1..0b2de6c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4135,3 +4135,25 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @GICCapability:
+#
+# This struct describes capability for a specific GIC version. These
+# bits are not only decided by QEMU/KVM software version, but also
+# decided by the hardware that the program is running upon.
+#
+# @version:  version of GIC to be described.
+#
+# @emulated: whether current QEMU/hardware supports emulated GIC
+#            device in user space.
+#
+# @kernel:   whether current QEMU/hardware supports hardware
+#            accelerated GIC device in kernel.
+#
+# Since: 2.6
+##
+{ 'struct': 'GICCapability',
+  'data': { 'version': 'int',
+            'emulated': 'bool',
+            'kernel': 'bool' } }
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/3] arm: qmp: add query-gic-capability interface

[Peter Xu]

This implement the command "query-gic-capability" but not implemnet
it. The command is ARM-only. Return of the command is a list of
GICCapability struct that describes all GIC versions that current
QEMU and system support.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c            |  8 ++++++++
 qapi-schema.json     | 11 +++++++++++
 qmp-commands.hx      | 26 ++++++++++++++++++++++++++
 scripts/qapi.py      |  1 +
 target-arm/machine.c |  7 +++++++
 5 files changed, 53 insertions(+)

diff --git a/monitor.c b/monitor.c
index 73eac17..3b34feb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4241,3 +4241,11 @@ void qmp_dump_skeys(const char *filename, Error **errp)
     error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
 }
 #endif
+
+#ifndef TARGET_ARM
+GICCapabilityList *qmp_query_gic_capability(Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-gic-capability");
+    return NULL;
+}
+#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 0b2de6c..f42c8f7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4157,3 +4157,14 @@
   'data': { 'version': 'int',
             'emulated': 'bool',
             'kernel': 'bool' } }
+
+##
+# @query-gic-capability:
+#
+# Return a list of supported GIC version capabilities.
+#
+# Returns: a list of GICCapability.
+#
+# Since: 2.6
+##
+{ 'command': 'query-gic-capability', 'returns': ['GICCapability'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 13f158d..5e843f2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4852,3 +4852,29 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+#if defined TARGET_ARM
+    {
+        .name       = "query-gic-capability",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_gic_capability,
+    },
+#endif
+
+SQMP
+query-gic-capability
+---------------
+
+Return a list of supported ARM GIC versions and their capabilities.
+
+Arguments: None
+
+Example:
+
+-> { "execute": "query-gic-capability" }
+<- { "return": [{ "version": 2, "emulated": true, "kernel": false },
+                { "version": 3, "emulated": false, "kernel": true } ] }
+
+EQMP
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8497777..9dc8f73 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -46,6 +46,7 @@ returns_whitelist = [
     'query-tpm-models',
     'query-tpm-types',
     'ringbuf-read',
+    'query-gic-capability',
 
     # From QGA:
     'guest-file-open',
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 03a73d9..b3fa64c 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -345,3 +345,10 @@ const char *gicv3_class_name(void)
 
     exit(1);
 }
+
+GICCapabilityList *qmp_query_gic_capability(Error **errp);
+
+GICCapabilityList *qmp_query_gic_capability(Error **errp)
+{
+    return NULL;
+}
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/3] arm: qmp: add query-gic-capability interface

[Andrew Jones]

On Thu, Mar 03, 2016 at 04:21:11PM +0800, Peter Xu wrote:
> This implement the command "query-gic-capability" but not implemnet
> it. The command is ARM-only. Return of the command is a list of
> GICCapability struct that describes all GIC versions that current
> QEMU and system support.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c            |  8 ++++++++
>  qapi-schema.json     | 11 +++++++++++
>  qmp-commands.hx      | 26 ++++++++++++++++++++++++++
>  scripts/qapi.py      |  1 +
>  target-arm/machine.c |  7 +++++++
>  5 files changed, 53 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 73eac17..3b34feb 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4241,3 +4241,11 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>      error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
>  }
>  #endif
> +
> +#ifndef TARGET_ARM
> +GICCapabilityList *qmp_query_gic_capability(Error **errp)
> +{
> +    error_setg(errp, QERR_FEATURE_DISABLED, "query-gic-capability");
> +    return NULL;
> +}
> +#endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0b2de6c..f42c8f7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4157,3 +4157,14 @@
>    'data': { 'version': 'int',
>              'emulated': 'bool',
>              'kernel': 'bool' } }
> +
> +##
> +# @query-gic-capability:
> +#
> +# Return a list of supported GIC version capabilities.
> +#
> +# Returns: a list of GICCapability.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-gic-capability', 'returns': ['GICCapability'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 13f158d..5e843f2 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4852,3 +4852,29 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +#if defined TARGET_ARM
> +    {
> +        .name       = "query-gic-capability",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_gic_capability,
> +    },
> +#endif
> +
> +SQMP
> +query-gic-capability
> +---------------
> +
> +Return a list of supported ARM GIC versions and their capabilities.
> +
> +Arguments: None
> +
> +Example:
> +
> +-> { "execute": "query-gic-capability" }
> +<- { "return": [{ "version": 2, "emulated": true, "kernel": false },
> +                { "version": 3, "emulated": false, "kernel": true } ] }
> +
> +EQMP
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8497777..9dc8f73 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -46,6 +46,7 @@ returns_whitelist = [
>      'query-tpm-models',
>      'query-tpm-types',
>      'ringbuf-read',
> +    'query-gic-capability',
>  
>      # From QGA:
>      'guest-file-open',
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 03a73d9..b3fa64c 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -345,3 +345,10 @@ const char *gicv3_class_name(void)
>  
>      exit(1);
>  }
> +
> +GICCapabilityList *qmp_query_gic_capability(Error **errp);

I don't know anything about QMP, so just offering a superficial
review comment. Is the prototype necessary here? It seems redundant,
considering the function is defined right below.

drew

> +
> +GICCapabilityList *qmp_query_gic_capability(Error **errp)
> +{
> +    return NULL;
> +}
> -- 
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH v2 2/3] arm: qmp: add query-gic-capability interface

[Peter Xu]

On Thu, Mar 03, 2016 at 12:55:51PM +0100, Andrew Jones wrote:
> On Thu, Mar 03, 2016 at 04:21:11PM +0800, Peter Xu wrote:
> > +
> > +GICCapabilityList *qmp_query_gic_capability(Error **errp);
> 
> I don't know anything about QMP, so just offering a superficial
> review comment. Is the prototype necessary here? It seems redundant,
> considering the function is defined right below.
> 
> drew

I added this to avoid a "missing prototype" warning. However found
the correct way to do it is possibly to include "qmp-commands.h" in
target-arm/machine.c. Thanks to point out! Will fix.

Peter

[Qemu-devel] [PATCH v2 3/3] arm: implement query-gic-capability

[Peter Xu]

For emulated ARM VM, only gicv2 is supported. We need to add gicv3 in
when emulated gicv3 ready. For KVM accelerated ARM VM, we detect the
capability bits using ioctls.

if we want to know GIC kernel capabilities, we need to make sure we have
enabled KVM when querying (like, with "-enable-kvm").

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 target-arm/machine.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/target-arm/machine.c b/target-arm/machine.c
index b3fa64c..98a4094 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -1,3 +1,4 @@
+#include <linux/kvm.h>
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/boards.h"
@@ -346,9 +347,54 @@ const char *gicv3_class_name(void)
     exit(1);
 }
 
+static GICCapability *gic_cap_new(int version)
+{
+    GICCapability *cap = g_new0(GICCapability, 1);
+    cap->version = version;
+    /* by default, support none */
+    cap->emulated = false;
+    cap->kernel = false;
+    return cap;
+}
+
+static GICCapabilityList *gic_cap_list_add(GICCapabilityList *head,
+                                           GICCapability *cap)
+{
+    GICCapabilityList *item = g_new0(GICCapabilityList, 1);
+    item->value = cap;
+    item->next = head;
+    return item;
+}
+
 GICCapabilityList *qmp_query_gic_capability(Error **errp);
 
 GICCapabilityList *qmp_query_gic_capability(Error **errp)
 {
-    return NULL;
+    GICCapabilityList *head = NULL;
+    GICCapability *v2 = gic_cap_new(2), *v3 = gic_cap_new(3);
+
+    v2->emulated = true;
+    /* FIXME: we'd change to true after we get emulated GICv3 */
+    v3->emulated = false;
+
+#ifdef CONFIG_KVM
+    if (kvm_enabled()) {
+        /* Test KVM GICv2 */
+        if (kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V2,
+                              true) >= 0) {
+            v2->kernel = true;
+        }
+
+        /* Test KVM GICv3 */
+        if (kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_V3,
+                              true) >= 0) {
+            v3->kernel = true;
+        }
+    }
+#endif
+
+    head = gic_cap_list_add(head, v2);
+    head = gic_cap_list_add(head, v3);
+
+    return head;
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/3] arm: implement query-gic-capability

[Andrea Bolognani]

On Thu, 2016-03-03 at 16:21 +0800, Peter Xu wrote:
> For emulated ARM VM, only gicv2 is supported. We need to add gicv3
in
> when emulated gicv3 ready. For KVM accelerated ARM VM, we detect the
> capability bits using ioctls.
> 
> if we want to
know GIC kernel capabilities, we need to make sure we have
> enabled KVM when querying (like, with "-enable-kvm").
> 
>
Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  target-arm/machine.c | 48
+++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)

Sorry for not catching this earlier, but I'm afraid this is not
going to work -- libvirt doesn't pass either -enable-kvm or the
machine option accel=kvm when probing for capabilities, which
means that, with the current implementation, it will only get
information about emulated GIC.

Is there a way to make probing work without requiring KVM to
be enabled?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

Re: [Qemu-devel] [PATCH v2 3/3] arm: implement query-gic-capability

[Peter Xu]

On Thu, Mar 03, 2016 at 01:19:47PM +0100, Andrea Bolognani wrote:
> On Thu, 2016-03-03 at 16:21 +0800, Peter Xu wrote:
> > For emulated ARM VM, only gicv2 is supported. We need to add gicv3
> in
> > when emulated gicv3 ready. For KVM accelerated ARM VM, we detect the
> > capability bits using ioctls.
> > 
> > if we want to
> know GIC kernel capabilities, we need to make sure we have
> > enabled KVM when querying (like, with "-enable-kvm").
> > 
> >
> Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  target-arm/machine.c | 48
> +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> Sorry for not catching this earlier, but I'm afraid this is not
> going to work -- libvirt doesn't pass either -enable-kvm or the
> machine option accel=kvm when probing for capabilities, which
> means that, with the current implementation, it will only get
> information about emulated GIC.
> 
> Is there a way to make probing work without requiring KVM to
> be enabled?

Ah.. If so, this is a good point...

I can do this, but I just feel it a bit hacky if I do ioctl()s
directly in one QMP command handle:

qmp_query_gic_capability()
{
    kvm = open("/dev/kvm");
    vm = ioctl(KVM_CREATE_VM);

    ...test create devices using KVM_CREATE_DEVICE ioctls...

    close(vm);
    close(kvm);
}

Rather than leveraging current KVMState stuffs (of course, I can
make things a little bit prettier than above...).

Another way to do is to generalize kvm_init() maybe? That's some
work too.

Andrea, do you know how much effort we need to add this support for
libvirt, say, we can specify "accel=" or "-enable-kvm" as extra
parameter when probing?

Or, does anyone on the list has suggestion on how to better do this?

Thanks.
Peter

Re: [Qemu-devel] [PATCH v2 3/3] arm: implement query-gic-capability

[Andrea Bolognani]

On Fri, 2016-03-04 at 10:52 +0800, Peter Xu wrote:
> > Sorry for not catching this earlier, but I'm afraid this is not
> > going to work -- libvirt doesn't pass either -enable-kvm or the
> > machine option accel=kvm when probing for capabilities, which
> > means that, with the current implementation, it will only get
> > information about emulated GIC.
> > 
> > Is there a way to make probing work without requiring KVM to
> > be enabled?
> Ah.. If so, this is a good point...
> 
> I can do this, but I just feel it a bit hacky if I do ioctl()s
> directly in one QMP command handle:
> 
> qmp_query_gic_capability()
> {
>     kvm = open("/dev/kvm");
>     vm = ioctl(KVM_CREATE_VM);
> 
>     ...test create devices using KVM_CREATE_DEVICE ioctls...
> 
>     close(vm);
>     close(kvm);
> }
> 
> Rather than leveraging current KVMState stuffs (of course, I can
> make things a little bit prettier than above...).
> 
> Another way to do is to generalize kvm_init() maybe? That's some
> work too.
> 
> Andrea, do you know how much effort we need to add this support for
> libvirt, say, we can specify "accel=" or "-enable-kvm" as extra
> parameter when probing?

I'm afraid this is not going to be possible for the same reason
we have to use '-M none' when probing: at that point in time, we
simply have no idea what the guests will look like. Actually,
it's the other way around, in that the result of probing (host
and domain capabilities) will influence the guest configuration
created by the user / management tool.

And we definitely can't use 'accel=kvm' unconditionally, because
then we won't be able to probe eg. the qemu-system-aarch64 binary
installed on a x86_64 host.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

Re: [Qemu-devel] [PATCH v2 3/3] arm: implement query-gic-capability

[Peter Xu]

On Fri, Mar 04, 2016 at 09:43:24AM +0100, Andrea Bolognani wrote:
> On Fri, 2016-03-04 at 10:52 +0800, Peter Xu wrote:
> > Andrea, do you know how much effort we need to add this support for
> > libvirt, say, we can specify "accel=" or "-enable-kvm" as extra
> > parameter when probing?
> 
> I'm afraid this is not going to be possible for the same reason
> we have to use '-M none' when probing: at that point in time, we
> simply have no idea what the guests will look like. Actually,
> it's the other way around, in that the result of probing (host
> and domain capabilities) will influence the guest configuration
> created by the user / management tool.
> 
> And we definitely can't use 'accel=kvm' unconditionally, because
> then we won't be able to probe eg. the qemu-system-aarch64 binary
> installed on a x86_64 host.

Agreed. It is awkward to specify these for probing.

Posting v3 to allow it work even without kvm enabled. I am using the
hacky way to do it since I still have no better way to do it. Let's
see whether we can get more review comments on that.

Thanks!
Peter