Eduardo Habkost <ehabk...@redhat.com> writes:
> 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 <vkuzn...@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
Not exactly sure what was supposed to be here but yes, the hack is not
needed with system KVM_GET_SUPPORTED_HV_CPUID ioctl but we want to
support older kernels.
>
>
>> + 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?
>
Could you please elaborate on the issue? I read the above is: when
evmcs' feature was requested, make an attempt to enable
KVM_CAP_HYPERV_ENLIGHTENED_VMCS, and bail out if this fails. Propagate
the the acquired evmcs version to 'cpu->hyperv_nested[]' otherwise.
>
>> + }
>> +
>> 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 <ehabk...@redhat.com>
>
Thanks!
>>
>> fail:
>> --
>> 2.30.2
>>
--
Vitaly
