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

[Vitaly Kuznetsov]

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. 

-- 
Vitaly