From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34725) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGO4j-0004xf-81 for qemu-devel@nongnu.org; Fri, 24 Jun 2016 06:11:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGO4e-00085k-Rj for qemu-devel@nongnu.org; Fri, 24 Jun 2016 06:11:00 -0400 Received: from mail-db3on0108.outbound.protection.outlook.com ([157.55.234.108]:47393 helo=emea01-db3-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGO4d-000846-RW for qemu-devel@nongnu.org; Fri, 24 Jun 2016 06:10:56 -0400 References: <1466436580-1309-1-git-send-email-den@openvz.org> <20160622210133.GP2048@thinpad.lan.raisama.net> From: Evgeny Yakovlev Message-ID: <576D0725.7060003@virtuozzo.com> Date: Fri, 24 Jun 2016 13:10:45 +0300 MIME-Version: 1.0 In-Reply-To: <20160622210133.GP2048@thinpad.lan.raisama.net> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Paolo Bonzini , Richard Henderson , Marcelo Tosatti 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 >> >> 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 >> Signed-off-by: Denis V. Lunev >> CC: Paolo Bonzini >> CC: Richard Henderson >> CC: Eduardo Habkost >> CC: Marcelo Tosatti >> --- >> 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.