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

[Eduardo Habkost]

On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:
> 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:
> > >  
> > >> Igor Mammedov <imammedo@redhat.com> writes:
> > >>   
> > >> > On Thu,  7 Jan 2021 16:14:49 +0100
> > >> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > >> >    
> > >> >> Enabling Hyper-V emulation for a Windows VM is a tiring experience as 
> > >> >> it
> > >> >> requires listing all currently supported enlightenments ("hv-*" CPU
> > >> >> features) explicitly. We do have 'hv-passthrough' mode enabling
> > >> >> everything but it can't be used in production as it prevents 
> > >> >> migration.
> > >> >> 
> > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently 
> > >> >> supported
> > >> >> Hyper-V enlightenments. Later, when new enlightenments get 
> > >> >> implemented,
> > >> >> compat_props mechanism will be used to disable them for legacy 
> > >> >> machine types,
> > >> >> this will keep 'hv-default=on' configurations migratable.
> > >> >> 
> > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > >> >> ---
> > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> > >> >>  target/i386/cpu.h |  5 +++++
> > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> > >> >> 
> > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > >> >> index 5df00da54fc4..a54c066cab09 100644
> > >> >> --- a/docs/hyperv.txt
> > >> >> +++ b/docs/hyperv.txt
> > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific 
> > >> >> features.
> > >> >>  
> > >> >>  2. Setup
> > >> >>  =========
> > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or 
> > >> >> QEMU. In
> > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, 
> > >> >> e.g:
> > >> >> +All currently supported Hyper-V enlightenments can be enabled by 
> > >> >> specifying
> > >> >> +'hv-default=on' CPU flag:
> > >> >>  
> > >> >> -  qemu-system-x86_64 --enable-kvm --cpu 
> > >> >> host,hv_relaxed,hv_vpindex,hv_time, ...
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > >> >> +
> > >> >> +Alternatively, it is possible to do fine-grained enablement through 
> > >> >> CPU flags,
> > >> >> +e.g:
> > >> >> +
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu 
> > >> >> host,hv-relaxed,hv-vpindex,hv-time ...    
> > >> >
> > >> > I'd put here not '...' but rather recommended list of flags, and update
> > >> > it every time when new feature added if necessary.
> > >> >    
> > >
> > > 1)
> > >    
> > >> This is an example of fine-grained enablement, there is no point to put
> > >> all the existing flags there (hv-default is the only recommended way
> > >> now, the rest is 'expert'/'debugging').  
> > > so users are kept in dark what hv-default disables/enables (and it might 
> > > depend
> > > on machine version on top that). Doesn't look like a good documentation 
> > > to me
> > > (sure everyone can go and read source code for it and try to figure out 
> > > how
> > > it's supposed to work)  
> > 
> > 'hv-default' enables *all* currently supported enlightenments. When
> > using with an old machine type, it will enable *all* Hyper-V
> > enlightenmnets which were supported when the corresponding machine type
> > was released. I don't think we document all other cases when a machine
> > type is modified (i.e. where can I read how pc-q35-5.1 is different from
> > pc-q35-5.0 if I refuse to read the source code?)
> > 
> > >  
> > >>  
> > >> > (not to mention that if we had it to begin with, then new 'hv-default' 
> > >> > won't
> > >> > be necessary, I still see it as functionality duplication but I will 
> > >> > not oppose it)
> > >> >    
> > >> 
> > >> Unfortunately, upper layer tools don't read this doc and update
> > >> themselves to enable new features when they appear.  
> > > rant: (just merge all libvirt into QEMU, and make VM configuration less 
> > > low-level.
> > > why stop there, just merge with yet another upper layer, it would save us 
> > > a lot
> > > on communication protocols and simplify VM creation even more,
> > > and no one will have to read docs and write anything new on top.)
> > > There should be limit somewhere, where QEMU job ends and others pile hw 
> > > abstraction
> > > layers on top of it.  
> > 
> > We have '-machine q35' and we don't require to list all the devices from
> > it. We have '-cpu Skylake-Server' and we don't require to configure all
> > the features manually. Why can't we have similar enablement for Hyper-V
> > emulation where we can't even see a real need for anything but 'enable
> > everything' option?
> > 
> > There is no 'one libvirt to rule them all' (fortunately or
> > unfortunately). And sometimes QEMU is the uppermost layer and there's no
> > 'libvirt' on top of it, this is also a perfectly valid use-case.
> > 
> > >  
> > >> Similarly, if when these tools use '-machine q35' they get all the new 
> > >> features we add
> > >> automatically, right?  
> > > it depends, in case of CPUs, new features usually 'off' by default
> > > for existing models. In case of bugs, features sometimes could be
> > > flipped and versioned machines were used to keep broken CPU models
> > > on old machine types.
> > >  
> > 
> > That's why I was saying that Hyper-V enlightenments hardly resemble
> > 'hardware' CPU features.
> Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
> and I prefer to treat them the same way as any other CPUID bits.
> 
> > 
> > >      
> > >> >> +It is also possible to disable individual enlightenments from the 
> > >> >> default list,
> > >> >> +this can be used for debugging purposes:
> > >> >> +
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu 
> > >> >> host,hv-default=on,hv-evmcs=off ...
> > >> >>  
> > >> >>  Sometimes there are dependencies between enlightenments, QEMU is 
> > >> >> supposed to
> > >> >>  check that the supplied configuration is sane.
> > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > >> >> index 48007a876e32..99338de00f78 100644
> > >> >> --- a/target/i386/cpu.c
> > >> >> +++ b/target/i386/cpu.c
> > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object 
> > >> >> *obj, Visitor *v, const char *name,
> > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > >> >>  }
> > >> >>  
> > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> > >> >> +{
> > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > >> >> +
> > >> >> +    return cpu->hyperv_default;
> > >> >> +}
> > >> >> +
> > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> > >> >> +{
> > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > >> >> +
> > >> >> +    cpu->hyperv_default = value;
> > >> >> +
> > >> >> +    if (value) {
> > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;    
> > >> >
> > >> > s/|="/=/ please,
> > >> > i.e. no option overrides whatever was specified before to keep 
> > >> > semantics consistent.
> > >> >    
> > >> 
> > >> Hm,
> > >>   
> > >  
> > >> this doesn't matter for the most recent machine type as
> > >> hyperv_default_features has all the features but imagine you're running
> > >> an older machine type which doesn't have 'hv_feature'. Now your  
> > > normally one shouldn't use new feature with old machine type as it makes
> > > VM non-migratable to older QEMU that has this machine type but not this 
> > > feature.
> > >
> > > nitpicking:
> > >   according to (1) user should not use 'hv_feature' on old machine since
> > >   hv_default should cover all their needs (well they don't know what
> > > hv_default actually is).  
> > 
> > Normally yes but I can imagine sticking to some old machine type for
> > other-than-hyperv-enlightenments purposes and still wanting to add a
> > newly introduced enlightenment. Migration is not always a must.
> > 
> > >  
> > >> 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)

I don't know why you chose to define "hv_default" as
hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
doesn't need to be touched by hv_default at all.


> 
>  
> > 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.
> We are trying hard to reduce special cases and unify interfaces for same
> components to simplify qemu and make it predictable/easier for users.
> 

What you are proposing diverges from other targets, actually.
See target/s390x/cpu_models.c:set_feature_group() for example.
Enabling a feature group in s390x only enables a set of feature
bits, and doesn't touch the rest.

In other words, if hv_default includes hv_f1+hv_f2 (and not hv_f3
or hv_f4), this means:

   hv_default,hv_f3=on,hv_f4=off => (hv_f1=on,hv_f2=on),hv_f3=on,hv_f4=off
          ==
   hv_f3=on,hv_f4=off,hv_default => hv_f3=on,hv_f4=off,(hv_f2=on,hv_f2=on)

That would also mean:

   hv_default,hv_f1=on,hv_f2=off => (hv_f1=on,hv_f2=on),hv_f1=on,hv_f2=off
          !=
   hv_f1=on,hv_f2=off,hv_default => hv_f1=on,hv_f2=off,(hv_f2=on,hv_f2=on)

That's the behavior implemented by Vitaly.

> [...]

-- 
Eduardo