diff mbox

[v2,3/3] arm: implement query-gic-capability

Message ID 1456993272-32292-4-git-send-email-peterx@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Xu March 3, 2016, 8:21 a.m. UTC
For emulated ARM VM, only gicv2 is supported. We need to add gicv3 in
when emulated gicv3 ready. For KVM accelerated ARM VM, we detect the
capability bits using ioctls.

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

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

Comments

Andrea Bolognani March 3, 2016, 12:19 p.m. UTC | #1
On Thu, 2016-03-03 at 16:21 +0800, Peter Xu wrote:
> For emulated ARM VM, only gicv2 is supported. We need to add gicv3
in
> when emulated gicv3 ready. For KVM accelerated ARM VM, we detect the
> capability bits using ioctls.

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

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

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

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

Cheers.

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

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

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

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

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

    close(vm);
    close(kvm);
}

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

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

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

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

Thanks.
Peter
Andrea Bolognani March 4, 2016, 8:43 a.m. UTC | #3
On Fri, 2016-03-04 at 10:52 +0800, Peter Xu wrote:
> > Sorry for not catching this earlier, but I'm afraid this is not
> > going to work -- libvirt doesn't pass either -enable-kvm or the
> > machine option accel=kvm when probing for capabilities, which
> > means that, with the current implementation, it will only get
> > information about emulated GIC.
> > 
> > Is there a way to make probing work without requiring KVM to
> > be enabled?
> Ah.. If so, this is a good point...

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

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

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

>     close(vm);
>     close(kvm);
> }

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

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

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

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

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

Cheers.

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

Agreed. It is awkward to specify these for probing.

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

Thanks!
Peter
diff mbox

Patch

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