diff mbox

[2/2] target-i386: Intel MPX support

Message ID DE8DF0795D48FD4CA783C40EC8292335013EF14B@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Liu, Jinsong Nov. 29, 2013, 1:17 p.m. UTC
From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00 2001
From: root <root@ljs.(none)>
Date: Fri, 22 Nov 2013 00:24:35 +0800
Subject: [PATCH 2/2] target-i386: Intel MPX support

Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest.
Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0).

Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
---
 target-i386/cpu.c |   34 ++++++++++++++++++++++++++--------
 target-i386/cpu.h |    1 +
 2 files changed, 27 insertions(+), 8 deletions(-)

--
1.7.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

Comments

Paolo Bonzini Nov. 29, 2013, 1:24 p.m. UTC | #1
Il 29/11/2013 14:17, Liu, Jinsong ha scritto:
> From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00 2001
> From: root <root@ljs.(none)>
> Date: Fri, 22 Nov 2013 00:24:35 +0800
> Subject: [PATCH 2/2] target-i386: Intel MPX support
> 
> Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest.
> Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0).

There is no reason to get the size and offset from the host.  Peter
Anvin confirmed that the sizes and offsets will never change (as should
be the case for migration to work across different CPU versions).  In
fact, the size and offset is documented for every XSAVE feature except
MPX in the copy I have of the Intel documentation.

Please get the size and offset from the documentation, if it has been
updated, or from a real host, and hardcode them in QEMU.

Paolo

> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
> ---
>  target-i386/cpu.c |   34 ++++++++++++++++++++++++++--------
>  target-i386/cpu.h |    1 +
>  2 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 544b57f..7d04f28 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -330,12 +330,12 @@ X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
> 
>  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 = 0x240, .size = 0x100 },
> +    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX },
> +    [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
> +    [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
>  };
> 
>  const char *get_register_name_32(unsigned int reg)
> @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32);
> 
>          if (count == 0) {
> -            *ecx = 0x240;
> +            *ebx = *ecx = 0x240;
>              for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) {
> +                uint32_t offset, size;
>                  const ExtSaveArea *esa = &ext_save_areas[i];
> +
>                  if ((env->features[esa->feature] & esa->bits) == esa->bits &&
>                      (kvm_mask & (1 << i)) != 0) {
>                      if (i < 32) {
> @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                      } else {
>                          *edx |= 1 << (i - 32);
>                      }
> -                    *ecx = MAX(*ecx, esa->offset + esa->size);
> +
> +                    size = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EAX);
> +                    offset = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX);
> +                    *ecx = MAX(*ecx, offset + size);
> +
> +                    /*
> +                     * EBX here just in order to
> +                     * 1. keep compatible with old qemu version, take AVX
> +                     *    into account;
> +                     * 2. keep compatible with old kernel version. Currently
> +                     *    KVM has bug when expose cpuid 0xd to guest (include
> +                     *    static value when guest booting and dynamic value
> +                     *    when guest enables XCR0 features. EBX here can
> +                     *    co-work with old buggy and new updated KVM, keep
> +                     *    same value independent to CPU and kernel version.
> +                     */
> +                    if (i == 2)
> +                        *ebx = MAX(*ebx, offset + size);
>                  }
>              }
>              *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE);
> -            *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->size;
> -                *ebx = esa->offset;
> +                *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
> +                *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
>              }
>          }
>          break;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index ea373e8..9a838d1 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  #define CPUID_7_0_EBX_ERMS     (1 << 9)
>  #define CPUID_7_0_EBX_INVPCID  (1 << 10)
>  #define CPUID_7_0_EBX_RTM      (1 << 11)
> +#define CPUID_7_0_EBX_MPX      (1 << 14)
>  #define CPUID_7_0_EBX_RDSEED   (1 << 18)
>  #define CPUID_7_0_EBX_ADX      (1 << 19)
>  #define CPUID_7_0_EBX_SMAP     (1 << 20)
> --
> 1.7.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
Liu, Jinsong Nov. 29, 2013, 2:50 p.m. UTC | #2
Paolo Bonzini wrote:
> Il 29/11/2013 14:17, Liu, Jinsong ha scritto:
>> From aac033473bc88befe39a9add99820c0a7118ac90 Mon Sep 17 00:00:00
>> 2001 
>> From: root <root@ljs.(none)>
>> Date: Fri, 22 Nov 2013 00:24:35 +0800
>> Subject: [PATCH 2/2] target-i386: Intel MPX support
>> 
>> Expose cpuid leaf (0xd, 3) and (0xd, 4) to guest.
>> Fix ebx and re-calculate ecx of cpuid leaf (0xd, 0).
> 
> There is no reason to get the size and offset from the host.  Peter
> Anvin confirmed that the sizes and offsets will never change (as
> should be the case for migration to work across different CPU
> versions).  In fact, the size and offset is documented for every
> XSAVE feature except MPX in the copy I have of the Intel
> documentation. 

If the sizes and offsets will never change, what's the bad effect of getting them from host?

> 
> Please get the size and offset from the documentation, if it has been
> updated, or from a real host, and hardcode them in QEMU.
> 

Hmm, the problem is what I get is not equal to real test :(
For example, I was told XSTATE_BNDCSR_SIZE is 0x40, but real test shows it's 0x10.

Maybe getting from real h/w is not bad than hardcode it?

Thanks
Jinsong

> 
>> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
>> ---
>>  target-i386/cpu.c |   34 ++++++++++++++++++++++++++-------- 
>>  target-i386/cpu.h |    1 + 2 files changed, 27 insertions(+), 8
>> deletions(-) 
>> 
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 544b57f..7d04f28 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -330,12 +330,12 @@ X86RegisterInfo32
>> x86_reg_info_32[CPU_NB_REGS32] = { 
>> 
>>  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 = 0x240, .size = 0x100 },
>> +    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX },
>> +    [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
>> +    [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX }, 
>> }; 
>> 
>>  const char *get_register_name_32(unsigned int reg)
>> @@ -2204,9 +2204,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
>>              index, uint32_t count,
>> ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32); 
>> 
>>          if (count == 0) {
>> -            *ecx = 0x240;
>> +            *ebx = *ecx = 0x240;
>>              for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) {
>> +                uint32_t offset, size;
>>                  const ExtSaveArea *esa = &ext_save_areas[i]; +
>>                  if ((env->features[esa->feature] & esa->bits) ==
>>                      esa->bits && (kvm_mask & (1 << i)) != 0) {
>>                      if (i < 32) {
>> @@ -2214,19 +2216,35 @@ void cpu_x86_cpuid(CPUX86State *env,
>>                          uint32_t index, uint32_t count,            
>>                      } else { *edx |= 1 << (i - 32); }
>> -                    *ecx = MAX(*ecx, esa->offset + esa->size); +
>> +                    size = kvm_arch_get_supported_cpuid(s, 0xd, i,
>> R_EAX); +                    offset =
>> kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX); +                   
>> *ecx = MAX(*ecx, offset + size); + +                    /*
>> +                     * EBX here just in order to
>> +                     * 1. keep compatible with old qemu version,
>> take AVX +                     *    into account;
>> +                     * 2. keep compatible with old kernel version.
>> Currently +                     *    KVM has bug when expose cpuid
>> 0xd to guest (include +                     *    static value when
>> guest booting and dynamic value +                     *    when
>> guest enables XCR0 features. EBX here can +                     *   
>> co-work with old buggy and new updated KVM, keep +                  
>> *    same value independent to CPU and kernel version. +            
>> */ +                    if (i == 2) +                        *ebx =
>>              MAX(*ebx, offset + size);                  } }
>>              *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE); -         
>>          *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->size;
>> -                *ebx = esa->offset;
>> +                *eax = kvm_arch_get_supported_cpuid(s, 0xd, count,
>> R_EAX); +                *ebx = kvm_arch_get_supported_cpuid(s, 0xd,
>>          count, R_EBX);              } }
>>          break;
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index ea373e8..9a838d1 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -545,6 +545,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>>  #define CPUID_7_0_EBX_ERMS     (1 << 9)
>>  #define CPUID_7_0_EBX_INVPCID  (1 << 10)
>>  #define CPUID_7_0_EBX_RTM      (1 << 11)
>> +#define CPUID_7_0_EBX_MPX      (1 << 14)
>>  #define CPUID_7_0_EBX_RDSEED   (1 << 18)
>>  #define CPUID_7_0_EBX_ADX      (1 << 19)
>>  #define CPUID_7_0_EBX_SMAP     (1 << 20)
>> --
>> 1.7.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
Paolo Bonzini Nov. 29, 2013, 3:42 p.m. UTC | #3
Il 29/11/2013 15:50, Liu, Jinsong ha scritto:
> > There is no reason to get the size and offset from the host.  Peter
> > Anvin confirmed that the sizes and offsets will never change (as
> > should be the case for migration to work across different CPU
> > versions).  In fact, the size and offset is documented for every
> > XSAVE feature except MPX in the copy I have of the Intel
> > documentation. 
> 
> If the sizes and offsets will never change, what's the bad effect of getting them from host?

In case TCG gets AVX/MPX support later, you will not be able to get
CPUID values from the host.  The leaf 0xd code was written so that it
would work for both KVM and TCG.

When QEMU got AVX support, we decided not to treat XSAVE data as opaque
blobs, and instead "unmarshal" data out of it into the CPUX86State
struct and back.  This is again useful for TCG, but it also makes for
easier interpretation of migration state.  You will have to rely on
precise sizes and offsets in the marshaling/unmarshaling code of
kvm_get_xsave/kvm_put_xsave, so it is not a big problem to have them
here as well.

Paolo
--
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
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 544b57f..7d04f28 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -330,12 +330,12 @@  X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {

 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 = 0x240, .size = 0x100 },
+    [2] = { .feature = FEAT_1_ECX, .bits = CPUID_EXT_AVX },
+    [3] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
+    [4] = { .feature = FEAT_7_0_EBX, .bits = CPUID_7_0_EBX_MPX },
 };

 const char *get_register_name_32(unsigned int reg)
@@ -2204,9 +2204,11 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             ((uint64_t)kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX) << 32);

         if (count == 0) {
-            *ecx = 0x240;
+            *ebx = *ecx = 0x240;
             for (i = 2; i < ARRAY_SIZE(ext_save_areas); i++) {
+                uint32_t offset, size;
                 const ExtSaveArea *esa = &ext_save_areas[i];
+
                 if ((env->features[esa->feature] & esa->bits) == esa->bits &&
                     (kvm_mask & (1 << i)) != 0) {
                     if (i < 32) {
@@ -2214,19 +2216,35 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                     } else {
                         *edx |= 1 << (i - 32);
                     }
-                    *ecx = MAX(*ecx, esa->offset + esa->size);
+
+                    size = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EAX);
+                    offset = kvm_arch_get_supported_cpuid(s, 0xd, i, R_EBX);
+                    *ecx = MAX(*ecx, offset + size);
+
+                    /*
+                     * EBX here just in order to
+                     * 1. keep compatible with old qemu version, take AVX
+                     *    into account;
+                     * 2. keep compatible with old kernel version. Currently
+                     *    KVM has bug when expose cpuid 0xd to guest (include
+                     *    static value when guest booting and dynamic value
+                     *    when guest enables XCR0 features. EBX here can
+                     *    co-work with old buggy and new updated KVM, keep
+                     *    same value independent to CPU and kernel version.
+                     */
+                    if (i == 2)
+                        *ebx = MAX(*ebx, offset + size);
                 }
             }
             *eax |= kvm_mask & (XSTATE_FP | XSTATE_SSE);
-            *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->size;
-                *ebx = esa->offset;
+                *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
+                *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
             }
         }
         break;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ea373e8..9a838d1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -545,6 +545,7 @@  typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 #define CPUID_7_0_EBX_ERMS     (1 << 9)
 #define CPUID_7_0_EBX_INVPCID  (1 << 10)
 #define CPUID_7_0_EBX_RTM      (1 << 11)
+#define CPUID_7_0_EBX_MPX      (1 << 14)
 #define CPUID_7_0_EBX_RDSEED   (1 << 18)
 #define CPUID_7_0_EBX_ADX      (1 << 19)
 #define CPUID_7_0_EBX_SMAP     (1 << 20)