diff mbox

[3/3] Add svm cpuid features

Message ID 1284479530-4748-4-git-send-email-joerg.roedel@amd.com
State New
Headers show

Commit Message

Joerg Roedel Sept. 14, 2010, 3:52 p.m. UTC 
This patch adds the svm cpuid feature flags to the qemu
intialization path. It also adds the svm features available
on phenom to its cpu-definition and extends the host cpu
type to support all svm features KVM can provide.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpu.h   |   12 ++++++++
 target-i386/cpuid.c |   77 +++++++++++++++++++++++++++++++++++++++-----------
 target-i386/kvm.c   |    3 ++
 3 files changed, 75 insertions(+), 17 deletions(-)

Comments

Avi Kivity Sept. 16, 2010, 2:06 p.m. UTC | #1 
On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> This patch adds the svm cpuid feature flags to the qemu
> intialization path. It also adds the svm features available
> on phenom to its cpu-definition and extends the host cpu
> type to support all svm features KVM can provide.
>
>

Does phenom really have vmcbclean?  I thought it was a really new feature.
Joerg Roedel Sept. 16, 2010, 2:12 p.m. UTC | #2 
On Thu, Sep 16, 2010 at 10:06:48AM -0400, Avi Kivity wrote:
>   On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> > This patch adds the svm cpuid feature flags to the qemu
> > intialization path. It also adds the svm features available
> > on phenom to its cpu-definition and extends the host cpu
> > type to support all svm features KVM can provide.
> >
> >
> 
> Does phenom really have vmcbclean?  I thought it was a really new feature.

No, but we could emulate it on almost all hardware that has SVM. Its
basically the same as with x2apic.

	Joerg
Alexander Graf Sept. 16, 2010, 2:17 p.m. UTC | #3 
Roedel, Joerg wrote:
> On Thu, Sep 16, 2010 at 10:06:48AM -0400, Avi Kivity wrote:
>   
>>   On 09/14/2010 05:52 PM, Joerg Roedel wrote:
>>     
>>> This patch adds the svm cpuid feature flags to the qemu
>>> intialization path. It also adds the svm features available
>>> on phenom to its cpu-definition and extends the host cpu
>>> type to support all svm features KVM can provide.
>>>
>>>
>>>       
>> Does phenom really have vmcbclean?  I thought it was a really new feature.
>>     
>
> No, but we could emulate it on almost all hardware that has SVM. Its
> basically the same as with x2apic.
>   

-cpu <non-host> is not about what we could emulate, but what the
hardware really is. Features not supported by the particular CPU do not
belong there.


Alex
Joerg Roedel Sept. 16, 2010, 2:26 p.m. UTC | #4 
On Thu, Sep 16, 2010 at 10:17:09AM -0400, Alexander Graf wrote:
> Roedel, Joerg wrote:
> > On Thu, Sep 16, 2010 at 10:06:48AM -0400, Avi Kivity wrote:
> >   
> >>   On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> >>     
> >>> This patch adds the svm cpuid feature flags to the qemu
> >>> intialization path. It also adds the svm features available
> >>> on phenom to its cpu-definition and extends the host cpu
> >>> type to support all svm features KVM can provide.
> >>>
> >>>
> >>>       
> >> Does phenom really have vmcbclean?  I thought it was a really new feature.
> >>     
> >
> > No, but we could emulate it on almost all hardware that has SVM. Its
> > basically the same as with x2apic.
> >   
> 
> -cpu <non-host> is not about what we could emulate, but what the
> hardware really is. Features not supported by the particular CPU do not
> belong there.

I'll remove it then. It would have been nice to have it there because it
will improve performance of svm emulation. But if it is about emulating
the real cpu then I'll just drop it.

	Joerg
Alexander Graf Sept. 16, 2010, 2:28 p.m. UTC | #5 
Roedel, Joerg wrote:
> On Thu, Sep 16, 2010 at 10:17:09AM -0400, Alexander Graf wrote:
>   
>> Roedel, Joerg wrote:
>>     
>>> On Thu, Sep 16, 2010 at 10:06:48AM -0400, Avi Kivity wrote:
>>>   
>>>       
>>>>   On 09/14/2010 05:52 PM, Joerg Roedel wrote:
>>>>     
>>>>         
>>>>> This patch adds the svm cpuid feature flags to the qemu
>>>>> intialization path. It also adds the svm features available
>>>>> on phenom to its cpu-definition and extends the host cpu
>>>>> type to support all svm features KVM can provide.
>>>>>
>>>>>
>>>>>       
>>>>>           
>>>> Does phenom really have vmcbclean?  I thought it was a really new feature.
>>>>     
>>>>         
>>> No, but we could emulate it on almost all hardware that has SVM. Its
>>> basically the same as with x2apic.
>>>   
>>>       
>> -cpu <non-host> is not about what we could emulate, but what the
>> hardware really is. Features not supported by the particular CPU do not
>> belong there.
>>     
>
> I'll remove it then. It would have been nice to have it there because it
> will improve performance of svm emulation. But if it is about emulating
> the real cpu then I'll just drop it.
>   

speed == -cpu host :). If the performance increase is significant, we
can always add a new cpu type for a cpu that does support those flags so
people can use it there and still be migration safe. In fact, that would
even benefit you guys as you'd sell more new machines for speed ;).


Alex
Joerg Roedel Sept. 16, 2010, 2:35 p.m. UTC | #6 
On Thu, Sep 16, 2010 at 10:28:01AM -0400, Alexander Graf wrote:
> Roedel, Joerg wrote:
> > I'll remove it then. It would have been nice to have it there because it
> > will improve performance of svm emulation. But if it is about emulating
> > the real cpu then I'll just drop it.
> 
> speed == -cpu host :). If the performance increase is significant, we
> can always add a new cpu type for a cpu that does support those flags so
> people can use it there and still be migration safe. In fact, that would
> even benefit you guys as you'd sell more new machines for speed ;).

Well, since its emulated completly in software it would be available on
all hosts that start with -cpu phenom (at least if they use the same kvm
version). So this would not break migration.

	Joerg
diff mbox

Patch

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1144d4e..77eeab1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -405,6 +405,17 @@ 
 #define CPUID_EXT3_IBS     (1 << 10)
 #define CPUID_EXT3_SKINIT  (1 << 12)
 
+#define CPUID_SVM_NPT          (1 << 0)
+#define CPUID_SVM_LBRV         (1 << 1)
+#define CPUID_SVM_SVMLOCK      (1 << 2)
+#define CPUID_SVM_NRIPSAVE     (1 << 3)
+#define CPUID_SVM_TSCSCALE     (1 << 4)
+#define CPUID_SVM_VMCBCLEAN    (1 << 5)
+#define CPUID_SVM_FLUSHASID    (1 << 6)
+#define CPUID_SVM_DECODEASSIST (1 << 7)
+#define CPUID_SVM_PAUSEFILTER  (1 << 10)
+#define CPUID_SVM_PFTHRESHOLD  (1 << 12)
+
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
@@ -702,6 +713,7 @@  typedef struct CPUX86State {
     uint8_t has_error_code;
     uint32_t sipi_vector;
     uint32_t cpuid_kvm_features;
+    uint32_t cpuid_svm_features;
     
     /* in order to simplify APIC support, we leave this pointer to the
        user */
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 3fcf78f..8e67af0 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -79,6 +79,17 @@  static const char *kvm_feature_name[] = {
     NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 };
 
+static const char *svm_feature_name[] = {
+    "npt", "lbrv", "svm_lock", "nrip_save",
+    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
+    NULL, NULL, "pause_filter", NULL,
+    "pfthreshold", NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+    NULL, NULL, NULL, NULL,
+};
+
 /* collects per-function cpuid data
  */
 typedef struct model_features_t {
@@ -192,13 +203,15 @@  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 *kvm_features)
+                                    uint32_t *kvm_features,
+                                    uint32_t *svm_features)
 {
     if (!lookup_feature(features, flagname, NULL, feature_name) &&
         !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
         !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
         !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
-        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name))
+        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
+        !lookup_feature(svm_features, flagname, NULL, svm_feature_name))
             fprintf(stderr, "CPU feature %s not found\n", flagname);
 }
 
@@ -210,7 +223,8 @@  typedef struct x86_def_t {
     int family;
     int model;
     int stepping;
-    uint32_t features, ext_features, ext2_features, ext3_features, kvm_features;
+    uint32_t features, ext_features, ext2_features, ext3_features;
+    uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
     int vendor_override;
@@ -253,6 +267,7 @@  typedef struct x86_def_t {
           CPUID_EXT2_PDPE1GB */
 #define TCG_EXT3_FEATURES (CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | \
           CPUID_EXT3_CR8LEG | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A)
+#define TCG_SVM_FEATURES 0
 
 /* maintains list of cpu model definitions
  */
@@ -305,6 +320,7 @@  static x86_def_t builtin_x86_defs[] = {
                     CPUID_EXT3_OSVW, CPUID_EXT3_IBS */
         .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM |
             CPUID_EXT3_ABM | CPUID_EXT3_SSE4A,
+        .svm_features = CPUID_SVM_NPT | CPUID_SVM_LBRV | CPUID_SVM_VMCBCLEAN,
         .xlevel = 0x8000001A,
         .model_id = "AMD Phenom(tm) 9550 Quad-Core Processor"
     },
@@ -505,6 +521,15 @@  static int cpu_x86_fill_host(x86_def_t *x86_cpu_def)
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
     x86_cpu_def->vendor_override = 0;
 
+
+    /*
+     * Every SVM feature requires emulation support in KVM - so we can't just
+     * read the host features here. KVM might even support SVM features not
+     * available on the host hardware. Just set all bits and mask out the
+     * unsupported ones later.
+     */
+    x86_cpu_def->svm_features = -1;
+
     return 0;
 }
 
@@ -560,8 +585,14 @@  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, 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;
+    /* Features to be added*/
+    uint32_t plus_features = 0, plus_ext_features = 0;
+    uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
+    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
+    /* Features to be removed */
+    uint32_t minus_features = 0, minus_ext_features = 0;
+    uint32_t minus_ext2_features = 0, minus_ext3_features = 0;
+    uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t numvalue;
 
     for (def = x86_defs; def; def = def->next)
@@ -579,16 +610,22 @@  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
     add_flagname_to_bitmaps("hypervisor", &plus_features,
         &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-        &plus_kvm_features);
+        &plus_kvm_features, &plus_svm_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, &plus_kvm_features);
+            add_flagname_to_bitmaps(featurestr + 1, &plus_features,
+                            &plus_ext_features, &plus_ext2_features,
+                            &plus_ext3_features, &plus_kvm_features,
+                            &plus_svm_features);
         } else if (featurestr[0] == '-') {
-            add_flagname_to_bitmaps(featurestr + 1, &minus_features, &minus_ext_features, &minus_ext2_features, &minus_ext3_features, &minus_kvm_features);
+            add_flagname_to_bitmaps(featurestr + 1, &minus_features,
+                            &minus_ext_features, &minus_ext2_features,
+                            &minus_ext3_features, &minus_kvm_features,
+                            &minus_svm_features);
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             if (!strcmp(featurestr, "family")) {
@@ -670,11 +707,13 @@  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     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->svm_features |= plus_svm_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;
+    x86_cpu_def->svm_features &= ~minus_svm_features;
     if (check_cpuid) {
         if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
@@ -816,6 +855,7 @@  int cpu_x86_register (CPUX86State *env, const char *cpu_model)
     env->cpuid_ext3_features = def->ext3_features;
     env->cpuid_xlevel = def->xlevel;
     env->cpuid_kvm_features = def->kvm_features;
+    env->cpuid_svm_features = def->svm_features;
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
@@ -825,6 +865,7 @@  int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 #endif
             );
         env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
+        env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     {
         const char *model_id = def->model_id;
@@ -1135,11 +1176,6 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
-
-        if (kvm_enabled()) {
-            /* Nested SVM not yet supported in upstream QEMU */
-            *ecx &= ~CPUID_EXT3_SVM;
-        }
         break;
     case 0x80000002:
     case 0x80000003:
@@ -1184,10 +1220,17 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         }
         break;
     case 0x8000000A:
-        *eax = 0x00000001; /* SVM Revision */
-        *ebx = 0x00000010; /* nr of ASIDs */
-        *ecx = 0;
-        *edx = 0; /* optional features */
+	if (env->cpuid_ext3_features & CPUID_EXT3_SVM) {
+		*eax = 0x00000001; /* SVM Revision */
+		*ebx = 0x00000010; /* nr of ASIDs */
+		*ecx = 0;
+		*edx = env->cpuid_svm_features; /* optional features */
+	} else {
+		*eax = 0;
+		*ebx = 0;
+		*ecx = 0;
+		*edx = 0;
+	}
         break;
     default:
         /* reserved values: zero */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index a33d2fa..74e7b4f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -192,6 +192,9 @@  int kvm_arch_init_vcpu(CPUState *env)
                                                              0, R_EDX);
     env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(env, 0x80000001,
                                                              0, R_ECX);
+    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(env, 0x8000000A,
+                                                             0, R_EDX);
+
 
     cpuid_i = 0;