Re: [PATCH 5/5] i386: provide simple 'hyperv=on' option to x86 machine types

[Igor Mammedov]

On Wed, 06 Jan 2021 14:38:56 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Tue, 05 Jan 2021 17:31:43 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Igor Mammedov <imammedo@redhat.com> writes:
> >>   
> >> > On Tue, 05 Jan 2021 12:50:05 +0100
> >> >
> >> > I think there is a misunderstanding, idea was:
> >> >
> >> > cpu_initfn() {
> >> >     //current set
> >> >     cpu->default_hyperv_cpu_features = ACD
> >> > }
> >> >
> >> > compat_props_5.1 {
> >> >    cpu.default_hyperv_cpu_features = AB
> >> > }
> >> >
> >> > compat_props_5.2 {
> >> >    cpu.default_hyperv_cpu_features = ABC
> >> > }
> >> >    
> >> 
> >> ...
> >>   
> >> > I was talking about CPU features/properties only, it doesn't apply to 
> >> > other devices.
> >> > It makes sense for machine to have a knob to create onboard hyperv 
> >> > specific
> >> > devices if there is any (do we have any?).
> >> >
> >> > If there aren't any currently, I wouldn't bother with machine knob
> >> > and just use -cpu foo,hv_default=on or -device cpu,hv_default=on
> >> > like any other cpu feature.
> >> >    
> >> 
> >> We don't currently have any devices which are not 'CPU features' (in
> >> QEMU terminology), however, we already have Vmbus and I can easily
> >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We
> >> *may* want to enable these 'automatically' and that's what make
> >> '-machine' option preferable. It is, however, not a *must* right now and
> >> we can indeed wait until these devices appear and be happy with
> >> 'hv_default' -cpu option for now. We will, however, need to teach upper
> >> layers about the change when/if it happens.  
> >
> > which makes me think we are trying to bite something that we shouldn't.
> > Do we really need this patch (QEMU knob) to magically enable subset of
> > features and/or devices for a specific OS flavor?
> >
> > It's job of upper layers to abstract low level QEMU details in to coarse
> > grained knobs (libvirt/virt-install/virt-manager/...).
> > For example virt-install may know that it installing a specific Windows
> > version, and can build a tailored for that OS configuration including
> > needed hyperv CPU features and hyperv specific devices.
> > (if I'm not mistaken libosinfo is used to get metadata for preferred
> > configuration, so perhaps this should become a patch for that library
> > and its direct users).
> >
> > What we actually lack is a documentation for preferred configuration
> > in docs/hyperv.txt, currently it just enumerates possible features.
> > We can just document a recommended 'best practices' there without
> > putting it in QEMU code and let upper layers to do their job in
> > the stack.  
> 
> The problem we're facing here is that when a new enlightenment is
> implemented it takes forever to propagate to the whole stack. We don't
It's true not only for Hyper-V, I guess it's price to pay for modular solution.

> have any different recommendations for different Windows versions,
> neither does genuine Hyper-V. The 'fine grained' mechanis we have just
> contributes to the creation of various Frankenstein configurations
> (which look nothing like real Hyper-V), people just google for 'Windows
> KVM slow', add something to their scripts and this keeps propagating.
That's why I mentioned lack of documentation.
If someone manually configures QEMU, one should understand what they do
enable and why or enlist help of virt-install and likes.

> Every time I see a configuration with only a few 'hv_*' options I ask
> 'why don't you enable the rest?' and I'm yet to receive an answer
> different from 'hm, I don't know, I copied it from somewhere and it
> worked'.

If individual features are are composed by virt-install or other tools
based on libosinfo data, then we don't have to maintain versioning
of new default_set_features per machine type, which will only become
worse if we include hv specific devices into it.

Also with libosinfo approach, old machine types and old QEMU versions
can also benefit from it without need to change whole stack.
And no versioning is necessary since chosen config set is stored in
domain XML at the moment VM is created.

> Setting 'hv_*' options individually should be considered debug only.
that's how cpu's features were designed, a helper knob on top is fine
as long as it doesn't mess the way it used to work and preferably is
build on top of existing features.

PS:
another wild idea how to implement it using '-machine hyperv=on',
based on compat props idea:

// replaces bit set in your version
hv_default_set[] =
  "hv_feat1", "hv_feat2",
 ...
};

// probably should be done before -cpu is parsed
then if machine hyperv=on
   foreach in hv_default_set[]
      object_register_sugar_prop(hv_default_set[i], "on")

PS2:
my preferred approach is still -cpu hyperv=on, since it doesn't
depend on order CLI is currently parsed (which is fragile thing),
but rather on what user asked us to do with CPU.