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

[Vitaly Kuznetsov]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, May 24, 2021 at 02:00:37PM +0200, Vitaly Kuznetsov wrote:
> [...]
>> >> @@ -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.
>
> This will be visible to the guest at CPUID[0x4000000A].EAX,
> correct?  You are initializing CPUID data with a value that
> change depending on the host.
>
> What is supposed to happen if live migrating to to a host with a
> different evmcs_version?

(Note: 'evmcs_version' here is the 'maximum supported evmcs version',
not 'used evmcs version').

This is a purely theoretical question at this moment as there's only one
existing (and supported) eVMCS version: 1.

In future, when (and if) e.g. EVMCSv2 appears, we'll have to introduce a
different QEMU option for it most likely (or something like
'hv-evmcs=1', 'hv-evmcs=2' ... ) as how else would we prevent migration
to a host which doesn't support certain eVMCS version (e.g. EVMCSv2 ->
EVMCSv1)?

I'd be fine with hardcoding '1' and just checking that the returned
version is >= 1 for now. Migration blocker seems to be an overkill (as
there's no real problem, we're just trying to make the code future
proof). 

-- 
Vitaly