hv_cpuid_check_and_set() does too much:
- Checks if the feature is supported by KVM;
- Checks if all dependencies are enabled;
- Sets the feature bit in cpu->hyperv_features for 'passthrough' mode.
To reduce the complexity, move all the logic except for dependencies
check out of it. Also, in 'passthrough' mode we don't really need to
check dependencies because KVM is supposed to provide a consistent
set anyway.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
target/i386/kvm/kvm.c | 105 +++++++++++++++---------------------------
1 file changed, 36 insertions(+), 69 deletions(-)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index d5551c4ab5cf..2c1a77f9b00f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1144,16 +1144,12 @@ static bool hyperv_feature_supported(CPUState *cs, int feature)
return true;
}
-static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
+/* Checks that all feature dependencies are enabled */
+static void hv_feature_check_deps(X86CPU *cpu, int feature, Error **errp)
{
- X86CPU *cpu = X86_CPU(cs);
uint64_t deps;
int dep_feat;
- if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
- return 0;
- }
-
deps = kvm_hyperv_properties[feature].dependencies;
while (deps) {
dep_feat = ctz64(deps);
@@ -1161,26 +1157,10 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
error_setg(errp, "Hyper-V %s requires Hyper-V %s",
kvm_hyperv_properties[feature].desc,
kvm_hyperv_properties[dep_feat].desc);
- return 1;
+ return;
}
deps &= ~(1ull << dep_feat);
}
-
- if (!hyperv_feature_supported(cs, feature)) {
- if (hyperv_feat_enabled(cpu, feature)) {
- error_setg(errp, "Hyper-V %s is not supported by kernel",
- kvm_hyperv_properties[feature].desc);
- return 1;
- } else {
- return 0;
- }
- }
-
- if (cpu->hyperv_passthrough) {
- cpu->hyperv_features |= BIT(feature);
- }
-
- return 0;
}
static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
@@ -1219,6 +1199,8 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
{
CPUState *cs = CPU(cpu);
+ Error *local_err = NULL;
+ int feat;
if (!hyperv_enabled(cpu))
return;
@@ -1274,53 +1256,38 @@ void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
cpu->hyperv_spinlock_attempts =
hv_cpuid_get_host(cs, HV_CPUID_ENLIGHTMENT_INFO, R_EBX);
- }
- /* Features */
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
- return;
- }
- if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
- return;
+ /*
+ * Mark feature as enabled in 'cpu->hyperv_features' as
+ * hv_build_cpuid_leaf() uses this info to build guest CPUIDs.
+ */
+ for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
+ if (hyperv_feature_supported(cs, feat)) {
+ cpu->hyperv_features |= BIT(feat);
+ }
+ }
+ } else {
+ /* Check features availability and dependencies */
+ for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
+ /* If the feature was not requested skip it. */
+ if (!hyperv_feat_enabled(cpu, feat)) {
+ continue;
+ }
+
+ /* Check if the feature is supported by KVM */
+ if (!hyperv_feature_supported(cs, feat)) {
+ error_setg(errp, "Hyper-V %s is not supported by kernel",
+ kvm_hyperv_properties[feat].desc);
+ return;
+ }
+
+ /* Check dependencies */
+ hv_feature_check_deps(cpu, feat, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ return;
+ }
+ }
}
/* Additional dependencies not covered by kvm_hyperv_properties[] */
--
2.30.2
On Thu, Apr 22, 2021 at 06:11:27PM +0200, Vitaly Kuznetsov wrote:
> hv_cpuid_check_and_set() does too much:
> - Checks if the feature is supported by KVM;
> - Checks if all dependencies are enabled;
> - Sets the feature bit in cpu->hyperv_features for 'passthrough' mode.
>
> To reduce the complexity, move all the logic except for dependencies
> check out of it. Also, in 'passthrough' mode we don't really need to
> check dependencies because KVM is supposed to provide a consistent
> set anyway.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> target/i386/kvm/kvm.c | 105 +++++++++++++++---------------------------
> 1 file changed, 36 insertions(+), 69 deletions(-)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index d5551c4ab5cf..2c1a77f9b00f 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -1144,16 +1144,12 @@ static bool hyperv_feature_supported(CPUState *cs, int feature)
> return true;
> }
>
> -static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
> +/* Checks that all feature dependencies are enabled */
> +static void hv_feature_check_deps(X86CPU *cpu, int feature, Error **errp)
Same suggestion as in patch 11/19: also returning a value to
indicate error is preferred. I would return a boolean.
(I don't think this alone should block the series, though)
> {
> - X86CPU *cpu = X86_CPU(cs);
> uint64_t deps;
> int dep_feat;
>
> - if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
> - return 0;
> - }
> -
> deps = kvm_hyperv_properties[feature].dependencies;
> while (deps) {
> dep_feat = ctz64(deps);
> @@ -1161,26 +1157,10 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
> error_setg(errp, "Hyper-V %s requires Hyper-V %s",
> kvm_hyperv_properties[feature].desc,
> kvm_hyperv_properties[dep_feat].desc);
> - return 1;
> + return;
> }
> deps &= ~(1ull << dep_feat);
> }
> -
> - if (!hyperv_feature_supported(cs, feature)) {
> - if (hyperv_feat_enabled(cpu, feature)) {
> - error_setg(errp, "Hyper-V %s is not supported by kernel",
> - kvm_hyperv_properties[feature].desc);
> - return 1;
> - } else {
> - return 0;
> - }
> - }
> -
> - if (cpu->hyperv_passthrough) {
> - cpu->hyperv_features |= BIT(feature);
> - }
> -
> - return 0;
> }
>
> static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
> @@ -1219,6 +1199,8 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
> void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
> {
> CPUState *cs = CPU(cpu);
> + Error *local_err = NULL;
> + int feat;
>
> if (!hyperv_enabled(cpu))
> return;
> @@ -1274,53 +1256,38 @@ void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>
> cpu->hyperv_spinlock_attempts =
> hv_cpuid_get_host(cs, HV_CPUID_ENLIGHTMENT_INFO, R_EBX);
> - }
>
> - /* Features */
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
> - return;
> - }
> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
> - return;
> + /*
> + * Mark feature as enabled in 'cpu->hyperv_features' as
> + * hv_build_cpuid_leaf() uses this info to build guest CPUIDs.
> + */
> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
> + if (hyperv_feature_supported(cs, feat)) {
> + cpu->hyperv_features |= BIT(feat);
> + }
> + }
> + } else {
> + /* Check features availability and dependencies */
> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
> + /* If the feature was not requested skip it. */
> + if (!hyperv_feat_enabled(cpu, feat)) {
> + continue;
> + }
That's the loop I suggested in patch 11/19. Nice. :)
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> +
> + /* Check if the feature is supported by KVM */
> + if (!hyperv_feature_supported(cs, feat)) {
> + error_setg(errp, "Hyper-V %s is not supported by kernel",
> + kvm_hyperv_properties[feat].desc);
> + return;
> + }
> +
> + /* Check dependencies */
> + hv_feature_check_deps(cpu, feat, &local_err);
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> + }
> + }
> }
>
> /* Additional dependencies not covered by kvm_hyperv_properties[] */
> --
> 2.30.2
>
--
Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Thu, Apr 22, 2021 at 06:11:27PM +0200, Vitaly Kuznetsov wrote:
>> hv_cpuid_check_and_set() does too much:
>> - Checks if the feature is supported by KVM;
>> - Checks if all dependencies are enabled;
>> - Sets the feature bit in cpu->hyperv_features for 'passthrough' mode.
>>
>> To reduce the complexity, move all the logic except for dependencies
>> check out of it. Also, in 'passthrough' mode we don't really need to
>> check dependencies because KVM is supposed to provide a consistent
>> set anyway.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> target/i386/kvm/kvm.c | 105 +++++++++++++++---------------------------
>> 1 file changed, 36 insertions(+), 69 deletions(-)
>>
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index d5551c4ab5cf..2c1a77f9b00f 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1144,16 +1144,12 @@ static bool hyperv_feature_supported(CPUState *cs, int feature)
>> return true;
>> }
>>
>> -static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
>> +/* Checks that all feature dependencies are enabled */
>> +static void hv_feature_check_deps(X86CPU *cpu, int feature, Error **errp)
>
> Same suggestion as in patch 11/19: also returning a value to
> indicate error is preferred. I would return a boolean.
>
Sure, can do.
> (I don't think this alone should block the series, though)
>
>
>> {
>> - X86CPU *cpu = X86_CPU(cs);
>> uint64_t deps;
>> int dep_feat;
>>
>> - if (!hyperv_feat_enabled(cpu, feature) && !cpu->hyperv_passthrough) {
>> - return 0;
>> - }
>> -
>> deps = kvm_hyperv_properties[feature].dependencies;
>> while (deps) {
>> dep_feat = ctz64(deps);
>> @@ -1161,26 +1157,10 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
>> error_setg(errp, "Hyper-V %s requires Hyper-V %s",
>> kvm_hyperv_properties[feature].desc,
>> kvm_hyperv_properties[dep_feat].desc);
>> - return 1;
>> + return;
>> }
>> deps &= ~(1ull << dep_feat);
>> }
>> -
>> - if (!hyperv_feature_supported(cs, feature)) {
>> - if (hyperv_feat_enabled(cpu, feature)) {
>> - error_setg(errp, "Hyper-V %s is not supported by kernel",
>> - kvm_hyperv_properties[feature].desc);
>> - return 1;
>> - } else {
>> - return 0;
>> - }
>> - }
>> -
>> - if (cpu->hyperv_passthrough) {
>> - cpu->hyperv_features |= BIT(feature);
>> - }
>> -
>> - return 0;
>> }
>>
>> static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
>> @@ -1219,6 +1199,8 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg)
>> void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>> {
>> CPUState *cs = CPU(cpu);
>> + Error *local_err = NULL;
>> + int feat;
>>
>> if (!hyperv_enabled(cpu))
>> return;
>> @@ -1274,53 +1256,38 @@ void kvm_hyperv_expand_features(X86CPU *cpu, Error **errp)
>>
>> cpu->hyperv_spinlock_attempts =
>> hv_cpuid_get_host(cs, HV_CPUID_ENLIGHTMENT_INFO, R_EBX);
>> - }
>>
>> - /* Features */
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) {
>> - return;
>> - }
>> - if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) {
>> - return;
>> + /*
>> + * Mark feature as enabled in 'cpu->hyperv_features' as
>> + * hv_build_cpuid_leaf() uses this info to build guest CPUIDs.
>> + */
>> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
>> + if (hyperv_feature_supported(cs, feat)) {
>> + cpu->hyperv_features |= BIT(feat);
>> + }
>> + }
>> + } else {
>> + /* Check features availability and dependencies */
>> + for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
>> + /* If the feature was not requested skip it. */
>> + if (!hyperv_feat_enabled(cpu, feat)) {
>> + continue;
>> + }
>
> That's the loop I suggested in patch 11/19. Nice. :)
>
:-)
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Thanks!
>
>> +
>> + /* Check if the feature is supported by KVM */
>> + if (!hyperv_feature_supported(cs, feat)) {
>> + error_setg(errp, "Hyper-V %s is not supported by kernel",
>> + kvm_hyperv_properties[feat].desc);
>> + return;
>> + }
>> +
>> + /* Check dependencies */
>> + hv_feature_check_deps(cpu, feat, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> + }
>> }
>>
>> /* Additional dependencies not covered by kvm_hyperv_properties[] */
>> --
>> 2.30.2
>>
--
Vitaly
© 2016 - 2026 Red Hat, Inc.