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

[Eduardo Habkost]

On Thu, May 27, 2021 at 09:27:01AM +0200, Vitaly Kuznetsov wrote:
> 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.

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