[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] [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 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

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] 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 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 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] 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

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

[Marcelo Tosatti]

On Mon, Sep 27, 2010 at 03:16:15PM +0200, Joerg Roedel wrote:
> As requested by Alex this patch makes kvm64 the default CPU
> model when qemu is started with -enable-kvm. This takes only
> effect for qemu-versions newer or equal to 0.14.0.
> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  hw/boards.h    |    1 +
>  hw/pc.c        |   21 ++++++++++++++++-----
>  hw/pc_piix.c   |    6 ++++++
>  qemu-version.h |   35 +++++++++++++++++++++++++++++++++++
>  vl.c           |    4 ++++
>  5 files changed, 62 insertions(+), 5 deletions(-)
>  create mode 100644 qemu-version.h
> 
> diff --git a/hw/boards.h b/hw/boards.h
> index 6f0f0d7..2d41b2d 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -19,6 +19,7 @@ typedef struct QEMUMachine {
>      QEMUMachineInitFunc *init;
>      int use_scsi;
>      int max_cpus;
> +    unsigned int compat_version;
>      unsigned int no_serial:1,
>          no_parallel:1,
>          use_virtcon:1,
> diff --git a/hw/pc.c b/hw/pc.c
> index 69b13bf..372ec4c 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"
> +#include "qemu-version.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,12 @@ 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() &&
> +            qemu_compat_version >= QEMU_COMPAT_VERSION(0, 14, 0)) {
> +            cpu_model = DEFAULT_KVM_CPU_MODEL;
> +        } else {
> +            cpu_model = DEFAULT_QEMU_CPU_MODEL;
> +        }
>      }
>  
>      for(i = 0; i < smp_cpus; i++) {
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 12359a7..9e46b71 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -35,6 +35,7 @@
>  #include "sysemu.h"
>  #include "sysbus.h"
>  #include "blockdev.h"
> +#include "qemu-version.h"
>  
>  #define MAX_IDE_BUS 2
>  
> @@ -217,6 +218,7 @@ static QEMUMachine pc_machine = {
>      .desc = "Standard PC",
>      .init = pc_init_pci,
>      .max_cpus = 255,
> +    .compat_version = QEMU_COMPAT_VERSION(0, 13, 0),
>      .is_default = 1,
>  };
>  
> @@ -225,6 +227,7 @@ static QEMUMachine pc_machine_v0_12 = {
>      .desc = "Standard PC",
>      .init = pc_init_pci,
>      .max_cpus = 255,
> +    .compat_version = QEMU_COMPAT_VERSION(0, 12, 0),
>      .compat_props = (GlobalProperty[]) {
>          {
>              .driver   = "virtio-serial-pci",
> @@ -244,6 +247,7 @@ static QEMUMachine pc_machine_v0_11 = {
>      .desc = "Standard PC, qemu 0.11",
>      .init = pc_init_pci,
>      .max_cpus = 255,
> +    .compat_version = QEMU_COMPAT_VERSION(0, 11, 0),
>      .compat_props = (GlobalProperty[]) {
>          {
>              .driver   = "virtio-blk-pci",
> @@ -279,6 +283,7 @@ static QEMUMachine pc_machine_v0_10 = {
>      .desc = "Standard PC, qemu 0.10",
>      .init = pc_init_pci,
>      .max_cpus = 255,
> +    .compat_version = QEMU_COMPAT_VERSION(0, 10, 0),
>      .compat_props = (GlobalProperty[]) {
>          {
>              .driver   = "virtio-blk-pci",
> @@ -325,6 +330,7 @@ static QEMUMachine isapc_machine = {
>      .name = "isapc",
>      .desc = "ISA-only PC",
>      .init = pc_init_isa,
> +    .compat_version = QEMU_COMPAT_VERSION(0, 10, 0),
>      .max_cpus = 1,
>  };
>  
> diff --git a/qemu-version.h b/qemu-version.h
> new file mode 100644
> index 0000000..b4bfe48
> --- /dev/null
> +++ b/qemu-version.h
> @@ -0,0 +1,35 @@
> +/*
> + * qemu-version.h
> + *
> + * Defines needed for handling QEMU version compatibility
> + *
> + * Copyright (c) 2010 Joerg Roedel <joerg.roedel@amd.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef _QEMU_VERSION_H_
> +#define _QEMU_VERSION_H_
> +
> +extern unsigned int qemu_compat_version;
> +
> +#define QEMU_COMPAT_VERSION(major, minor, patchlevel) \
> +        ((unsigned int)(major << 16) | (minor << 8) | (patchlevel))
> +
> +#endif
> diff --git a/vl.c b/vl.c
> index d352d18..37727f3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -161,6 +161,7 @@ int main(int argc, char **argv)
>  #include "qemu-queue.h"
>  #include "cpus.h"
>  #include "arch_init.h"
> +#include "qemu-version.h"
>  
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
> @@ -169,6 +170,7 @@ int main(int argc, char **argv)
>  
>  #define MAX_VIRTIO_CONSOLES 1
>  
> +unsigned int qemu_compat_version;
>  static const char *data_dir;
>  const char *bios_name = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> @@ -2696,6 +2698,8 @@ int main(int argc, char **argv, char **envp)
>          default_sdcard = 0;
>      }
>  
> +    qemu_compat_version = machine->compat_version;
> +
>      if (display_type == DT_NOGRAPHIC) {
>          if (default_parallel)
>              add_device_config(DEV_PARALLEL, "null");
> -- 
> 1.7.0.4

Looks fine to me, given CPUs are not in qdev. Anthony?

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

[Anthony Liguori]

On 10/06/2010 01:53 PM, Marcelo Tosatti wrote:
> On Mon, Sep 27, 2010 at 03:16:15PM +0200, Joerg Roedel wrote:
>    
>> As requested by Alex this patch makes kvm64 the default CPU
>> model when qemu is started with -enable-kvm. This takes only
>> effect for qemu-versions newer or equal to 0.14.0.
>>
>> Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
>> ---
>>   hw/boards.h    |    1 +
>>   hw/pc.c        |   21 ++++++++++++++++-----
>>   hw/pc_piix.c   |    6 ++++++
>>   qemu-version.h |   35 +++++++++++++++++++++++++++++++++++
>>   vl.c           |    4 ++++
>>   5 files changed, 62 insertions(+), 5 deletions(-)
>>   create mode 100644 qemu-version.h
>>
>> diff --git a/hw/boards.h b/hw/boards.h
>> index 6f0f0d7..2d41b2d 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -19,6 +19,7 @@ typedef struct QEMUMachine {
>>       QEMUMachineInitFunc *init;
>>       int use_scsi;
>>       int max_cpus;
>> +    unsigned int compat_version;
>>       unsigned int no_serial:1,
>>           no_parallel:1,
>>           use_virtcon:1,
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 69b13bf..372ec4c 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"
>> +#include "qemu-version.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,12 @@ 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()&&
>> +            qemu_compat_version>= QEMU_COMPAT_VERSION(0, 14, 0)) {
>> +            cpu_model = DEFAULT_KVM_CPU_MODEL;
>> +        } else {
>> +            cpu_model = DEFAULT_QEMU_CPU_MODEL;
>> +        }
>>       }
>>
>>       for(i = 0; i<  smp_cpus; i++) {
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 12359a7..9e46b71 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -35,6 +35,7 @@
>>   #include "sysemu.h"
>>   #include "sysbus.h"
>>   #include "blockdev.h"
>> +#include "qemu-version.h"
>>
>>   #define MAX_IDE_BUS 2
>>
>> @@ -217,6 +218,7 @@ static QEMUMachine pc_machine = {
>>       .desc = "Standard PC",
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>> +    .compat_version = QEMU_COMPAT_VERSION(0, 13, 0),
>>       .is_default = 1,
>>   };
>>
>> @@ -225,6 +227,7 @@ static QEMUMachine pc_machine_v0_12 = {
>>       .desc = "Standard PC",
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>> +    .compat_version = QEMU_COMPAT_VERSION(0, 12, 0),
>>       .compat_props = (GlobalProperty[]) {
>>           {
>>               .driver   = "virtio-serial-pci",
>> @@ -244,6 +247,7 @@ static QEMUMachine pc_machine_v0_11 = {
>>       .desc = "Standard PC, qemu 0.11",
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>> +    .compat_version = QEMU_COMPAT_VERSION(0, 11, 0),
>>       .compat_props = (GlobalProperty[]) {
>>           {
>>               .driver   = "virtio-blk-pci",
>> @@ -279,6 +283,7 @@ static QEMUMachine pc_machine_v0_10 = {
>>       .desc = "Standard PC, qemu 0.10",
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>> +    .compat_version = QEMU_COMPAT_VERSION(0, 10, 0),
>>       .compat_props = (GlobalProperty[]) {
>>           {
>>               .driver   = "virtio-blk-pci",
>> @@ -325,6 +330,7 @@ static QEMUMachine isapc_machine = {
>>       .name = "isapc",
>>       .desc = "ISA-only PC",
>>       .init = pc_init_isa,
>> +    .compat_version = QEMU_COMPAT_VERSION(0, 10, 0),
>>       .max_cpus = 1,
>>   };
>>
>> diff --git a/qemu-version.h b/qemu-version.h
>> new file mode 100644
>> index 0000000..b4bfe48
>> --- /dev/null
>> +++ b/qemu-version.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * qemu-version.h
>> + *
>> + * Defines needed for handling QEMU version compatibility
>> + *
>> + * Copyright (c) 2010 Joerg Roedel<joerg.roedel@amd.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef _QEMU_VERSION_H_
>> +#define _QEMU_VERSION_H_
>> +
>> +extern unsigned int qemu_compat_version;
>> +
>> +#define QEMU_COMPAT_VERSION(major, minor, patchlevel) \
>> +        ((unsigned int)(major<<  16) | (minor<<  8) | (patchlevel))
>> +
>> +#endif
>> diff --git a/vl.c b/vl.c
>> index d352d18..37727f3 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -161,6 +161,7 @@ int main(int argc, char **argv)
>>   #include "qemu-queue.h"
>>   #include "cpus.h"
>>   #include "arch_init.h"
>> +#include "qemu-version.h"
>>
>>   //#define DEBUG_NET
>>   //#define DEBUG_SLIRP
>> @@ -169,6 +170,7 @@ int main(int argc, char **argv)
>>
>>   #define MAX_VIRTIO_CONSOLES 1
>>
>> +unsigned int qemu_compat_version;
>>   static const char *data_dir;
>>   const char *bios_name = NULL;
>>   enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>> @@ -2696,6 +2698,8 @@ int main(int argc, char **argv, char **envp)
>>           default_sdcard = 0;
>>       }
>>
>> +    qemu_compat_version = machine->compat_version;
>> +
>>       if (display_type == DT_NOGRAPHIC) {
>>           if (default_parallel)
>>               add_device_config(DEV_PARALLEL, "null");
>> -- 
>> 1.7.0.4
>>      
> Looks fine to me, given CPUs are not in qdev. Anthony?
>    

The idea is fine, but why not just add the default CPU to the machine 
description?

Regards,

Anthony Liguori

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

[Roedel, Joerg]

On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
> >> +    qemu_compat_version = machine->compat_version;
> >> +
> >>       if (display_type == DT_NOGRAPHIC) {
> >>           if (default_parallel)
> >>               add_device_config(DEV_PARALLEL, "null");
> >> -- 
> >> 1.7.0.4
> >>      
> > Looks fine to me, given CPUs are not in qdev. Anthony?
> >    
> 
> The idea is fine, but why not just add the default CPU to the machine 
> description?

If I remember correctly the reason was that the machine description was
not accessible in the cpuid initialization path because it is a function
local variable. I could have made it a global variable but considered
the compat_version approach simpler. The qemu_compat_version might also
be useful at other places.

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

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>    
>>>> +    qemu_compat_version = machine->compat_version;
>>>> +
>>>>        if (display_type == DT_NOGRAPHIC) {
>>>>            if (default_parallel)
>>>>                add_device_config(DEV_PARALLEL, "null");
>>>> -- 
>>>> 1.7.0.4
>>>>
>>>>          
>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>
>>>        
>> The idea is fine, but why not just add the default CPU to the machine
>> description?
>>      
> If I remember correctly the reason was that the machine description was
> not accessible in the cpuid initialization path because it is a function
> local variable.

Not tested at all but I think the attached patch addresses it in a 
pretty nice way.

There's a couple ways you could support your patch on top of this.  You 
could add a kvm_cpu_model to the machine structure that gets defaulted 
too if kvm_enabled().  You could also introduce a new KVM machine type 
that gets defaulted to if no explicit machine is specified.

>   I could have made it a global variable but considered
> the compat_version approach simpler. The qemu_compat_version might also
> be useful at other places.
>    

The reason we've avoided having a builtin notion of versions is that we 
have many downstreams where versioning would get very complicated.  If 
we stick to features it makes it much easier for downstreams.

Regards,

Anthony Liguori

> 	Joerg
>
>    


[-- Attachment #2: 0001-machine-make-default-cpu-model-part-of-machine-struc.patch --]
[-- Type: text/x-patch, Size: 3253 bytes --]

>From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Thu, 7 Oct 2010 07:43:42 -0500
Subject: [PATCH] machine: make default cpu model part of machine structure

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/hw/boards.h b/hw/boards.h
index 6f0f0d7..8c6ef27 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -16,6 +16,7 @@ typedef struct QEMUMachine {
     const char *name;
     const char *alias;
     const char *desc;
+    const char *cpu_model;
     QEMUMachineInitFunc *init;
     int use_scsi;
     int max_cpus;
diff --git a/hw/pc.c b/hw/pc.c
index 69b13bf..0826107 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model)
     int i;
 
     /* init CPUs */
-    if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
-    }
-
     for(i = 0; i < smp_cpus; i++) {
         pc_new_cpu(cpu_model);
     }
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 12359a7..919b4d6 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size,
                         const char *initrd_filename,
                         const char *cpu_model)
 {
-    if (cpu_model == NULL)
-        cpu_model = "486";
     pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
              initrd_filename, cpu_model, 0);
 }
 
+#ifdef TARGET_X86_64
+#define DEF_CPU_MODEL "qemu64"
+#else
+#define DEF_CPU_MODEL "qemu32"
+#endif
+
 static QEMUMachine pc_machine = {
     .name = "pc-0.13",
     .alias = "pc",
     .desc = "Standard PC",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .is_default = 1,
@@ -223,6 +228,7 @@ static QEMUMachine pc_machine = {
 static QEMUMachine pc_machine_v0_12 = {
     .name = "pc-0.12",
     .desc = "Standard PC",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = {
 static QEMUMachine pc_machine_v0_11 = {
     .name = "pc-0.11",
     .desc = "Standard PC, qemu 0.11",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = {
 static QEMUMachine pc_machine_v0_10 = {
     .name = "pc-0.10",
     .desc = "Standard PC, qemu 0.10",
+    .cpu_model = DEF_CPU_MODEL,
     .init = pc_init_pci,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
@@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = {
 static QEMUMachine isapc_machine = {
     .name = "isapc",
     .desc = "ISA-only PC",
+    .cpu_model = "486",
     .init = pc_init_isa,
     .max_cpus = 1,
 };
diff --git a/vl.c b/vl.c
index df414ef..3a55cc8 100644
--- a/vl.c
+++ b/vl.c
@@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp)
     }
     qemu_add_globals();
 
+    if (cpu_model == NULL) {
+        cpu_model = machine->cpu_model;
+    }
+
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
-- 
1.7.0.4

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

[Roedel, Joerg]

(Sorry for the late reply)

On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
> > On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
> >    
> >>>> +    qemu_compat_version = machine->compat_version;
> >>>> +
> >>>>        if (display_type == DT_NOGRAPHIC) {
> >>>>            if (default_parallel)
> >>>>                add_device_config(DEV_PARALLEL, "null");
> >>>> -- 
> >>>> 1.7.0.4
> >>>>
> >>>>          
> >>> Looks fine to me, given CPUs are not in qdev. Anthony?
> >>>
> >>>        
> >> The idea is fine, but why not just add the default CPU to the machine
> >> description?
> >>      
> > If I remember correctly the reason was that the machine description was
> > not accessible in the cpuid initialization path because it is a function
> > local variable.
> 
> Not tested at all but I think the attached patch addresses it in a 
> pretty nice way.
> 
> There's a couple ways you could support your patch on top of this.  You 
> could add a kvm_cpu_model to the machine structure that gets defaulted 
> too if kvm_enabled().  You could also introduce a new KVM machine type 
> that gets defaulted to if no explicit machine is specified.

I had something similar in mind but then I realized that we need at
least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
the KVM case.
Further the QEMUMachine data structure is used for all architectures in
QEMU and the model-names only make sense for x86. So I decided for the
comapt-version way (which doesn't mean I object against this one ;-) )

	Joerg

> From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Thu, 7 Oct 2010 07:43:42 -0500
> Subject: [PATCH] machine: make default cpu model part of machine structure
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/hw/boards.h b/hw/boards.h
> index 6f0f0d7..8c6ef27 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -16,6 +16,7 @@ typedef struct QEMUMachine {
>      const char *name;
>      const char *alias;
>      const char *desc;
> +    const char *cpu_model;
>      QEMUMachineInitFunc *init;
>      int use_scsi;
>      int max_cpus;
> diff --git a/hw/pc.c b/hw/pc.c
> index 69b13bf..0826107 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model)
>      int i;
>  
>      /* init CPUs */
> -    if (cpu_model == NULL) {
> -#ifdef TARGET_X86_64
> -        cpu_model = "qemu64";
> -#else
> -        cpu_model = "qemu32";
> -#endif
> -    }
> -
>      for(i = 0; i < smp_cpus; i++) {
>          pc_new_cpu(cpu_model);
>      }
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 12359a7..919b4d6 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size,
>                          const char *initrd_filename,
>                          const char *cpu_model)
>  {
> -    if (cpu_model == NULL)
> -        cpu_model = "486";
>      pc_init1(ram_size, boot_device,
>               kernel_filename, kernel_cmdline,
>               initrd_filename, cpu_model, 0);
>  }
>  
> +#ifdef TARGET_X86_64
> +#define DEF_CPU_MODEL "qemu64"
> +#else
> +#define DEF_CPU_MODEL "qemu32"
> +#endif
> +
>  static QEMUMachine pc_machine = {
>      .name = "pc-0.13",
>      .alias = "pc",
>      .desc = "Standard PC",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .is_default = 1,
> @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = {
>  static QEMUMachine pc_machine_v0_12 = {
>      .name = "pc-0.12",
>      .desc = "Standard PC",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = {
>  static QEMUMachine pc_machine_v0_11 = {
>      .name = "pc-0.11",
>      .desc = "Standard PC, qemu 0.11",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = {
>  static QEMUMachine pc_machine_v0_10 = {
>      .name = "pc-0.10",
>      .desc = "Standard PC, qemu 0.10",
> +    .cpu_model = DEF_CPU_MODEL,
>      .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
> @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = {
>  static QEMUMachine isapc_machine = {
>      .name = "isapc",
>      .desc = "ISA-only PC",
> +    .cpu_model = "486",
>      .init = pc_init_isa,
>      .max_cpus = 1,
>  };
> diff --git a/vl.c b/vl.c
> index df414ef..3a55cc8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp)
>      }
>      qemu_add_globals();
>  
> +    if (cpu_model == NULL) {
> +        cpu_model = machine->cpu_model;
> +    }
> +
>      machine->init(ram_size, boot_devices,
>                    kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>  
> -- 
> 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()

[Anthony Liguori]

On 10/18/2010 03:22 AM, Roedel, Joerg wrote:
> (Sorry for the late reply)
>
> On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
>    
>> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
>>      
>>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>>>
>>>        
>>>>>> +    qemu_compat_version = machine->compat_version;
>>>>>> +
>>>>>>         if (display_type == DT_NOGRAPHIC) {
>>>>>>             if (default_parallel)
>>>>>>                 add_device_config(DEV_PARALLEL, "null");
>>>>>> -- 
>>>>>> 1.7.0.4
>>>>>>
>>>>>>
>>>>>>              
>>>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>>>
>>>>>
>>>>>            
>>>> The idea is fine, but why not just add the default CPU to the machine
>>>> description?
>>>>
>>>>          
>>> If I remember correctly the reason was that the machine description was
>>> not accessible in the cpuid initialization path because it is a function
>>> local variable.
>>>        
>> Not tested at all but I think the attached patch addresses it in a
>> pretty nice way.
>>
>> There's a couple ways you could support your patch on top of this.  You
>> could add a kvm_cpu_model to the machine structure that gets defaulted
>> too if kvm_enabled().  You could also introduce a new KVM machine type
>> that gets defaulted to if no explicit machine is specified.
>>      
> I had something similar in mind but then I realized that we need at
> least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
> the KVM case.
>    

I would think that having different default machines for KVM and TCG 
would be a better solution.

> Further the QEMUMachine data structure is used for all architectures in
> QEMU and the model-names only make sense for x86.

SPARC uses cpu_model too FWIW.  I believe Blue Swirl has even discussed 
using a feature-format similar to how x86 does it for SPARC CPUs.

Regards,

Anthony Liguori

>   So I decided for the
> comapt-version way (which doesn't mean I object against this one ;-) )
>
> 	Joerg
>
>    
>>  From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001
>> From: Anthony Liguori<aliguori@us.ibm.com>
>> Date: Thu, 7 Oct 2010 07:43:42 -0500
>> Subject: [PATCH] machine: make default cpu model part of machine structure
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/hw/boards.h b/hw/boards.h
>> index 6f0f0d7..8c6ef27 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -16,6 +16,7 @@ typedef struct QEMUMachine {
>>       const char *name;
>>       const char *alias;
>>       const char *desc;
>> +    const char *cpu_model;
>>       QEMUMachineInitFunc *init;
>>       int use_scsi;
>>       int max_cpus;
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 69b13bf..0826107 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model)
>>       int i;
>>
>>       /* init CPUs */
>> -    if (cpu_model == NULL) {
>> -#ifdef TARGET_X86_64
>> -        cpu_model = "qemu64";
>> -#else
>> -        cpu_model = "qemu32";
>> -#endif
>> -    }
>> -
>>       for(i = 0; i<  smp_cpus; i++) {
>>           pc_new_cpu(cpu_model);
>>       }
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index 12359a7..919b4d6 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size,
>>                           const char *initrd_filename,
>>                           const char *cpu_model)
>>   {
>> -    if (cpu_model == NULL)
>> -        cpu_model = "486";
>>       pc_init1(ram_size, boot_device,
>>                kernel_filename, kernel_cmdline,
>>                initrd_filename, cpu_model, 0);
>>   }
>>
>> +#ifdef TARGET_X86_64
>> +#define DEF_CPU_MODEL "qemu64"
>> +#else
>> +#define DEF_CPU_MODEL "qemu32"
>> +#endif
>> +
>>   static QEMUMachine pc_machine = {
>>       .name = "pc-0.13",
>>       .alias = "pc",
>>       .desc = "Standard PC",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .is_default = 1,
>> @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = {
>>   static QEMUMachine pc_machine_v0_12 = {
>>       .name = "pc-0.12",
>>       .desc = "Standard PC",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = {
>>   static QEMUMachine pc_machine_v0_11 = {
>>       .name = "pc-0.11",
>>       .desc = "Standard PC, qemu 0.11",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = {
>>   static QEMUMachine pc_machine_v0_10 = {
>>       .name = "pc-0.10",
>>       .desc = "Standard PC, qemu 0.10",
>> +    .cpu_model = DEF_CPU_MODEL,
>>       .init = pc_init_pci,
>>       .max_cpus = 255,
>>       .compat_props = (GlobalProperty[]) {
>> @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = {
>>   static QEMUMachine isapc_machine = {
>>       .name = "isapc",
>>       .desc = "ISA-only PC",
>> +    .cpu_model = "486",
>>       .init = pc_init_isa,
>>       .max_cpus = 1,
>>   };
>> diff --git a/vl.c b/vl.c
>> index df414ef..3a55cc8 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp)
>>       }
>>       qemu_add_globals();
>>
>> +    if (cpu_model == NULL) {
>> +        cpu_model = machine->cpu_model;
>> +    }
>> +
>>       machine->init(ram_size, boot_devices,
>>                     kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>>
>> -- 
>> 1.7.0.4
>>
>>      
>
>    

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

[Blue Swirl]

On Mon, Oct 18, 2010 at 2:16 PM, Anthony Liguori
<aliguori@linux.vnet.ibm.com> wrote:
> On 10/18/2010 03:22 AM, Roedel, Joerg wrote:
>>
>> (Sorry for the late reply)
>>
>> On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote:
>>
>>>
>>> On 10/07/2010 03:42 AM, Roedel, Joerg wrote:
>>>
>>>>
>>>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote:
>>>>
>>>>
>>>>>>>
>>>>>>> +    qemu_compat_version = machine->compat_version;
>>>>>>> +
>>>>>>>        if (display_type == DT_NOGRAPHIC) {
>>>>>>>            if (default_parallel)
>>>>>>>                add_device_config(DEV_PARALLEL, "null");
>>>>>>> --
>>>>>>> 1.7.0.4
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Looks fine to me, given CPUs are not in qdev. Anthony?
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> The idea is fine, but why not just add the default CPU to the machine
>>>>> description?
>>>>>
>>>>>
>>>>
>>>> If I remember correctly the reason was that the machine description was
>>>> not accessible in the cpuid initialization path because it is a function
>>>> local variable.
>>>>
>>>
>>> Not tested at all but I think the attached patch addresses it in a
>>> pretty nice way.
>>>
>>> There's a couple ways you could support your patch on top of this.  You
>>> could add a kvm_cpu_model to the machine structure that gets defaulted
>>> too if kvm_enabled().  You could also introduce a new KVM machine type
>>> that gets defaulted to if no explicit machine is specified.
>>>
>>
>> I had something similar in mind but then I realized that we need at
>> least a cpu_model and a cpu_model_kvm to distinguish between the TCG and
>> the KVM case.
>>
>
> I would think that having different default machines for KVM and TCG would
> be a better solution.
>
>> Further the QEMUMachine data structure is used for all architectures in
>> QEMU and the model-names only make sense for x86.
>
> SPARC uses cpu_model too FWIW.  I believe Blue Swirl has even discussed
> using a feature-format similar to how x86 does it for SPARC CPUs.

Actually I copied Sparc feature support from x86. Generic feature
support would be nice.