| Message ID | 20200904145431.196885-23-vkuznets@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: KVM: expand Hyper-V features early | expand |
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? > > 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 > -- Eduardo
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). > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- [...] > +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); This makes a function that sounds like it doesn't have any side effect ("get supported cpuid") have an unintended side effect (hyperv_expand_features() will change all CPUID data inside the cpu object). What about making it more similar to kvm_arch_get_supported_cpuid(), and be just a wrapper to get_supported_hv_cpuid()? I would also make sure get_supported_hv_cpuid() doesn't get CPUState as argument, just to be sure it will never touch the CPU object state. > + > + 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 > -- Eduardo
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
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). >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- > [...] >> +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); > > This makes a function that sounds like it doesn't have any side > effect ("get supported cpuid") have an unintended side effect > (hyperv_expand_features() will change all CPUID data inside the > cpu object). > > What about making it more similar to > kvm_arch_get_supported_cpuid(), and be just a wrapper to > get_supported_hv_cpuid()? > > I would also make sure get_supported_hv_cpuid() doesn't get > CPUState as argument, just to be sure it will never touch the CPU > object state. Sure, I can try. The difference with get_supported_cpuid() is that get_supported_hv_cpuid() currently doesn't have cache and we don't probably want to call KVM_GET_SUPPORTED_HV_CPUID ioctl for every CPUID leaf. I don't see why it can't be introduced though. > >> + >> + 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
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
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). 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(-)