Re: [PATCH v6 05/19] i386: introduce hyperv_feature_supported()

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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!

-- 
Vitaly