Re: [Qemu-devel] [PATCH v2 0/7] Hyper-V parameters update

[Paolo Bonzini <pbonzini@redhat.com>]

Il 23/01/2014 14:40, Vadim Rozenfeld ha scritto:
> This series consists of several clean-ups, hyper-v MSRs migration
> fixes, and adding support for new "hv-time" parameter, which designed
> for activating hyper-v timers feature.

Hi Vadim!

I think patches 1-4 have some problems:

(1) patches 1 and 4: the "KVMKVMKVM" and "Microsoft Hv" signatures are
used by Linux to understand the format of the leaves starting at
0x40000001.  These are of course different between KVM and Hyper-V.

Microsoft suggests that guests detect Hyper-V by checking if
eax=0x31237648 at CPUID[0x40000001].  Linux should be corrected to do
this check (and KVM probably should reserve some bit, e.g. bit 16, to
avoid that its own feature mask ever is 0x31237648), but anyway we
cannot change the vendor signature as that conflicts with the detection
scheme for KVM's own leaves.

(2) patches 2 and 3 should not be applied without some command-line
option or versioning scheme, because they would cause CPUID to change
across migration.

(3) patches 4, in addition, misses the point of
KVM_CPUID_SIGNATURE_NEXT, which is to signal that the KVM_CPUID_FEATURES
leaf is not at 0x40000001 but rather at 0x40000101 (KVM_CPUID_FEATURES +
KVM_CPUID_SIGNATURE_NEXT - KVM_CPUID_SIGNATURE, if you want).  This way, 
Linux can use the KVM features even if Hyper-V enlightenments enabled.

Unfortunately, the logic is broken because KVM_CPUID_FEATURES is not to 
0x40000101 if hyperv_enabled().  I include an untested patch to fix this 
at the end of the message.

Luckily they are unnecessary, I'll review patches 5-7 tomorrow but at a
first glance they seem good.

Paolo

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7522e98..d5cff89 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -454,6 +454,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));
@@ -461,26 +462,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->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];
-
-    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;
@@ -512,15 +509,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);