[Qemu-devel] Re: [PATCH 3/4] mask out forbidden cpuid features with kvm ioctl

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

Glauber Costa wrote:
> KVM has a (so far unused) ioctl to inform userspace
> about supported cpuid bits in the current host.
>
> The lack of this kind of checking can lead to bugs in which
> a cpuid bit is exposed but not supported by the host kernel
> (an example is fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=481274)
>
> The current host_cpuid code is replaced by this general check.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
>   

So what do we do about live migration?  Does KVM mask a set of bits 
unconditionally?  Before, we were masking unconditionally which should 
insulate migration reasonable well.

Regards,

Anthony Liguori

> ---
>  target-i386/helper.c |   89 ++++++++++++++++++++++---------------------------
>  1 files changed, 40 insertions(+), 49 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index a28ab93..1410002 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1370,37 +1370,33 @@ static void breakpoint_handler(CPUState *env)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>
> -static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
> +static void host_cpuid(CPUState *env, uint32_t function, uint32_t *eax, uint32_t *ebx,
>                         uint32_t *ecx, uint32_t *edx)
>  {
>  #if defined(CONFIG_KVM)
> -    uint32_t vec[4];
> +    static struct {
> +        struct kvm_cpuid2 cpuid;
> +        struct kvm_cpuid_entry2 entries[100];
> +    } __attribute__((packed)) cpuid_base;
> +    int i;
>
> -#ifdef __x86_64__
> -    asm volatile("cpuid"
> -		 : "=a"(vec[0]), "=b"(vec[1]),
> -		   "=c"(vec[2]), "=d"(vec[3])
> -		 : "0"(function) : "cc");
> -#else
> -    asm volatile("pusha \n\t"
> -		 "cpuid \n\t"
> -		 "mov %%eax, 0(%1) \n\t"
> -		 "mov %%ebx, 4(%1) \n\t"
> -		 "mov %%ecx, 8(%1) \n\t"
> -		 "mov %%edx, 12(%1) \n\t"
> -		 "popa"
> -		 : : "a"(function), "S"(vec)
> -		 : "memory", "cc");
> -#endif
> +    if (cpuid_base.cpuid.nent == 0) {
> +        cpuid_base.cpuid.nent = 100;
> +        kvm_ioctl(env->kvm_state, KVM_GET_SUPPORTED_CPUID, &cpuid_base);
> +    }
>
> -    if (eax)
> -	*eax = vec[0];
> -    if (ebx)
> -	*ebx = vec[1];
> -    if (ecx)
> -	*ecx = vec[2];
> -    if (edx)
> -	*edx = vec[3];
> +    for (i = 0; i < cpuid_base.cpuid.nent; i++) {
> +        if (cpuid_base.entries[i].function == function) {
> +            if (eax)
> +                *eax = cpuid_base.entries[i].eax;
> +            if (ebx)
> +                *ebx = cpuid_base.entries[i].ebx;
> +            if (ecx)
> +                *ecx = cpuid_base.entries[i].ecx;
> +            if (edx)
> +                *edx = cpuid_base.entries[i].edx;
> +        }
> +    }
>  #endif
>  }
>
> @@ -1429,7 +1425,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>           * actuall cpu, and say goodbye to migration between different vendors
>           * is you use compatibility mode. */
>          if (kvm_enabled())
> -            host_cpuid(0, NULL, ebx, ecx, edx);
> +            host_cpuid(env, 0, NULL, ebx, ecx, edx);
>          break;
>      case 1:
>          *eax = env->cpuid_version;
> @@ -1437,9 +1433,18 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>          *ecx = env->cpuid_ext_features;
>          *edx = env->cpuid_features;
>
> -        /* "Hypervisor present" bit required for Microsoft SVVP */
> -        if (kvm_enabled())
> +        if (kvm_enabled()) {
> +            uint32_t h_eax;
> +            uint32_t h_ecx = ~0U;
> +            uint32_t h_edx = ~0U;
> +
> +            host_cpuid(env, 1, &h_eax, NULL, &h_ecx, &h_edx);
> +
> +            *ecx &= h_ecx;
> +            *edx &= h_edx;
> +            /* "Hypervisor present" bit required for Microsoft SVVP */
>              *ecx |= (1 << 31);
> +        }
>          break;
>      case 2:
>          /* cache info: needed for Pentium Pro compatibility */
> @@ -1519,28 +1524,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
>          *edx = env->cpuid_ext2_features;
>
>          if (kvm_enabled()) {
> -            uint32_t h_eax, h_edx;
> -
> -            host_cpuid(0x80000001, &h_eax, NULL, NULL, &h_edx);
> -
> -            /* disable CPU features that the host does not support */
> -
> -            /* long mode */
> -            if ((h_edx & 0x20000000) == 0 /* || !lm_capable_kernel */)
> -                *edx &= ~0x20000000;
> -            /* syscall */
> -            if ((h_edx & 0x00000800) == 0)
> -                *edx &= ~0x00000800;
> -            /* nx */
> -            if ((h_edx & 0x00100000) == 0)
> -                *edx &= ~0x00100000;
> +            uint32_t h_eax;
> +            uint32_t h_ecx = ~0U;
> +            uint32_t h_edx = ~0U;
>
> -            /* disable CPU features that KVM cannot support */
> +            host_cpuid(env, 0x80000001, &h_eax, NULL, &h_ecx, &h_edx);
>
> -            /* svm */
> -            *ecx &= ~4UL;
> -            /* 3dnow */
> -            *edx &= ~0xc0000000;
> +            *ecx &= h_ecx;
> +            *edx &= h_edx;
>          }
>          break;
>      case 0x80000002:
>