On Mon, Jun 20, 2016 at 06:29:40PM +0300, Denis V. Lunev wrote:
> From: Evgeny Yakovlev <eyakov...@virtuozzo.com>
>
> 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 <eyakov...@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <d...@openvz.org>
> CC: Paolo Bonzini <pbonz...@redhat.com>
> CC: Richard Henderson <r...@twiddle.net>
> CC: Eduardo Habkost <ehabk...@redhat.com>
> CC: Marcelo Tosatti <mtosa...@redhat.com>
> ---
> 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
