Re: [PATCH v3 18/19] i386: provide simple 'hv-default=on' option

[Igor Mammedov]

On Thu, 21 Jan 2021 09:45:33 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Wed, 20 Jan 2021 15:38:33 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>   
> >> > On Fri, 15 Jan 2021 10:20:23 +0100
> >> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >> >    
> >> >> suggestion is 
> >> >> 
> >> >> if I do:
> >> >> 
> >> >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | 
> >> >> hv_feature"
> >> >> 
> >> >> but if I do
> >> >> 
> >> >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> >> >> (as hv_default enablement will overwrite everything)
> >> >> 
> >> >> How is this consistent?    
> >> > usual semantics for properties, is that the latest property overwrites,
> >> > the previous property value parsed from left to right.
> >> > (i.e. if one asked for hv_default, one gets it related CPUID bit 
> >> > set/unset,
> >> > if one needs more than that one should add more related features after 
> >> > that.
> >> >    
> >> 
> >> This semantics probably doesn't apply to 'hv-default' case IMO as my
> >> brain refuses to accept the fact that  
> > it's difficult probably because 'hv-default' is 'alias' property 
> > that covers all individual hv-foo features in one go and that individual
> > features are exposed to user, but otherwise it is just a property that
> > sets CPUID features or like any other property, and should be treated
> > like such.
> >  
> >> 'hv_default,hv_feature' != 'hv_feature,hv_default'
> >>
> >> which should express the same desire 'the default set PLUS the feature I
> >> want'.  
> > if hv_default were touching different data, I'd agree.
> > But in the end hv_default boils down to the same CPUID bits as individual
> > features:
> >
> >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
> >          !=
> >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)
> >  
> 
> In your case I treat 'hv_default' as 'hv_f1=on' and it says nothing
> about 'hv_f2' - neither it is enabled, nor it is disabled because when
> the corresponding machine type was released it just wasn't there.
> 
> >    
> >> I think I prefer sanity over purity in this case.  
> > what is sanity to one could be insanity for another,
> > so I pointed out the way properties expected to work today.
> >
> > But you are adding new semantic ('combine') to property/features parsing
> > (instead of current 'set' policy), and users will have to be aware of
> > this new behavior and add/maintain code for this special case.
> > (maybe I worry in vain, and no one will read docs and know about this
> > new property anyways)
> >
> > That will also push x86 CPUs consolidation farther away from other targets,
> > where there aren't any special casing for features parsing, just simple
> > left to right parsing with the latest property having overwriting previously
> > set value.  
> 
> In case this is somewhat important I suggest we get back to adding
> 'hyperv=on' machine type option and not do the 'aliasing' with
> 'hv_default'. I think it would be possible to support
> 
> '-M q35,hyper=on -cpu host,hv-stimer-direct=off' 
> 
> even if we need to add a custom handler for Hyper-V feature setting
> instead of just using bits in u64 as we need to remember both what was
> enabled and what was disabled to combine this with machine type property
> correctly.
> 
> > We are trying hard to reduce special cases and unify interfaces for same
> > components to simplify qemu and make it predictable/easier for users.
> >  
> 
> That's exactly the reason why we need simpler Hyper-V feature
> enablement! :-)
> 
> >  
> >> >> >> +    }
> >> >> >> +}
> >> >> >> +
> >> >> >>  /* Generic getter for "feature-words" and "filtered-features" 
> >> >> >> properties */
> >> >> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> >> >> >>                                        const char *name, void 
> >> >> >> *opaque,
> >> >> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
> >> >> >>      object_property_add_alias(obj, "pause_filter", obj, 
> >> >> >> "pause-filter");
> >> >> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
> >> >> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
> >> >> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
> >> >> >>  
> >> >> >>      if (xcc->model) {
> >> >> >>          x86_cpu_load_model(cpu, xcc->model);
> >> >> >>      }
> >> >> >> +
> >> >> >> +    /* Hyper-V features enabled with 'hv-default=on' */
> >> >> >> +    cpu->hyperv_default_features = BIT(HYPERV_FEAT_RELAXED) |
> >> >> >> +        BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
> >> >> >> +        BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
> >> >> >> +        BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
> >> >> >> +        BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
> >> >> >> +        BIT(HYPERV_FEAT_FREQUENCIES) | 
> >> >> >> BIT(HYPERV_FEAT_REENLIGHTENMENT) |
> >> >> >> +        BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_IPI) |
> >> >> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
> >> >> >> +
> >> >> >> +    /* Enlightened VMCS is only available on Intel/VMX */
> >> >> >> +    if (kvm_hv_evmcs_available()) {
> >> >> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
> >> >> >> +    }      
> >> >> > what if VVM is migrated to another host without evmcs,
> >> >> > will it change CPUID?
> >> >> >      
> >> >> 
> >> >> Evmcs is tightly coupled with VMX, we can't migrate when it's not
> >> >> there.    
> >> >
> >> > Are you saying mgmt will check and refuse to migrate to such host?
> >> >    
> >> 
> >> Is it possible to migrate a VM from a VMX-enabled host to a VMX-disabled
> >> one if VMX feature was exposed to the VM? Probably not, you will fail to
> >> create a VM on the destination host. Evmcs doesn't change anything in
> >> this regard, there are no hosts where VMX is available but EVMCS is not.  
> >
> > I'm not sure how evmcs should be handled,
> > can you point out what in this series makes sure that migration fails or
> > makes qemu not able to start in case kvm_hv_evmcs_available() returns false.
> >
> > So far I read snippet above as a problem:
> > 1:
> >   host supports evmcs:
> >   and exposes HYPERV_FEAT_EVMCS in CPUID  
> 
> Host with EVMCS is Intel
> 
> > 2: we migrate to host without evmcs  
> 
> Host without EVMCS is AMD, there are no other options. It is a pure
> software feature available for KVM-intel. And if your KVM is so old that
> it doesn't know anything about EVMCS, a bunch of other options from
> 'hv-default' will not start as well.
> > 2.1 start target QEMU, it happily creates vCPUs without
> > HYPERV_FEAT_EVMCS in CPUID  
> 
> No, it doesn't as on host1 we had at least VMX CPU feature enabled (or a
> CPU model implying it) to make this all work.
> 
> > 2.2 if I'm not mistaken CPUID is not part of migration stream,
> >     nothing could check and fail migration
> > 2.3 guest runs fine till it tries to use non existing feature, ..  
> 
> I'm also very sceptical about possibilities for migration
> Windows/Hyper-V VMs from Intel to AMD. Hyper-V doesn't even boot if you
> don't have fresh-enough CPU so the common denominator for Intel/AMD
> would definitely not work. 

Like you said host doesn't have to be AMD, just old enough kernel will
do the job. What exactly will prevent migration 'successfully' completing?

The way it's currently written migration stream won't prevent it.

One way that might solve issue is to add subsection that's enabled when
kvm_hv_evmcs_available() == true, and check on target that the feature
is available or fail migration.

Maybe Eduardo or David can add more how to deal with it if needed.