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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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