| Message ID | 20210422161130.652779-6-vkuznets@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: KVM: expand Hyper-V features early | expand |
On Thu, Apr 22, 2021 at 06:11:16PM +0200, Vitaly Kuznetsov wrote: > Clean up hv_cpuid_check_and_set() by separating hyperv_feature_supported() > off it. No functional change intended. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > target/i386/kvm/kvm.c | 49 ++++++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 19 deletions(-) > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index f791baa29acf..ba093dba4d23 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1107,13 +1107,33 @@ static int hv_cpuid_get_fw(struct kvm_cpuid2 *cpuid, int fw, uint32_t *r) > return 0; > } > > +static bool hyperv_feature_supported(struct kvm_cpuid2 *cpuid, int feature) > +{ > + uint32_t r, fw, bits; > + int i; > + > + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { > + fw = kvm_hyperv_properties[feature].flags[i].fw; > + bits = kvm_hyperv_properties[feature].flags[i].bits; > + > + if (!fw) { > + continue; > + } > + > + if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { > + return false; > + } > + } > + > + return true; > +} > + > static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > int feature) > { > X86CPU *cpu = X86_CPU(cs); > - uint32_t r, fw, bits; > uint64_t deps; > - int i, dep_feat; > + int dep_feat; > > if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) { > return 0; > @@ -1132,23 +1152,14 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, > deps &= ~(1ull << dep_feat); > } > > - for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { > - fw = kvm_hyperv_properties[feature].flags[i].fw; > - bits = kvm_hyperv_properties[feature].flags[i].bits; > - > - if (!fw) { > - continue; > - } > - > - if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { > - if (hyperv_feat_enabled(cpu, feature)) { > - fprintf(stderr, > - "Hyper-V %s is not supported by kernel\n", > - kvm_hyperv_properties[feature].desc); > - return 1; > - } else { > - return 0; > - } > + if (!hyperv_feature_supported(cpuid, feature)) { > + if (hyperv_feat_enabled(cpu, feature)) { > + fprintf(stderr, > + "Hyper-V %s is not supported by kernel\n", > + kvm_hyperv_properties[feature].desc); > + return 1; > + } else { > + return 0; The reason for returning prematurely here when !hyperv_feat_enabled() is not clear to me, but you are keeping the existing behavior, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > } > } > > -- > 2.30.2 >
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Apr 22, 2021 at 06:11:16PM +0200, Vitaly Kuznetsov wrote: >> Clean up hv_cpuid_check_and_set() by separating hyperv_feature_supported() >> off it. No functional change intended. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> target/i386/kvm/kvm.c | 49 ++++++++++++++++++++++++++----------------- >> 1 file changed, 30 insertions(+), 19 deletions(-) >> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c >> index f791baa29acf..ba093dba4d23 100644 >> --- a/target/i386/kvm/kvm.c >> +++ b/target/i386/kvm/kvm.c >> @@ -1107,13 +1107,33 @@ static int hv_cpuid_get_fw(struct kvm_cpuid2 *cpuid, int fw, uint32_t *r) >> return 0; >> } >> >> +static bool hyperv_feature_supported(struct kvm_cpuid2 *cpuid, int feature) >> +{ >> + uint32_t r, fw, bits; >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { >> + fw = kvm_hyperv_properties[feature].flags[i].fw; >> + bits = kvm_hyperv_properties[feature].flags[i].bits; >> + >> + if (!fw) { >> + continue; >> + } >> + >> + if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { >> + return false; >> + } >> + } >> + >> + return true; >> +} >> + >> static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, >> int feature) >> { >> X86CPU *cpu = X86_CPU(cs); >> - uint32_t r, fw, bits; >> uint64_t deps; >> - int i, dep_feat; >> + int dep_feat; >> >> if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) { >> return 0; >> @@ -1132,23 +1152,14 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, >> deps &= ~(1ull << dep_feat); >> } >> >> - for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { >> - fw = kvm_hyperv_properties[feature].flags[i].fw; >> - bits = kvm_hyperv_properties[feature].flags[i].bits; >> - >> - if (!fw) { >> - continue; >> - } >> - >> - if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { >> - if (hyperv_feat_enabled(cpu, feature)) { >> - fprintf(stderr, >> - "Hyper-V %s is not supported by kernel\n", >> - kvm_hyperv_properties[feature].desc); >> - return 1; >> - } else { >> - return 0; >> - } >> + if (!hyperv_feature_supported(cpuid, feature)) { >> + if (hyperv_feat_enabled(cpu, feature)) { >> + fprintf(stderr, >> + "Hyper-V %s is not supported by kernel\n", >> + kvm_hyperv_properties[feature].desc); >> + return 1; >> + } else { >> + return 0; > > The reason for returning prematurely here when > !hyperv_feat_enabled() is not clear to me This hopefully gets much better at the end of the series where hv_cpuid_check_and_set() is split into 'check' and 'set'. The reason to return immediately is: if the feature was not requested explicitly and we're not in 'hv-passthrough' mode, there's no need to check whether KVM supports it, if we have all the dependencies,... > but you are keeping the existing behavior, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Thanks!
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index f791baa29acf..ba093dba4d23 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1107,13 +1107,33 @@ static int hv_cpuid_get_fw(struct kvm_cpuid2 *cpuid, int fw, uint32_t *r) return 0; } +static bool hyperv_feature_supported(struct kvm_cpuid2 *cpuid, int feature) +{ + uint32_t r, fw, bits; + int i; + + for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { + fw = kvm_hyperv_properties[feature].flags[i].fw; + bits = kvm_hyperv_properties[feature].flags[i].bits; + + if (!fw) { + continue; + } + + if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { + return false; + } + } + + return true; +} + static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, int feature) { X86CPU *cpu = X86_CPU(cs); - uint32_t r, fw, bits; uint64_t deps; - int i, dep_feat; + int dep_feat; if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) { return 0; @@ -1132,23 +1152,14 @@ static int hv_cpuid_check_and_set(CPUState *cs, struct kvm_cpuid2 *cpuid, deps &= ~(1ull << dep_feat); } - for (i = 0; i < ARRAY_SIZE(kvm_hyperv_properties[feature].flags); i++) { - fw = kvm_hyperv_properties[feature].flags[i].fw; - bits = kvm_hyperv_properties[feature].flags[i].bits; - - if (!fw) { - continue; - } - - if (hv_cpuid_get_fw(cpuid, fw, &r) || (r & bits) != bits) { - if (hyperv_feat_enabled(cpu, feature)) { - fprintf(stderr, - "Hyper-V %s is not supported by kernel\n", - kvm_hyperv_properties[feature].desc); - return 1; - } else { - return 0; - } + if (!hyperv_feature_supported(cpuid, feature)) { + if (hyperv_feat_enabled(cpu, feature)) { + fprintf(stderr, + "Hyper-V %s is not supported by kernel\n", + kvm_hyperv_properties[feature].desc); + return 1; + } else { + return 0; } }
Clean up hv_cpuid_check_and_set() by separating hyperv_feature_supported() off it. No functional change intended. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- target/i386/kvm/kvm.c | 49 ++++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 19 deletions(-)