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

[Eduardo Habkost]

On Wed, Jan 06, 2021 at 05:45:42PM +0100, Igor Mammedov wrote:
> 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.

Yes, this discussion applies to other features as well.

> 
> > 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.

Why?

QEMU's lack of usability is an unfortunate accident, not a
desirable goal.

> 
> > 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.

Versioning is extra work for us QEMU developers, but it has a
purpose.  It saves everybody else's valuable time.


> 
> Also with libosinfo approach, old machine types and old QEMU versions
> can also benefit from it without need to change whole stack.

Except that you need to update the whole stack (QEMU + libvirt +
libosinfo + the glue code between libosinfo and libvirt) every
time a new feature is available.

This is unnecessary overhead, and this is not working.


> And no versioning is necessary since chosen config set is stored in
> domain XML at the moment VM is created.

I don't even think that is a good thing.

I would agree completely with you if the people maintaining the
upper layers were asking us to just let them manage low level
details of guest ABI.  They are not.


> 
> > 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")

This sounds interesting.

> 
> 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.

-- 
Eduardo