On Tue, Jun 14, 2016 at 01:28: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 featured 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>
Which QEMU version did you use to test this? Some of those properties already
exist. See:
static Property x86_cpu_properties[] = {
[...]
{ .name = "hv-spinlocks", .info = &qdev_prop_spinlocks },
DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
DEFINE_PROP_BOOL("hv-crash", X86CPU, hyperv_crash, false),
DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false),
DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false),
DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
[...]
DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
DEFINE_PROP_END_OF_LIST()
};
QEMU will crash if you try to register the properties twice:
$ ./x86_64-softmmu/qemu-system-x86_64
qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:3094:
x86_cpu_register_bit_prop: Assertion `fp->ptr == field' failed.
Aborted (core dumped)
I like the idea of moving hyperv feature information inside the features array,
though.
See additional comments below:
> ---
> target-i386/cpu.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> target-i386/cpu.h | 3 +++
> target-i386/kvm.c | 3 +++
> 3 files changed, 53 insertions(+)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 895a386..ea01f06 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -244,6 +244,41 @@ static const char *kvm_feature_name[] = {
> NULL, NULL, NULL, NULL,
> };
>
> +static const char *hyperv_priv_feature_name[] = {
> + "hv_runtime", "hv_refcount", "hv_synic", "hv_stimer",
> + "hv_apic", "hv_hypercall", "hv_vpindex", "hv_reset",
> + "hv_stats", "hv_reftsc", "hv_idle", "hv_frequency",
I sent a patch some time ago to change all feature names to use "-" instead of
"_". But this can be fixed when applying the patch, if necessary.
> + 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",
> @@ -410,6 +445,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 0426459..d3b7ad4 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 abf50e6..f4ca8c4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -679,6 +679,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
> }
> c->eax |= HV_X64_MSR_SYNTIMER_AVAILABLE;
> }
> + env->features[FEAT_HYPERV_EAX] = c->eax;
> + env->features[FEAT_HYPERV_EBX] = c->ebx;
> + env->features[FEAT_HYPERV_EDX] = c->edx;
We normally store the configured features inside env->features
(using the properties), and then CPUID data is filled using
env->features. Not the other way around.
In other words, the new env->features[FEAT_HYPERV_*] bits can simply replace
the existing X86CPU::hyperv_* booleans.
--
Eduardo
