Re: [Qemu-devel] [PATCH v4 3/3] arm: implement query-gic-capabilities

[Peter Xu]

On Wed, Mar 16, 2016 at 10:30:36AM +0000, Peter Maydell wrote:
> On 8 March 2016 at 07:36, Peter Xu <address@hidden> wrote:
> > For emulated GIC capabilities, currently only gicv2 is supported. We
> > need to add gicv3 in when emulated gicv3 ready. For KVM accelerated ARM
> > VM, we detect the capability bits using ioctls.
> >
> > When probing the KVM capabilities, we cannot leverage existing helper
> > functions like kvm_create_device() since QEMU might be using TCG while
> > probing (actually this is the case for libvirt probing). So, one
> > temporary VM is created to do the probing.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  target-arm/machine.c | 94 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 93 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/machine.c b/target-arm/machine.c
> > index 813909e..8f52f74 100644
> > --- a/target-arm/machine.c
> > +++ b/target-arm/machine.c
> > @@ -1,3 +1,5 @@
> > +#include <linux/kvm.h>
> 
> This will break compilation on non-Linux hosts; you can't include
> linux headers like this.
> 
> (This is all in the wrong file anyway, machine.c is for migration.)

Okay. Will move all kvm lines into kvm*.c.

> 
> > +#include <sys/ioctl.h>
> >  #include "qemu/osdep.h"
> >  #include "hw/hw.h"
> >  #include "hw/boards.h"
> > @@ -347,7 +349,97 @@ const char *gicv3_class_name(void)
> >      exit(1);
> >  }
> >
> > +static GICCapability *gic_cap_new(int version)
> > +{
> > +    GICCapability *cap = g_new0(GICCapability, 1);
> > +    cap->version = version;
> > +    /* by default, support none */
> > +    cap->emulated = false;
> > +    cap->kernel = false;
> > +    return cap;
> > +}
> > +
> > +static GICCapabilityList *gic_cap_list_add(GICCapabilityList *head,
> > +                                           GICCapability *cap)
> > +{
> > +    GICCapabilityList *item = g_new0(GICCapabilityList, 1);
> > +    item->value = cap;
> > +    item->next = head;
> > +    return item;
> > +}
> > +
> > +#ifdef CONFIG_KVM
> > +/* Test whether KVM support specific device. */
> > +static inline int kvm_support_device(int vmfd, uint64_t type)
> > +{
> > +    struct kvm_create_device create_dev = {
> > +        .type = type,
> > +        .fd = -1,
> > +        .flags = KVM_CREATE_DEVICE_TEST,
> > +    };
> > +    return ioctl(vmfd, KVM_CREATE_DEVICE, &create_dev);
> > +}
> > +#endif
> 
> This is not ARM specific so it should go in kvm-all.c.

Will do.

> 
> > +
> >  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
> 
> KVM specific code should be factored out and live in one of
> the target-arm/kvm*.c files.

Ok, will fix.

> 
> > +    {
> > +        int kvm_fd = -1;
> > +        int vmfd = -1;
> > +        /*
> > +         * HACK: here we create one temporary VM, do the probing,
> > +         * then release it properly.
> > +         */
> > +        kvm_fd = qemu_open("/dev/kvm", O_RDWR);
> > +        if (kvm_fd == -1) {
> > +            /* KVM may not enabled on host, which is fine. */
> > +            goto out;
> > +        }
> > +
> > +        do {
> > +            /* For ARM, VM type could only be zero now. */
> > +            vmfd = ioctl(kvm_fd, KVM_CREATE_VM, 0);
> > +        } while (vmfd == -EINTR);
> > +
> > +        if (vmfd < 0) {
> > +            goto kvm_fd_close;
> > +        }
> 
> Rather than open-coding this you might as well use
> kvm_arm_creat_scratch_host_vcpu() (you don't need the vcpu fd but
> it's pretty harmless to create it.)

Thanks for the func pointer! That's what I was looking for.

> 
> 
> > +
> > +        if (ioctl(kvm_fd, KVM_CHECK_EXTENSION,
> > +                  KVM_CAP_DEVICE_CTRL) <= 0) {
> > +            /* older version of KVM possibly */
> > +            goto kvm_vmfd_close;
> > +        }
> 
> Do this in kvm_support_device() [mostly just to parallel how
> kvm_create_device() does it.]

Will fix.

Thanks!

-- peterx