Re: [PATCH RFC 22/22] i386: expand Hyper-V features early

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Sep 04, 2020 at 04:54:31PM +0200, Vitaly Kuznetsov wrote:
>> To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we
>> need to expand and set the corresponding CPUID leaves early. Modify
>> x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V
>> specific kvm_hv_get_supported_cpuid() instead of
>> kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid()
>> as Hyper-V specific CPUID leaves intersect with KVM's.
>> 
>> Note, early expansion will only happen when KVM supports system wide
>> KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID).
>
> Will this implicitly enable hyperv CPUID passthrough when using
> "-cpu host"?  Do we want it to?
>

I hope it won't. With 'hv_*' features set explicitly we will get just
them (if the host supports), with 'hv_passthrough' we will get
everything supported by the host. With no 'hv_*' parameters on the
command line we should get no Hyper-V features. I'll check this.

Personally, I don't think '-host' mode should enable any Hyper-V
features as these features have side-effects. E.g. if you enable SynIC
you won't be able to use vAPIC. Probably not a reasonable default for
the majority of the guests which (hopefully) are Linux.

>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  target/i386/cpu.c      | 15 +++++++++------
>>  target/i386/kvm.c      | 15 +++++++++++++++
>>  target/i386/kvm_i386.h |  7 +++++++
>>  3 files changed, 31 insertions(+), 6 deletions(-)
>> 
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 479c4bbbf459..d3c4ecb3535c 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5147,7 +5147,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
>> **errp)
>>      return cpu_list;
>>  }
>>  
>> -static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
>> +static uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord 
>> w,
>>                                                     bool migratable_only)
>>  {
>>      FeatureWordInfo *wi = &feature_word_info[w];
>> @@ -5156,9 +5156,12 @@ static uint64_t 
>> x86_cpu_get_supported_feature_word(FeatureWord w,
>>      if (kvm_enabled()) {
>>          switch (wi->type) {
>>          case CPUID_FEATURE_WORD:
>> -            r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid.eax,
>> -                                                        wi->cpuid.ecx,
>> -                                                        wi->cpuid.reg);
>> +            if (hyperv_feature_word(w))
>> +                r = kvm_hv_get_supported_cpuid(cpu, w);
>> +            else
>> +                r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid.eax,
>> +                                                 wi->cpuid.ecx,
>> +                                                 wi->cpuid.reg);
>>              break;
>>          case MSR_FEATURE_WORD:
>>              r = kvm_arch_get_supported_msr_feature(kvm_state,
>> @@ -6485,7 +6488,7 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error 
>> **errp)
>>               * by the user.
>>               */
>>              env->features[w] |=
>> -                x86_cpu_get_supported_feature_word(w, cpu->migratable) &
>> +                x86_cpu_get_supported_feature_word(cpu, w, cpu->migratable) 
>> &
>>                  ~env->user_features[w] &
>>                  ~feature_word_info[w].no_autoenable_flags;
>>          }
>> @@ -6589,7 +6592,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool 
>> verbose)
>>  
>>      for (w = 0; w < FEATURE_WORDS; w++) {
>>          uint64_t host_feat =
>> -            x86_cpu_get_supported_feature_word(w, false);
>> +            x86_cpu_get_supported_feature_word(cpu, w, false);
>>          uint64_t requested_features = env->features[w];
>>          uint64_t unavailable_features = requested_features & ~host_feat;
>>          mark_unavailable_features(cpu, w, unavailable_features, prefix);
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 961241528a5c..764b96fbbb7d 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1449,6 +1449,21 @@ static int hyperv_fill_cpuids(CPUState *cs,
>>      return cpuid_i;
>>  }
>>  
>> +uint32_t kvm_hv_get_supported_cpuid(X86CPU *cpu, enum FeatureWord w)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    CPUX86State *env = &cpu->env;
>> +    Error *local_err = NULL;
>> +
>> +    hyperv_expand_features(cs, &local_err);
>> +
>> +    if (local_err) {
>> +            error_report_err(local_err);
>> +    }
>> +
>> +    return env->features[w];
>> +}
>> +
>>  static Error *hv_passthrough_mig_blocker;
>>  static Error *hv_no_nonarch_cs_mig_blocker;
>>  
>> diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
>> index 064b8798a26c..2e7da4f39668 100644
>> --- a/target/i386/kvm_i386.h
>> +++ b/target/i386/kvm_i386.h
>> @@ -48,4 +48,11 @@ bool kvm_has_waitpkg(void);
>>  
>>  bool kvm_hv_vpindex_settable(void);
>>  
>> +static inline bool hyperv_feature_word(enum FeatureWord w)
>> +{
>> +    return w >= FEAT_HYPERV_EAX && w <= FEAT_HV_NESTED_EDX;
>> +}
>> +
>> +uint32_t kvm_hv_get_supported_cpuid(X86CPU *cpu, enum FeatureWord w);
>> +
>>  #endif
>> -- 
>> 2.25.4
>> 

-- 
Vitaly