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

[Eduardo Habkost]

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.

>  {
>      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;
        }
    }

> +    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.

-- 
Eduardo