[Qemu-devel] [PULL 00/16] KVM changes for 2014-02-03

[Qemu-devel] [PULL 00/16] KVM changes for 2014-02-03

[Paolo Bonzini]

Anthony, Peter,

the following changes since commit 0169c511554cb0014a00290b0d3d26c31a49818f:

  Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2014-01-24 15:52:44 -0800)

are available in the git repository at:


  git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git 

for you to fetch changes up to 7c08db30e6a43f7083a881eb07bfbc878e001e08

  target-i386: Move KVM default-vendor hack to instance_init (2014-01-30 17:48:55 -0200)

Thanks,

Paolo
----------------------------------------------------------------
Eduardo Habkost (10):
      target-i386: kvm_cpu_fill_host(): Kill unused code
      target-i386: kvm_cpu_fill_host(): No need to check level
      target-i386: kvm_cpu_fill_host(): No need to check CPU vendor
      target-i386: kvm_cpu_fill_host(): No need to check xlevel2
      target-i386: kvm_cpu_fill_host(): Set all feature words at end of function
      target-i386: kvm_cpu_fill_host(): Fill feature words in a loop
      target-i386: kvm_check_features_against_host(): Kill feature word array
      target-i386: Eliminate CONFIG_KVM #ifdefs
      target-i386: Don't change x86_def_t struct on cpu_x86_register()
      target-i386: Move KVM default-vendor hack to instance_init

Paolo Bonzini (2):
      KVM: fix coexistence of KVM and Hyper-V leaves
      kvm: make availability of Hyper-V enlightenments dependent on KVM_CAP_HYPERV

Radim Krčmář (1):
      kvm: print suberror on all internal errors

Vadim Rozenfeld (3):
      kvm: make hyperv hypercall and guest os id MSRs migratable.
      kvm: make hyperv vapic assist page migratable
      kvm: add support for hyper-v timers

 kvm-all.c                      |   7 +-
 linux-headers/asm-x86/hyperv.h |   3 +
 linux-headers/linux/kvm.h      |   1 +
 target-i386/cpu-qom.h          |   1 +
 target-i386/cpu.c              | 148 ++++++++++++++---------------------------
 target-i386/cpu.h              |   4 ++
 target-i386/kvm.c              | 109 +++++++++++++++++++++---------
 target-i386/machine.c          |  67 +++++++++++++++++++
 13 files changed, 206 insertions(+), 134 deletions(-)
 delete mode 100644 pc-bios/vgabios.bin
-- 
1.8.3.1

[Qemu-devel] [PULL 01/16] target-i386: kvm_cpu_fill_host(): Kill unused code

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

Those host_cpuid() calls are useless. They are leftovers from when the
old code using host_cpuid() was removed.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e6f7eaf..07f7f82 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1182,12 +1182,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
-        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
             /* Support VIA max extended level */
             x86_cpu_def->xlevel2 = eax;
-            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
             x86_cpu_def->features[FEAT_C000_0001_EDX] =
                     kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 02/16] target-i386: kvm_cpu_fill_host(): No need to check level

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

There's no need to check level (CPUID[0].EAX) before calling
kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX), because:

 * The kernel won't return any entry for CPUID 7 if CPUID[0].EAX is < 7
   on the host (See kvm_dev_ioctl_get_cpuid() on the kernel code);
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf.

This will simplify the kvm_cpu_fill_host() code a little.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 07f7f82..05138bd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1165,12 +1165,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->features[FEAT_1_ECX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
 
-    if (x86_cpu_def->level >= 7) {
-        x86_cpu_def->features[FEAT_7_0_EBX] =
-                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-    } else {
-        x86_cpu_def->features[FEAT_7_0_EBX] = 0;
-    }
+    x86_cpu_def->features[FEAT_7_0_EBX] =
+                kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
 
     x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
     x86_cpu_def->features[FEAT_8000_0001_EDX] =
-- 
1.8.3.1

[Qemu-devel] [PULL 03/16] target-i386: kvm_cpu_fill_host(): No need to check CPU vendor

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

There's no need to check CPU vendor before calling
kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX), because:

 * The kernel won't return any entry for 0xC0000000 if host CPU vendor
   is not Centaur (See kvm_dev_ioctl_get_cpuid() on the kernel code);
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf.

This will simplify the kvm_cpu_fill_host() code a little.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 05138bd..1e115cf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1177,14 +1177,12 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
     /* Call Centaur's CPUID instruction. */
-    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
-        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-        if (eax >= 0xC0000001) {
-            /* Support VIA max extended level */
-            x86_cpu_def->xlevel2 = eax;
-            x86_cpu_def->features[FEAT_C000_0001_EDX] =
-                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-        }
+    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    if (eax >= 0xC0000001) {
+        /* Support VIA max extended level */
+        x86_cpu_def->xlevel2 = eax;
+        x86_cpu_def->features[FEAT_C000_0001_EDX] =
+                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
     }
 
     /* Other KVM-specific feature fields: */
-- 
1.8.3.1

[Qemu-devel] [PULL 04/16] target-i386: kvm_cpu_fill_host(): No need to check xlevel2

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

There's no need to check CPU xlevel2 before calling
kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX), because:

 * The kernel won't return any entry for 0xC0000000 if host CPU vendor
   is not Centaur (See kvm_dev_ioctl_get_supported_cpuid() on the kernel
   code)
 * Similarly, the kernel won't return any entry for 0xC0000001 if
   CPUID[0xC0000000].EAX is < 0xC0000001
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf

For similar reasons, we can simply set x86_cpu_def->xlevel2 directly
instead of making it conditional, because it will be set to 0 CPU vendor
is not Centaur.

This will simplify the kvm_cpu_fill_host() code a little.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[Remove unparseable comment. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1e115cf..5c3817c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1177,13 +1177,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
     /* Call Centaur's CPUID instruction. */
-    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-    if (eax >= 0xC0000001) {
-        /* Support VIA max extended level */
-        x86_cpu_def->xlevel2 = eax;
-        x86_cpu_def->features[FEAT_C000_0001_EDX] =
-                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-    }
+    x86_cpu_def->xlevel2 =
+        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    x86_cpu_def->features[FEAT_C000_0001_EDX] =
+        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
 
     /* Other KVM-specific feature fields: */
     x86_cpu_def->features[FEAT_SVM] =
-- 
1.8.3.1

[Qemu-devel] [PULL 05/16] target-i386: kvm_cpu_fill_host(): Set all feature words at end of function

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

Reorder the code so all the code that sets x86_cpu_def->features is at
the end of the function.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5c3817c..237af97 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1160,29 +1160,24 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->stepping = eax & 0x0F;
 
     x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+    x86_cpu_def->xlevel2 =
+        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+
+    cpu_x86_fill_model_id(x86_cpu_def->model_id);
+
     x86_cpu_def->features[FEAT_1_EDX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
     x86_cpu_def->features[FEAT_1_ECX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
-
     x86_cpu_def->features[FEAT_7_0_EBX] =
-                kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-
-    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+        kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
     x86_cpu_def->features[FEAT_8000_0001_EDX] =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
     x86_cpu_def->features[FEAT_8000_0001_ECX] =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
-
-    cpu_x86_fill_model_id(x86_cpu_def->model_id);
-
-    /* Call Centaur's CPUID instruction. */
-    x86_cpu_def->xlevel2 =
-        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
     x86_cpu_def->features[FEAT_C000_0001_EDX] =
         kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-
-    /* Other KVM-specific feature fields: */
     x86_cpu_def->features[FEAT_SVM] =
         kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
     x86_cpu_def->features[FEAT_KVM] =
-- 
1.8.3.1

[Qemu-devel] [PULL 06/16] target-i386: kvm_cpu_fill_host(): Fill feature words in a loop

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

Now that the kvm_cpu_fill_host() code is simplified, we can simply set
the feature word array using a simple loop.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 237af97..200ad42 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1166,22 +1166,13 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
-    x86_cpu_def->features[FEAT_1_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
-    x86_cpu_def->features[FEAT_1_ECX] =
-        kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
-    x86_cpu_def->features[FEAT_7_0_EBX] =
-        kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-    x86_cpu_def->features[FEAT_8000_0001_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
-    x86_cpu_def->features[FEAT_8000_0001_ECX] =
-        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
-    x86_cpu_def->features[FEAT_C000_0001_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-    x86_cpu_def->features[FEAT_SVM] =
-        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
-    x86_cpu_def->features[FEAT_KVM] =
-        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+    FeatureWord w;
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        FeatureWordInfo *wi = &feature_word_info[w];
+        x86_cpu_def->features[w] =
+            kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
+                                         wi->cpuid_reg);
+    }
 
 #endif /* CONFIG_KVM */
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 07/16] target-i386: kvm_check_features_against_host(): Kill feature word array

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

We don't need the ft[] array on kvm_check_features_against_host()
anymore, as we can simply use the feature_word_info[] array, that has
everything we need.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 48 ++++++++++++------------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 200ad42..2e0be01 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1201,48 +1201,23 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
  *
  * This function may be called only if KVM is enabled.
  */
-static int kvm_check_features_against_host(X86CPU *cpu)
+static int kvm_check_features_against_host(KVMState *s, X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    x86_def_t host_def;
-    uint32_t mask;
-    int rv, i;
-    struct model_features_t ft[] = {
-        {&env->features[FEAT_1_EDX],
-            &host_def.features[FEAT_1_EDX],
-            FEAT_1_EDX },
-        {&env->features[FEAT_1_ECX],
-            &host_def.features[FEAT_1_ECX],
-            FEAT_1_ECX },
-        {&env->features[FEAT_8000_0001_EDX],
-            &host_def.features[FEAT_8000_0001_EDX],
-            FEAT_8000_0001_EDX },
-        {&env->features[FEAT_8000_0001_ECX],
-            &host_def.features[FEAT_8000_0001_ECX],
-            FEAT_8000_0001_ECX },
-        {&env->features[FEAT_C000_0001_EDX],
-            &host_def.features[FEAT_C000_0001_EDX],
-            FEAT_C000_0001_EDX },
-        {&env->features[FEAT_7_0_EBX],
-            &host_def.features[FEAT_7_0_EBX],
-            FEAT_7_0_EBX },
-        {&env->features[FEAT_SVM],
-            &host_def.features[FEAT_SVM],
-            FEAT_SVM },
-        {&env->features[FEAT_KVM],
-            &host_def.features[FEAT_KVM],
-            FEAT_KVM },
-    };
+    int rv = 0;
+    FeatureWord w;
 
     assert(kvm_enabled());
 
-    kvm_cpu_fill_host(&host_def);
-    for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
-        FeatureWord w = ft[i].feat_word;
+    for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
+        uint32_t guest_feat = env->features[w];
+        uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
+                                                             wi->cpuid_ecx,
+                                                             wi->cpuid_reg);
+        uint32_t mask;
         for (mask = 1; mask; mask <<= 1) {
-            if (*ft[i].guest_feat & mask &&
-                !(*ft[i].host_feat & mask)) {
+            if (guest_feat & mask && !(host_feat & mask)) {
                 unavailable_host_feature(wi, mask);
                 rv = 1;
             }
@@ -2563,8 +2538,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
         env->features[FEAT_SVM] &= TCG_SVM_FEATURES;
     } else {
+        KVMState *s = kvm_state;
         if ((cpu->check_cpuid || cpu->enforce_cpuid)
-            && kvm_check_features_against_host(cpu) && cpu->enforce_cpuid) {
+            && kvm_check_features_against_host(s, cpu) && cpu->enforce_cpuid) {
             error_setg(&local_err,
                        "Host's CPU doesn't support requested features");
             goto out;
-- 
1.8.3.1

[Qemu-devel] [PULL 08/16] kvm: print suberror on all internal errors

[Paolo Bonzini]

From: Radim Krčmář <rkrcmar@redhat.com>

KVM introduced internal error exit reason and suberror at the same time,
and later extended it with internal error data.
QEMU does not report suberror on hosts between these two events because
we check for the extension. (half a year in 2009, but it is misleading)

Fix by removing KVM_CAP_INTERNAL_ERROR_DATA condition on printf.

(partially improved by bb44e0d12df70 and ba4047cf848a3 in the past)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index a3fb8de..f742f8d 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1545,17 +1545,16 @@ static void kvm_handle_io(uint16_t port, void *data, int direction, int size,
 
 static int kvm_handle_internal_error(CPUState *cpu, struct kvm_run *run)
 {
-    fprintf(stderr, "KVM internal error.");
+    fprintf(stderr, "KVM internal error. Suberror: %d\n",
+            run->internal.suberror);
+
     if (kvm_check_extension(kvm_state, KVM_CAP_INTERNAL_ERROR_DATA)) {
         int i;
 
-        fprintf(stderr, " Suberror: %d\n", run->internal.suberror);
         for (i = 0; i < run->internal.ndata; ++i) {
             fprintf(stderr, "extra data[%d]: %"PRIx64"\n",
                     i, (uint64_t)run->internal.data[i]);
         }
-    } else {
-        fprintf(stderr, "\n");
     }
     if (run->internal.suberror == KVM_INTERNAL_ERROR_EMULATION) {
         fprintf(stderr, "emulation failure\n");
-- 
1.8.3.1

[Qemu-devel] [PULL 09/16] KVM: fix coexistence of KVM and Hyper-V leaves

[Paolo Bonzini]

kvm_arch_init_vcpu's initialization of the KVM leaves at 0x40000100
is broken, because KVM_CPUID_FEATURES is left at 0x40000001.  Move
it to 0x40000101 if Hyper-V is enabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0a21c30..5738911 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -455,6 +455,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     uint32_t unused;
     struct kvm_cpuid_entry2 *c;
     uint32_t signature[3];
+    int kvm_base = KVM_CPUID_SIGNATURE;
     int r;
 
     memset(&cpuid_data, 0, sizeof(cpuid_data));
@@ -462,26 +463,22 @@ int kvm_arch_init_vcpu(CPUState *cs)
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
-    c = &cpuid_data.entries[cpuid_i++];
-    c->function = KVM_CPUID_SIGNATURE;
-    if (!hyperv_enabled(cpu)) {
-        memcpy(signature, "KVMKVMKVM\0\0\0", 12);
-        c->eax = 0;
-    } else {
+    if (hyperv_enabled(cpu)) {
+        c = &cpuid_data.entries[cpuid_i++];
+        c->function = HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
         memcpy(signature, "Microsoft Hv", 12);
         c->eax = HYPERV_CPUID_MIN;
-    }
-    c->ebx = signature[0];
-    c->ecx = signature[1];
-    c->edx = signature[2];
-
-    c = &cpuid_data.entries[cpuid_i++];
-    c->function = KVM_CPUID_FEATURES;
-    c->eax = env->features[FEAT_KVM];
+        c->ebx = signature[0];
+        c->ecx = signature[1];
+        c->edx = signature[2];
 
-    if (hyperv_enabled(cpu)) {
+        c = &cpuid_data.entries[cpuid_i++];
+        c->function = HYPERV_CPUID_INTERFACE;
         memcpy(signature, "Hv#1\0\0\0\0\0\0\0\0", 12);
         c->eax = signature[0];
+        c->ebx = 0;
+        c->ecx = 0;
+        c->edx = 0;
 
         c = &cpuid_data.entries[cpuid_i++];
         c->function = HYPERV_CPUID_VERSION;
@@ -513,15 +510,21 @@ int kvm_arch_init_vcpu(CPUState *cs)
         c->eax = 0x40;
         c->ebx = 0x40;
 
-        c = &cpuid_data.entries[cpuid_i++];
-        c->function = KVM_CPUID_SIGNATURE_NEXT;
-        memcpy(signature, "KVMKVMKVM\0\0\0", 12);
-        c->eax = 0;
-        c->ebx = signature[0];
-        c->ecx = signature[1];
-        c->edx = signature[2];
+        kvm_base = KVM_CPUID_SIGNATURE_NEXT;
     }
 
+    memcpy(signature, "KVMKVMKVM\0\0\0", 12);
+    c = &cpuid_data.entries[cpuid_i++];
+    c->function = KVM_CPUID_SIGNATURE | kvm_base;
+    c->eax = 0;
+    c->ebx = signature[0];
+    c->ecx = signature[1];
+    c->edx = signature[2];
+
+    c = &cpuid_data.entries[cpuid_i++];
+    c->function = KVM_CPUID_FEATURES | kvm_base;
+    c->eax = env->features[FEAT_KVM];
+
     has_msr_async_pf_en = c->eax & (1 << KVM_FEATURE_ASYNC_PF);
 
     has_msr_pv_eoi_en = c->eax & (1 << KVM_FEATURE_PV_EOI);
-- 
1.8.3.1

[Qemu-devel] [PULL 10/16] kvm: make availability of Hyper-V enlightenments dependent on KVM_CAP_HYPERV

[Paolo Bonzini]

The MS docs specify HV_X64_MSR_HYPERCALL as a mandatory interface,
thus we must provide the MSRs even if the user only specified
features that, like relaxed timing, in principle don't require them.
And the MSRs are only there if the hypervisor has KVM_CAP_HYPERV.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5738911..e6831b2 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -72,6 +72,8 @@ static bool has_msr_misc_enable;
 static bool has_msr_bndcfgs;
 static bool has_msr_kvm_steal_time;
 static int lm_capable_kernel;
+static bool has_msr_hv_hypercall;
+static bool has_msr_hv_vapic;
 
 static bool has_msr_architectural_pmu;
 static uint32_t num_architectural_pmu_counters;
@@ -437,8 +439,10 @@ static bool hyperv_hypercall_available(X86CPU *cpu)
 
 static bool hyperv_enabled(X86CPU *cpu)
 {
-    return hyperv_hypercall_available(cpu) ||
-           cpu->hyperv_relaxed_timing;
+    CPUState *cs = CPU(cpu);
+    return kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV) > 0 &&
+           (hyperv_hypercall_available(cpu) ||
+            cpu->hyperv_relaxed_timing);
 }
 
 #define KVM_MAX_CPUID_ENTRIES  100
@@ -493,6 +497,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         if (cpu->hyperv_vapic) {
             c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE;
             c->eax |= HV_X64_MSR_APIC_ACCESS_AVAILABLE;
+            has_msr_hv_vapic = true;
         }
 
         c = &cpuid_data.entries[cpuid_i++];
@@ -500,7 +505,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         if (cpu->hyperv_relaxed_timing) {
             c->eax |= HV_X64_RELAXED_TIMING_RECOMMENDED;
         }
-        if (cpu->hyperv_vapic) {
+        if (has_msr_hv_vapic) {
             c->eax |= HV_X64_APIC_ACCESS_RECOMMENDED;
         }
         c->ebx = cpu->hyperv_spinlock_attempts;
@@ -511,6 +516,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         c->ebx = 0x40;
 
         kvm_base = KVM_CPUID_SIGNATURE_NEXT;
+        has_msr_hv_hypercall = true;
     }
 
     memcpy(signature, "KVMKVMKVM\0\0\0", 12);
@@ -1223,11 +1229,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_set(&msrs[n++], MSR_CORE_PERF_GLOBAL_CTRL,
                               env->msr_global_ctrl);
         }
-        if (hyperv_hypercall_available(cpu)) {
+        if (has_msr_hv_hypercall) {
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_GUEST_OS_ID, 0);
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_HYPERCALL, 0);
         }
-        if (cpu->hyperv_vapic) {
+        if (has_msr_hv_vapic) {
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0);
         }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 11/16] kvm: make hyperv hypercall and guest os id MSRs migratable.

[Paolo Bonzini]

From: Vadim Rozenfeld <vrozenfe@redhat.com>

Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.h     |  2 ++
 target-i386/kvm.c     | 16 ++++++++++++++--
 target-i386/machine.c | 23 +++++++++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 1fcbc82..3dba5ef 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -862,6 +862,8 @@ typedef struct CPUX86State {
     uint64_t msr_fixed_counters[MAX_FIXED_COUNTERS];
     uint64_t msr_gp_counters[MAX_GP_COUNTERS];
     uint64_t msr_gp_evtsel[MAX_GP_COUNTERS];
+    uint64_t msr_hv_hypercall;
+    uint64_t msr_hv_guest_os_id;
 
     /* exception/interrupt handling */
     int error_code;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index e6831b2..fade2c9 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1230,8 +1230,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
                               env->msr_global_ctrl);
         }
         if (has_msr_hv_hypercall) {
-            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_GUEST_OS_ID, 0);
-            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_HYPERCALL, 0);
+            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_GUEST_OS_ID,
+                              env->msr_hv_guest_os_id);
+            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_HYPERCALL,
+                              env->msr_hv_hypercall);
         }
         if (has_msr_hv_vapic) {
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0);
@@ -1520,6 +1522,10 @@ static int kvm_get_msrs(X86CPU *cpu)
         }
     }
 
+    if (has_msr_hv_hypercall) {
+        msrs[n++].index = HV_X64_MSR_HYPERCALL;
+        msrs[n++].index = HV_X64_MSR_GUEST_OS_ID;
+    }
     msr_data.info.nmsrs = n;
     ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
     if (ret < 0) {
@@ -1627,6 +1633,12 @@ static int kvm_get_msrs(X86CPU *cpu)
         case MSR_P6_EVNTSEL0 ... MSR_P6_EVNTSEL0 + MAX_GP_COUNTERS - 1:
             env->msr_gp_evtsel[index - MSR_P6_EVNTSEL0] = msrs[i].data;
             break;
+        case HV_X64_MSR_HYPERCALL:
+            env->msr_hv_hypercall = msrs[i].data;
+            break;
+        case HV_X64_MSR_GUEST_OS_ID:
+            env->msr_hv_guest_os_id = msrs[i].data;
+            break;
         }
     }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 2de1964..96fd045 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -554,6 +554,26 @@ static const VMStateDescription vmstate_mpx = {
     }
 };
 
+static bool hyperv_hypercall_enable_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->msr_hv_hypercall != 0 || env->msr_hv_guest_os_id != 0;
+}
+
+static const VMStateDescription vmstate_msr_hypercall_hypercall = {
+    .name = "cpu/msr_hyperv_hypercall",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
+        VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -688,6 +708,9 @@ const VMStateDescription vmstate_x86_cpu = {
         } , {
             .vmsd = &vmstate_mpx,
             .needed = mpx_needed,
+        }, {
+            .vmsd = &vmstate_msr_hypercall_hypercall,
+            .needed = hyperv_hypercall_enable_needed,
         } , {
             /* empty */
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 12/16] kvm: make hyperv vapic assist page migratable

[Paolo Bonzini]

From: Vadim Rozenfeld <vrozenfe@redhat.com>

Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 10 +++++++++-
 target-i386/machine.c | 22 ++++++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 3dba5ef..45bd554 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -864,6 +864,7 @@ typedef struct CPUX86State {
     uint64_t msr_gp_evtsel[MAX_GP_COUNTERS];
     uint64_t msr_hv_hypercall;
     uint64_t msr_hv_guest_os_id;
+    uint64_t msr_hv_vapic;
 
     /* exception/interrupt handling */
     int error_code;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index fade2c9..ddd437f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -1236,7 +1236,8 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
                               env->msr_hv_hypercall);
         }
         if (has_msr_hv_vapic) {
-            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE, 0);
+            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE,
+                              env->msr_hv_vapic);
         }
 
         /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
@@ -1526,6 +1527,10 @@ static int kvm_get_msrs(X86CPU *cpu)
         msrs[n++].index = HV_X64_MSR_HYPERCALL;
         msrs[n++].index = HV_X64_MSR_GUEST_OS_ID;
     }
+    if (has_msr_hv_vapic) {
+        msrs[n++].index = HV_X64_MSR_APIC_ASSIST_PAGE;
+    }
+
     msr_data.info.nmsrs = n;
     ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
     if (ret < 0) {
@@ -1639,6 +1644,9 @@ static int kvm_get_msrs(X86CPU *cpu)
         case HV_X64_MSR_GUEST_OS_ID:
             env->msr_hv_guest_os_id = msrs[i].data;
             break;
+        case HV_X64_MSR_APIC_ASSIST_PAGE:
+            env->msr_hv_vapic = msrs[i].data;
+            break;
         }
     }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 96fd045..e72e270 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -574,6 +574,25 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
     }
 };
 
+static bool hyperv_vapic_enable_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->msr_hv_vapic != 0;
+}
+
+static const VMStateDescription vmstate_msr_hyperv_vapic = {
+    .name = "cpu/msr_hyperv_vapic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT64(env.msr_hv_vapic, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -711,6 +730,9 @@ const VMStateDescription vmstate_x86_cpu = {
         }, {
             .vmsd = &vmstate_msr_hypercall_hypercall,
             .needed = hyperv_hypercall_enable_needed,
+        }, {
+            .vmsd = &vmstate_msr_hyperv_vapic,
+            .needed = hyperv_vapic_enable_needed,
         } , {
             /* empty */
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 13/16] kvm: add support for hyper-v timers

[Paolo Bonzini]

From: Vadim Rozenfeld <vrozenfe@redhat.com>

http://msdn.microsoft.com/en-us/library/windows/hardware/ff541625%28v=vs.85%29.aspx

This code is generic for activating reference time counter or virtual reference time stamp counter

Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 linux-headers/asm-x86/hyperv.h |  3 +++
 linux-headers/linux/kvm.h      |  1 +
 target-i386/cpu-qom.h          |  1 +
 target-i386/cpu.c              |  1 +
 target-i386/cpu.h              |  1 +
 target-i386/kvm.c              | 20 +++++++++++++++++++-
 target-i386/machine.c          | 22 ++++++++++++++++++++++
 7 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/linux-headers/asm-x86/hyperv.h b/linux-headers/asm-x86/hyperv.h
index b8f1c01..3b400ee 100644
--- a/linux-headers/asm-x86/hyperv.h
+++ b/linux-headers/asm-x86/hyperv.h
@@ -149,6 +149,9 @@
 /* MSR used to read the per-partition time reference counter */
 #define HV_X64_MSR_TIME_REF_COUNT		0x40000020
 
+/* A partition's reference time stamp counter (TSC) page */
+#define HV_X64_MSR_REFERENCE_TSC		0x40000021
+
 /* MSR used to retrieve the TSC frequency */
 #define HV_X64_MSR_TSC_FREQUENCY		0x40000022
 
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 5a49671..999fb13 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -674,6 +674,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_ARM_EL1_32BIT 93
 #define KVM_CAP_SPAPR_MULTITCE 94
 #define KVM_CAP_EXT_EMUL_CPUID 95
+#define KVM_CAP_HYPERV_TIME 96
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index d1751a4..722f11a 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -69,6 +69,7 @@ typedef struct X86CPU {
     bool hyperv_vapic;
     bool hyperv_relaxed_timing;
     int hyperv_spinlock_attempts;
+    bool hyperv_time;
     bool check_cpuid;
     bool enforce_cpuid;
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2e0be01..1f30efd 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2702,6 +2702,7 @@ static Property x86_cpu_properties[] = {
     { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
     DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
     DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
+    DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_END_OF_LIST()
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 45bd554..1b94f0f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -865,6 +865,7 @@ typedef struct CPUX86State {
     uint64_t msr_hv_hypercall;
     uint64_t msr_hv_guest_os_id;
     uint64_t msr_hv_vapic;
+    uint64_t msr_hv_tsc;
 
     /* exception/interrupt handling */
     int error_code;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ddd437f..e555040 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -74,6 +74,7 @@ static bool has_msr_kvm_steal_time;
 static int lm_capable_kernel;
 static bool has_msr_hv_hypercall;
 static bool has_msr_hv_vapic;
+static bool has_msr_hv_tsc;
 
 static bool has_msr_architectural_pmu;
 static uint32_t num_architectural_pmu_counters;
@@ -442,6 +443,7 @@ static bool hyperv_enabled(X86CPU *cpu)
     CPUState *cs = CPU(cpu);
     return kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV) > 0 &&
            (hyperv_hypercall_available(cpu) ||
+            cpu->hyperv_time  ||
             cpu->hyperv_relaxed_timing);
 }
 
@@ -499,7 +501,13 @@ int kvm_arch_init_vcpu(CPUState *cs)
             c->eax |= HV_X64_MSR_APIC_ACCESS_AVAILABLE;
             has_msr_hv_vapic = true;
         }
-
+        if (cpu->hyperv_time &&
+            kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV_TIME) > 0) {
+            c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE;
+            c->eax |= HV_X64_MSR_TIME_REF_COUNT_AVAILABLE;
+            c->eax |= 0x200;
+            has_msr_hv_tsc = true;
+        }
         c = &cpuid_data.entries[cpuid_i++];
         c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
         if (cpu->hyperv_relaxed_timing) {
@@ -1239,6 +1247,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_APIC_ASSIST_PAGE,
                               env->msr_hv_vapic);
         }
+        if (has_msr_hv_tsc) {
+            kvm_msr_entry_set(&msrs[n++], HV_X64_MSR_REFERENCE_TSC,
+                              env->msr_hv_tsc);
+        }
 
         /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
          *       kvm_put_msr_feature_control. */
@@ -1530,6 +1542,9 @@ static int kvm_get_msrs(X86CPU *cpu)
     if (has_msr_hv_vapic) {
         msrs[n++].index = HV_X64_MSR_APIC_ASSIST_PAGE;
     }
+    if (has_msr_hv_tsc) {
+        msrs[n++].index = HV_X64_MSR_REFERENCE_TSC;
+    }
 
     msr_data.info.nmsrs = n;
     ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, &msr_data);
@@ -1647,6 +1662,9 @@ static int kvm_get_msrs(X86CPU *cpu)
         case HV_X64_MSR_APIC_ASSIST_PAGE:
             env->msr_hv_vapic = msrs[i].data;
             break;
+        case HV_X64_MSR_REFERENCE_TSC:
+            env->msr_hv_tsc = msrs[i].data;
+            break;
         }
     }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index e72e270..d548c05 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -593,6 +593,25 @@ static const VMStateDescription vmstate_msr_hyperv_vapic = {
     }
 };
 
+static bool hyperv_time_enable_needed(void *opaque)
+{
+    X86CPU *cpu = opaque;
+    CPUX86State *env = &cpu->env;
+
+    return env->msr_hv_tsc != 0;
+}
+
+static const VMStateDescription vmstate_msr_hyperv_time = {
+    .name = "cpu/msr_hyperv_time",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT64(env.msr_hv_tsc, X86CPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_x86_cpu = {
     .name = "cpu",
     .version_id = 12,
@@ -733,6 +752,9 @@ const VMStateDescription vmstate_x86_cpu = {
         }, {
             .vmsd = &vmstate_msr_hyperv_vapic,
             .needed = hyperv_vapic_enable_needed,
+        }, {
+            .vmsd = &vmstate_msr_hyperv_time,
+            .needed = hyperv_time_enable_needed,
         } , {
             /* empty */
         }
-- 
1.8.3.1

[Qemu-devel] [PULL 14/16] target-i386: Eliminate CONFIG_KVM #ifdefs

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

The compiler is already able to eliminate the kvm_arch_get_supported_cpuid()
calls in kvm_cpu_fill_host() and filter_features_for_kvm(), so we can
eliminate the CONFIG_KVM #ifdefs there.

Also, kvm_cpu_fill_host() and host_cpuid() don't need to check
CONFIG_KVM, as they don't have any KVM-specific function calls.

Tested to build successfully with CONFIG_KVM disabled, using the
following CFLAGS combinations: "-DNDEBUG", "-DNDEBUG -O', "-DNDEBUG
-O0", "-DNDEBUG -O1", "-DNDEBUG -O2".

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1f30efd..8425212 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -374,7 +374,6 @@ void disable_kvm_pv_eoi(void)
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
-#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -382,7 +381,7 @@ void host_cpuid(uint32_t function, uint32_t count,
                  : "=a"(vec[0]), "=b"(vec[1]),
                    "=c"(vec[2]), "=d"(vec[3])
                  : "0"(function), "c"(count) : "cc");
-#else
+#elif defined(__i386__)
     asm volatile("pusha \n\t"
                  "cpuid \n\t"
                  "mov %%eax, 0(%2) \n\t"
@@ -392,6 +391,8 @@ void host_cpuid(uint32_t function, uint32_t count,
                  "popa"
                  : : "a"(function), "c"(count), "S"(vec)
                  : "memory", "cc");
+#else
+    abort();
 #endif
 
     if (eax)
@@ -402,7 +403,6 @@ void host_cpuid(uint32_t function, uint32_t count,
         *ecx = vec[2];
     if (edx)
         *edx = vec[3];
-#endif
 }
 
 #define iswhite(c) ((c) && ((c) <= ' ' || '~' < (c)))
@@ -1119,7 +1119,6 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w,
     }
 }
 
-#ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1134,7 +1133,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     return 0;
 }
-#endif
 
 /* Fill a x86_def_t struct with information about the host CPU, and
  * the CPU features supported by the host hardware + host kernel
@@ -1143,7 +1141,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 {
-#ifdef CONFIG_KVM
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
@@ -1173,8 +1170,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
             kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
                                          wi->cpuid_reg);
     }
-
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
@@ -1817,7 +1812,6 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
-#ifdef CONFIG_KVM
 static void filter_features_for_kvm(X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
@@ -1834,7 +1828,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
         cpu->filtered_features[w] = requested_features & ~env->features[w];
     }
 }
-#endif
 
 static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
@@ -2545,9 +2538,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                        "Host's CPU doesn't support requested features");
             goto out;
         }
-#ifdef CONFIG_KVM
         filter_features_for_kvm(cpu);
-#endif
     }
 
 #ifndef CONFIG_USER_ONLY
-- 
1.8.3.1

[Qemu-devel] [PULL 15/16] target-i386: Don't change x86_def_t struct on cpu_x86_register()

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

As eventually the x86_def_t data is going to be provided by the CPU
class, it's better to not touch it, and handle the special cases on the
X86CPU object itself.

Current behavior of the code should stay exactly the same.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8425212..be54f84 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1841,11 +1841,6 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         return;
     }
 
-    if (kvm_enabled()) {
-        def->features[FEAT_KVM] |= kvm_default_features;
-    }
-    def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
-
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
@@ -1864,6 +1859,12 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
     cpu->cache_info_passthrough = def->cache_info_passthrough;
 
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+
+    /* Special cases not set in the x86_def_t structs: */
+    if (kvm_enabled()) {
+        env->features[FEAT_KVM] |= kvm_default_features;
+    }
+    env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
 }
 
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
-- 
1.8.3.1

[Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

As we will not have a cpu_x86_find_by_name() function anymore,
move the KVM default-vendor hack to instance_init.

Unfortunately we can't move that code to class_init because it depends
on KVM being initialized.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index be54f84..0e8812a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1601,18 +1601,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
         def = &builtin_x86_defs[i];
         if (strcmp(name, def->name) == 0) {
             memcpy(x86_cpu_def, def, sizeof(*def));
-            /* sysenter isn't supported in compatibility mode on AMD,
-             * syscall isn't supported in compatibility mode on Intel.
-             * Normally we advertise the actual CPU vendor, but you can
-             * override this using the 'vendor' property if you want to use
-             * KVM's sysenter/syscall emulation in compatibility mode and
-             * when doing cross vendor migration
-             */
-            if (kvm_enabled()) {
-                uint32_t  ebx = 0, ecx = 0, edx = 0;
-                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
-                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
-            }
             return 0;
         }
     }
@@ -1841,7 +1829,6 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         return;
     }
 
-    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     object_property_set_int(OBJECT(cpu), def->level, "level", errp);
     object_property_set_int(OBJECT(cpu), def->family, "family", errp);
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
@@ -1865,6 +1852,25 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
         env->features[FEAT_KVM] |= kvm_default_features;
     }
     env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
+
+    /* sysenter isn't supported in compatibility mode on AMD,
+     * syscall isn't supported in compatibility mode on Intel.
+     * Normally we advertise the actual CPU vendor, but you can
+     * override this using the 'vendor' property if you want to use
+     * KVM's sysenter/syscall emulation in compatibility mode and
+     * when doing cross vendor migration
+     */
+    const char *vendor = def->vendor;
+    char host_vendor[CPUID_VENDOR_SZ + 1];
+    if (kvm_enabled()) {
+        uint32_t  ebx = 0, ecx = 0, edx = 0;
+        host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+        x86_cpu_vendor_words2str(host_vendor, ebx, edx, ecx);
+        vendor = host_vendor;
+    }
+
+    object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
+
 }
 
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 00/16] KVM changes for 2014-02-03

[Peter Maydell]

On 3 February 2014 16:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Anthony, Peter,
>
> the following changes since commit 0169c511554cb0014a00290b0d3d26c31a49818f:
>
>   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2014-01-24 15:52:44 -0800)
>
> are available in the git repository at:
>
>
>   git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git
>
> for you to fetch changes up to 7c08db30e6a43f7083a881eb07bfbc878e001e08
>
>   target-i386: Move KVM default-vendor hack to instance_init (2014-01-30 17:48:55 -0200)

This pull request mail seems to be missing a branch or tag
name for the pull.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/16] KVM changes for 2014-02-03

[Paolo Bonzini]

Il 06/02/2014 12:12, Peter Maydell ha scritto:
> On 3 February 2014 16:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Anthony, Peter,
> >
> > the following changes since commit 0169c511554cb0014a00290b0d3d26c31a49818f:
> >
> >   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging (2014-01-24 15:52:44 -0800)
> >
> > are available in the git repository at:
> >
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git
> >
> > for you to fetch changes up to 7c08db30e6a43f7083a881eb07bfbc878e001e08
> >
> >   target-i386: Move KVM default-vendor hack to instance_init (2014-01-30 17:48:55 -0200)
>
> This pull request mail seems to be missing a branch or tag
> name for the pull.

Sorry, the branch name is uq/master.

Paolo

Re: [Qemu-devel] [PULL 00/16] KVM changes for 2014-02-03

[Peter Maydell]

On 6 February 2014 23:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/02/2014 12:12, Peter Maydell ha scritto:
>
>> On 3 February 2014 16:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Anthony, Peter,
>> >
>> > the following changes since commit
>> > 0169c511554cb0014a00290b0d3d26c31a49818f:
>> >
>> >   Merge remote-tracking branch 'qemu-kvm/uq/master' into staging
>> > (2014-01-24 15:52:44 -0800)
>> >
>> > are available in the git repository at:
>> >
>> >
>> >   git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git
>> >
>> > for you to fetch changes up to 7c08db30e6a43f7083a881eb07bfbc878e001e08
>> >
>> >   target-i386: Move KVM default-vendor hack to instance_init (2014-01-30
>> > 17:48:55 -0200)
>>
>> This pull request mail seems to be missing a branch or tag
>> name for the pull.
>
>
> Sorry, the branch name is uq/master.

Applied, thanks.
-- PMM

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Andreas Färber]

Am 03.02.2014 17:39, schrieb Paolo Bonzini:
> From: Eduardo Habkost <ehabkost@redhat.com>
> 
> As we will not have a cpu_x86_find_by_name() function anymore,
> move the KVM default-vendor hack to instance_init.
> 
> Unfortunately we can't move that code to class_init because it depends
> on KVM being initialized.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-i386/cpu.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index be54f84..0e8812a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1601,18 +1601,6 @@ static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
>          def = &builtin_x86_defs[i];
>          if (strcmp(name, def->name) == 0) {
>              memcpy(x86_cpu_def, def, sizeof(*def));
> -            /* sysenter isn't supported in compatibility mode on AMD,
> -             * syscall isn't supported in compatibility mode on Intel.
> -             * Normally we advertise the actual CPU vendor, but you can
> -             * override this using the 'vendor' property if you want to use
> -             * KVM's sysenter/syscall emulation in compatibility mode and
> -             * when doing cross vendor migration
> -             */
> -            if (kvm_enabled()) {
> -                uint32_t  ebx = 0, ecx = 0, edx = 0;
> -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> -            }
>              return 0;
>          }
>      }
> @@ -1841,7 +1829,6 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
>          return;
>      }
>  
> -    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>      object_property_set_int(OBJECT(cpu), def->level, "level", errp);
>      object_property_set_int(OBJECT(cpu), def->family, "family", errp);
>      object_property_set_int(OBJECT(cpu), def->model, "model", errp);
> @@ -1865,6 +1852,25 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
>          env->features[FEAT_KVM] |= kvm_default_features;
>      }
>      env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
> +
> +    /* sysenter isn't supported in compatibility mode on AMD,
> +     * syscall isn't supported in compatibility mode on Intel.
> +     * Normally we advertise the actual CPU vendor, but you can
> +     * override this using the 'vendor' property if you want to use
> +     * KVM's sysenter/syscall emulation in compatibility mode and
> +     * when doing cross vendor migration
> +     */
> +    const char *vendor = def->vendor;
> +    char host_vendor[CPUID_VENDOR_SZ + 1];

Since when is it OK to declare variables in the middle of the block?
Are you planning to fix that?

Once again I note that a patch to a file under my maintenance was
applied without my review - and promptly a style bug slipped through. It
was not a bug fix, so there was no urgency in applying it.

Andreas

> +    if (kvm_enabled()) {
> +        uint32_t  ebx = 0, ecx = 0, edx = 0;
> +        host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> +        x86_cpu_vendor_words2str(host_vendor, ebx, edx, ecx);
> +        vendor = host_vendor;
> +    }
> +
> +    object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
> +
>  }
>  
>  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Paolo Bonzini]

Il 08/02/2014 18:28, Andreas Färber ha scritto:
>> +    /* sysenter isn't supported in compatibility mode on AMD,
>> +     * syscall isn't supported in compatibility mode on Intel.
>> +     * Normally we advertise the actual CPU vendor, but you can
>> +     * override this using the 'vendor' property if you want to use
>> +     * KVM's sysenter/syscall emulation in compatibility mode and
>> +     * when doing cross vendor migration
>> +     */
>> +    const char *vendor = def->vendor;
>> +    char host_vendor[CPUID_VENDOR_SZ + 1];
>
> Since when is it OK to declare variables in the middle of the block?

When the code looks better, it is OK since always: checkpatch.pl doesn't 
complain and -Wdeclaration-after-statement is not added to the compiler 
flags.  Usually I prefer to split a function if I feel the need for 
variable declarations in the middle of a block.  In this case, the 
variable is used till the end of the function, and a "fake" {...} block 
is worse than this.

> Are you planning to fix that?

No, I am not.

> Once again I note that a patch to a file under my maintenance was
> applied without my review - and promptly a style bug slipped through.

If you consider it a style bug, post a patch to add the 
-Wdeclaration-after-statement flag and/or to detect in checkpatch.pl. 
Until then, things are left to everyone's taste.  AFAIU declarations 
after statements are discouraged but not prohibited.

I applied these three patches because they only affect KVM.  Eduardo's 
other series "target-i386: Simplify kvm_cpu_fill_host() and 
kvm_check_features_against_host()" is also touching only 
target-i386/cpu.c and it's obviously KVM stuff.

Paolo

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Peter Maydell]

On 8 February 2014 23:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/02/2014 18:28, Andreas Färber ha scritto:
>> Since when is it OK to declare variables in the middle of the block?

> When the code looks better, it is OK since always: checkpatch.pl doesn't
> complain and -Wdeclaration-after-statement is not added to the compiler
> flags.

Huh? checkpatch is notoriously not a reliable guide, and we've had
the "declarations at start of block" rule since forever.

thanks
-- PMM

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Peter Maydell]

On 9 February 2014 00:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 February 2014 23:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 08/02/2014 18:28, Andreas Färber ha scritto:
>>> Since when is it OK to declare variables in the middle of the block?
>
>> When the code looks better, it is OK since always: checkpatch.pl doesn't
>> complain and -Wdeclaration-after-statement is not added to the compiler
>> flags.
>
> Huh? checkpatch is notoriously not a reliable guide, and we've had
> the "declarations at start of block" rule since forever.

FWIW, we have just 39 files[*] which fail to follow the "declarations at
start of block" rule out of 2566, which is pretty good compliance for a
coding style rule which isn't enforced by technical means (I expect
it's much better than we achieve for "no hard tabs" or "braces on
all if statement arms", for instance).

[*] as tested by a compile with -Wdeclaration-after-statement,
so slightly undercounting but not I think seriously.

thanks
-- PMM

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Andreas Färber]

Am 09.02.2014 00:33, schrieb Paolo Bonzini:
> If you consider it a style bug, post a patch to add the
> -Wdeclaration-after-statement flag and/or to detect in checkpatch.pl.
> Until then, things are left to everyone's taste.  AFAIU declarations
> after statements are discouraged but not prohibited.

Huh? Where is the justice in people telling me not to do things and to
change my patches in certain ways and now Igor, Eduardo, you and others
taking different measures for yourselves? Either rules apply equally to
all, or they apply to none of us!

I have been fair to ping you for my CPUState series in the past until
you either provided Acked-bys or gave me a go-ahead for KVM parts; you
are not returning that courtesy now and are trying to justify that.
I further usually gave a clear last-call before including in a pull.

If you read MAINTAINERS, you will find that target-i386/cpu.c is part of
the CPU subsystem under my maintenance, so I expect that patches
touching it wait for an okay before they get applied through some tree.
If you want to take over that file, just ask nicely and we can come to
terms - at the time no one else had volunteered.

Similarly I feel that you have given quite some destructive feedback to
my favorite series the last few months, not clearly stating how you want
things done instead. If you're jealous that you didn't make top 1 at KVM
Forum 2013 then you have lots of chances to catch up for 2.0
(virtio-scsi qtests, QOM realize conversions, another go at recursive
realization, creating proper devices from legacy code, ... just to give
some ideas) rather than being unfriendly to me and obstructive to my
line of work. I don't personally consider such statistics telling. And
I'm not aware of anything I've broken in KVM or SCSI that might explain.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Paolo Bonzini]

First of all, sorry for snipping the technical parts.  You made good 
points, but I think the actual problem is not technical.  We should 
first sort out the social part.

Il 09/02/2014 02:41, Andreas Färber ha scritto:
> Similarly I feel that you have given quite some destructive feedback to
> my favorite series

The only one I can think of is recursive realization, and that's because 
I'm absolutely not sure of the way to do it.  It's not destructive 
feedback, it's "are you sure it's the right way?  Because I don't even 
know _what_ is right".

> the last few months, not clearly stating how you want
> things done instead. If you're jealous that you didn't make top 1 at KVM
> Forum 2013

Are you serious?!?

> you have lots of chances to catch up for 2.0 rather than being
> unfriendly to me and obstructive to my line of work.

FWIW, picking up obvious KVM-ish patches is not being unfriendly, it's 
trying to be helpful.  If you haven't reviewed a patch for 3 months it 
obviously means that you were busy.

> I don't personally consider such statistics telling.

Me neither, FWIW.

Paolo

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Paolo Bonzini]

Il 09/02/2014 01:23, Peter Maydell ha scritto:
> FWIW, we have just 39 files[*] which fail to follow the "declarations at
> start of block" rule out of 2566, which is pretty good compliance for a
> coding style rule which isn't enforced by technical means (I expect
> it's much better than we achieve for "no hard tabs" or "braces on
> all if statement arms", for instance).

Ok, I just missed that and thought most people simply didn't like them. 
  It would be nice to add that to CODING_STYLE, at least.

Eduardo, can you make a patch to fix that?

Paolo

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Eduardo Habkost]

On Sun, Feb 09, 2014 at 12:10:20AM +0000, Peter Maydell wrote:
> On 8 February 2014 23:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 08/02/2014 18:28, Andreas Färber ha scritto:
> >> Since when is it OK to declare variables in the middle of the block?
> 
> > When the code looks better, it is OK since always: checkpatch.pl doesn't
> > complain and -Wdeclaration-after-statement is not added to the compiler
> > flags.
> 
> Huh? checkpatch is notoriously not a reliable guide, and we've had
> the "declarations at start of block" rule since forever.

Sorry for my confusion, but I was not aware of that rule, and I don't
know what I should use as a guide, if checkpatch.pl and CODING_STYLE are
not enough. Is there additional coding style documentation or scripts I
should look at?

-- 
Eduardo

[Qemu-devel] [PATCH] target-i386: Don't declare variables in the middle of blocks

[Eduardo Habkost]

Some of my recent changes introduced variable declarations in the middle
of code blocks.

Fix the code so that it compiles without warnings when using
-Wdeclaration-after-statement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..f36383a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1143,6 +1143,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 {
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    FeatureWord w;
 
     assert(kvm_enabled());
 
@@ -1163,7 +1164,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
-    FeatureWord w;
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
         x86_cpu_def->features[w] =
@@ -1821,6 +1821,8 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
 {
     CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
+    const char *vendor;
+    char host_vendor[CPUID_VENDOR_SZ + 1];
 
     memset(def, 0, sizeof(*def));
 
@@ -1860,8 +1862,7 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
      * KVM's sysenter/syscall emulation in compatibility mode and
      * when doing cross vendor migration
      */
-    const char *vendor = def->vendor;
-    char host_vendor[CPUID_VENDOR_SZ + 1];
+    vendor = def->vendor;
     if (kvm_enabled()) {
         uint32_t  ebx = 0, ecx = 0, edx = 0;
         host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
-- 
1.8.5.3

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Andreas Färber]

Am 09.02.2014 09:06, schrieb Eduardo Habkost:
> On Sun, Feb 09, 2014 at 12:10:20AM +0000, Peter Maydell wrote:
>> On 8 February 2014 23:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 08/02/2014 18:28, Andreas Färber ha scritto:
>>>> Since when is it OK to declare variables in the middle of the block?
>>
>>> When the code looks better, it is OK since always: checkpatch.pl doesn't
>>> complain and -Wdeclaration-after-statement is not added to the compiler
>>> flags.
>>
>> Huh? checkpatch is notoriously not a reliable guide, and we've had
>> the "declarations at start of block" rule since forever.
> 
> Sorry for my confusion, but I was not aware of that rule, and I don't
> know what I should use as a guide, if checkpatch.pl and CODING_STYLE are
> not enough. Is there additional coding style documentation or scripts I
> should look at?

HACKING exists as additional document, but it doesn't list it either.
There's a lot of unwritten rules that reoccur in review, such as no
QERR_*, no trailing \n in error_setg() and error_report(), ...

I think the history here is that QEMU didn't allow C99 some time back
and C89 (or ANSI C?) didn't allow declarations elsewhere. Blue was
fiercely in favor of keeping that rule (just like the
no-single-line-comments rule despite allowed from C99 on), saying that
goto statements might do the wrong thing otherwise. And I have been
advocating to make this rule more clear by uniformly separating
declarations from statements with a white line.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 16/16] target-i386: Move KVM default-vendor hack to instance_init

[Peter Maydell]

On 9 February 2014 08:06, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Sorry for my confusion, but I was not aware of that rule, and I don't
> know what I should use as a guide, if checkpatch.pl and CODING_STYLE are
> not enough. Is there additional coding style documentation or scripts I
> should look at?

Unfortunately as Andreas says a lot of this stuff is just unwritten
standards that get applied in code review (to a greater or lesser
degree of consistency). For a contributor the documents/scripts you list
(plus HACKING) are the right ones. Other than that all you can do
is make changes if people suggest them in code review (and
follow the same approach for future patches where applicable).
We certainly can't expect everybody to telepathically know the
rules we don't bother to write down; I was just a little surprised
that Paolo wasn't aware of this particular one since he's been
active in QEMU for a pretty long time.

thanks
-- PMM

[Qemu-devel] [qom-cpu PATCH v2] target-i386: Don't declare variables in the middle of blocks

[Eduardo Habkost]

Some of my recent changes introduced variable declarations in the middle
of code blocks.

Fix the code so that it compiles without warnings when using
-Wdeclaration-after-statement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 * Rebased on top of qom-cpu
---
 target-i386/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 5a530b5..9ff1062 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1143,6 +1143,7 @@ static void kvm_cpu_fill_host(X86CPUDefinition *x86_cpu_def)
 {
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    FeatureWord w;
 
     assert(kvm_enabled());
 
@@ -1163,7 +1164,6 @@ static void kvm_cpu_fill_host(X86CPUDefinition *x86_cpu_def)
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
-    FeatureWord w;
     for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
         x86_cpu_def->features[w] =
@@ -1823,6 +1823,8 @@ static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
 {
     CPUX86State *env = &cpu->env;
     X86CPUDefinition def1, *def = &def1;
+    const char *vendor;
+    char host_vendor[CPUID_VENDOR_SZ + 1];
 
     memset(def, 0, sizeof(*def));
 
@@ -1862,8 +1864,7 @@ static void x86_cpu_load_def(X86CPU *cpu, const char *name, Error **errp)
      * KVM's sysenter/syscall emulation in compatibility mode and
      * when doing cross vendor migration
      */
-    const char *vendor = def->vendor;
-    char host_vendor[CPUID_VENDOR_SZ + 1];
+    vendor = def->vendor;
     if (kvm_enabled()) {
         uint32_t  ebx = 0, ecx = 0, edx = 0;
         host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
-- 
1.8.5.3

Re: [Qemu-devel] [qom-cpu PATCH v2] target-i386: Don't declare variables in the middle of blocks

[Andreas Färber]

Am 19.02.2014 20:39, schrieb Eduardo Habkost:
> Some of my recent changes introduced variable declarations in the middle
> of code blocks.
> 
> Fix the code so that it compiles without warnings when using
> -Wdeclaration-after-statement.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v2:
>  * Rebased on top of qom-cpu

I could've sworn that I applied v1, but it's indeed not on qom-cpu...

Thanks for rebasing, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PULL 13/16] kvm: add support for hyper-v timers

[Peter Maydell]

On 3 February 2014 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Vadim Rozenfeld <vrozenfe@redhat.com>
>
> http://msdn.microsoft.com/en-us/library/windows/hardware/ff541625%28v=vs.85%29.aspx
>
> This code is generic for activating reference time counter or virtual reference time stamp counter
>
> Signed-off-by: Vadim Rozenfeld <vrozenfe@redhat.com>
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  linux-headers/asm-x86/hyperv.h |  3 +++
>  linux-headers/linux/kvm.h      |  1 +
>  target-i386/cpu-qom.h          |  1 +
>  target-i386/cpu.c              |  1 +
>  target-i386/cpu.h              |  1 +
>  target-i386/kvm.c              | 20 +++++++++++++++++++-
>  target-i386/machine.c          | 22 ++++++++++++++++++++++
>  7 files changed, 48 insertions(+), 1 deletion(-)

Old patch, but if I'd spotted this at the time I'd have rejected this pull
request: we shouldn't be changing linux-headers except with commits
which are doing nothing except syncing the headers to some specified
kernel version/commit. As far as I can tell the changes in this commit
don't correspond to a particular version of the kernel headers at all :-(

thanks
-- PMM