Re: [Qemu-devel] [PATCHv3] Add KVM paravirt cpuid leaf

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/13/2010 07:25 AM, Gleb Natapov wrote:
> Initialize KVM paravirt cpuid leaf and allow user to control guest
> visible PV features through -cpu flag.
>
> Signed-off-by: Gleb Natapov<gleb@redhat.com>
> ---
>   v1->v2
>    fix indentation
>    remove unneeded ifdefs
>   v2->v3
>    added needed ifdefs (CONFIG_KVM_PARA)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index f3834b3..216b00e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -701,7 +701,8 @@ typedef struct CPUX86State {
>       uint8_t nmi_pending;
>       uint8_t has_error_code;
>       uint32_t sipi_vector;
> -
> +    uint32_t cpuid_kvm_features;
> +
>       /* in order to simplify APIC support, we leave this pointer to the
>          user */
>       struct APICState *apic_state;
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 049fccf..70762bb 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -58,10 +58,18 @@ static const char *ext3_feature_name[] = {
>       NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>   };
>
> +static const char *kvm_feature_name[] = {
> +    "kvmclock", "kvm_nopiodelay", "kvm_mmu", NULL, NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +};
> +
>   static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
>                                       uint32_t *ext_features,
>                                       uint32_t *ext2_features,
> -                                    uint32_t *ext3_features)
> +                                    uint32_t *ext3_features,
> +                                    uint32_t *kvm_features)
>   {
>       int i;
>       int found = 0;
> @@ -86,6 +94,12 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
>               *ext3_features |= 1<<  i;
>               found = 1;
>           }
> +    for ( i = 0 ; i<  32 ; i++ )
> +        if (kvm_feature_name[i]&&  !strcmp (flagname, kvm_feature_name[i])) {
> +            *kvm_features |= 1<<  i;
> +            found = 1;
> +        }
> +
>       if (!found) {
>           fprintf(stderr, "CPU feature %s not found\n", flagname);
>       }
> @@ -98,7 +112,7 @@ typedef struct x86_def_t {
>       int family;
>       int model;
>       int stepping;
> -    uint32_t features, ext_features, ext2_features, ext3_features;
> +    uint32_t features, ext_features, ext2_features, ext3_features, kvm_features;
>       uint32_t xlevel;
>       char model_id[48];
>       int vendor_override;
> @@ -375,8 +389,8 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>
>       char *s = strdup(cpu_model);
>       char *featurestr, *name = strtok(s, ",");
> -    uint32_t plus_features = 0, plus_ext_features = 0, plus_ext2_features = 0, plus_ext3_features = 0;
> -    uint32_t minus_features = 0, minus_ext_features = 0, minus_ext2_features = 0, minus_ext3_features = 0;
> +    uint32_t plus_features = 0, plus_ext_features = 0, plus_ext2_features = 0, plus_ext3_features = 0, plus_kvm_features = 0;
> +    uint32_t minus_features = 0, minus_ext_features = 0, minus_ext2_features = 0, minus_ext3_features = 0, minus_kvm_features = 0;
>       uint32_t numvalue;
>
>       def = NULL;
> @@ -394,17 +408,20 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>           memcpy(x86_cpu_def, def, sizeof(*def));
>       }
>
> +    plus_kvm_features = ~0; /* not supported bits will be filtered out later */
> +
>       add_flagname_to_bitmaps("hypervisor",&plus_features,
> -&plus_ext_features,&plus_ext2_features,&plus_ext3_features);
> +&plus_ext_features,&plus_ext2_features,&plus_ext3_features,
> +&plus_kvm_features);
>
>       featurestr = strtok(NULL, ",");
>
>       while (featurestr) {
>           char *val;
>           if (featurestr[0] == '+') {
> -            add_flagname_to_bitmaps(featurestr + 1,&plus_features,&plus_ext_features,&plus_ext2_features,&plus_ext3_features);
> +            add_flagname_to_bitmaps(featurestr + 1,&plus_features,&plus_ext_features,&plus_ext2_features,&plus_ext3_features,&plus_kvm_features);
>           } else if (featurestr[0] == '-') {
> -            add_flagname_to_bitmaps(featurestr + 1,&minus_features,&minus_ext_features,&minus_ext2_features,&minus_ext3_features);
> +            add_flagname_to_bitmaps(featurestr + 1,&minus_features,&minus_ext_features,&minus_ext2_features,&minus_ext3_features,&minus_kvm_features);
>           } else if ((val = strchr(featurestr, '='))) {
>               *val = 0; val++;
>               if (!strcmp(featurestr, "family")) {
> @@ -481,10 +498,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>       x86_cpu_def->ext_features |= plus_ext_features;
>       x86_cpu_def->ext2_features |= plus_ext2_features;
>       x86_cpu_def->ext3_features |= plus_ext3_features;
> +    x86_cpu_def->kvm_features |= plus_kvm_features;
>       x86_cpu_def->features&= ~minus_features;
>       x86_cpu_def->ext_features&= ~minus_ext_features;
>       x86_cpu_def->ext2_features&= ~minus_ext2_features;
>       x86_cpu_def->ext3_features&= ~minus_ext3_features;
> +    x86_cpu_def->kvm_features&= ~minus_kvm_features;
>       free(s);
>       return 0;
>
> @@ -529,7 +548,7 @@ static int cpu_x86_register (CPUX86State *env, const char *cpu_model)
>       env->cpuid_ext_features = def->ext_features;
>       env->cpuid_ext2_features = def->ext2_features;
>       env->cpuid_xlevel = def->xlevel;
> -    env->cpuid_ext3_features = def->ext3_features;
> +    env->cpuid_kvm_features = def->kvm_features;
>       {
>           const char *model_id = def->model_id;
>           int c, len, i;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 4084503..5b093ce 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -25,6 +25,10 @@
>   #include "gdbstub.h"
>   #include "host-utils.h"
>
> +#ifdef CONFIG_KVM_PARA
> +#include<linux/kvm_para.h>
> +#endif
> +//
>   //#define DEBUG_KVM
>
>   #ifdef DEBUG_KVM
> @@ -134,6 +138,39 @@ static void kvm_trim_features(uint32_t *features, uint32_t supported)
>       }
>   }
>
> +#ifdef CONFIG_KVM_PARA
> +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
> +#ifdef KVM_CAP_CR3_CACHE
> +        { KVM_CAP_CR3_CACHE, KVM_FEATURE_CR3_CACHE },
> +#endif
> +        { -1, -1 }
> +};
> +
> +static int get_para_features(CPUState *env)
> +{
> +        int i, features = 0;
> +
> +        for (i = 0; i<  ARRAY_SIZE(para_features) - 1; i++) {
> +                if (kvm_check_extension(env->kvm_state, para_features[i].cap))
> +                        features |= (1<<  para_features[i].feature);
> +        }
> +
> +        return features;
> +}
> +#endif
> +
>   int kvm_arch_init_vcpu(CPUState *env)
>   {
>       struct {
> @@ -142,6 +179,10 @@ int kvm_arch_init_vcpu(CPUState *env)
>       } __attribute__((packed)) cpuid_data;
>       uint32_t limit, i, j, cpuid_i;
>       uint32_t unused;
> +    struct kvm_cpuid_entry2 *c;
> +#ifdef KVM_CPUID_SIGNATURE
> +    uint32_t signature[3];
> +#endif
>
>       env->mp_state = KVM_MP_STATE_RUNNABLE;
>
> @@ -160,10 +201,27 @@ int kvm_arch_init_vcpu(CPUState *env)
>
>       cpuid_i = 0;
>
> +#ifdef CONFIG_KVM_PARA
> +    /* Paravirtualization CPUIDs */
> +    memcpy(signature, "KVMKVMKVM\0\0\0", 12);
> +    c =&cpuid_data.entries[cpuid_i++];
> +    memset(c, 0, sizeof(*c));
> +    c->function = KVM_CPUID_SIGNATURE;
> +    c->eax = 0;
> +    c->ebx = signature[0];
> +    c->ecx = signature[1];
> +    c->edx = signature[2];
> +
> +    c =&cpuid_data.entries[cpuid_i++];
> +    memset(c, 0, sizeof(*c));
> +    c->function = KVM_CPUID_FEATURES;
> +    c->eax = env->cpuid_kvm_features&  get_para_features(env);
> +#endif
> +
>       cpu_x86_cpuid(env, 0, 0,&limit,&unused,&unused,&unused);

Instead of hooking here, would it make more sense to tie into the 
generic cpuid function in helper.c?

Regards,

Anthony Liguori