Re: [Qemu-devel] [PATCH 1/1] cpu: report hyperv feature words through qom

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 01:28:40PM +0300, Denis V. Lunev wrote:
> From: Evgeny Yakovlev <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Richard Henderson <address@hidden>
> CC: Eduardo Habkost <address@hidden>
> CC: Marcelo Tosatti <address@hidden>

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