[Qemu-devel] [PATCH 0/3] SVM feature support for qemu

[Qemu-devel] [PATCH 0/3] SVM feature support for qemu

[Joerg Roedel]

Hi,

here is the next round of the svm feature support patches for qemu. Key
change in this version is that it now makes kvm{64|32} the default cpu
definition for qemu when kvm is enabled (as requested by Alex).
Otherwise I removed the NRIP_SAVE feature from the phenom definition and
set svm_features per default to -1 for -cpu host to support all svm
features that kvm can emulate. I appreciate your comments.

	Joerg

[Qemu-devel] [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Joerg Roedel]

As requested by Alex this patch makes kvm64 the default CPU
model when qemu is started with -enable-kvm.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 hw/pc.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..f531d0d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -40,6 +40,16 @@
 #include "sysbus.h"
 #include "sysemu.h"
 #include "blockdev.h"
+#include "kvm.h"
+
+
+#ifdef TARGET_X86_64
+#define DEFAULT_KVM_CPU_MODEL "kvm64"
+#define DEFAULT_QEMU_CPU_MODEL "qemu64"
+#else
+#define DEFAULT_KVM_CPU_MODEL "kvm32"
+#define DEFAULT_QEMU_CPU_MODEL "qemu32"
+#endif
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -867,11 +877,10 @@ void pc_cpus_init(const char *cpu_model)
 
     /* init CPUs */
     if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
+        if (kvm_enabled())
+            cpu_model = DEFAULT_KVM_CPU_MODEL;
+        else
+            cpu_model = DEFAULT_QEMU_CPU_MODEL;
     }
 
     for(i = 0; i < smp_cpus; i++) {
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Alexander Graf]

Joerg Roedel wrote:
> As requested by Alex this patch makes kvm64 the default CPU
> model when qemu is started with -enable-kvm.
>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  hw/pc.c |   19 ++++++++++++++-----
>  1 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 69b13bf..f531d0d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -40,6 +40,16 @@
>  #include "sysbus.h"
>  #include "sysemu.h"
>  #include "blockdev.h"
> +#include "kvm.h"
> +
> +
> +#ifdef TARGET_X86_64
> +#define DEFAULT_KVM_CPU_MODEL "kvm64"
> +#define DEFAULT_QEMU_CPU_MODEL "qemu64"
> +#else
> +#define DEFAULT_KVM_CPU_MODEL "kvm32"
> +#define DEFAULT_QEMU_CPU_MODEL "qemu32"
> +#endif
>  
>  /* output Bochs bios info messages */
>  //#define DEBUG_BIOS
> @@ -867,11 +877,10 @@ void pc_cpus_init(const char *cpu_model)
>  
>      /* init CPUs */
>      if (cpu_model == NULL) {
> -#ifdef TARGET_X86_64
> -        cpu_model = "qemu64";
> -#else
> -        cpu_model = "qemu32";
> -#endif
> +        if (kvm_enabled())
> +            cpu_model = DEFAULT_KVM_CPU_MODEL;
> +        else
> +            cpu_model = DEFAULT_QEMU_CPU_MODEL;
>   

Braces :(.


Alex

[Qemu-devel] Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Roedel, Joerg]

On Tue, Sep 14, 2010 at 11:58:03AM -0400, Alexander Graf wrote:

> > +        if (kvm_enabled())
> > +            cpu_model = DEFAULT_KVM_CPU_MODEL;
> > +        else
> > +            cpu_model = DEFAULT_QEMU_CPU_MODEL;
> >   
> 
> Braces :(.

Okay, here is the new patch:

>From f49e78edbd4143d05128228d9cc24bd5abc3abf1 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Tue, 14 Sep 2010 16:52:11 +0200
Subject: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

As requested by Alex this patch makes kvm64 the default CPU
model when qemu is started with -enable-kvm.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 hw/pc.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..a6355f3 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -40,6 +40,16 @@
 #include "sysbus.h"
 #include "sysemu.h"
 #include "blockdev.h"
+#include "kvm.h"
+
+
+#ifdef TARGET_X86_64
+#define DEFAULT_KVM_CPU_MODEL "kvm64"
+#define DEFAULT_QEMU_CPU_MODEL "qemu64"
+#else
+#define DEFAULT_KVM_CPU_MODEL "kvm32"
+#define DEFAULT_QEMU_CPU_MODEL "qemu32"
+#endif
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -867,11 +877,11 @@ void pc_cpus_init(const char *cpu_model)
 
     /* init CPUs */
     if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
+        if (kvm_enabled()) {
+            cpu_model = DEFAULT_KVM_CPU_MODEL;
+        } else {
+            cpu_model = DEFAULT_QEMU_CPU_MODEL;
+        }
     }
 
     for(i = 0; i < smp_cpus; i++) {
-- 
1.7.0.4


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Avi Kivity]

  On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> As requested by Alex this patch makes kvm64 the default CPU
> model when qemu is started with -enable-kvm.
>
>

This breaks compatiblity; if started with -M 0.13 or prior we should 
default to qemu64.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Roedel, Joerg]

On Thu, Sep 16, 2010 at 10:03:19AM -0400, Avi Kivity wrote:
>   On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> > As requested by Alex this patch makes kvm64 the default CPU
> > model when qemu is started with -enable-kvm.
> >
> >
> 
> This breaks compatiblity; if started with -M 0.13 or prior we should 
> default to qemu64.

Ok, I can change that. But its  ok to make kvm64 the default for
anything > 0.13?

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 1/3] Make kvm64 the default cpu model when kvm_enabled()

[Avi Kivity]

  On 09/16/2010 04:21 PM, Roedel, Joerg wrote:
> On Thu, Sep 16, 2010 at 10:03:19AM -0400, Avi Kivity wrote:
> >    On 09/14/2010 05:52 PM, Joerg Roedel wrote:
> >  >  As requested by Alex this patch makes kvm64 the default CPU
> >  >  model when qemu is started with -enable-kvm.
> >  >
> >  >
> >
> >  This breaks compatiblity; if started with -M 0.13 or prior we should
> >  default to qemu64.
>
> Ok, I can change that. But its  ok to make kvm64 the default for
> anything>  0.13?
>

Sure.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH 2/3] Set cpuid definition to 0 before initializing it

[Joerg Roedel]

This patch cleans the (stack-allocated) cpuid definition to
0 before actually initializing it.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpuid.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 04ba8d5..3fcf78f 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -788,6 +788,8 @@ int cpu_x86_register (CPUX86State *env, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
 
+    memset(def, 0, sizeof(*def));
+
     if (cpu_x86_find_by_name(def, cpu_model) < 0)
         return -1;
     if (def->vendor1) {
-- 
1.7.0.4

[Qemu-devel] [PATCH 3/3] Add svm cpuid features

[Joerg Roedel]

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(-)

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;
 
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

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

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

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

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Alexander Graf]

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

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

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

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [Qemu-devel] [PATCH 0/3] SVM feature support for qemu

[Alexander Graf]

On 14.09.2010, at 17:52, Joerg Roedel wrote:

> Hi,
> 
> here is the next round of the svm feature support patches for qemu. Key
> change in this version is that it now makes kvm{64|32} the default cpu
> definition for qemu when kvm is enabled (as requested by Alex).
> Otherwise I removed the NRIP_SAVE feature from the phenom definition and
> set svm_features per default to -1 for -cpu host to support all svm
> features that kvm can emulate. I appreciate your comments.

With the updated 1/3 patch, it looks really good to me.

Acked-by: Alexander Graf <agraf@suse.de>

[Qemu-devel] [PATCH 0/3] SVM feature support for qemu v3

[Joerg Roedel]

Hi,

here is the third round of the patches to add svm-features support to
qemu. The patches are rebases against latest qemu/master. Other changes
since v2:

	* Made kvm64 the default model only for qemu-versions >= 0.14.0
	* Removed clean-bits from the phenom cpu-model

Feedback and/or merge appreciated :-)

	Joerg

[Qemu-devel] [PATCH 3/3] Add svm cpuid features

[Joerg Roedel]

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(-)

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..0e0bf60 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,
         .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;
 
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  On 09/27/2010 03:16 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.
>

This should really be "all svm features" since we'll later mask them 
with kvm capabilities.  This way, if we later add more capabilities, we 
automatically gain support in userspace.

(or rather, all svm features that don't need save/restore support in 
userspace; I don't think any do, beyond svm itself)

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  On 09/27/2010 04:58 PM, Avi Kivity wrote:
>  On 09/27/2010 03:16 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.
>>
>
> This should really be "all svm features" since we'll later mask them 
> with kvm capabilities.  This way, if we later add more capabilities, 
> we automatically gain support in userspace.
>
> (or rather, all svm features that don't need save/restore support in 
> userspace; I don't think any do, beyond svm itself)
>

I applied 2 and 3 (to branch uq/master).  If you want to add more svm 
features, please send a patch.  For patch 1, I'd like a review by 
someone who understands the compat code.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

On Mon, Sep 27, 2010 at 11:02:23AM -0400, Avi Kivity wrote:
>   On 09/27/2010 04:58 PM, Avi Kivity wrote:
> >  On 09/27/2010 03:16 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.
> >>
> >
> > This should really be "all svm features" since we'll later mask them 
> > with kvm capabilities.  This way, if we later add more capabilities, 
> > we automatically gain support in userspace.

Yes, that is what -cpu host does with these patches applied. The
svm_features variable is set to -1 in this case and masked out later
with the KVM capabilities.

> >
> > (or rather, all svm features that don't need save/restore support in 
> > userspace; I don't think any do, beyond svm itself)
> >
> 
> I applied 2 and 3 (to branch uq/master).  If you want to add more svm 
> features, please send a patch.  For patch 1, I'd like a review by 
> someone who understands the compat code.

Great, thanks.

	Joerg
-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  On 09/27/2010 05:40 PM, Roedel, Joerg wrote:
> On Mon, Sep 27, 2010 at 11:02:23AM -0400, Avi Kivity wrote:
> >    On 09/27/2010 04:58 PM, Avi Kivity wrote:
> >  >   On 09/27/2010 03:16 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.
> >  >>
> >  >
> >  >  This should really be "all svm features" since we'll later mask them
> >  >  with kvm capabilities.  This way, if we later add more capabilities,
> >  >  we automatically gain support in userspace.
>
> Yes, that is what -cpu host does with these patches applied. The
> svm_features variable is set to -1 in this case and masked out later
> with the KVM capabilities.
>

Well, running current uq/master I get:

   has_svm: can't execute cpuid 0x8000000a
   kvm: no hardware support

?

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

On Mon, Sep 27, 2010 at 12:22:13PM -0400, Avi Kivity wrote:
>   On 09/27/2010 05:40 PM, Roedel, Joerg wrote:
> > On Mon, Sep 27, 2010 at 11:02:23AM -0400, Avi Kivity wrote:
> > >    On 09/27/2010 04:58 PM, Avi Kivity wrote:
> > >  >   On 09/27/2010 03:16 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.
> > >  >>
> > >  >
> > >  >  This should really be "all svm features" since we'll later mask them
> > >  >  with kvm capabilities.  This way, if we later add more capabilities,
> > >  >  we automatically gain support in userspace.
> >
> > Yes, that is what -cpu host does with these patches applied. The
> > svm_features variable is set to -1 in this case and masked out later
> > with the KVM capabilities.
> >
> 
> Well, running current uq/master I get:
> 
>    has_svm: can't execute cpuid 0x8000000a
>    kvm: no hardware support
> 
> ?

Weird, it worked here as I tested it. I had it on qemu/master and with
all three patches. But patch 1 should not make the difference. I take a
look, have you pushed the failing uq/master? What was your command line?

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  On 09/28/2010 11:28 AM, Roedel, Joerg wrote:
> On Mon, Sep 27, 2010 at 12:22:13PM -0400, Avi Kivity wrote:
> >    On 09/27/2010 05:40 PM, Roedel, Joerg wrote:
> >  >  On Mon, Sep 27, 2010 at 11:02:23AM -0400, Avi Kivity wrote:
> >  >  >     On 09/27/2010 04:58 PM, Avi Kivity wrote:
> >  >  >   >    On 09/27/2010 03:16 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.
> >  >  >   >>
> >  >  >   >
> >  >  >   >   This should really be "all svm features" since we'll later mask them
> >  >  >   >   with kvm capabilities.  This way, if we later add more capabilities,
> >  >  >   >   we automatically gain support in userspace.
> >  >
> >  >  Yes, that is what -cpu host does with these patches applied. The
> >  >  svm_features variable is set to -1 in this case and masked out later
> >  >  with the KVM capabilities.
> >  >
> >
> >  Well, running current uq/master I get:
> >
> >     has_svm: can't execute cpuid 0x8000000a
> >     kvm: no hardware support
> >
> >  ?
>
> Weird, it worked here as I tested it. I had it on qemu/master and with
> all three patches. But patch 1 should not make the difference. I take a
> look, have you pushed the failing uq/master?

Yes, 8fe6a21c76.

> What was your command line?

qemu-system-x86_64 -m 2G -cpu kvm64,+svm,+npt -enable-kvm ...

Note this is qemu.git, so -enable-kvm is needed.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Roedel, Joerg]

On Tue, Sep 28, 2010 at 05:37:58AM -0400, Avi Kivity wrote:
>   On 09/28/2010 11:28 AM, Roedel, Joerg wrote:

> > Weird, it worked here as I tested it. I had it on qemu/master and with
> > all three patches. But patch 1 should not make the difference. I take a
> > look, have you pushed the failing uq/master?
> 
> Yes, 8fe6a21c76.
> 
> > What was your command line?
> 
> qemu-system-x86_64 -m 2G -cpu kvm64,+svm,+npt -enable-kvm ...
> 
> Note this is qemu.git, so -enable-kvm is needed.

Ok, I apparently forgot to force the CPUID xlevel to be 0x8000000A when
SVM is enabled, probably because I only tested CPUID models where xlevel
already defaults to 0x8000000A. Attached is a fix, thanks for catching
this.

	Joerg


>From d18d973d037f1af1869e114aea1f076ea3a1ee7a Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Tue, 28 Sep 2010 11:58:49 +0200
Subject: [PATCH 1/1] qemu: Force xlevel to at least 0x8000000A if SVM is enabled

This patch forces the extended CPUID level to be at least
0x8000000A if SVM is enabled in the CPU model.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 target-i386/cpuid.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 0e0bf60..0630fe1 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -714,6 +714,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
     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 ((x86_cpu_def->ext3_features & CPUID_EXT3_SVM) &&
+        (x86_cpu_def->xlevel < 0x8000000A)) {
+        /* Force xlevel to at least 0x8000000A if SVM enabled */
+        x86_cpu_def->xlevel = 0x8000000A;
+    }
     if (check_cpuid) {
         if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
-- 
1.7.0.4


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Avi Kivity]

  On 09/28/2010 12:05 PM, Roedel, Joerg wrote:
> On Tue, Sep 28, 2010 at 05:37:58AM -0400, Avi Kivity wrote:
> >    On 09/28/2010 11:28 AM, Roedel, Joerg wrote:
>
> >  >  Weird, it worked here as I tested it. I had it on qemu/master and with
> >  >  all three patches. But patch 1 should not make the difference. I take a
> >  >  look, have you pushed the failing uq/master?
> >
> >  Yes, 8fe6a21c76.
> >
> >  >  What was your command line?
> >
> >  qemu-system-x86_64 -m 2G -cpu kvm64,+svm,+npt -enable-kvm ...
> >
> >  Note this is qemu.git, so -enable-kvm is needed.
>
> Ok, I apparently forgot to force the CPUID xlevel to be 0x8000000A when
> SVM is enabled, probably because I only tested CPUID models where xlevel
> already defaults to 0x8000000A. Attached is a fix, thanks for catching
> this.
>
> 	Joerg
>
>
>  From d18d973d037f1af1869e114aea1f076ea3a1ee7a Mon Sep 17 00:00:00 2001
> From: Joerg Roedel<joerg.roedel@amd.com>
> Date: Tue, 28 Sep 2010 11:58:49 +0200
> Subject: [PATCH 1/1] qemu: Force xlevel to at least 0x8000000A if SVM is enabled
>
> This patch forces the extended CPUID level to be at least
> 0x8000000A if SVM is enabled in the CPU model.
>
> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
> ---
>   target-i386/cpuid.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index 0e0bf60..0630fe1 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -714,6 +714,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>       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 ((x86_cpu_def->ext3_features&  CPUID_EXT3_SVM)&&
> +        (x86_cpu_def->xlevel<  0x8000000A)) {
> +        /* Force xlevel to at least 0x8000000A if SVM enabled */
> +        x86_cpu_def->xlevel = 0x8000000A;
> +    }
>       if (check_cpuid) {
>           if (check_features_against_host(x86_cpu_def)&&  enforce_cpuid)
>               goto error;

I can't say I like the interdependency, but saying something like -cpu 
kvm64,+svm,xlevel=0x8000000a is much worse.

If no one has a better idea, I'll apply the patch.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 3/3] Add svm cpuid features

[Marcelo Tosatti]

On Tue, Sep 28, 2010 at 12:05:20PM +0200, Roedel, Joerg wrote:
> On Tue, Sep 28, 2010 at 05:37:58AM -0400, Avi Kivity wrote:
> >   On 09/28/2010 11:28 AM, Roedel, Joerg wrote:
> 
> > > Weird, it worked here as I tested it. I had it on qemu/master and with
> > > all three patches. But patch 1 should not make the difference. I take a
> > > look, have you pushed the failing uq/master?
> > 
> > Yes, 8fe6a21c76.
> > 
> > > What was your command line?
> > 
> > qemu-system-x86_64 -m 2G -cpu kvm64,+svm,+npt -enable-kvm ...
> > 
> > Note this is qemu.git, so -enable-kvm is needed.
> 
> Ok, I apparently forgot to force the CPUID xlevel to be 0x8000000A when
> SVM is enabled, probably because I only tested CPUID models where xlevel
> already defaults to 0x8000000A. Attached is a fix, thanks for catching
> this.
> 
> 	Joerg

Applied, thanks.