Re: [Qemu-devel] [PATCH v2 uq/master 2/2] x86: cpuid: reconstruct leaf 0Dh data

[Gleb Natapov <gleb@redhat.com>]

On Fri, Sep 13, 2013 at 03:55:58PM +0200, Paolo Bonzini wrote:
> The data in leaf 0Dh depends on information from other feature bits.
> Instead of passing it blindly from the host, compute it based on
> whether these feature bits are enabled.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-i386/cpu.c | 63 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ac83106..e6179f4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -328,6 +328,15 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
>  };
>  #undef REGISTER
>  
> +typedef struct ExtSaveArea {
> +    uint32_t feature, bits;
> +    uint32_t offset, size;
> +} ExtSaveArea;
> +
> +static const ExtSaveArea ext_save_areas[] = {
> +    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX,
> +            .offset = 0x100, .size = 0x240 },
> +};
>  
>  const char *get_register_name_32(unsigned int reg)
>  {
> @@ -2169,29 +2178,51 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              *edx = 0;
>          }
>          break;
> -    case 0xD:
> +    case 0xD: {
> +        KVMState *s = cs->kvm_state;
> +        uint64_t kvm_mask;
> +        int i;
> +
>          /* Processor Extended State */
> -        if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> -            *eax = 0;
> -            *ebx = 0;
> -            *ecx = 0;
> -            *edx = 0;
> +        *eax = 0;
> +        *ebx = 0;
> +        *ecx = 0;
> +        *edx = 0;
> +        if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) {
>              break;
>          }
> -        if (kvm_enabled()) {
> -            KVMState *s = cs->kvm_state;
> +        kvm_mask =
> +            kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX) |
> +            ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32);
>  
> -            *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
> -            *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
> -            *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX);
> -            *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX);
> -        } else {
> -            *eax = 0;
> -            *ebx = 0;
> -            *ecx = 0;
> -            *edx = 0;
> +        if (count == 0) {
> +            *ecx = 0x240;
> +            for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) {
> +                const ExtSaveArea *esa = &ext_save_areas[i];
> +                if ((env->features[esa->feature] & esa->bits) == esa->bits &&
> +                    (kvm_mask & (1 << i)) != 0) {
> +                    if (i < 32) {
> +                        *eax |= 1 << i;
> +                    } else {
> +                        *edx |= 1 << (i - 32);
> +                    }
> +                    *ecx = MAX(*ecx, esa->offset + esa->size);
> +                }
> +            }
> +            *eax |= kvm_mask & 3;
Lets use define from previous patch.

> +            *ebx = *ecx;
> +        } else if (count == 1) {
> +            *eax = kvm_arch_get_supported_cpuid(s, 0xd, 1, R_EAX);
> +        } else if (count < ARRAY_SIZE(ext_save_areas)) {
> +            const ExtSaveArea *esa = &ext_save_areas[count];
> +            if ((env->features[esa->feature] & esa->bits) == esa->bits &&
> +                (kvm_mask & (1 << count)) != 0) {
> +                *eax = esa->offset;
> +                *ebx = esa->size;
Why do you hard code them instead of querying kernel? What if they
depend on cpu type? (well if this happens we can forget about
migration, but still...)

> +            }
>          }
>          break;
> +    }
>      case 0x80000000:
>          *eax = env->cpuid_xlevel;
>          *ebx = env->cpuid_vendor1;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.