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

[Eduardo Habkost]

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, but you are keeping
the existing behavior, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>          }
>      }
>  
> -- 
> 2.30.2
> 

-- 
Eduardo