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

[Anthony Liguori <aliguori@us.ibm.com>]

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