* [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
@ 2016-06-20 15:29 Denis V. Lunev
2016-06-21 12:00 ` Paolo Bonzini
2016-06-22 21:01 ` Eduardo Habkost
0 siblings, 2 replies; 6+ messages in thread
From: Denis V. Lunev @ 2016-06-20 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: den, Evgeny Yakovlev, Paolo Bonzini, Richard Henderson,
Eduardo Habkost, Marcelo Tosatti
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,
+};
+
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) {
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
2016-06-20 15:29 [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom Denis V. Lunev
@ 2016-06-21 12:00 ` Paolo Bonzini
2016-06-22 21:05 ` Eduardo Habkost
2016-06-22 21:01 ` Eduardo Habkost
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2016-06-21 12:00 UTC (permalink / raw)
To: Denis V. Lunev, qemu-devel
Cc: Evgeny Yakovlev, Richard Henderson, Eduardo Habkost,
Marcelo Tosatti
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
2016-06-21 12:00 ` Paolo Bonzini
@ 2016-06-22 21:05 ` Eduardo Habkost
0 siblings, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2016-06-22 21:05 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Denis V. Lunev, qemu-devel, Evgeny Yakovlev, Richard Henderson,
Marcelo Tosatti
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.
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
2016-06-20 15:29 [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom Denis V. Lunev
2016-06-21 12:00 ` Paolo Bonzini
@ 2016-06-22 21:01 ` Eduardo Habkost
2016-06-24 10:10 ` Evgeny Yakovlev
1 sibling, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2016-06-22 21:01 UTC (permalink / raw)
To: Denis V. Lunev
Cc: qemu-devel, Evgeny Yakovlev, Paolo Bonzini, Richard Henderson,
Marcelo Tosatti
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?
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
2016-06-22 21:01 ` Eduardo Habkost
@ 2016-06-24 10:10 ` Evgeny Yakovlev
2016-06-24 13:15 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: Evgeny Yakovlev @ 2016-06-24 10:10 UTC (permalink / raw)
To: Eduardo Habkost, Denis V. Lunev
Cc: qemu-devel, 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 <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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom
2016-06-24 10:10 ` Evgeny Yakovlev
@ 2016-06-24 13:15 ` Eduardo Habkost
0 siblings, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2016-06-24 13:15 UTC (permalink / raw)
To: Evgeny Yakovlev
Cc: Denis V. Lunev, qemu-devel, Paolo Bonzini, Richard Henderson,
Marcelo Tosatti
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!
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-24 13:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-20 15:29 [Qemu-devel] [PATCH v2 1/1] cpu: report hyperv feature words through qom Denis V. Lunev
2016-06-21 12:00 ` Paolo Bonzini
2016-06-22 21:05 ` Eduardo Habkost
2016-06-22 21:01 ` Eduardo Habkost
2016-06-24 10:10 ` Evgeny Yakovlev
2016-06-24 13:15 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).