Re: [PATCH v6 16/19] i386: kill off hv_cpuid_check_and_set()

[Eduardo Habkost]

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