On Thu, May 27, 2021 at 09:27:01AM +0200, Vitaly Kuznetsov wrote:
> Eduardo Habkost <ehabk...@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.
Good to know. :)
>
> 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).
Sounds good to me. I agree a migration blocker is not the right
solution if currently all hosts have evmcs_version==1.
--
Eduardo
