[Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Peter Xu]

v6 changes:
- patch 1 (squashed into patch 2)
  - explain more about the following in commit message: why we need
    this command, and what does the entries mean [Markus]
  - explain what GIC is [Eric]
  - squash this patch into patch 2 [Eric]
- patch 2 (new patch 1)
  - fix "does not implement" english error [Eric]
  - remove useless headers in target-arm/monitor.c [Markus]
  - remove this command from whitelist [Markus, Eric]
  - return dict rather than array, with single key points to the
    original array results [Markus, Eric]
- patch 5 (new patch 4)
  - tiny change in implementation to suite the new interface.

v5 changes:
- patch 2: moved to target-arm/monitor.c (from target-arm/machine.c)
           [Peter]
- patch 3: splitted into three patches: [all from Peter's comments]
  - patch 3 (new): leverage kvm_arm_create_scratch_host_vcpu(), tiny
    enhancement of old one to suite our need
  - patch 4: introduce kvm_support_device() in kvm-all.c
  - patch 5: do the implementation.

v4 changes:
- all: rename query-gic-capability to query-gic-capabilities [Andrea]
- patch 3: rename helper function to kvm_support_device, make it
  inline and lighter. [Drew]

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 (4):
  arm: qmp: add query-gic-capabilities interface
  arm: enhance kvm_arm_create_scratch_host_vcpu
  kvm: add kvm_support_device() helper function
  arm: implement query-gic-capabilities

 include/sysemu/kvm.h     |  9 +++++
 kvm-all.c                | 15 +++++++++
 monitor.c                |  8 +++++
 qapi-schema.json         | 50 ++++++++++++++++++++++++++++
 qmp-commands.hx          | 28 ++++++++++++++++
 target-arm/Makefile.objs |  2 +-
 target-arm/kvm.c         | 10 +++++-
 target-arm/kvm_arm.h     |  6 ++--
 target-arm/monitor.c     | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 210 insertions(+), 4 deletions(-)
 create mode 100644 target-arm/monitor.c

-- 
2.4.3

[Qemu-devel] [PATCH v6 1/4] arm: qmp: add query-gic-capabilities interface

[Peter Xu]

This patch add "query-gic-capabilities" but does not implemnet it. The
command is ARM-only. The command will return one GICCapabilityResult
object, which contains a array of GICCapability struct that describes
all GIC versions that current QEMU and system support.

Libvirt is possibly the first consumer of this new command.

Before this patch, user will successfully configure all kinds of GIC
devices for ARM guests, no matter whether current QEMU/kernel support
it. If the specified GIC version/type is not supported, user will got an
ambiguous "QEMU boot failure" when trying to start the VM. This is not
user-friendly.

With this patch, libvirt should be able to query which type (and which
version) of GIC device that we support. Use this information, libvirt
can warn the user during configuration of guests when specified GIC
device type is not supported. Or better, we can just list those versions
that we support, and filter out those not-supported ones.

Besides, the community is working on a more generic way to query these
kind of information. However, due to the eagerness of this command, we
decided to first implement this ad-hoc one, then when the generic method
is ready, we can move on to that one smoothly.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c                |  8 ++++++++
 qapi-schema.json         | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx          | 28 +++++++++++++++++++++++++++
 target-arm/Makefile.objs |  2 +-
 target-arm/monitor.c     | 28 +++++++++++++++++++++++++++
 5 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 target-arm/monitor.c

diff --git a/monitor.c b/monitor.c
index 4c02f0f..3845213 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4258,3 +4258,11 @@ void qmp_dump_skeys(const char *filename, Error **errp)
     error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
 }
 #endif
+
+#ifndef TARGET_ARM
+GICCapabilityResult *qmp_query_gic_capabilities(Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "query-gic-capabilities");
+    return NULL;
+}
+#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 88f9b81..72359dd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4126,3 +4126,53 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @GICCapability:
+#
+# The struct describes capability for a specific GIC (Generic
+# Interrupt Controller) 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. Currently, only 2 and 3
+#            are supported.
+#
+# @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' } }
+
+##
+# @GICCapabilityResult:
+#
+# Query result for GIC capabilities.
+#
+# @data:  currently with a single key 'capabilities', which contains
+#         a list of GICCapability elements, describing all supported
+#         versions/kinds of GIC devices.
+#
+# Since: 2.6
+##
+{ 'struct': 'GICCapabilityResult',
+  'data': { 'capabilities': ['GICCapability'] } }
+
+##
+# @query-gic-capabilities:
+#
+# This command is ARM-only. It will return a GICCapabilityResult
+# object that describes its capability bits.
+#
+# Returns: GICCapabilityResult object.
+#
+# Since: 2.6
+##
+{ 'command': 'query-gic-capabilities', 'returns': 'GICCapabilityResult' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9e05365..4144982 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4853,3 +4853,31 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+#if defined TARGET_ARM
+    {
+        .name       = "query-gic-capabilities",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_gic_capabilities,
+    },
+#endif
+
+SQMP
+query-gic-capabilities
+---------------
+
+Return a GICCapabilityResult object, which contains a list of
+GICCapability elements, describing supported GIC versions.
+
+Arguments: None
+
+Example:
+
+-> { "execute": "query-gic-capabilities" }
+<- { "return": { "capabilities": [
+                { "version": 2, "emulated": true, "kernel": false },
+                { "version": 3, "emulated": false, "kernel": true } ] } }
+
+EQMP
diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index a80eb39..334074c 100644
--- a/target-arm/Makefile.objs
+++ b/target-arm/Makefile.objs
@@ -8,4 +8,4 @@ obj-y += translate.o op_helper.o helper.o cpu.o
 obj-y += neon_helper.o iwmmxt_helper.o
 obj-y += gdbstub.o
 obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
-obj-y += crypto_helper.o
+obj-y += crypto_helper.o monitor.o
diff --git a/target-arm/monitor.c b/target-arm/monitor.c
new file mode 100644
index 0000000..254a9c9
--- /dev/null
+++ b/target-arm/monitor.c
@@ -0,0 +1,28 @@
+/*
+ * QEMU monitor.c for ARM.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "qemu/osdep.h"
+#include "qmp-commands.h"
+
+GICCapabilityResult *qmp_query_gic_capabilities(Error **errp)
+{
+    return NULL;
+}
-- 
2.4.3

[Qemu-devel] [PATCH v6 2/4] arm: enhance kvm_arm_create_scratch_host_vcpu

[Peter Xu]

Some more lines to make sure we allow NULL for 1st/3rd parameter.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 target-arm/kvm.c     | 10 +++++++++-
 target-arm/kvm_arm.h |  6 ++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 969ab0b..0a7f9a6 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -62,13 +62,17 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
         goto err;
     }
 
+    if (!init) {
+        goto finish;
+    }
+
     ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init);
     if (ret >= 0) {
         ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init);
         if (ret < 0) {
             goto err;
         }
-    } else {
+    } else if (cpus_to_try) {
         /* Old kernel which doesn't know about the
          * PREFERRED_TARGET ioctl: we know it will only support
          * creating one kind of guest CPU which is its preferred
@@ -85,8 +89,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
         if (ret < 0) {
             goto err;
         }
+    } else {
+        /* Not providing cpus_to_try, do nothing. */
+        ;
     }
 
+finish:
     fdarray[0] = kvmfd;
     fdarray[1] = vmfd;
     fdarray[2] = cpufd;
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 07f0c72..6bcfe6c 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -124,9 +124,11 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu);
  * kvm_arm_create_scratch_host_vcpu:
  * @cpus_to_try: array of QEMU_KVM_ARM_TARGET_* values (terminated with
  * QEMU_KVM_ARM_TARGET_NONE) to try as fallback if the kernel does not
- * know the PREFERRED_TARGET ioctl
+ * know the PREFERRED_TARGET ioctl. If NULL is provided, will try
+ * nothing.
  * @fdarray: filled in with kvmfd, vmfd, cpufd file descriptors in that order
- * @init: filled in with the necessary values for creating a host vcpu
+ * @init: filled in with the necessary values for creating a host
+ * vcpu. If NULL is provided, will not init the vCPU.
  *
  * Create a scratch vcpu in its own VM of the type preferred by the host
  * kernel (as would be used for '-cpu host'), for purposes of probing it
-- 
2.4.3

[Qemu-devel] [PATCH v6 3/4] kvm: add kvm_support_device() helper function

[Peter Xu]

This can be used when probing whether KVM support specific device. Here,
a raw vmfd is used.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/kvm.h |  9 +++++++++
 kvm-all.c            | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6695fa7..8738fa1 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -306,6 +306,15 @@ void kvm_device_access(int fd, int group, uint64_t attr,
  */
 int kvm_create_device(KVMState *s, uint64_t type, bool test);
 
+/**
+ * kvm_support_device - probe whether KVM support specific device
+ *
+ * @vmfd: The fd handler for VM
+ * @type: type of device
+ *
+ * @return: true if supported, otherwise false.
+ */
+bool kvm_support_device(int vmfd, uint64_t type);
 
 /* Arch specific hooks */
 
diff --git a/kvm-all.c b/kvm-all.c
index 44c0464..77deccc 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2339,6 +2339,21 @@ int kvm_create_device(KVMState *s, uint64_t type, bool test)
     return test ? 0 : create_dev.fd;
 }
 
+bool kvm_support_device(int vmfd, uint64_t type)
+{
+    struct kvm_create_device create_dev = {
+        .type = type,
+        .fd = -1,
+        .flags = KVM_CREATE_DEVICE_TEST,
+    };
+
+    if (ioctl(vmfd, KVM_CHECK_EXTENSION, KVM_CAP_DEVICE_CTRL) <= 0) {
+        return false;
+    }
+
+    return (ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev) >= 0);
+}
+
 int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source)
 {
     struct kvm_one_reg reg;
-- 
2.4.3

[Qemu-devel] [PATCH v6 4/4] arm: implement query-gic-capabilities

[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 by creating a scratch VM.

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

diff --git a/target-arm/monitor.c b/target-arm/monitor.c
index 254a9c9..4a2db59 100644
--- a/target-arm/monitor.c
+++ b/target-arm/monitor.c
@@ -21,8 +21,66 @@
  */
 #include "qemu/osdep.h"
 #include "qmp-commands.h"
+#include "hw/boards.h"
+#include "kvm_arm.h"
+
+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;
+}
 
 GICCapabilityResult *qmp_query_gic_capabilities(Error **errp)
 {
-    return NULL;
+    GICCapabilityResult *result = g_new0(GICCapabilityResult, 1);
+    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
+    {
+        int fdarray[3];
+
+        if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
+            goto out;
+        }
+
+        /* Test KVM GICv2 */
+        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V2)) {
+            v2->kernel = true;
+        }
+
+        /* Test KVM GICv3 */
+        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V3)) {
+            v3->kernel = true;
+        }
+
+        kvm_arm_destroy_scratch_host_vcpu(fdarray);
+out:
+        ;
+    }
+#endif
+
+    head = gic_cap_list_add(head, v2);
+    head = gic_cap_list_add(head, v3);
+
+    result->capabilities = head;
+
+    return result;
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Peter Xu]

On Wed, Mar 23, 2016 at 01:32:29PM +0800, Peter Xu wrote:
> 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}]}

Sorry! This is wrong, which is v5 version of interface.

The new interface should return a dict rather than array:

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

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Andrea Bolognani]

On Wed, 2016-03-23 at 13:32 +0800, Peter Xu wrote:
> v6 changes:
> - patch 1 (squashed into patch 2)
>   - explain more about the following in commit message: why we need
>     this command, and what does the entries mean [Markus]
>   - explain what GIC is [Eric]
>   - squash this patch into patch 2 [Eric]
> - patch 2 (new patch 1)
>   - fix "does not implement" english error [Eric]
>   - remove useless headers in target-arm/monitor.c [Markus]
>   - remove this command from whitelist [Markus, Eric]
>   - return dict rather than array, with single key points to the
>     original array results [Markus, Eric]
> - patch 5 (new patch 4)
>   - tiny change in implementation to suite the new interface.

Tested again on two different aarch64 hosts, one with GIC
v3 support and the other one with GIC v2 only; seems to be
working just fine.

Now on to tweak my RFC libvirt patches to cope with the
interface change :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

Re: [Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Peter Xu]

On Wed, Mar 23, 2016 at 09:54:03AM +0100, Andrea Bolognani wrote:
> On Wed, 2016-03-23 at 13:32 +0800, Peter Xu wrote:
> > v6 changes:
> > - patch 1 (squashed into patch 2)
> >   - explain more about the following in commit message: why we need
> >     this command, and what does the entries mean [Markus]
> >   - explain what GIC is [Eric]
> >   - squash this patch into patch 2 [Eric]
> > - patch 2 (new patch 1)
> >   - fix "does not implement" english error [Eric]
> >   - remove useless headers in target-arm/monitor.c [Markus]
> >   - remove this command from whitelist [Markus, Eric]
> >   - return dict rather than array, with single key points to the
> >     original array results [Markus, Eric]
> > - patch 5 (new patch 4)
> >   - tiny change in implementation to suite the new interface.
> 
> Tested again on two different aarch64 hosts, one with GIC
> v3 support and the other one with GIC v2 only; seems to be
> working just fine.
> 
> Now on to tweak my RFC libvirt patches to cope with the
> interface change :)

Andrea, 

Thank you very much for your quick follow-ups (merely every
time). :-)

Hope that the interface (at least) could suffice this time.

-- peterx

Re: [Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Christoffer Dall]

Hi Peter,

Stupid question: The subject says "SMP command".  Did you mean "QMP
command" ?

Thanks,
-Christoffer

On Wed, Mar 23, 2016 at 01:32:29PM +0800, Peter Xu wrote:
> v6 changes:
> - patch 1 (squashed into patch 2)
>   - explain more about the following in commit message: why we need
>     this command, and what does the entries mean [Markus]
>   - explain what GIC is [Eric]
>   - squash this patch into patch 2 [Eric]
> - patch 2 (new patch 1)
>   - fix "does not implement" english error [Eric]
>   - remove useless headers in target-arm/monitor.c [Markus]
>   - remove this command from whitelist [Markus, Eric]
>   - return dict rather than array, with single key points to the
>     original array results [Markus, Eric]
> - patch 5 (new patch 4)
>   - tiny change in implementation to suite the new interface.
> 
> v5 changes:
> - patch 2: moved to target-arm/monitor.c (from target-arm/machine.c)
>            [Peter]
> - patch 3: splitted into three patches: [all from Peter's comments]
>   - patch 3 (new): leverage kvm_arm_create_scratch_host_vcpu(), tiny
>     enhancement of old one to suite our need
>   - patch 4: introduce kvm_support_device() in kvm-all.c
>   - patch 5: do the implementation.
> 
> v4 changes:
> - all: rename query-gic-capability to query-gic-capabilities [Andrea]
> - patch 3: rename helper function to kvm_support_device, make it
>   inline and lighter. [Drew]
> 
> 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 (4):
>   arm: qmp: add query-gic-capabilities interface
>   arm: enhance kvm_arm_create_scratch_host_vcpu
>   kvm: add kvm_support_device() helper function
>   arm: implement query-gic-capabilities
> 
>  include/sysemu/kvm.h     |  9 +++++
>  kvm-all.c                | 15 +++++++++
>  monitor.c                |  8 +++++
>  qapi-schema.json         | 50 ++++++++++++++++++++++++++++
>  qmp-commands.hx          | 28 ++++++++++++++++
>  target-arm/Makefile.objs |  2 +-
>  target-arm/kvm.c         | 10 +++++-
>  target-arm/kvm_arm.h     |  6 ++--
>  target-arm/monitor.c     | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 210 insertions(+), 4 deletions(-)
>  create mode 100644 target-arm/monitor.c
> 
> -- 
> 2.4.3
> 
> 

Re: [Qemu-devel] [PATCH v6 0/4] ARM: add query-gic-capabilities SMP command

[Peter Xu]

On Wed, Mar 23, 2016 at 11:32:06AM +0100, Christoffer Dall wrote:
> Hi Peter,
> 
> Stupid question: The subject says "SMP command".  Did you mean "QMP
> command" ?

It's for sure a good question... My fault. It should be QMP.

Thanks to point out.

-- peterx

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 2/4] arm: enhance kvm_arm_create_scratch_host_vcpu

[Sergey Fedorov]

On 23/03/16 08:32, Peter Xu wrote:
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 969ab0b..0a7f9a6 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -62,13 +62,17 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
>          goto err;
>      }
>  
> +    if (!init) {
> +        goto finish;
> +    }
> +
>      ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init);
>      if (ret >= 0) {
>          ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init);
>          if (ret < 0) {
>              goto err;
>          }
> -    } else {
> +    } else if (cpus_to_try) {
>          /* Old kernel which doesn't know about the
>           * PREFERRED_TARGET ioctl: we know it will only support
>           * creating one kind of guest CPU which is its preferred
> @@ -85,8 +89,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
>          if (ret < 0) {
>              goto err;
>          }
> +    } else {
> +        /* Not providing cpus_to_try, do nothing. */
> +        ;

I think it's probably not the best idea to skip CPU initialization here.
I'd rather raise an error in such case. If we supplied non-NULL init
argument then we need VCPU been initialized, don't we? If we pass NULL
as init then we actually skip this code.

Kind regards,
Sergey

>      }
>  
> +finish:
>      fdarray[0] = kvmfd;
>      fdarray[1] = vmfd;
>      fdarray[2] = cpufd;

Re: [Qemu-devel] [PATCH v6 3/4] kvm: add kvm_support_device() helper function

[Sergey Fedorov]

On 23/03/16 08:32, Peter Xu wrote:
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 6695fa7..8738fa1 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -306,6 +306,15 @@ void kvm_device_access(int fd, int group, uint64_t attr,
>   */
>  int kvm_create_device(KVMState *s, uint64_t type, bool test);
>  
> +/**
> + * kvm_support_device - probe whether KVM support specific device
> + *
> + * @vmfd: The fd handler for VM
> + * @type: type of device
> + *
> + * @return: true if supported, otherwise false.
> + */
> +bool kvm_support_device(int vmfd, uint64_t type);

Why don't name the function like 'kvm_device_supported' to better express its predicative nature?

Kind regards,
Sergey

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 4/4] arm: implement query-gic-capabilities

[Sergey Fedorov]

On 23/03/16 08:32, Peter Xu wrote:
> diff --git a/target-arm/monitor.c b/target-arm/monitor.c
> index 254a9c9..4a2db59 100644
> --- a/target-arm/monitor.c
> +++ b/target-arm/monitor.c
> @@ -21,8 +21,66 @@
(snip)
>  GICCapabilityResult *qmp_query_gic_capabilities(Error **errp)
>  {
> -    return NULL;
> +    GICCapabilityResult *result = g_new0(GICCapabilityResult, 1);
> +    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. */

Maybewe'd better use'NOTE:' or 'TODO:' instead of 'FIXME:'?

> +    v3->emulated = false;
> +
> +#ifdef CONFIG_KVM
> +    {
> +        int fdarray[3];
> +
> +        if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
> +            goto out;
> +        }
> +
> +        /* Test KVM GICv2 */
> +        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V2)) {
> +            v2->kernel = true;
> +        }
> +
> +        /* Test KVM GICv3 */
> +        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V3)) {
> +            v3->kernel = true;
> +        }
> +
> +        kvm_arm_destroy_scratch_host_vcpu(fdarray);
> +out:
> +        ;
> +    }
> +#endif

Probably, it would be neater to put KVM part into a separate static
inline function.

Kind regards,
Sergey

> +
> +    head = gic_cap_list_add(head, v2);
> +    head = gic_cap_list_add(head, v3);
> +
> +    result->capabilities = head;
> +
> +    return result;
>  }

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 2/4] arm: enhance kvm_arm_create_scratch_host_vcpu

[Peter Xu]

On Wed, Mar 23, 2016 at 03:24:16PM +0300, Sergey Fedorov wrote:
> On 23/03/16 08:32, Peter Xu wrote:
> > diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> > index 969ab0b..0a7f9a6 100644
> > --- a/target-arm/kvm.c
> > +++ b/target-arm/kvm.c
> > @@ -62,13 +62,17 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
> >          goto err;
> >      }
> >  
> > +    if (!init) {
> > +        goto finish;
> > +    }
> > +
> >      ret = ioctl(vmfd, KVM_ARM_PREFERRED_TARGET, init);
> >      if (ret >= 0) {
> >          ret = ioctl(cpufd, KVM_ARM_VCPU_INIT, init);
> >          if (ret < 0) {
> >              goto err;
> >          }
> > -    } else {
> > +    } else if (cpus_to_try) {
> >          /* Old kernel which doesn't know about the
> >           * PREFERRED_TARGET ioctl: we know it will only support
> >           * creating one kind of guest CPU which is its preferred
> > @@ -85,8 +89,12 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
> >          if (ret < 0) {
> >              goto err;
> >          }
> > +    } else {
> > +        /* Not providing cpus_to_try, do nothing. */
> > +        ;
> 
> I think it's probably not the best idea to skip CPU initialization here.
> I'd rather raise an error in such case. If we supplied non-NULL init
> argument then we need VCPU been initialized, don't we? If we pass NULL
> as init then we actually skip this code.

Though current we will never go into this else... I agree that we
should raise error if stepped into it.

Will fix. Thanks!

-- peterx

Re: [Qemu-devel] [PATCH v6 3/4] kvm: add kvm_support_device() helper function

[Peter Xu]

On Wed, Mar 23, 2016 at 03:28:28PM +0300, Sergey Fedorov wrote:
> On 23/03/16 08:32, Peter Xu wrote:
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 6695fa7..8738fa1 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -306,6 +306,15 @@ void kvm_device_access(int fd, int group, uint64_t attr,
> >   */
> >  int kvm_create_device(KVMState *s, uint64_t type, bool test);
> >  
> > +/**
> > + * kvm_support_device - probe whether KVM support specific device
> > + *
> > + * @vmfd: The fd handler for VM
> > + * @type: type of device
> > + *
> > + * @return: true if supported, otherwise false.
> > + */
> > +bool kvm_support_device(int vmfd, uint64_t type);
> 
> Why don't name the function like 'kvm_device_supported' to better express its predicative nature?

Because I am trying to follow existing naming style, like:
"kvm_create_device" (please see above).

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v6 3/4] kvm: add kvm_support_device() helper function

[Sergey Fedorov]

On 23/03/16 17:56, Peter Xu wrote:
> On Wed, Mar 23, 2016 at 03:28:28PM +0300, Sergey Fedorov wrote:
>> On 23/03/16 08:32, Peter Xu wrote:
>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>> index 6695fa7..8738fa1 100644
>>> --- a/include/sysemu/kvm.h
>>> +++ b/include/sysemu/kvm.h
>>> @@ -306,6 +306,15 @@ void kvm_device_access(int fd, int group, uint64_t attr,
>>>   */
>>>  int kvm_create_device(KVMState *s, uint64_t type, bool test);
>>>  
>>> +/**
>>> + * kvm_support_device - probe whether KVM support specific device
>>> + *
>>> + * @vmfd: The fd handler for VM
>>> + * @type: type of device
>>> + *
>>> + * @return: true if supported, otherwise false.
>>> + */
>>> +bool kvm_support_device(int vmfd, uint64_t type);
>> Why don't name the function like 'kvm_device_supported' to better express its predicative nature?
> Because I am trying to follow existing naming style, like:
> "kvm_create_device" (please see above).

Yes, but kvm_create_device() returns a file descriptor whereas this
function is predicative. Personally, I like the convention described in
chapter 16 of Linux kernel coding style [1]:

	If the name of a function is an action or an imperative command,
	the function should return an error-code integer.  If the name
	is a predicate, the function should return a "succeeded" boolean.


[1] https://www.kernel.org/doc/Documentation/CodingStyle

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH v6 3/4] kvm: add kvm_support_device() helper function

[Peter Xu]

On Wed, Mar 23, 2016 at 06:03:32PM +0300, Sergey Fedorov wrote:
> Yes, but kvm_create_device() returns a file descriptor whereas this
> function is predicative. Personally, I like the convention described in
> chapter 16 of Linux kernel coding style [1]:
> 
> 	If the name of a function is an action or an imperative command,
> 	the function should return an error-code integer.  If the name
> 	is a predicate, the function should return a "succeeded" boolean.

The above is talking about return values. Maybe you were trying to
reference this one as example about add_work() and
pci_dev_present():

    For example, "add work" is a command, and the add_work()
    function returns 0 for success or -EBUSY for failure.  In the
    same way, "PCI device present" is a predicate, and the
    pci_dev_present() function returns 1 if it succeeds in finding a
    matching device or 0 if it doesn't.

Seems make sense. Will take your advice.  Thanks.

-- peterx

Re: [Qemu-devel] [Qemu-arm] [PATCH v6 4/4] arm: implement query-gic-capabilities

[Peter Xu]

On Wed, Mar 23, 2016 at 03:33:25PM +0300, Sergey Fedorov wrote:
> On 23/03/16 08:32, Peter Xu wrote:
> > diff --git a/target-arm/monitor.c b/target-arm/monitor.c
> > index 254a9c9..4a2db59 100644
> > --- a/target-arm/monitor.c
> > +++ b/target-arm/monitor.c
> > @@ -21,8 +21,66 @@
> (snip)
> >  GICCapabilityResult *qmp_query_gic_capabilities(Error **errp)
> >  {
> > -    return NULL;
> > +    GICCapabilityResult *result = g_new0(GICCapabilityResult, 1);
> > +    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. */
> 
> Maybewe'd better use'NOTE:' or 'TODO:' instead of 'FIXME:'?

Right.

> 
> > +    v3->emulated = false;
> > +
> > +#ifdef CONFIG_KVM
> > +    {
> > +        int fdarray[3];
> > +
> > +        if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
> > +            goto out;
> > +        }
> > +
> > +        /* Test KVM GICv2 */
> > +        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V2)) {
> > +            v2->kernel = true;
> > +        }
> > +
> > +        /* Test KVM GICv3 */
> > +        if (kvm_support_device(fdarray[1], KVM_DEV_TYPE_ARM_VGIC_V3)) {
> > +            v3->kernel = true;
> > +        }
> > +
> > +        kvm_arm_destroy_scratch_host_vcpu(fdarray);
> > +out:
> > +        ;
> > +    }
> > +#endif
> 
> Probably, it would be neater to put KVM part into a separate static
> inline function.

Seems so. Will fix.

Thanks.

-- peterx