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

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

[Peter Xu]

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 (5):
  arm: qmp: add GICCapability struct
  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         | 33 +++++++++++++++++++
 qmp-commands.hx          | 26 +++++++++++++++
 scripts/qapi.py          |  1 +
 target-arm/Makefile.objs |  2 +-
 target-arm/kvm.c         | 10 +++++-
 target-arm/kvm_arm.h     |  6 ++--
 target-arm/monitor.c     | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 192 insertions(+), 4 deletions(-)
 create mode 100644 target-arm/monitor.c

-- 
2.4.3

[Qemu-devel] [PATCH v5 1/5] 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 f253a37..da9671a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4134,3 +4134,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 v5 2/5] arm: qmp: add query-gic-capabilities interface

[Peter Xu]

This patch adds the command "query-gic-capabilities" 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/Makefile.objs |  2 +-
 target-arm/monitor.c     | 31 +++++++++++++++++++++++++++++++
 6 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 target-arm/monitor.c

diff --git a/monitor.c b/monitor.c
index 894f862..d463dc4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4257,3 +4257,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_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 da9671a..b2ef149 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4156,3 +4156,14 @@
   'data': { 'version': 'int',
             'emulated': 'bool',
             'kernel': 'bool' } }
+
+##
+# @query-gic-capabilities:
+#
+# Return a list of supported GIC version capabilities.
+#
+# Returns: a list of GICCapability.
+#
+# Since: 2.6
+##
+{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9e05365..a124ea8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4853,3 +4853,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-capabilities",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_gic_capabilities,
+    },
+#endif
+
+SQMP
+query-gic-capabilities
+---------------
+
+Return a list of supported ARM GIC versions and their capabilities.
+
+Arguments: None
+
+Example:
+
+-> { "execute": "query-gic-capabilities" }
+<- { "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 6b2aa6e..716474e 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/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..5678eb8
--- /dev/null
+++ b/target-arm/monitor.c
@@ -0,0 +1,31 @@
+/*
+ * 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 "hw/boards.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "qmp-commands.h"
+
+GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
+{
+    return NULL;
+}
-- 
2.4.3

[Qemu-devel] [PATCH v5 3/5] 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 v5 4/5] 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 v5 5/5] 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/target-arm/monitor.c b/target-arm/monitor.c
index 5678eb8..5493ea6 100644
--- a/target-arm/monitor.c
+++ b/target-arm/monitor.c
@@ -24,8 +24,63 @@
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "qmp-commands.h"
+#include "kvm_arm.h"
+#include <sys/ioctl.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;
+}
 
 GICCapabilityList *qmp_query_gic_capabilities(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
+    {
+        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);
+
+    return head;
 }
-- 
2.4.3

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

[Andrea Bolognani]

On Fri, 2016-03-18 at 11:27 +0800, Peter Xu wrote:
> 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.

Tested on two separate aarch64 hosts, seems to work fine.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[Peter Xu]

On Mon, Mar 21, 2016 at 04:56:07PM +0100, Andrea Bolognani wrote:
> On Fri, 2016-03-18 at 11:27 +0800, Peter Xu wrote:
> > 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.
> 
> Tested on two separate aarch64 hosts, seems to work fine.

Thanks Andrea!

Sorry I forgot to CC libvir-list. To avoid duplication, not
re-sending but CCing in this reply. If anyone interested, please
review in the following link:

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html

(which points to exactly current thread.)

Sorry for the inconvenience!

-- peterx

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

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> 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.

We discussed the need for a yet another ad hoc query command.  Can you
please summarize the discussion and its conclusion?  Explaining *why* a
feature is needed is always a good idea.  Cover letter and commit
messages are good places for it.

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

[Peter Maydell]

On 18 March 2016 at 03:27, Peter Xu <peterx@redhat.com> wrote:
> 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}]}

Reminder: I still need confirmation from the libvirt folks
that this API meets their needs, and review from somebody
on the QEMU QMP protocol design side. This should go into
2.6 but we are getting closer to hardfreeze...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Copying Eric in case further review is needed in my absence.

Peter Xu <peterx@redhat.com> writes:

> This patch adds the command "query-gic-capabilities" 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/Makefile.objs |  2 +-
>  target-arm/monitor.c     | 31 +++++++++++++++++++++++++++++++
>  6 files changed, 78 insertions(+), 1 deletion(-)
>  create mode 100644 target-arm/monitor.c
>
> diff --git a/monitor.c b/monitor.c
> index 894f862..d463dc4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4257,3 +4257,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_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 da9671a..b2ef149 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4156,3 +4156,14 @@
>    'data': { 'version': 'int',
>              'emulated': 'bool',
>              'kernel': 'bool' } }
> +
> +##
> +# @query-gic-capabilities:
> +#
> +# Return a list of supported GIC version capabilities.
> +#
> +# Returns: a list of GICCapability.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9e05365..a124ea8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4853,3 +4853,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-capabilities",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_query_gic_capabilities,
> +    },
> +#endif
> +
> +SQMP
> +query-gic-capabilities
> +---------------
> +
> +Return a list of supported ARM GIC versions and their capabilities.
> +
> +Arguments: None
> +
> +Example:
> +
> +-> { "execute": "query-gic-capabilities" }
> +<- { "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 6b2aa6e..716474e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -46,6 +46,7 @@ returns_whitelist = [
   # Whitelist of commands allowed to return a non-dictionary
   returns_whitelist = [
       'human-monitor-command',
       'qom-get',
       'query-migrate-cache-size',
>      'query-tpm-models',
>      'query-tpm-types',
>      'ringbuf-read',
> +    'query-gic-capability',
>  
>      # From QGA:
>      'guest-file-open',

The whitelist exists to except existing commands from design rules on
return types.  New commands don't get to violate the rules without a
really, really compelling reason.

Do you actually need this?

If yes, why should your command be permitted to violate the design
rules?

> 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..5678eb8
> --- /dev/null
> +++ b/target-arm/monitor.c
> @@ -0,0 +1,31 @@
> +/*
> + * 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 "hw/boards.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qmp-commands.h"

I very much doubt you need all these includes.  Try dropping all but the
first and the last one.

> +
> +GICCapabilityList *qmp_query_gic_capabilities(Error **errp)
> +{
> +    return NULL;
> +}

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> 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 f253a37..da9671a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4134,3 +4134,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' } }

Are all four combinations of (emulated, kernel) possible?

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On 03/17/2016 09:27 PM, Peter Xu wrote:
> 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 f253a37..da9671a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4134,3 +4134,25 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @GICCapability:
> +#
> +# This struct describes capability for a specific GIC version. These

Might be nice to spell out what the acronym GIC means, but that's cosmetic.

> +# 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' } }
> 

I might have squashed this with the patch that first uses GICCapability,
as defining a type in isolation doesn't do much.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> 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 f253a37..da9671a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -4134,3 +4134,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' } }
>
> Are all four combinations of (emulated, kernel) possible?

Moreover, what do the combinations mean from a practical point of view?
What would a management application do with the information?

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On 03/17/2016 09:27 PM, Peter Xu wrote:
> This patch adds the command "query-gic-capabilities" but not implemnet

s/not implemnet/does not implement/

> 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>
> ---

> +++ b/qapi-schema.json
> @@ -4156,3 +4156,14 @@
>    'data': { 'version': 'int',
>              'emulated': 'bool',
>              'kernel': 'bool' } }
> +
> +##
> +# @query-gic-capabilities:
> +#
> +# Return a list of supported GIC version capabilities.
> +#
> +# Returns: a list of GICCapability.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }

On the surface, this seems okay.  As mentioned before, I would have
squashed 1 and 2 into a single patch.  The GICCapability type is
extensible, and introspection is sufficient at seeing what the type is
currently capable of exposing.

On the other hand...

> +++ b/scripts/qapi.py
> @@ -46,6 +46,7 @@ returns_whitelist = [
>      'query-tpm-models',
>      'query-tpm-types',
>      'ringbuf-read',
> +    'query-gic-capability',

...it required a whitelist, because you are violating the usual
convention of returning a dict.  If you DO need the whitelist, your
addition should have been kept sorted.  But you don't need it, if you
would modify your QAPI to return a dict:

{ 'struct': 'GICCapabilitiesReturn',
  'data': { 'capabilities': ['GICCapability'] } }
{ 'command': 'query-gic-capabilities',
  'returns': 'GICCapabilitiesReturn' }

Yes, the dict has only a single key, and that key points to the same
list; but now you have future extensibility: in the future, we could
return any future global data as a sibling to the array, without having
to modify every element of the array to repeat redundant information.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> This patch adds the command "query-gic-capabilities" but not implemnet
>
> s/not implemnet/does not implement/
>
>> 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>
>> ---
>
>> +++ b/qapi-schema.json
>> @@ -4156,3 +4156,14 @@
>>    'data': { 'version': 'int',
>>              'emulated': 'bool',
>>              'kernel': 'bool' } }
>> +
>> +##
>> +# @query-gic-capabilities:
>> +#
>> +# Return a list of supported GIC version capabilities.
>> +#
>> +# Returns: a list of GICCapability.
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
>
> On the surface, this seems okay.  As mentioned before, I would have
> squashed 1 and 2 into a single patch.  The GICCapability type is
> extensible, and introspection is sufficient at seeing what the type is
> currently capable of exposing.
>
> On the other hand...
>
>> +++ b/scripts/qapi.py
>> @@ -46,6 +46,7 @@ returns_whitelist = [
>>      'query-tpm-models',
>>      'query-tpm-types',
>>      'ringbuf-read',
>> +    'query-gic-capability',
>
> ...it required a whitelist, because you are violating the usual
> convention of returning a dict.  If you DO need the whitelist, your
> addition should have been kept sorted.  But you don't need it, if you
> would modify your QAPI to return a dict:
>
> { 'struct': 'GICCapabilitiesReturn',
>   'data': { 'capabilities': ['GICCapability'] } }
> { 'command': 'query-gic-capabilities',
>   'returns': 'GICCapabilitiesReturn' }
>
> Yes, the dict has only a single key, and that key points to the same
> list; but now you have future extensibility: in the future, we could
> return any future global data as a sibling to the array, without having
> to modify every element of the array to repeat redundant information.

I just tested it, the whitelist entry is superfluous, check_command()
accepts a list of dicts just fine.

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> >> +##
> >> +# @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' } }
> >
> > Are all four combinations of (emulated, kernel) possible?

Currently, version can only be 2 or 3. And for now, we should only
support:

- emulated/kernel support for v2
- kernel support for v3

and emulated v3 is still not supported.

> Moreover, what do the combinations mean from a practical point of view?
> What would a management application do with the information?

AFAIU, this command will mostly be used by libvirt. E.g., on ARM
host, user can choose which kind of GIC device he/she uses. However,
not all of them are supported, and it is decided by both the ARM
hardware and the software (here software should include at least
QEMU and the kernel version). With this command, libvirt can easily
know what is supported, and what is not.

So it can: warn the user before-hand during configuration like "GIC
emulated/kvm-accelerated version X is/isn't supported",

rather than: firstly, configuration success. After that, QEMU boot
failed with ambiguous error.

I can add more explainations into the commit message (some can be
added to the schema directly possibly) about why we need this
command, and what does every entry mean.

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
> On 03/17/2016 09:27 PM, Peter Xu wrote:
> > +##
> > +# @GICCapability:
> > +#
> > +# This struct describes capability for a specific GIC version. These
> 
> Might be nice to spell out what the acronym GIC means, but that's cosmetic.

Ah! I thought I have added that... It's missing again. Will do in
next spin.

> 
> > +# 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' } }
> > 
> 
> I might have squashed this with the patch that first uses GICCapability,
> as defining a type in isolation doesn't do much.

I can do the squash in next spin if you prefer that one. Actually I
got this question before, about when I should split and when to
squash. E.g., shall I make sure that I should have no "definition
only" patches in the future?

-- peterx

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Peter Xu]

On Tue, Mar 22, 2016 at 12:42:42PM -0600, Eric Blake wrote:
> On 03/17/2016 09:27 PM, Peter Xu wrote:
> > This patch adds the command "query-gic-capabilities" but not implemnet
> 
> s/not implemnet/does not implement/

Yep, again. Thanks.

> 
> > 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>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -4156,3 +4156,14 @@
> >    'data': { 'version': 'int',
> >              'emulated': 'bool',
> >              'kernel': 'bool' } }
> > +
> > +##
> > +# @query-gic-capabilities:
> > +#
> > +# Return a list of supported GIC version capabilities.
> > +#
> > +# Returns: a list of GICCapability.
> > +#
> > +# Since: 2.6
> > +##
> > +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
> 
> On the surface, this seems okay.  As mentioned before, I would have
> squashed 1 and 2 into a single patch.  The GICCapability type is
> extensible, and introspection is sufficient at seeing what the type is
> currently capable of exposing.
> 
> On the other hand...
> 
> > +++ b/scripts/qapi.py
> > @@ -46,6 +46,7 @@ returns_whitelist = [
> >      'query-tpm-models',
> >      'query-tpm-types',
> >      'ringbuf-read',
> > +    'query-gic-capability',
> 
> ...it required a whitelist, because you are violating the usual
> convention of returning a dict.  If you DO need the whitelist, your
> addition should have been kept sorted.  But you don't need it, if you
> would modify your QAPI to return a dict:
> 
> { 'struct': 'GICCapabilitiesReturn',
>   'data': { 'capabilities': ['GICCapability'] } }
> { 'command': 'query-gic-capabilities',
>   'returns': 'GICCapabilitiesReturn' }
> 
> Yes, the dict has only a single key, and that key points to the same
> list; but now you have future extensibility: in the future, we could
> return any future global data as a sibling to the array, without having
> to modify every element of the array to repeat redundant information.

Yes, I think this is better solution. Will adopt this in next version.

Thanks for the comments!

-- peterx

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Peter Xu]

On Tue, Mar 22, 2016 at 07:28:13PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index 6b2aa6e..716474e 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -46,6 +46,7 @@ returns_whitelist = [
>    # Whitelist of commands allowed to return a non-dictionary
>    returns_whitelist = [
>        'human-monitor-command',
>        'qom-get',
>        'query-migrate-cache-size',
> >      'query-tpm-models',
> >      'query-tpm-types',
> >      'ringbuf-read',
> > +    'query-gic-capability',
> >  
> >      # From QGA:
> >      'guest-file-open',
> 
> The whitelist exists to except existing commands from design rules on
> return types.  New commands don't get to violate the rules without a
> really, really compelling reason.
> 
> Do you actually need this?
> 
> If yes, why should your command be permitted to violate the design
> rules?

This might not be required. Agree with you and Eric. Will use a hash
instead with single key.

> > +#include "qemu/osdep.h"
> > +#include "hw/boards.h"
> > +#include "qemu/error-report.h"
> > +#include "sysemu/kvm.h"
> > +#include "qmp-commands.h"
> 
> I very much doubt you need all these includes.  Try dropping all but the
> first and the last one.

I just copied them from machine.c and dropped lots, seems still
redundant... Will keep it a minimum subset in next version. Thanks!

-- peterx

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

[Peter Xu]

On Tue, Mar 22, 2016 at 03:20:25PM +0100, Markus Armbruster wrote:
> We discussed the need for a yet another ad hoc query command.  Can you
> please summarize the discussion and its conclusion?  Explaining *why* a
> feature is needed is always a good idea.  Cover letter and commit
> messages are good places for it.

Sure. I will mention that in commit message of patch 1 in the next
spin.  Thanks.

-- peterx

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

[Peter Xu]

On Tue, Mar 22, 2016 at 02:49:19PM +0000, Peter Maydell wrote:
> On 18 March 2016 at 03:27, Peter Xu <peterx@redhat.com> wrote:
> > 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}]}
> 
> Reminder: I still need confirmation from the libvirt folks
> that this API meets their needs, and review from somebody
> on the QEMU QMP protocol design side. This should go into
> 2.6 but we are getting closer to hardfreeze...

Hi, Peter,

Regarding to QMP part, I have got review comments from Eric/Markus,
and will do more spins if necessary (latest v6). For libvirt part,
Andrea should be the one to finish the libvirt part of this
feature, so I am considering him to be the libvirt guy. Do you think
we need more review besides CCed, especially on libvirt part?

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> >> +##
>> >> +# @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' } }
>> >
>> > Are all four combinations of (emulated, kernel) possible?
>
> Currently, version can only be 2 or 3. And for now, we should only
> support:
>
> - emulated/kernel support for v2
> - kernel support for v3
>
> and emulated v3 is still not supported.
>
>> Moreover, what do the combinations mean from a practical point of view?
>> What would a management application do with the information?
>
> AFAIU, this command will mostly be used by libvirt. E.g., on ARM
> host, user can choose which kind of GIC device he/she uses. However,
> not all of them are supported, and it is decided by both the ARM
> hardware and the software (here software should include at least
> QEMU and the kernel version). With this command, libvirt can easily
> know what is supported, and what is not.
>
> So it can: warn the user before-hand during configuration like "GIC
> emulated/kvm-accelerated version X is/isn't supported",
>
> rather than: firstly, configuration success. After that, QEMU boot
> failed with ambiguous error.
>
> I can add more explainations into the commit message (some can be
> added to the schema directly possibly) about why we need this
> command, and what does every entry mean.

So GICCapability essentially tells its users whether certain
configurations have a chance to work.

I think what's missing in your patch is the connection from
GICCapability to the actual configuration options.  As is, you just have
to know what options the presence of each possible GICCapability value
unlocks.  It needs to be documented instead.

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 12:32:28PM -0600, Eric Blake wrote:
>> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> > +##
>> > +# @GICCapability:
>> > +#
>> > +# This struct describes capability for a specific GIC version. These
>> 
>> Might be nice to spell out what the acronym GIC means, but that's cosmetic.
>
> Ah! I thought I have added that... It's missing again. Will do in
> next spin.
>
>> 
>> > +# 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' } }
>> > 
>> 
>> I might have squashed this with the patch that first uses GICCapability,
>> as defining a type in isolation doesn't do much.
>
> I can do the squash in next spin if you prefer that one. Actually I
> got this question before, about when I should split and when to
> squash. E.g., shall I make sure that I should have no "definition
> only" patches in the future?

Depends.

The general rule is to keep separate things separate, and patches
self-contained.  The narrow sense of self-contained is each patch
compiles and works.  The wider sense is each patch makes sense to its
readers on its own.  You can't always have a perfect score on the
latter, but you should try.

Adding a definition without a user is generally not advised, because you
generally need to see the user to make sense of it.

For a complex feature, adding its abstract interface before its concrete
implementation may help liberate interface review from implementation
details.

Note that your interface consists of type GICCapability and command
query-gic-capabilities.  You could add just the interface with a stub
implementation first, then flesh out the implementation.  But I doubt
the thing is complex enough to justify that.

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 07:28:13PM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> > diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > index 6b2aa6e..716474e 100644
>> > --- a/scripts/qapi.py
>> > +++ b/scripts/qapi.py
>> > @@ -46,6 +46,7 @@ returns_whitelist = [
>>    # Whitelist of commands allowed to return a non-dictionary
>>    returns_whitelist = [
>>        'human-monitor-command',
>>        'qom-get',
>>        'query-migrate-cache-size',
>> >      'query-tpm-models',
>> >      'query-tpm-types',
>> >      'ringbuf-read',
>> > +    'query-gic-capability',
>> >  
>> >      # From QGA:
>> >      'guest-file-open',
>> 
>> The whitelist exists to except existing commands from design rules on
>> return types.  New commands don't get to violate the rules without a
>> really, really compelling reason.
>> 
>> Do you actually need this?

I've since tested it and double-checked the code: you don't need this.

>> If yes, why should your command be permitted to violate the design
>> rules?
>
> This might not be required. Agree with you and Eric. Will use a hash
> instead with single key.

The rule against returning non-dictionaries exists to avoid interfaces
that cannot evolve.  With a dictionary, you can evolve by adding
members.

The rule does *not* forbid returning lists of dictionaries.  When a
command fundamentally returns a list of things, being able to evolve the
things suffices.

query-gic-capabilities looks like it fundamentally returns a list of
capabilities.  Returning ['GICCapability'] is just fine then.

Only if query-gic-capabilities doesn't have this list nature should you
complicate its return value by wrapping it in another object type.

In either case, drop the change to returns_whitelist.

[...]

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 22, 2016 at 12:42:42PM -0600, Eric Blake wrote:
>> On 03/17/2016 09:27 PM, Peter Xu wrote:
>> > This patch adds the command "query-gic-capabilities" but not implemnet
>> 
>> s/not implemnet/does not implement/
>
> Yep, again. Thanks.
>
>> 
>> > 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>
>> > ---
>> 
>> > +++ b/qapi-schema.json
>> > @@ -4156,3 +4156,14 @@
>> >    'data': { 'version': 'int',
>> >              'emulated': 'bool',
>> >              'kernel': 'bool' } }
>> > +
>> > +##
>> > +# @query-gic-capabilities:
>> > +#
>> > +# Return a list of supported GIC version capabilities.
>> > +#
>> > +# Returns: a list of GICCapability.
>> > +#
>> > +# Since: 2.6
>> > +##
>> > +{ 'command': 'query-gic-capabilities', 'returns': ['GICCapability'] }
>> 
>> On the surface, this seems okay.  As mentioned before, I would have
>> squashed 1 and 2 into a single patch.  The GICCapability type is
>> extensible, and introspection is sufficient at seeing what the type is
>> currently capable of exposing.
>> 
>> On the other hand...
>> 
>> > +++ b/scripts/qapi.py
>> > @@ -46,6 +46,7 @@ returns_whitelist = [
>> >      'query-tpm-models',
>> >      'query-tpm-types',
>> >      'ringbuf-read',
>> > +    'query-gic-capability',
>> 
>> ...it required a whitelist, because you are violating the usual

It doesn't :)

>> convention of returning a dict.  If you DO need the whitelist, your
>> addition should have been kept sorted.  But you don't need it, if you
>> would modify your QAPI to return a dict:
>> 
>> { 'struct': 'GICCapabilitiesReturn',
>>   'data': { 'capabilities': ['GICCapability'] } }
>> { 'command': 'query-gic-capabilities',
>>   'returns': 'GICCapabilitiesReturn' }
>> 
>> Yes, the dict has only a single key, and that key points to the same
>> list; but now you have future extensibility: in the future, we could
>> return any future global data as a sibling to the array, without having
>> to modify every element of the array to repeat redundant information.
>
> Yes, I think this is better solution. Will adopt this in next version.

As explained in my other message, do this only if query-gic-capabilities
truly needs it.  There's plenty of precedence for returning a list of a
structured type.

> Thanks for the comments!
>
> -- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Wed, Mar 23, 2016 at 10:33:09AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> >> +##
> >> >> +# @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' } }

[*] Marking here...

> So GICCapability essentially tells its users whether certain
> configurations have a chance to work.
> 
> I think what's missing in your patch is the connection from
> GICCapability to the actual configuration options.  As is, you just have
> to know what options the presence of each possible GICCapability value
> unlocks.  It needs to be documented instead.

What I understand is that, above [*] should have explained what does
each entry mean. E.g., as mentioned in the qapi-schema, there are
explainations about "version", "emulated" and "kernel" key words. If
we go deeper into e.g., "emulated" key word, we will got:

"whether current QEMU/hardware supports emulated GIC device in user
 space."

So this boolean will tell just as it is explained.

Maybe I failed to get the point of your review comment... :( Would
you please give an example on how should I better express this
relationship?

(btw, I have updated the commit message in v6 for current patch, to
tell more about why we need this, and why we decided to add this ad
hoc command. I'd be glad if we can continue the discussion based on
that one.  Thanks!)

-- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Wed, Mar 23, 2016 at 10:44:04AM +0100, Markus Armbruster wrote:
> Depends.
> 
> The general rule is to keep separate things separate, and patches
> self-contained.  The narrow sense of self-contained is each patch
> compiles and works.  The wider sense is each patch makes sense to its
> readers on its own.  You can't always have a perfect score on the
> latter, but you should try.
> 
> Adding a definition without a user is generally not advised, because you
> generally need to see the user to make sense of it.
> 
> For a complex feature, adding its abstract interface before its concrete
> implementation may help liberate interface review from implementation
> details.
> 
> Note that your interface consists of type GICCapability and command
> query-gic-capabilities.  You could add just the interface with a stub
> implementation first, then flesh out the implementation.  But I doubt
> the thing is complex enough to justify that.

Thanks for the thorough explaination on this.

It's indeed not easy to figure out the best way every time for
me... Now I do feel it strange to split the first patch alone from
the 2nd one. Anyway, it's squashed into the 2nd patch in v6.

-- peterx

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Peter Xu]

On Wed, Mar 23, 2016 at 10:52:29AM +0100, Markus Armbruster wrote:
> The rule against returning non-dictionaries exists to avoid interfaces
> that cannot evolve.  With a dictionary, you can evolve by adding
> members.
> 
> The rule does *not* forbid returning lists of dictionaries.  When a
> command fundamentally returns a list of things, being able to evolve the
> things suffices.

Ok.

> 
> query-gic-capabilities looks like it fundamentally returns a list of
> capabilities.  Returning ['GICCapability'] is just fine then.

I have posted v6 just as Eric has suggested. At least one advantage
is that it is easier to be extended (if needed) in the future, also
to follow the more-generic format to use dicts rather than
arrays. If you would not mind, I'll keep the dict interface there.

[...]

> In either case, drop the change to returns_whitelist.

Yep. Dropped in v6.

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 23, 2016 at 10:33:09AM +0100, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Tue, Mar 22, 2016 at 07:41:17PM +0100, Markus Armbruster wrote:
>> >> Markus Armbruster <armbru@redhat.com> writes:
>> >> >> +##
>> >> >> +# @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' } }
>
> [*] Marking here...
>
>> So GICCapability essentially tells its users whether certain
>> configurations have a chance to work.
>> 
>> I think what's missing in your patch is the connection from
>> GICCapability to the actual configuration options.  As is, you just have
>> to know what options the presence of each possible GICCapability value
>> unlocks.  It needs to be documented instead.
>
> What I understand is that, above [*] should have explained what does
> each entry mean. E.g., as mentioned in the qapi-schema, there are
> explainations about "version", "emulated" and "kernel" key words. If
> we go deeper into e.g., "emulated" key word, we will got:
>
> "whether current QEMU/hardware supports emulated GIC device in user
>  space."
>
> So this boolean will tell just as it is explained.
>
> Maybe I failed to get the point of your review comment... :( Would
> you please give an example on how should I better express this
> relationship?

Can you tell me what a management application is supposed to do with the
information returned by query-gic-capabilities?  Not just in general
terms, like "using this information, libvirt can warn the user during
configuration of guests when specified GIC device type is not supported,
but specifics.  Something like "-frobnicate mutter=mumble won't work
unless query-gic-capabilities reports emulated version 2 is supported"
for every piece of configuration that should be vetted against
query-gic-capabilities.

> (btw, I have updated the commit message in v6 for current patch, to
> tell more about why we need this, and why we decided to add this ad
> hoc command. I'd be glad if we can continue the discussion based on
> that one.  Thanks!)

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 23, 2016 at 10:52:29AM +0100, Markus Armbruster wrote:
>> The rule against returning non-dictionaries exists to avoid interfaces
>> that cannot evolve.  With a dictionary, you can evolve by adding
>> members.
>> 
>> The rule does *not* forbid returning lists of dictionaries.  When a
>> command fundamentally returns a list of things, being able to evolve the
>> things suffices.
>
> Ok.
>
>> 
>> query-gic-capabilities looks like it fundamentally returns a list of
>> capabilities.  Returning ['GICCapability'] is just fine then.
>
> I have posted v6 just as Eric has suggested. At least one advantage
> is that it is easier to be extended (if needed) in the future, also
> to follow the more-generic format to use dicts rather than
> arrays. If you would not mind, I'll keep the dict interface there.

Returning such a list of a structured type is a well-established
convention by now --- qmp-introspect.c shows 28 commands doing it, most
of them named query-FOO.  I'd rather not start a different convention
now, just because we got temporarily confused about our own rules.  So,
unless there's something about query-gic-capabilities that makes it more
likely to need extension outside its list element type, let's stick to
returning the list.

Eric?

>
> [...]
>
>> In either case, drop the change to returns_whitelist.
>
> Yep. Dropped in v6.
>
> Thanks.
>
> -- peterx

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
> Can you tell me what a management application is supposed to do with the
> information returned by query-gic-capabilities?  Not just in general
> terms, like "using this information, libvirt can warn the user during
> configuration of guests when specified GIC device type is not supported,
> but specifics.  Something like "-frobnicate mutter=mumble won't work
> unless query-gic-capabilities reports emulated version 2 is supported"
> for every piece of configuration that should be vetted against
> query-gic-capabilities.

I suppose that won't be a very big problem since possibly only
libvirt will use it... I agree that it's better to explain it more
clearly though. How about adding these lines into patch 1 commit
message:

"""
For example, if we got the query result:

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

Then it means that we support emulated GIC version 2 using:

  qemu-system-aarch64 -M virt,gic-version=2 ...

or kvm-accelerated GIC version 3 using:

  qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...

If we specify other explicit GIC version rather than the above, QEMU
will not be able to boot.
"""

Thanks.

-- peterx

Re: [Qemu-devel] [PATCH v5 2/5] arm: qmp: add query-gic-capabilities interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On 03/23/2016 08:06 AM, Markus Armbruster wrote:

>>> query-gic-capabilities looks like it fundamentally returns a list of
>>> capabilities.  Returning ['GICCapability'] is just fine then.
>>
>> I have posted v6 just as Eric has suggested. At least one advantage
>> is that it is easier to be extended (if needed) in the future, also
>> to follow the more-generic format to use dicts rather than
>> arrays. If you would not mind, I'll keep the dict interface there.
> 
> Returning such a list of a structured type is a well-established
> convention by now --- qmp-introspect.c shows 28 commands doing it, most
> of them named query-FOO.  I'd rather not start a different convention
> now, just because we got temporarily confused about our own rules.  So,
> unless there's something about query-gic-capabilities that makes it more
> likely to need extension outside its list element type, let's stick to
> returning the list.
> 
> Eric?

Makes sense to me, and perhaps my own fault for getting confused by not
even trying to delete just the whitelist line and seeing what would
happen.  Having a consistent style of query-* returning an array type is
reasonable to continue.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
>> Can you tell me what a management application is supposed to do with the
>> information returned by query-gic-capabilities?  Not just in general
>> terms, like "using this information, libvirt can warn the user during
>> configuration of guests when specified GIC device type is not supported,
>> but specifics.  Something like "-frobnicate mutter=mumble won't work
>> unless query-gic-capabilities reports emulated version 2 is supported"
>> for every piece of configuration that should be vetted against
>> query-gic-capabilities.
>
> I suppose that won't be a very big problem since possibly only
> libvirt will use it... I agree that it's better to explain it more
> clearly though. How about adding these lines into patch 1 commit
> message:
>
> """
> For example, if we got the query result:
>
> {"return": {"capabilities":
>   [ {"emulated": false, "version": 3, "kernel": true},
>     {"emulated": true, "version": 2, "kernel": false} ] } }
>
> Then it means that we support emulated GIC version 2 using:
>
>   qemu-system-aarch64 -M virt,gic-version=2 ...
>
> or kvm-accelerated GIC version 3 using:
>
>   qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...

I'd say "qemu-system-aarch64 -M virt,accel=kvm,gic=version=3"

> If we specify other explicit GIC version rather than the above, QEMU
> will not be able to boot.
> """

Works for me.

Re: [Qemu-devel] [PATCH v5 1/5] arm: qmp: add GICCapability struct

[Peter Xu]

On Wed, Mar 23, 2016 at 04:17:25PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Mar 23, 2016 at 01:21:52PM +0100, Markus Armbruster wrote:
> >> Can you tell me what a management application is supposed to do with the
> >> information returned by query-gic-capabilities?  Not just in general
> >> terms, like "using this information, libvirt can warn the user during
> >> configuration of guests when specified GIC device type is not supported,
> >> but specifics.  Something like "-frobnicate mutter=mumble won't work
> >> unless query-gic-capabilities reports emulated version 2 is supported"
> >> for every piece of configuration that should be vetted against
> >> query-gic-capabilities.
> >
> > I suppose that won't be a very big problem since possibly only
> > libvirt will use it... I agree that it's better to explain it more
> > clearly though. How about adding these lines into patch 1 commit
> > message:
> >
> > """
> > For example, if we got the query result:
> >
> > {"return": {"capabilities":
> >   [ {"emulated": false, "version": 3, "kernel": true},
> >     {"emulated": true, "version": 2, "kernel": false} ] } }
> >
> > Then it means that we support emulated GIC version 2 using:
> >
> >   qemu-system-aarch64 -M virt,gic-version=2 ...
> >
> > or kvm-accelerated GIC version 3 using:
> >
> >   qemu-system-aarch64 -enable-kvm -M virt,gic-version=3 ...
> 
> I'd say "qemu-system-aarch64 -M virt,accel=kvm,gic=version=3"
> 
> > If we specify other explicit GIC version rather than the above, QEMU
> > will not be able to boot.
> > """
> 
> Works for me.

Thanks, will include this in v7.

-- peterx