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

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

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

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