Re: [PATCH 1/5] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()

[Claudio Fontana]

On 11/19/20 11:32 AM, Vitaly Kuznetsov wrote:
> As a preparation to expanding Hyper-V CPU features early, move
> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
> itself.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>


Hi Vitaly,

If building for TCG-only, does the x86_cpu_hyperv_realize function and other 
hyperv related functions
make sense to keep in the build?

Now that we have per-accelerator subdirs in target/i386, should the hyperv code 
be moved over?

Thanks,

Claudio


> ---
>  target/i386/cpu.c | 23 ++++++++++++++++++++++-
>  target/i386/cpu.h |  3 ++-
>  target/i386/kvm.c | 25 ++++++++++---------------
>  3 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 5a8c96072e41..2a6885753378 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6509,6 +6509,25 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool 
> verbose)
>      }
>  }
>  
> +static void x86_cpu_hyperv_realize(X86CPU *cpu)
> +{
> +    size_t len;
> +
> +    /* Hyper-V vendor id */
> +    if (!cpu->hyperv_vendor) {
> +        memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);
> +    } else {
> +        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);
> +    }
> +}
> +
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -6680,6 +6699,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>          env->cache_info_amd.l3_cache = &legacy_l3_cache;
>      }
>  
> +    /* Process Hyper-V enlightenments */
> +    x86_cpu_hyperv_realize(cpu);
>  
>      cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
> @@ -7198,7 +7219,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 88e8586f8fb4..be640bf45c29 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1655,11 +1655,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 a2934dda027c..788a2cf2ec51 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1205,6 +1205,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;
> @@ -1279,23 +1286,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;
>