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 <vkuzn...@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 <ehabk...@redhat.com>
But I still encourage you to implement those suggestions, anyway.
--
Eduardo
