Eduardo Habkost <ehabk...@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 <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.
>
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 <ehabk...@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
