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

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

[Peter Xu]

v3 changes:
- patch 2: remove func declaration, add qmp header [Drew]
- patch 3: being able to detect KVM GIC capabilities even without
  kvm enabled [Andrea]: this is a little bit hacky, need some more
  review on this.

v2 changes:
- 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]

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.

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.

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 | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 177 insertions(+)

-- 
2.4.3

[Qemu-devel] [PATCH v3 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 v3 2/3] arm: qmp: add query-gic-capability interface

[Peter Xu]

This patch adds 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 |  6 ++++++
 5 files changed, 52 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..60bd5c1 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -5,6 +5,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "internals.h"
+#include "qmp-commands.h"
 
 static bool vfp_needed(void *opaque)
 {
@@ -345,3 +346,8 @@ const char *gicv3_class_name(void)
 
     exit(1);
 }
+
+GICCapabilityList *qmp_query_gic_capability(Error **errp)
+{
+    return NULL;
+}
-- 
2.4.3

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

[Peter Xu]

For emulated GIC capabilities, currently 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.

When probing the KVM capabilities, we cannot leverage existing helper
functions like kvm_create_device() since QEMU might be using TCG while
probing (actually this is the case for libvirt probing). So, one
temporary VM is created to do the probing.

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

diff --git a/target-arm/machine.c b/target-arm/machine.c
index 60bd5c1..ff50411 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -1,3 +1,5 @@
+#include <linux/kvm.h>
+#include <sys/ioctl.h>
 #include "qemu/osdep.h"
 #include "hw/hw.h"
 #include "hw/boards.h"
@@ -347,7 +349,108 @@ 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;
+}
+
+#ifdef CONFIG_KVM
+/*
+ * This is merely the same as kvm_create_device(). The only
+ * difference is we are using raw fds rather than KVMState, so that
+ * we can use it even without kvm_state initialized.
+ */
+static int kvm_create_device_fds(int kvm_fd, int vmfd,
+                                 uint64_t type, bool test)
+{
+    int ret;
+    struct kvm_create_device create_dev;
+
+    create_dev.type = type;
+    create_dev.fd = -1;
+    create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
+
+    if (ioctl(kvm_fd, KVM_CHECK_EXTENSION, KVM_CAP_DEVICE_CTRL) <= 0) {
+        return -ENOTSUP;
+    }
+
+    ret = ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev);
+    if (ret) {
+        return ret;
+    }
+
+    return test ? 0 : create_dev.fd;
+}
+#endif
+
 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
+    {
+        /*
+         * HACK: here we create one temporary VM, do the probing,
+         * then release it properly.
+         */
+        int kvm_fd = -1;
+        int vmfd = -1;
+
+        kvm_fd = qemu_open("/dev/kvm", O_RDWR);
+        if (kvm_fd == -1) {
+            /* KVM may not enabled on host, which is fine. */
+            goto out;
+        }
+
+        do {
+            /* For ARM, VM type could only be zero now. */
+            vmfd = ioctl(kvm_fd, KVM_CREATE_VM, 0);
+        } while (vmfd == -EINTR);
+
+        if (vmfd < 0) {
+            goto kvm_fd_close;
+        }
+
+        /* Test KVM GICv2 */
+        if (kvm_create_device_fds(kvm_fd, vmfd, KVM_DEV_TYPE_ARM_VGIC_V2,
+                                  true) >= 0) {
+            v2->kernel = true;
+        }
+
+        /* Test KVM GICv3 */
+        if (kvm_create_device_fds(kvm_fd, vmfd, KVM_DEV_TYPE_ARM_VGIC_V3,
+                                  true) >= 0) {
+            v3->kernel = true;
+        }
+
+        close(vmfd);
+kvm_fd_close:
+        close(kvm_fd);
+    }
+#endif
+
+out:
+    head = gic_cap_list_add(head, v2);
+    head = gic_cap_list_add(head, v3);
+
+    return head;
 }
-- 
2.4.3

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

[Andrew Jones]

On Mon, Mar 07, 2016 at 12:23:28PM +0800, Peter Xu wrote:
> For emulated GIC capabilities, currently 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.
> 
> When probing the KVM capabilities, we cannot leverage existing helper
> functions like kvm_create_device() since QEMU might be using TCG while
> probing (actually this is the case for libvirt probing). So, one
> temporary VM is created to do the probing.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  target-arm/machine.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 60bd5c1..ff50411 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -1,3 +1,5 @@
> +#include <linux/kvm.h>
> +#include <sys/ioctl.h>
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
>  #include "hw/boards.h"
> @@ -347,7 +349,108 @@ 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;
> +}
> +
> +#ifdef CONFIG_KVM
> +/*
> + * This is merely the same as kvm_create_device(). The only
> + * difference is we are using raw fds rather than KVMState, so that
> + * we can use it even without kvm_state initialized.
> + */
> +static int kvm_create_device_fds(int kvm_fd, int vmfd,
> +                                 uint64_t type, bool test)

I don't think we need this helper function. Who else will call it?
Particularly without test==true? Anyway, I think three ioctls directly
called from qmp_query_gic_capability should be OK.

> +{
> +    int ret;
> +    struct kvm_create_device create_dev;
> +
> +    create_dev.type = type;
> +    create_dev.fd = -1;
> +    create_dev.flags = test ? KVM_CREATE_DEVICE_TEST : 0;
> +
> +    if (ioctl(kvm_fd, KVM_CHECK_EXTENSION, KVM_CAP_DEVICE_CTRL) <= 0) {
> +        return -ENOTSUP;
> +    }
> +
> +    ret = ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    return test ? 0 : create_dev.fd;
> +}
> +#endif
> +
>  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
> +    {
> +        /*
> +         * HACK: here we create one temporary VM, do the probing,
> +         * then release it properly.
> +         */
> +        int kvm_fd = -1;
> +        int vmfd = -1;
> +
> +        kvm_fd = qemu_open("/dev/kvm", O_RDWR);
> +        if (kvm_fd == -1) {
> +            /* KVM may not enabled on host, which is fine. */
> +            goto out;
> +        }
> +
> +        do {
> +            /* For ARM, VM type could only be zero now. */
> +            vmfd = ioctl(kvm_fd, KVM_CREATE_VM, 0);
> +        } while (vmfd == -EINTR);
> +
> +        if (vmfd < 0) {
> +            goto kvm_fd_close;
> +        }
> +
> +        /* Test KVM GICv2 */
> +        if (kvm_create_device_fds(kvm_fd, vmfd, KVM_DEV_TYPE_ARM_VGIC_V2,
> +                                  true) >= 0) {
> +            v2->kernel = true;
> +        }
> +
> +        /* Test KVM GICv3 */
> +        if (kvm_create_device_fds(kvm_fd, vmfd, KVM_DEV_TYPE_ARM_VGIC_V3,
> +                                  true) >= 0) {
> +            v3->kernel = true;
> +        }
> +
> +        close(vmfd);
> +kvm_fd_close:
> +        close(kvm_fd);
> +    }
> +#endif
> +
> +out:
> +    head = gic_cap_list_add(head, v2);
> +    head = gic_cap_list_add(head, v3);
> +
> +    return head;
>  }
> -- 
> 2.4.3
> 
> 

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

[Peter Xu]

On Mon, Mar 07, 2016 at 06:12:38AM +0100, Andrew Jones wrote:
> On Mon, Mar 07, 2016 at 12:23:28PM +0800, Peter Xu wrote:
> > +#ifdef CONFIG_KVM
> > +/*
> > + * This is merely the same as kvm_create_device(). The only
> > + * difference is we are using raw fds rather than KVMState, so that
> > + * we can use it even without kvm_state initialized.
> > + */
> > +static int kvm_create_device_fds(int kvm_fd, int vmfd,
> > +                                 uint64_t type, bool test)
> 
> I don't think we need this helper function. Who else will call it?
> Particularly without test==true? Anyway, I think three ioctls directly
> called from qmp_query_gic_capability should be OK.

Right. However, I would still consider using a helper function if
you would not mind. E.g, how about this:

#ifdef CONFIG_KVM
/* Test whether KVM support specific device. */
static inline int kvm_support_device(int vmfd, uint64_t type)
{
    struct kvm_create_device create_dev = {
        .type = type,
        .fd = -1,
        .flags = KVM_CREATE_DEVICE_TEST,
    };
    return ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev);
}
#endif

Thanks.
Peter

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

[Andrew Jones]

On Mon, Mar 07, 2016 at 01:38:24PM +0800, Peter Xu wrote:
> On Mon, Mar 07, 2016 at 06:12:38AM +0100, Andrew Jones wrote:
> > On Mon, Mar 07, 2016 at 12:23:28PM +0800, Peter Xu wrote:
> > > +#ifdef CONFIG_KVM
> > > +/*
> > > + * This is merely the same as kvm_create_device(). The only
> > > + * difference is we are using raw fds rather than KVMState, so that
> > > + * we can use it even without kvm_state initialized.
> > > + */
> > > +static int kvm_create_device_fds(int kvm_fd, int vmfd,
> > > +                                 uint64_t type, bool test)
> > 
> > I don't think we need this helper function. Who else will call it?
> > Particularly without test==true? Anyway, I think three ioctls directly
> > called from qmp_query_gic_capability should be OK.
> 
> Right. However, I would still consider using a helper function if
> you would not mind. E.g, how about this:

Fine by me.

Thanks,
drew

> 
> #ifdef CONFIG_KVM
> /* Test whether KVM support specific device. */
> static inline int kvm_support_device(int vmfd, uint64_t type)
> {
>     struct kvm_create_device create_dev = {
>         .type = type,
>         .fd = -1,
>         .flags = KVM_CREATE_DEVICE_TEST,
>     };
>     return ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev);
> }
> #endif
> 
> Thanks.
> Peter
> 

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

[Andrea Bolognani]

On Mon, 2016-03-07 at 12:23 +0800, Peter Xu wrote:
> v3 changes:
> - patch 2: remove func declaration, add qmp header [Drew]
> - patch 3: being able to detect KVM GIC capabilities even without
>   kvm enabled [Andrea]: this is a little bit hacky, need some more
>   review on this.
> 
> v2 changes:
> - 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]
> 
> 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.
> 
> 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.
> 
> 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 | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 177 insertions(+)

I tested this both on a v2-only machine

  {"return": [
    {"emulated": false, "version": 3, "kernel": false},
    {"emulated": true, "version": 2, "kernel": true}
  ]}

and on a v3-only machine

  {"return": [
    {"emulated": false, "version": 3, "kernel": true},
    {"emulated": true, "version": 2, "kernel": false}
  ]}

The output looks good to me - I'll leave reviewing the
implementation to someone with more QEMU knowledge than me, that
is, any QEMU knowledge at all ;)

One last, purely cosmetic, bit: should the command name be
query-gic-capabilities rather than query-gic-capability? Any
native speaker feels like weighing in?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[Peter Xu]

On Mon, Mar 07, 2016 at 10:55:54AM +0100, Andrea Bolognani wrote:
> I tested this both on a v2-only machine
> 
>   {"return": [
>     {"emulated": false, "version": 3, "kernel": false},
>     {"emulated": true, "version": 2, "kernel": true}
>   ]}
> 
> and on a v3-only machine
> 
>   {"return": [
>     {"emulated": false, "version": 3, "kernel": true},
>     {"emulated": true, "version": 2, "kernel": false}
>   ]}
> 
> The output looks good to me - I'll leave reviewing the
> implementation to someone with more QEMU knowledge than me, that
> is, any QEMU knowledge at all ;)

Appreciated for helping verify this! :-)

> 
> One last, purely cosmetic, bit: should the command name be
> query-gic-capabilities rather than query-gic-capability? Any
> native speaker feels like weighing in?

Yah, it seems that "query-gic-capabilities" sounds better. Will use
that in next spin if no one oppose.

Thanks!
Peter