Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom

[Eduardo Habkost]

On Mon, Jun 20, 2016 at 06:29:40PM +0300, Denis V. Lunev wrote:
> From: Evgeny Yakovlev <address@hidden>
> 
> This change adds hyperv feature words report through qom rpc.
> 
> When VM is configured with hyperv features enabled
> libvirt will check that required feature words are set
> in cpuid leaf 40000003 through qom request.
> 
> Currently qemu does not report hyperv feature words
> which prevents windows guests from starting with libvirt.
> 
> Signed-off-by: Evgeny Yakovlev <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Richard Henderson <address@hidden>
> CC: Eduardo Habkost <address@hidden>
> CC: Marcelo Tosatti <address@hidden>
> ---
> Changes from v1:
> - renamed hyperv features so they don't conflict with hyperv properties
> - refactored kvm_arch_init_vcpu to process hyperv props into feature words
> 
>  target-i386/cpu.c |  50 ++++++++++++++++++++++++
>  target-i386/cpu.h |   3 ++
>  target-i386/kvm.c | 114 
> +++++++++++++++++++++++++++++++-----------------------
>  3 files changed, 119 insertions(+), 48 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3665fec..c79b4e3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -245,6 +245,44 @@ static const char *kvm_feature_name[] = {
>      NULL, NULL, NULL, NULL,
>  };
>  
> +static const char *hyperv_priv_feature_name[] = {
> +    "hv_msr_vp_runtime_access", "hv_msr_time_refcount_access",
> +    "hv_msr_synic_access", "hv_msr_stimer_access",
> +    "hv_msr_apic_access", "hv_msr_hypercall_access",
> +    "hv_vpindex_access", "hv_msr_reset_access",
> +    "hv_msr_stats_access", "hv_reftsc_access",
> +    "hv_msr_idle_access", "hv_msr_frequency_access",
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
> +static const char *hyperv_ident_feature_name[] = {
> +    "hv_create_partitions", "hv_access_partition_id",
> +    "hv_access_memory_pool", "hv_adjust_message_buffers",
> +    "hv_post_messages", "hv_signal_events",
> +    "hv_create_port", "hv_connect_port",
> +    "hv_access_stats", NULL, NULL, "hv_debugging",
> +    "hv_cpu_power_management", "hv_configure_profiler", NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};
> +
> +static const char *hyperv_misc_feature_name[] = {
> +    "hv_mwait", "hv_guest_debugging", "hv_perf_monitor", 
> "hv_cpu_dynamic_part",
> +    "hv_hypercall_params_xmm", "hv_guest_idle_state", NULL, NULL,
> +    NULL, NULL, "hv_guest_crash_msr", NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +    NULL, NULL, NULL, NULL,
> +};

Adding these feature bit names will let individual bits to be
configured from the command-line, not just reported using QOM.

I am not sure this is intentional, as now we have conflicting
ways to configure some hyperv features. For example: now
HV_X64_MSR_VP_RUNTIME_AVAILABLE can be set using "+hv-runtime" or
"+hv_msr_vp_runtime_access". And the difference between both
methods is not clear for users.

Also, as kvm_arch_get_supported_cpuid() won't return anything
about those feature flags, QEMU will get confused if you try to
use "+hv_msr_vp_runtime_access" in the command-line.

I believe this can be addressed by doing the work in three steps:

1) Add hyperv CPUID leaves to FeatureWord/feature_word_info
   without any name arrays, make the changes you made below to
   change env->features inside kvm_arch_init_vcpu().
   * In other words: this patch, but without the feature_name
     arrays.
   * This wil make QEMU report all the hyperv feature bits in the
     "feature-words" QOM property (read-only)
   * This won't change any command-line interface.
   * This shouldn't confuse QEMU due to
     lack of kvm_arch_get_supported_cpuid() support, because
     env->features is being set up after
     x86_cpu_filter_features() was already called.

If all you want is to report low-level CPUID data through QMP,
step (1) is enough: it will already include the low-level hyperv
CPUID data in the "feature-words" property.

2) Replace the hyperv_* boolean fields with env->feature bits.
   * See below how this could be done for each specific case.
   * This requires making kvm_arch_get_supported_cpuid() report
     them (after making the appropriate capability checks).
   * This makes the check/enforce flags support hyperv
     capabilities.

3) (optional) Add the remaining feature names (that are not
   configurable yet) to the feature_names arrays.
   * This won't let them be configured in the command-line by
     now, if kvm_arch_get_supported_cpuid() doesn't report them
     as supported.
   * I am not sure we really want that. What would be the point
     of adding feature names that we don't even support yet?

> +
>  static const char *svm_feature_name[] = {
>      "npt", "lbrv", "svm_lock", "nrip_save",
>      "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
[...]
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ff92b1d..5a3f14d 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -574,6 +574,66 @@ static int kvm_arch_set_tsc_khz(CPUState *cs)
>      return 0;
>  }
>  
> +static int hyperv_handle_properties(CPUState *cs)
> +{
> +    X86CPU *cpu = X86_CPU(cs);
> +    CPUX86State *env = &cpu->env;
> +
> +    if (cpu->hyperv_relaxed_timing) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
> +    }

HYPERV_CPUID_ENLIGHTMENT_INFO is not in FeatureWordInfo yet, so
this looks OK by now.

> +    if (cpu->hyperv_vapic) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_APIC_ACCESS_AVAILABLE;
> +        has_msr_hv_vapic = true;
> +    }

Now we can control HV_X64_MSR_APIC_ACCESS_AVAILABLE using
"+hv-vapic" and "+hv_msr_apic_access", and the difference between
both is unclear.

I suggest the following:

1) Remove the "hv-vapic" static property from
   x86_cpu_properties, and the hyperv_vapic field
2) Change "hv_msr_apic_access" to "hv-vapic"
   in hyperv_priv_feature_name.
2) Replace code using cpu->hyperv_vapic with
   (env->features[FEAT_HYPERV_EAX] & HV_X64_MSR_APIC_ACCESS_AVAILABLE)
3) Change the setup code to:

    if (env->features[FEAT_HYPERV_EAX] & HV_X64_MSR_APIC_ACCESS_AVAILABLE) {
        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
        has_msr_hv_vapic = true;
    }

> +    if (cpu->hyperv_time &&
> +            kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV_TIME) > 0) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
> +        env->features[FEAT_HYPERV_EAX] |= 
> HV_X64_MSR_TIME_REF_COUNT_AVAILABLE;
> +        env->features[FEAT_HYPERV_EAX] |= 0x200;
> +        has_msr_hv_tsc = true;
> +    }

This is similar to hyperv_vapic, but with the
kvm_check_extension() check that needs to be moved to
kvm_arch_get_supported_cpuid().

I suggest:

1) Remove the "hv-time" static property from
   x86_cpu_properties, and the hyperv_time field
2) Change "hv_msr_time_refcount_access" to "hv-time"
   in hyperv_priv_feature_name.
2) Replace code using cpu->hyperv_time field with
   (env->features[FEAT_HYPERV_EAX] & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE)
3) Change the setup code to:

    if (env->features[FEAT_HYPERV_EAX] & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE) {
        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_HYPERCALL_AVAILABLE;
        /*TODO: replace magic number with macro */
        env->features[FEAT_HYPERV_EAX] |= 0x200;
        has_msr_hv_tsc = true;
    }

4) Check KVM_CAP_HYPERV_TIME inside
   kvm_arch_get_supported_cpuid()

This will add support for the check/enforce flags for hv-time
automatically.


> +    if (cpu->hyperv_crash && has_msr_hv_crash) {
> +        env->features[FEAT_HYPERV_EDX] |= HV_X64_GUEST_CRASH_MSR_AVAILABLE;
> +    }

This is similar to hv-time, if the has_msr_hv_crash check is
moved to kvm_arch_get_supported_cpuid().

> +    if (cpu->hyperv_reset && has_msr_hv_reset) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_RESET_AVAILABLE;
> +    }

This is similar to hv-time/hv-crash above.

> +    if (cpu->hyperv_vpindex && has_msr_hv_vpindex) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_INDEX_AVAILABLE;
> +    }

This is similar to hv-time/hv-crash above.

> +    if (cpu->hyperv_runtime && has_msr_hv_runtime) {
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_RUNTIME_AVAILABLE;
> +    }

Similar to hv-time/hv-crash.

> +    if (cpu->hyperv_synic) {
> +        int sint;
> +
> +        if (!has_msr_hv_synic ||
> +            kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
> +            fprintf(stderr, "Hyper-V SynIC is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
> +
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_SYNIC_AVAILABLE;
> +        env->msr_hv_synic_version = HV_SYNIC_VERSION_1;
> +        for (sint = 0; sint < ARRAY_SIZE(env->msr_hv_synic_sint); sint++) {
> +            env->msr_hv_synic_sint[sint] = HV_SYNIC_SINT_MASKED;
> +        }
> +    }

This is a bit more complex, but the general idea is the same: add
"hv-synic" to the feature name array, replace cpu->hyperv_synic
with (env->features[FEAT_HYPERV_EAX] & HV_X64_MSR_SYNIC_AVAILABLE),
move the capability check to kvm_arch_get_supported_cpuid(), keep
the remaining setup code (for msr_hv_synic_*) here.

> +    if (cpu->hyperv_stimer) {
> +        if (!has_msr_hv_stimer) {
> +            fprintf(stderr, "Hyper-V timers aren't supported by kernel\n");
> +            return -ENOSYS;
> +        }
> +        env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_SYNTIMER_AVAILABLE;
> +    }

Similar to hv-time/hv-crash.

Interestingly, the last two features are the only ones that don't
get silently disabled by the setup code if unsupported by KVM.
Does anybody know why?

> +    if (MACHINE_GET_CLASS(current_machine)->hot_add_cpu != NULL) {
> +        env->features[FEAT_HYPERV_EDX] |= 
> HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
> +    }

This probably can be left as-is.

> +    return 0;
> +}
> +
>  static Error *invtsc_mig_blocker;
>  
>  #define KVM_MAX_CPUID_ENTRIES  100
> @@ -633,56 +693,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
[...]
>          c = &cpuid_data.entries[cpuid_i++];
>          c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
>          if (cpu->hyperv_relaxed_timing) {

Do you plan to add HYPERV_CPUID_ENLIGHTMENT_INFO to
FeatureWord/feature_word_info later?

-- 
Eduardo