kvm: Add helpers for checking and requiring kvm extensions

Commit Message

Avi Kivity May 3, 2009, 11:34 a.m. UTC

Instead of open-coding the check extension sequence, provide helpers
for checking whether an extension exists, and for aborting if an
extension is missing.

Signed-off-by: Avi Kivity <[e-mail]>
---
 kvm-all.c         |   63 +++++++++++++++++++++++++++--------------------------
 kvm.h             |    6 +++++
 target-i386/kvm.c |    8 +------
 3 files changed, 39 insertions(+), 38 deletions(-)

Comments

Juan Quintela May 4, 2009, 6:49 a.m. UTC | #1

Avi Kivity <[e-mail]> wrote:

Hi

> diff --git a/kvm-all.c b/kvm-all.c
> index 36659a9..1642a2a 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -64,6 +64,30 @@ struct KVMState
>  
>  static KVMState *kvm_state;
>  
> +int kvm_check_extension(int extension)
> +{
> +    int ret;
> +
> +    ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, extension);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM_CHECK_EXTENSION failed: %s\n", strerror(errno));
> +        exit(1);
> +    }
> +    return ret;
> +}
>
Are you sure you want the exit(1) in this case?
With the exit() call, you are unable to check if one extension is
present at all.  And you check the return of the following code.
>      s->coalesced_mmio = 0;
>  #ifdef KVM_CAP_COALESCED_MMIO
> -    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
> -    if (ret > 0)
> -        s->coalesced_mmio = ret;
> +    s->coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
>  #endif

You can remove the ifdef at this point.

Later, Juan.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Avi Kivity May 4, 2009, 6:57 a.m. UTC | #2

Juan Quintela wrote:
> Avi Kivity <[e-mail]> wrote:
>
> Hi
>
>   
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 36659a9..1642a2a 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -64,6 +64,30 @@ struct KVMState
>>  
>>  static KVMState *kvm_state;
>>  
>> +int kvm_check_extension(int extension)
>> +{
>> +    int ret;
>> +
>> +    ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, extension);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM_CHECK_EXTENSION failed: %s\n", strerror(errno));
>> +        exit(1);
>> +    }
>> +    return ret;
>> +}
>>
>>     
> Are you sure you want the exit(1) in this case?
>   

A negative result of kvm_ioctl() means the ioctl itself failed.  This 
shouldn't happen, so we exit.

If the extension is not present, it returns 0.

> With the exit() call, you are unable to check if one extension is
> present at all.  And you check the return of the following code.
>   
>>      s->coalesced_mmio = 0;
>>  #ifdef KVM_CAP_COALESCED_MMIO
>> -    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
>> -    if (ret > 0)
>> -        s->coalesced_mmio = ret;
>> +    s->coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
>>  #endif
>>     
>
>   

Here we use the return value (0 or 1).

> You can remove the ifdef at this point.
>   

It won't compile if KVM_CAP_COLAESCED_MMIO is not defined.

Patch

diff --git a/kvm-all.c b/kvm-all.c
index 36659a9..1642a2a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -64,6 +64,30 @@  struct KVMState
 
 static KVMState *kvm_state;
 
+int kvm_check_extension(int extension)
+{
+    int ret;
+
+    ret = kvm_ioctl(kvm_state, KVM_CHECK_EXTENSION, extension);
+    if (ret < 0) {
+        fprintf(stderr, "KVM_CHECK_EXTENSION failed: %s\n", strerror(errno));
+        exit(1);
+    }
+    return ret;
+}
+
+int kvm_require_extension(int extension, const char *estr)
+{
+    int ret;
+
+    ret = kvm_check_extension(extension);
+    if (!ret) {
+        fprintf(stderr, "Required KVM extension %s not present\n", estr);
+        exit(1);
+    }
+    return ret;
+}
+
 static KVMSlot *kvm_alloc_slot(KVMState *s)
 {
     int i;
@@ -331,6 +355,8 @@  int kvm_init(int smp_cpus)
 
     s = qemu_mallocz(sizeof(KVMState));
 
+    kvm_state = s;
+
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     TAILQ_INIT(&s->kvm_sw_breakpoints);
 #endif
@@ -368,42 +394,22 @@  int kvm_init(int smp_cpus)
      * just use a user allocated buffer so we can use regular pages
      * unmodified.  Make sure we have a sufficiently modern version of KVM.
      */
-    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_USER_MEMORY);
-    if (ret <= 0) {
-        if (ret == 0)
-            ret = -EINVAL;
-        fprintf(stderr, "kvm does not support KVM_CAP_USER_MEMORY\n");
-        goto err;
-    }
+    KVM_REQUIRE_EXTENSION(KVM_CAP_USER_MEMORY);
 
     /* There was a nasty bug in < kvm-80 that prevents memory slots from being
      * destroyed properly.  Since we rely on this capability, refuse to work
      * with any kernel without this capability. */
-    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION,
-                    KVM_CAP_DESTROY_MEMORY_REGION_WORKS);
-    if (ret <= 0) {
-        if (ret == 0)
-            ret = -EINVAL;
-
-        fprintf(stderr,
-                "KVM kernel module broken (DESTROY_MEMORY_REGION)\n"
-                "Please upgrade to at least kvm-81.\n");
-        goto err;
-    }
+    KVM_REQUIRE_EXTENSION(KVM_CAP_DESTROY_MEMORY_REGION_WORKS);
 
     s->coalesced_mmio = 0;
 #ifdef KVM_CAP_COALESCED_MMIO
-    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);
-    if (ret > 0)
-        s->coalesced_mmio = ret;
+    s->coalesced_mmio = kvm_check_extension(KVM_CAP_COALESCED_MMIO);
 #endif
 
     ret = kvm_arch_init(s, smp_cpus);
     if (ret < 0)
         goto err;
 
-    kvm_state = s;
-
     return 0;
 
 err:
@@ -415,6 +421,8 @@  err:
     }
     qemu_free(s);
 
+    kvm_state = NULL;
+
     return ret;
 }
 
@@ -763,14 +771,7 @@  int kvm_vcpu_ioctl(CPUState *env, int type, ...)
 
 int kvm_has_sync_mmu(void)
 {
-#ifdef KVM_CAP_SYNC_MMU
-    KVMState *s = kvm_state;
-
-    if (kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_SYNC_MMU) > 0)
-        return 1;
-#endif
-
-    return 0;
+    return kvm_check_extension(KVM_CAP_SYNC_MMU);
 }
 
 void kvm_setup_guest_memory(void *start, size_t size)
diff --git a/kvm.h b/kvm.h
index 0ea2426..bd4e8d4 100644
--- a/kvm.h
+++ b/kvm.h
@@ -29,6 +29,12 @@  struct kvm_run;
 
 /* external API */
 
+#define KVM_REQUIRE_EXTENSION(extension) \
+    kvm_require_extension(extension, #extension)
+
+int kvm_check_extension(int extension);
+int kvm_require_extension(int extension, const char *estr);
+
 int kvm_init(int smp_cpus);
 
 int kvm_init_vcpu(CPUState *env);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2de8b81..b534b2d 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -154,19 +154,13 @@  static int kvm_has_msr_star(CPUState *env)
 
 int kvm_arch_init(KVMState *s, int smp_cpus)
 {
-    int ret;
-
     /* create vm86 tss.  KVM uses vm86 mode to emulate 16-bit code
      * directly.  In order to use vm86 mode, a TSS is needed.  Since this
      * must be part of guest physical memory, we need to allocate it.  Older
      * versions of KVM just assumed that it would be at the end of physical
      * memory but that doesn't work with more than 4GB of memory.  We simply
      * refuse to work with those older versions of KVM. */
-    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_SET_TSS_ADDR);
-    if (ret <= 0) {
-        fprintf(stderr, "kvm does not support KVM_CAP_SET_TSS_ADDR\n");
-        return ret;
-    }
+    KVM_REQUIRE_EXTENSION(KVM_CAP_SET_TSS_ADDR);
 
     /* this address is 3 pages before the bios, and the bios should present
      * as unavaible memory.  FIXME, need to ensure the e820 map deals with