[Qemu-devel] [PATCH 0/2] Expose KVM pv features

[Qemu-devel] [PATCH 0/2] Expose KVM pv features

[Glauber Costa]

Hi guys,

This is more or less the same patches I've sent, but with only
pv features being exposed. It seemed to me that this one faced
very little opposition, and so I decided to keep the host bit masking
out. Also, there seems to be an ongoing work by Amit on the same thing,
so let's not duplicate it.

[Qemu-devel] [PATCH 1/2] Factor out common code in filling cpuid code

[Glauber Costa]

Use kvm_fill_cpuid to query qemu for cpuid data. There are two exactly equal
instances of this code, and the future introduction of kvm paravirt features
will make it three.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 target-i386/kvm.c |   49 ++++++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 49766e2..4a55931 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -33,22 +33,19 @@
     do { } while (0)
 #endif
 
-int kvm_arch_init_vcpu(CPUState *env)
+typedef struct {
+    struct kvm_cpuid cpuid;
+    struct kvm_cpuid_entry entries[100];
+} KVMCpuid;
+
+static void kvm_fill_cpuid(CPUState *env, KVMCpuid *cpuid_data,
+                               uint32_t start, uint32_t limit)
 {
-    struct {
-        struct kvm_cpuid cpuid;
-        struct kvm_cpuid_entry entries[100];
-    } __attribute__((packed)) cpuid_data;
-    uint32_t limit, i, cpuid_i;
+    int i;
     uint32_t eax, ebx, ecx, edx;
 
-    cpuid_i = 0;
-
-    cpu_x86_cpuid(env, 0, &eax, &ebx, &ecx, &edx);
-    limit = eax;
-
-    for (i = 0; i <= limit; i++) {
-        struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+    for (i = start; i <= limit; i++) {
+        struct kvm_cpuid_entry *c = &cpuid_data->entries[cpuid_data->cpuid.nent++];
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;
@@ -57,22 +54,24 @@ int kvm_arch_init_vcpu(CPUState *env)
         c->ecx = ecx;
         c->edx = edx;
     }
+}
 
-    cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
-    limit = eax;
+int kvm_arch_init_vcpu(CPUState *env)
+{
+    KVMCpuid cpuid_data;
+    uint32_t limit;
+    uint32_t eax, ebx, ecx, edx;
 
-    for (i = 0x80000000; i <= limit; i++) {
-        struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+    cpuid_data.cpuid.nent = 0;
 
-        cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
-        c->function = i;
-        c->eax = eax;
-        c->ebx = ebx;
-        c->ecx = ecx;
-        c->edx = edx;
-    }
+    cpu_x86_cpuid(env, 0, &eax, &ebx, &ecx, &edx);
+    limit = eax;
 
-    cpuid_data.cpuid.nent = cpuid_i;
+    kvm_fill_cpuid(env, &cpuid_data, 0, limit);
+
+    cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
+    limit = eax;
+    kvm_fill_cpuid(env, &cpuid_data, 0x80000000, limit);
 
     return kvm_vcpu_ioctl(env, KVM_SET_CPUID, &cpuid_data);
 }
-- 
1.5.6.5

[Qemu-devel] [PATCH 2/2] expose kvm pv features

[Glauber Costa]

expose kvm paravirtual features into cpuid. This enables
the use of kvmclock in qemu guests. (and all the other
features too).

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 kvm.h             |    5 +++++
 target-i386/kvm.c |   29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/kvm.h b/kvm.h
index efce145..49a2653 100644
--- a/kvm.h
+++ b/kvm.h
@@ -17,6 +17,8 @@
 #include "config.h"
 
 #ifdef CONFIG_KVM
+#include <linux/kvm.h>
+#include <linux/kvm_para.h>
 extern int kvm_allowed;
 
 #define kvm_enabled() (kvm_allowed)
@@ -76,4 +78,7 @@ int kvm_arch_init(KVMState *s, int smp_cpus);
 
 int kvm_arch_init_vcpu(CPUState *env);
 
+/* x86 specific */
+uint32_t kvm_get_para_features(CPUState *env);
+
 #endif
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 4a55931..736ee38 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -33,6 +33,33 @@
     do { } while (0)
 #endif
 
+static struct kvm_para_features {
+    int cap;
+    int feature;
+} para_features[] = {
+#ifdef KVM_CAP_CLOCKSOURCE
+       { KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
+#endif
+#ifdef KVM_CAP_NOP_IO_DELAY
+       { KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
+#endif
+#ifdef KVM_CAP_PV_MMU
+       { KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
+#endif
+};
+
+uint32_t kvm_get_para_features(CPUState *env)
+ {
+    uint32_t i, features = 0;
+
+    for (i = 0; i < ARRAY_SIZE(para_features); i++) {
+        if (kvm_ioctl(env->kvm_state, KVM_CHECK_EXTENSION, para_features[i].cap))
+            features |= (1 << para_features[i].feature);
+    }
+
+    return features;
+}
+
 typedef struct {
     struct kvm_cpuid cpuid;
     struct kvm_cpuid_entry entries[100];
@@ -69,6 +96,8 @@ int kvm_arch_init_vcpu(CPUState *env)
 
     kvm_fill_cpuid(env, &cpuid_data, 0, limit);
 
+    kvm_fill_cpuid(env, &cpuid_data, KVM_CPUID_SIGNATURE, KVM_CPUID_FEATURES);
+
     cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
     limit = eax;
     kvm_fill_cpuid(env, &cpuid_data, 0x80000000, limit);
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH 2/2] expose kvm pv features

[Anthony Liguori]

Glauber Costa wrote:
> expose kvm paravirtual features into cpuid. This enables
> the use of kvmclock in qemu guests. (and all the other
> features too).
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  kvm.h             |    5 +++++
>  target-i386/kvm.c |   29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/kvm.h b/kvm.h
> index efce145..49a2653 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -17,6 +17,8 @@
>  #include "config.h"
>
>  #ifdef CONFIG_KVM
> +#include <linux/kvm.h>
> +#include <linux/kvm_para.h>
>  extern int kvm_allowed;
>   

This breaks the build in an admittedly subtle way.  kvm.h is included in 
various c files throughout QEMU.  However, in Makefile.target, we have:

kvm.o: CFLAGS+=$(KVM_CFLAGS)
kvm-all.o: CFLAGS+=$(KVM_CFLAGS)

And KVM_CFLAGS contains flags derived from --kerneldir.  But now you're 
relying on all C files being able to pull in kernel headers.

Looking at the patch, why put this includes in kvm.h at all?

Regards,

Anthony Liguori

>  #define kvm_enabled() (kvm_allowed)
> @@ -76,4 +78,7 @@ int kvm_arch_init(KVMState *s, int smp_cpus);
>
>  int kvm_arch_init_vcpu(CPUState *env);
>
> +/* x86 specific */
> +uint32_t kvm_get_para_features(CPUState *env);
> +
>  #endif
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 4a55931..736ee38 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -33,6 +33,33 @@
>      do { } while (0)
>  #endif
>
> +static struct kvm_para_features {
> +    int cap;
> +    int feature;
> +} para_features[] = {
> +#ifdef KVM_CAP_CLOCKSOURCE
> +       { KVM_CAP_CLOCKSOURCE, KVM_FEATURE_CLOCKSOURCE },
> +#endif
> +#ifdef KVM_CAP_NOP_IO_DELAY
> +       { KVM_CAP_NOP_IO_DELAY, KVM_FEATURE_NOP_IO_DELAY },
> +#endif
> +#ifdef KVM_CAP_PV_MMU
> +       { KVM_CAP_PV_MMU, KVM_FEATURE_MMU_OP },
> +#endif
> +};
> +
> +uint32_t kvm_get_para_features(CPUState *env)
> + {
> +    uint32_t i, features = 0;
> +
> +    for (i = 0; i < ARRAY_SIZE(para_features); i++) {
> +        if (kvm_ioctl(env->kvm_state, KVM_CHECK_EXTENSION, para_features[i].cap))
> +            features |= (1 << para_features[i].feature);
> +    }
> +
> +    return features;
> +}
> +
>  typedef struct {
>      struct kvm_cpuid cpuid;
>      struct kvm_cpuid_entry entries[100];
> @@ -69,6 +96,8 @@ int kvm_arch_init_vcpu(CPUState *env)
>
>      kvm_fill_cpuid(env, &cpuid_data, 0, limit);
>
> +    kvm_fill_cpuid(env, &cpuid_data, KVM_CPUID_SIGNATURE, KVM_CPUID_FEATURES);
> +
>      cpu_x86_cpuid(env, 0x80000000, &eax, &ebx, &ecx, &edx);
>      limit = eax;
>      kvm_fill_cpuid(env, &cpuid_data, 0x80000000, limit);
>   

[Qemu-devel] Re: [PATCH 2/2] expose kvm pv features

[Glauber Costa]

On Fri, Feb 06, 2009 at 12:46:18PM -0600, Anthony Liguori wrote:
> Glauber Costa wrote:
>> expose kvm paravirtual features into cpuid. This enables
>> the use of kvmclock in qemu guests. (and all the other
>> features too).
>>
>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>> ---
>>  kvm.h             |    5 +++++
>>  target-i386/kvm.c |   29 +++++++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/kvm.h b/kvm.h
>> index efce145..49a2653 100644
>> --- a/kvm.h
>> +++ b/kvm.h
>> @@ -17,6 +17,8 @@
>>  #include "config.h"
>>
>>  #ifdef CONFIG_KVM
>> +#include <linux/kvm.h>
>> +#include <linux/kvm_para.h>
>>  extern int kvm_allowed;
>>   
>
> This breaks the build in an admittedly subtle way.  kvm.h is included in  
> various c files throughout QEMU.  However, in Makefile.target, we have:
for the record, it wfm ;-)

>
> kvm.o: CFLAGS+=$(KVM_CFLAGS)
> kvm-all.o: CFLAGS+=$(KVM_CFLAGS)
>
> And KVM_CFLAGS contains flags derived from --kerneldir.  But now you're  
> relying on all C files being able to pull in kernel headers.
yeah, agreed.
>
> Looking at the patch, why put this includes in kvm.h at all?
You mean we should put them somewhere else, or not put them at all?

We need the headers to exist since we'll be dealing with capabilities
and feature bits that kvm defines. But given this problem, they can go fine
in kvm.c