Re: [PATCH v6 11/19] i386: switch hyperv_expand_features() to using error_setg()

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Apr 22, 2021 at 06:11:22PM +0200, Vitaly Kuznetsov wrote:
>> Use standard error_setg() mechanism in hyperv_expand_features().
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> No objections, but only suggestions below:
>
>> ---
>>  target/i386/kvm/kvm.c | 101 +++++++++++++++++++++++++-----------------
>>  1 file changed, 61 insertions(+), 40 deletions(-)
>> 
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index a2ef2dc154a2..f33ba325187f 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -1135,7 +1135,7 @@ static bool hyperv_feature_supported(CPUState *cs, int 
>> feature)
>>      return true;
>>  }
>>  
>> -static int hv_cpuid_check_and_set(CPUState *cs, int feature)
>> +static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp)
>
> If changing the function signature, and the function only returns 0 or 1, 
> maybe
> it's a good opportunity to change to a bool return value format?
>
> From include/qapi/error.h:
>
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
>
>
>>  {
>>      X86CPU *cpu = X86_CPU(cs);
>>      uint64_t deps;
>> @@ -1149,20 +1149,18 @@ static int hv_cpuid_check_and_set(CPUState *cs, int 
>> feature)
>>      while (deps) {
>>          dep_feat = ctz64(deps);
>>          if (!(hyperv_feat_enabled(cpu, dep_feat))) {
>> -                fprintf(stderr,
>> -                        "Hyper-V %s requires Hyper-V %s\n",
>> -                        kvm_hyperv_properties[feature].desc,
>> -                        kvm_hyperv_properties[dep_feat].desc);
>> -                return 1;
>> +            error_setg(errp, "Hyper-V %s requires Hyper-V %s",
>> +                       kvm_hyperv_properties[feature].desc,
>> +                       kvm_hyperv_properties[dep_feat].desc);
>> +            return 1;
>>          }
>>          deps &= ~(1ull << dep_feat);
>>      }
>>  
>>      if (!hyperv_feature_supported(cs, feature)) {
>>          if (hyperv_feat_enabled(cpu, feature)) {
>> -            fprintf(stderr,
>> -                    "Hyper-V %s is not supported by kernel\n",
>> -                    kvm_hyperv_properties[feature].desc);
>> +            error_setg(errp, "Hyper-V %s is not supported by kernel",
>> +                       kvm_hyperv_properties[feature].desc);
>>              return 1;
>>          } else {
>>              return 0;
>> @@ -1209,13 +1207,12 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, 
>> uint32_t func, int reg)
>>   * of 'hv_passthrough' mode and fills the environment with all supported
>>   * Hyper-V features.
>>   */
>> -static int hyperv_expand_features(CPUState *cs)
>> +static void hyperv_expand_features(CPUState *cs, Error **errp)
>
> Same as above: returning a value to indicate error is preferred.  If you are 
> no
> longer returning an integer error code, I suggest returning bool instead.
>

hv_cpuid_check_and_set() is eliminated later in the series but
hyperv_expand_features() stays, I can make it bool.


>>  {
>>      X86CPU *cpu = X86_CPU(cs);
>> -    int r;
>>  
>>      if (!hyperv_enabled(cpu))
>> -        return 0;
>> +        return;
>>  
>>      if (cpu->hyperv_passthrough) {
>>          cpu->hyperv_vendor_id[0] =
>> @@ -1262,37 +1259,60 @@ static int hyperv_expand_features(CPUState *cs)
>>      }
>>  
>>      /* Features */
>> -    r = hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI);
>> -    r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT);
>> +    if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) {
>> +        return;
>> +    }
>
> What about a loop?
>
>     for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) {
>         if (hv_cpuid_check_and_set(cs, feat, errp)) {
>             return;
>         }
>     }
>

This is done later in the series ("i386: kill off hv_cpuid_check_and_set()").

>> +    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;
>> +    }
>>  
>>      /* Additional dependencies not covered by kvm_hyperv_properties[] */
>>      if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) &&
>>          !cpu->hyperv_synic_kvm_only &&
>>          !hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX)) {
>> -        fprintf(stderr, "Hyper-V %s requires Hyper-V %s\n",
>> -                kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc,
>> -                kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc);
>> -        r |= 1;
>> -    }
>> -
>> -    if (r) {
>> -        return -ENOSYS;
>> +        error_setg(errp, "Hyper-V %s requires Hyper-V %s",
>> +                   kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc,
>> +                   kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc);
>>      }
>> -
>> -    return 0;
>>  }
>>  
>>  /*
>> @@ -1527,9 +1547,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>      env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY;
>>  
>>      /* Paravirtualization CPUIDs */
>> -    r = hyperv_expand_features(cs);
>> -    if (r < 0) {
>> -        return r;
>> +    hyperv_expand_features(cs, &local_err);
>> +    if (local_err) {
>> +        error_report_err(local_err);
>> +        return -ENOSYS;
>>      }
>>  
>>      if (hyperv_enabled(cpu)) {
>
> I don't want to block this series because of the suggestions above, so:
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> But I still encourage you to implement those suggestions, anyway.

'Loop' idea is already implemented and hv_cpuid_check_and_set() is gone
but I'll remember to make hyperv_expand_features() bool. Thanks!

-- 
Vitaly