| Message ID | 1466436580-1309-1-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/06/2016 17:29, Denis V. Lunev wrote: > +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; > + } > + 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; > + } > + 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; > + } > + if (cpu->hyperv_crash && has_msr_hv_crash) { > + env->features[FEAT_HYPERV_EDX] |= HV_X64_GUEST_CRASH_MSR_AVAILABLE; > + } > + if (cpu->hyperv_reset && has_msr_hv_reset) { > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_RESET_AVAILABLE; > + } > + if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_INDEX_AVAILABLE; > + } > + if (cpu->hyperv_runtime && has_msr_hv_runtime) { > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_RUNTIME_AVAILABLE; Yay, thanks for this! I'm curious if there's anything more you need to support "-cpu ...,hv_time,enforce", and have it fail if KVM_CAP_HYPERV_TIME is not available. Thanks, Paolo
On Mon, Jun 20, 2016 at 06:29:40PM +0300, Denis V. Lunev wrote: > From: Evgeny Yakovlev <eyakovlev@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 <eyakovlev@virtuozzo.com> > Signed-off-by: Denis V. Lunev <den@openvz.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Richard Henderson <rth@twiddle.net> > CC: Eduardo Habkost <ehabkost@redhat.com> > CC: Marcelo Tosatti <mtosatti@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?
On Tue, Jun 21, 2016 at 02:00:22PM +0200, Paolo Bonzini wrote: > On 20/06/2016 17:29, Denis V. Lunev wrote: > > +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; > > + } > > + 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; > > + } > > + 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; > > + } > > + if (cpu->hyperv_crash && has_msr_hv_crash) { > > + env->features[FEAT_HYPERV_EDX] |= HV_X64_GUEST_CRASH_MSR_AVAILABLE; > > + } > > + if (cpu->hyperv_reset && has_msr_hv_reset) { > > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_RESET_AVAILABLE; > > + } > > + if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { > > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_INDEX_AVAILABLE; > > + } > > + if (cpu->hyperv_runtime && has_msr_hv_runtime) { > > + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_RUNTIME_AVAILABLE; > > Yay, thanks for this! > > I'm curious if there's anything more you need to support "-cpu > ...,hv_time,enforce", and have it fail if KVM_CAP_HYPERV_TIME is not > available. To do that, we need to: 1) Replace the cpu->hyperv_XXX boolean with (env->features[...] & HV_XXX) 2) Move the "hv-XXX" property from x86_cpu_properties to the corresponding feature_name array 3) Report the features in kvm_arch_get_supported_cpuid(), with the corresponding kvm_check_extension() or has_msr_XXX checks.
On 23.06.2016 00:01, Eduardo Habkost wrote: > On Mon, Jun 20, 2016 at 06:29:40PM +0300, Denis V. Lunev wrote: >> From: Evgeny Yakovlev <eyakovlev@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 <eyakovlev@virtuozzo.com> >> Signed-off-by: Denis V. Lunev <den@openvz.org> >> CC: Paolo Bonzini <pbonzini@redhat.com> >> CC: Richard Henderson <rth@twiddle.net> >> CC: Eduardo Habkost <ehabkost@redhat.com> >> CC: Marcelo Tosatti <mtosatti@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? > Thanks for all the comments! Removing hyperv_* booleans sounds like the proper way forward however it will break compatibility with how libvirt enables hyperv enlightenments. I think we will now concentrate on the QOM feature-words and later get back to reworking hv_* properties. You suggestions will be very helpful for that.
On Fri, Jun 24, 2016 at 01:10:45PM +0300, Evgeny Yakovlev wrote: [...] > Thanks for all the comments! > Removing hyperv_* booleans sounds like the proper way forward however it > will break compatibility with how libvirt enables hyperv enlightenments. It won't break any compatibility, if the property name in the feature_names arrays is the same as the previous boolean property name. The only difference is internal to QEMU: now the property will set a bit inside env->features instead of a boolean field in X86CPU. > I think we will now concentrate on the QOM feature-words and later get back > to reworking hv_* properties. You suggestions will be very helpful for that. OK!
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, +}; + static const char *svm_feature_name[] = { "npt", "lbrv", "svm_lock", "nrip_save", "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists", @@ -411,6 +449,18 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX, .tcg_features = TCG_KVM_FEATURES, }, + [FEAT_HYPERV_EAX] = { + .feat_names = hyperv_priv_feature_name, + .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX, + }, + [FEAT_HYPERV_EBX] = { + .feat_names = hyperv_ident_feature_name, + .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX, + }, + [FEAT_HYPERV_EDX] = { + .feat_names = hyperv_misc_feature_name, + .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX, + }, [FEAT_SVM] = { .feat_names = svm_feature_name, .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX, diff --git a/target-i386/cpu.h b/target-i386/cpu.h index d9ab884..4496c8b 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -440,6 +440,9 @@ typedef enum FeatureWord { FEAT_8000_0007_EDX, /* CPUID[8000_0007].EDX */ FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */ FEAT_KVM, /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */ + FEAT_HYPERV_EAX, /* CPUID[4000_0003].EAX */ + FEAT_HYPERV_EBX, /* CPUID[4000_0003].EBX */ + FEAT_HYPERV_EDX, /* CPUID[4000_0003].EDX */ FEAT_SVM, /* CPUID[8000_000A].EDX */ FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ FEAT_6_EAX, /* CPUID[6].EAX */ 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; + } + 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; + } + 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; + } + if (cpu->hyperv_crash && has_msr_hv_crash) { + env->features[FEAT_HYPERV_EDX] |= HV_X64_GUEST_CRASH_MSR_AVAILABLE; + } + if (cpu->hyperv_reset && has_msr_hv_reset) { + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_RESET_AVAILABLE; + } + if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_INDEX_AVAILABLE; + } + if (cpu->hyperv_runtime && has_msr_hv_runtime) { + env->features[FEAT_HYPERV_EAX] |= HV_X64_MSR_VP_RUNTIME_AVAILABLE; + } + 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; + } + } + 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; + } + if (MACHINE_GET_CLASS(current_machine)->hot_add_cpu != NULL) { + env->features[FEAT_HYPERV_EDX] |= HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE; + } + 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_FEATURES; - if (cpu->hyperv_relaxed_timing) { - c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; - } - if (cpu->hyperv_vapic) { - c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; - c->eax |= HV_X64_MSR_APIC_ACCESS_AVAILABLE; - has_msr_hv_vapic = true; - } - if (cpu->hyperv_time && - kvm_check_extension(cs->kvm_state, KVM_CAP_HYPERV_TIME) > 0) { - c->eax |= HV_X64_MSR_HYPERCALL_AVAILABLE; - c->eax |= HV_X64_MSR_TIME_REF_COUNT_AVAILABLE; - c->eax |= 0x200; - has_msr_hv_tsc = true; - } - if (cpu->hyperv_crash && has_msr_hv_crash) { - c->edx |= HV_X64_GUEST_CRASH_MSR_AVAILABLE; - } - c->edx |= HV_X64_CPU_DYNAMIC_PARTITIONING_AVAILABLE; - if (cpu->hyperv_reset && has_msr_hv_reset) { - c->eax |= HV_X64_MSR_RESET_AVAILABLE; - } - if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { - c->eax |= HV_X64_MSR_VP_INDEX_AVAILABLE; - } - if (cpu->hyperv_runtime && has_msr_hv_runtime) { - c->eax |= HV_X64_MSR_VP_RUNTIME_AVAILABLE; + r = hyperv_handle_properties(cs); + if (r) { + return r; } - 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; - } + c->eax = env->features[FEAT_HYPERV_EAX]; + c->ebx = env->features[FEAT_HYPERV_EBX]; + c->edx = env->features[FEAT_HYPERV_EDX]; - c->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; - } - } - if (cpu->hyperv_stimer) { - if (!has_msr_hv_stimer) { - fprintf(stderr, "Hyper-V timers aren't supported by kernel\n"); - return -ENOSYS; - } - c->eax |= HV_X64_MSR_SYNTIMER_AVAILABLE; - } c = &cpuid_data.entries[cpuid_i++]; c->function = HYPERV_CPUID_ENLIGHTMENT_INFO; if (cpu->hyperv_relaxed_timing) {