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

[Evgeny Yakovlev <eyakovlev@virtuozzo.com>]

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.