Re: [PATCH v6 10/19] i386: move eVMCS enablement to hyperv_init_vcpu()

[Eduardo Habkost]

On Thu, Apr 22, 2021 at 06:11:21PM +0200, Vitaly Kuznetsov wrote:
> hyperv_expand_features() will be called before we create vCPU so
> evmcs enablement should go away. hyperv_init_vcpu() looks like the
> right place.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  target/i386/kvm/kvm.c | 60 ++++++++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 6b391db7a030..a2ef2dc154a2 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -962,6 +962,7 @@ static struct kvm_cpuid2 *get_supported_hv_cpuid(CPUState 
> *cs)
>  {
>      struct kvm_cpuid2 *cpuid;
>      int max = 7; /* 0x40000000..0x40000005, 0x4000000A */
> +    int i;
>  
>      /*
>       * When the buffer is too small, KVM_GET_SUPPORTED_HV_CPUID fails with
> @@ -971,6 +972,22 @@ static struct kvm_cpuid2 
> *get_supported_hv_cpuid(CPUState *cs)
>      while ((cpuid = try_get_hv_cpuid(cs, max)) == NULL) {
>          max++;
>      }
> +
> +    /*
> +     * KVM_GET_SUPPORTED_HV_CPUID does not set EVMCS CPUID bit before
> +     * KVM_CAP_HYPERV_ENLIGHTENED_VMCS is enabled but we want to get the
> +     * information early, just check for the capability and set the bit
> +     * manually.
> +     */

Should we add a comment noting that this hack won't be necessary
if using the system ioctl?  I assume we still want to support
Linux < v5.11 for a while, so simply 


> +    if (kvm_check_extension(cs->kvm_state,
> +                            KVM_CAP_HYPERV_ENLIGHTENED_VMCS) > 0) {
> +        for (i = 0; i < cpuid->nent; i++) {
> +            if (cpuid->entries[i].function == HV_CPUID_ENLIGHTMENT_INFO) {
> +                cpuid->entries[i].eax |= HV_ENLIGHTENED_VMCS_RECOMMENDED;
> +            }
> +        }
> +    }
> +
>      return cpuid;
>  }
>  
> @@ -1200,24 +1217,6 @@ static int hyperv_expand_features(CPUState *cs)
>      if (!hyperv_enabled(cpu))
>          return 0;
>  
> -    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) ||
> -        cpu->hyperv_passthrough) {
> -        uint16_t evmcs_version;
> -
> -        r = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> -                                (uintptr_t)&evmcs_version);
> -
> -        if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS) && r) {
> -            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
> -                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
> -            return -ENOSYS;
> -        }
> -
> -        if (!r) {
> -            cpu->hyperv_nested[0] = evmcs_version;
> -        }
> -    }
> -
>      if (cpu->hyperv_passthrough) {
>          cpu->hyperv_vendor_id[0] =
>              hv_cpuid_get_host(cs, HV_CPUID_VENDOR_AND_MAX_FUNCTIONS, R_EBX);
> @@ -1455,6 +1454,21 @@ static int hyperv_init_vcpu(X86CPU *cpu)
>          }
>      }
>  
> +    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_EVMCS)) {
> +        uint16_t evmcs_version;
> +
> +        ret = kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_ENLIGHTENED_VMCS, 0,
> +                                  (uintptr_t)&evmcs_version);
> +
> +        if (ret < 0) {
> +            fprintf(stderr, "Hyper-V %s is not supported by kernel\n",
> +                    kvm_hyperv_properties[HYPERV_FEAT_EVMCS].desc);
> +            return ret;
> +        }
> +
> +        cpu->hyperv_nested[0] = evmcs_version;

Wait, won't this break guest ABI?  Do we want to make
HYPERV_FEAT_EVMCS a migration blocker until this is fixed?


> +    }
> +
>      return 0;
>  }
>  
> @@ -1519,6 +1533,11 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      }
>  
>      if (hyperv_enabled(cpu)) {
> +        r = hyperv_init_vcpu(cpu);
> +        if (r) {
> +            return r;
> +        }
> +
>          cpuid_i = hyperv_fill_cpuids(cs, cpuid_data.entries);
>          kvm_base = KVM_CPUID_SIGNATURE_NEXT;
>          has_msr_hv_hypercall = true;
> @@ -1868,11 +1887,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
>  
>      kvm_init_msrs(cpu);
>  
> -    r = hyperv_init_vcpu(cpu);
> -    if (r) {
> -        goto fail;
> -    }
> -
>      return 0;

I would move the two hunks above to a separate patch, but not a
big deal.  The guest ABI issue is existing, and the comment
suggestion can be done in a follow up patch, so:

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

>  
>   fail:
> -- 
> 2.30.2
> 

-- 
Eduardo