diff mbox series

[RFC,03/22] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()

Message ID 20200904145431.196885-4-vkuznets@redhat.com (mailing list archive)
State New, archived
Headers show
Series i386: KVM: expand Hyper-V features early | expand

Commit Message

Vitaly Kuznetsov Sept. 4, 2020, 2:54 p.m. UTC 
As a preparation to expanding Hyper-V CPU features early, move
hyperv_vendor_id initialization to x86_cpu_realizefn().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 target/i386/cpu.c | 15 ++++++++++++++-
 target/i386/cpu.h |  3 ++-
 target/i386/kvm.c | 25 ++++++++++---------------
 3 files changed, 26 insertions(+), 17 deletions(-)

Comments

Eduardo Habkost Sept. 18, 2020, 10:14 p.m. UTC | #1 
On Fri, Sep 04, 2020 at 04:54:12PM +0200, Vitaly Kuznetsov wrote:
> As a preparation to expanding Hyper-V CPU features early, move
> hyperv_vendor_id initialization to x86_cpu_realizefn().
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/cpu.c | 15 ++++++++++++++-
>  target/i386/cpu.h |  3 ++-
>  target/i386/kvm.c | 25 ++++++++++---------------
>  3 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 14489def2177..07e9da9e567e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6625,6 +6625,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> +    if (!cpu->hyperv_vendor) {
> +        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
> +    } else {
> +        size_t len = strlen(cpu->hyperv_vendor);
> +
> +        if (len > 12) {
> +            warn_report("hv-vendor-id truncated to 12 characters");
> +            len = 12;
> +        }
> +        memset(cpu->hyperv_vendor_id, 0, 12);
> +        memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
> +    }
> +

The change makes sense, but considering that we'll have a lot of
new code added to x86_cpu_realizefn(), I would prefer to create a
separate x86_cpu_hyperv_realize() function to make
x86_cpu_realizefn() a bit more readable.


>      if (cpu->ucode_rev == 0) {
>          /* The default is the same as KVM's.  */
>          if (IS_AMD_CPU(env)) {
> @@ -7313,7 +7326,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>      DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>      DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
> -    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
> +    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>      DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>      DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index d3097be6a50a..903994818093 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1654,11 +1654,12 @@ struct X86CPU {
>      uint64_t ucode_rev;
>  
>      uint32_t hyperv_spinlock_attempts;
> -    char *hyperv_vendor_id;
> +    char *hyperv_vendor;
>      bool hyperv_synic_kvm_only;
>      uint64_t hyperv_features;
>      bool hyperv_passthrough;
>      OnOffAuto hyperv_no_nonarch_cs;
> +    uint32_t hyperv_vendor_id[3];
>  
>      bool check_cpuid;
>      bool enforce_cpuid;
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 205b68bc0ce8..47779c5e1efd 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1225,6 +1225,13 @@ static int hyperv_handle_properties(CPUState *cs,
>          memcpy(cpuid_ent, &cpuid->entries[0],
>                 cpuid->nent * sizeof(cpuid->entries[0]));
>  
> +        c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
> +        if (c) {
> +            cpu->hyperv_vendor_id[0] = c->ebx;
> +            cpu->hyperv_vendor_id[1] = c->ecx;
> +            cpu->hyperv_vendor_id[2] = c->edx;
> +        }
> +

I can't find the equivalent of this code in the current tree?  Is
hyperv vendor ID broken when using hv-passthrough today?

Maybe this could be done as a separate patch, as it changes
behavior of hv-passthrough?

>          c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>          if (c) {
>              env->features[FEAT_HYPERV_EAX] = c->eax;
> @@ -1299,23 +1306,11 @@ static int hyperv_handle_properties(CPUState *cs,
>  
>      c = &cpuid_ent[cpuid_i++];
>      c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
> -    if (!cpu->hyperv_vendor_id) {
> -        memcpy(signature, "Microsoft Hv", 12);
> -    } else {
> -        size_t len = strlen(cpu->hyperv_vendor_id);
> -
> -        if (len > 12) {
> -            error_report("hv-vendor-id truncated to 12 characters");
> -            len = 12;
> -        }
> -        memset(signature, 0, 12);
> -        memcpy(signature, cpu->hyperv_vendor_id, len);
> -    }
>      c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>          HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
> -    c->ebx = signature[0];
> -    c->ecx = signature[1];
> -    c->edx = signature[2];
> +    c->ebx = cpu->hyperv_vendor_id[0];
> +    c->ecx = cpu->hyperv_vendor_id[1];
> +    c->edx = cpu->hyperv_vendor_id[2];
>  
>      c = &cpuid_ent[cpuid_i++];
>      c->function = HV_CPUID_INTERFACE;
> -- 
> 2.25.4
>
Vitaly Kuznetsov Sept. 22, 2020, 10:23 a.m. UTC | #2 
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 04, 2020 at 04:54:12PM +0200, Vitaly Kuznetsov wrote:
>> As a preparation to expanding Hyper-V CPU features early, move
>> hyperv_vendor_id initialization to x86_cpu_realizefn().
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  target/i386/cpu.c | 15 ++++++++++++++-
>>  target/i386/cpu.h |  3 ++-
>>  target/i386/kvm.c | 25 ++++++++++---------------
>>  3 files changed, 26 insertions(+), 17 deletions(-)
>> 
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 14489def2177..07e9da9e567e 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6625,6 +6625,19 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> +    if (!cpu->hyperv_vendor) {
>> +        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
>> +    } else {
>> +        size_t len = strlen(cpu->hyperv_vendor);
>> +
>> +        if (len > 12) {
>> +            warn_report("hv-vendor-id truncated to 12 characters");
>> +            len = 12;
>> +        }
>> +        memset(cpu->hyperv_vendor_id, 0, 12);
>> +        memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
>> +    }
>> +
>
> The change makes sense, but considering that we'll have a lot of
> new code added to x86_cpu_realizefn(), I would prefer to create a
> separate x86_cpu_hyperv_realize() function to make
> x86_cpu_realizefn() a bit more readable.
>

Agreed.

>
>>      if (cpu->ucode_rev == 0) {
>>          /* The default is the same as KVM's.  */
>>          if (IS_AMD_CPU(env)) {
>> @@ -7313,7 +7326,7 @@ static Property x86_cpu_properties[] = {
>>      DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
>>      DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
>>      DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
>> -    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
>> +    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
>>      DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
>>      DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
>>      DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index d3097be6a50a..903994818093 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1654,11 +1654,12 @@ struct X86CPU {
>>      uint64_t ucode_rev;
>>  
>>      uint32_t hyperv_spinlock_attempts;
>> -    char *hyperv_vendor_id;
>> +    char *hyperv_vendor;
>>      bool hyperv_synic_kvm_only;
>>      uint64_t hyperv_features;
>>      bool hyperv_passthrough;
>>      OnOffAuto hyperv_no_nonarch_cs;
>> +    uint32_t hyperv_vendor_id[3];
>>  
>>      bool check_cpuid;
>>      bool enforce_cpuid;
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 205b68bc0ce8..47779c5e1efd 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1225,6 +1225,13 @@ static int hyperv_handle_properties(CPUState *cs,
>>          memcpy(cpuid_ent, &cpuid->entries[0],
>>                 cpuid->nent * sizeof(cpuid->entries[0]));
>>  
>> +        c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
>> +        if (c) {
>> +            cpu->hyperv_vendor_id[0] = c->ebx;
>> +            cpu->hyperv_vendor_id[1] = c->ecx;
>> +            cpu->hyperv_vendor_id[2] = c->edx;
>> +        }
>> +
>
> I can't find the equivalent of this code in the current tree?  Is
> hyperv vendor ID broken when using hv-passthrough today?
>
> Maybe this could be done as a separate patch, as it changes
> behavior of hv-passthrough?

(this and similar changes in other patches) Actually we don't change
anything. Before this series and with hv-passthrough we just don't
reflect host's CPUIDs in our internal QEMU structures so
e.g. X86CPU->hyperv_vendor remains 'Microsoft Hv' while in reality
guest sees what kernel told us ("Linux KVM Hv" BTW). We just copy
everything we get from KVM_GET_SUPPORTED_HV_CPUID into guest's CPUIDs.
This is fine as we didn't actually need the information in QEMU but
to achieve the goal of the series we need to keep proper in-QEMU
representation.

The real change is that post-series QEMU is not enabling any Hyper-V
features which it doesn't know about while pre-series it was actually
doing this. This is arguably a good change: enabling new features may
require some additional work (e.g. enabling capabilities in KVM) and
without it just passing CPUID feature bits the guest may get confused.

>
>>          c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
>>          if (c) {
>>              env->features[FEAT_HYPERV_EAX] = c->eax;
>> @@ -1299,23 +1306,11 @@ static int hyperv_handle_properties(CPUState *cs,
>>  
>>      c = &cpuid_ent[cpuid_i++];
>>      c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>> -    if (!cpu->hyperv_vendor_id) {
>> -        memcpy(signature, "Microsoft Hv", 12);
>> -    } else {
>> -        size_t len = strlen(cpu->hyperv_vendor_id);
>> -
>> -        if (len > 12) {
>> -            error_report("hv-vendor-id truncated to 12 characters");
>> -            len = 12;
>> -        }
>> -        memset(signature, 0, 12);
>> -        memcpy(signature, cpu->hyperv_vendor_id, len);
>> -    }
>>      c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
>>          HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
>> -    c->ebx = signature[0];
>> -    c->ecx = signature[1];
>> -    c->edx = signature[2];
>> +    c->ebx = cpu->hyperv_vendor_id[0];
>> +    c->ecx = cpu->hyperv_vendor_id[1];
>> +    c->edx = cpu->hyperv_vendor_id[2];
>>  
>>      c = &cpuid_ent[cpuid_i++];
>>      c->function = HV_CPUID_INTERFACE;
>> -- 
>> 2.25.4
>>
diff mbox series

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 14489def2177..07e9da9e567e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6625,6 +6625,19 @@  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    if (!cpu->hyperv_vendor) {
+        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
+    } else {
+        size_t len = strlen(cpu->hyperv_vendor);
+
+        if (len > 12) {
+            warn_report("hv-vendor-id truncated to 12 characters");
+            len = 12;
+        }
+        memset(cpu->hyperv_vendor_id, 0, 12);
+        memcpy(cpu->hyperv_vendor_id, cpu->hyperv_vendor, len);
+    }
+
     if (cpu->ucode_rev == 0) {
         /* The default is the same as KVM's.  */
         if (IS_AMD_CPU(env)) {
@@ -7313,7 +7326,7 @@  static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("min-xlevel2", X86CPU, env.cpuid_min_xlevel2, 0),
     DEFINE_PROP_UINT64("ucode-rev", X86CPU, ucode_rev, 0),
     DEFINE_PROP_BOOL("full-cpuid-auto-level", X86CPU, full_cpuid_auto_level, true),
-    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
+    DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor),
     DEFINE_PROP_BOOL("cpuid-0xb", X86CPU, enable_cpuid_0xb, true),
     DEFINE_PROP_BOOL("lmce", X86CPU, enable_lmce, false),
     DEFINE_PROP_BOOL("l3-cache", X86CPU, enable_l3_cache, true),
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index d3097be6a50a..903994818093 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1654,11 +1654,12 @@  struct X86CPU {
     uint64_t ucode_rev;
 
     uint32_t hyperv_spinlock_attempts;
-    char *hyperv_vendor_id;
+    char *hyperv_vendor;
     bool hyperv_synic_kvm_only;
     uint64_t hyperv_features;
     bool hyperv_passthrough;
     OnOffAuto hyperv_no_nonarch_cs;
+    uint32_t hyperv_vendor_id[3];
 
     bool check_cpuid;
     bool enforce_cpuid;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 205b68bc0ce8..47779c5e1efd 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1225,6 +1225,13 @@  static int hyperv_handle_properties(CPUState *cs,
         memcpy(cpuid_ent, &cpuid->entries[0],
                cpuid->nent * sizeof(cpuid->entries[0]));
 
+        c = cpuid_find_entry(cpuid, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, 0);
+        if (c) {
+            cpu->hyperv_vendor_id[0] = c->ebx;
+            cpu->hyperv_vendor_id[1] = c->ecx;
+            cpu->hyperv_vendor_id[2] = c->edx;
+        }
+
         c = cpuid_find_entry(cpuid, HV_CPUID_FEATURES, 0);
         if (c) {
             env->features[FEAT_HYPERV_EAX] = c->eax;
@@ -1299,23 +1306,11 @@  static int hyperv_handle_properties(CPUState *cs,
 
     c = &cpuid_ent[cpuid_i++];
     c->function = HV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
-    if (!cpu->hyperv_vendor_id) {
-        memcpy(signature, "Microsoft Hv", 12);
-    } else {
-        size_t len = strlen(cpu->hyperv_vendor_id);
-
-        if (len > 12) {
-            error_report("hv-vendor-id truncated to 12 characters");
-            len = 12;
-        }
-        memset(signature, 0, 12);
-        memcpy(signature, cpu->hyperv_vendor_id, len);
-    }
     c->eax = hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ?
         HV_CPUID_NESTED_FEATURES : HV_CPUID_IMPLEMENT_LIMITS;
-    c->ebx = signature[0];
-    c->ecx = signature[1];
-    c->edx = signature[2];
+    c->ebx = cpu->hyperv_vendor_id[0];
+    c->ecx = cpu->hyperv_vendor_id[1];
+    c->edx = cpu->hyperv_vendor_id[2];
 
     c = &cpuid_ent[cpuid_i++];
     c->function = HV_CPUID_INTERFACE;