diff mbox

[v2,uq/master,2/2] x86: cpuid: reconstruct leaf 0Dh data

Message ID 1379080558-16499-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Sept. 13, 2013, 1:55 p.m. UTC 
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(-)

Comments

Gleb Natapov Oct. 2, 2013, 3:21 p.m. UTC | #1 
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.
Paolo Bonzini Oct. 2, 2013, 3:37 p.m. UTC | #2 
Il 02/10/2013 17:21, Gleb Natapov ha scritto:
>> -        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.

Right.

>> +            *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...)

HPA confirmed (on xen-devel) that they will not depend on the CPU type.
 All offsets are documented in the SDM and in the additional Skylake
manual except for MPX, and he reported that he'd ask for MPX to be
documented as well.  As you said, if they changed it would be a total mess.

I hardcoded them because this is not KVM-specific knowledge.  TCG could
in principle reuse the same code, just skipping the part where it masks
away features not supported by KVM.

Paolo
Gleb Natapov Oct. 2, 2013, 3:39 p.m. UTC | #3 
On Wed, Oct 02, 2013 at 05:37:31PM +0200, Paolo Bonzini wrote:
> Il 02/10/2013 17:21, Gleb Natapov ha scritto:
> >> -        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.
> 
> Right.
> 
> >> +            *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...)
> 
> HPA confirmed (on xen-devel) that they will not depend on the CPU type.
>  All offsets are documented in the SDM and in the additional Skylake
> manual except for MPX, and he reported that he'd ask for MPX to be
> documented as well.  As you said, if they changed it would be a total mess.
> 
> I hardcoded them because this is not KVM-specific knowledge.  TCG could
> in principle reuse the same code, just skipping the part where it masks
> away features not supported by KVM.
> 
OK. Can you send new version with defines please?

--
			Gleb.
diff mbox

Patch

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;
+            *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;
+            }
         }
         break;
+    }
     case 0x80000000:
         *eax = env->cpuid_xlevel;
         *ebx = env->cpuid_vendor1;