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 <vkuzn...@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 <ehabk...@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
