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

[Igor Mammedov]

On Tue, 05 Jan 2021 16:10:36 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote:  
> >> 
> >> documenting is good, but if it adds new semantics to how CPU features are 
> >> handled
> >> users up the stack will need code it up as well and juggle with
> >>  -machine + -cpu + -device cpu-foo
> >> not to mention poor developers who will have to figure out why we do
> >> set CPU properties in multiple different ways.
> >> 
> >> however if we add it as CPU properties that behave the same way as other
> >> properties, all mgmt has to do is expose new property to user for usage.  
> >
> > I think we need to be careful here.  Sometimes just exposing the
> > QOM properties used to implemented a feature is not the best user
> > interface.  e.g.: even if using compat_props for implementing the
> > hyperv features preset, that doesn't automatically mean we want
> > hyperv=on to be a -cpu option.
> >
> > I would even argue we shouldn't be focusing on implementation
> > details (like we are doing right now) until the desired external
> > interface is described clearly.  
> 
> I agree, the interface is definitely more important than the
> implementation here. AFAIU we have two options suggested:
> 
> 1) 'hyperv=on' option for x86 machine types.
> 
> Pros: we can use it later to create non-CPU Hyper-V devices
> (e.g. Vmbus).
> Cons: two different places for the currently existing Hyper-V features
> enablement (-cpu and -machine), non-standard way of doing things
> (code-wise).
> 
> 2) 'hv_default=on' -cpu option
> 
> Pros: Single place to enable all Hyper-V enlightenments, we can make it
> mutually exclusive with other hv_* options including hv_passthrough
> (clear semantics).
> 
> Cons: This can't be reused to create non-CPU objects in the future and
> so upper layers will (again) need to be modified.
> 
> There's probably more, please feel free to add.
#1 can be implemented on top of #2, when it becomes necessary.


> >> however in this case we are talking about a set of cpu features,
> >> if there is no way to implement it as cpu properties + compat properties
> >> and requires opencodding it within machine code it might be fine
> >> but I fail to see a very good reason for doing that at this momment.  
> >
> > The reason would be just simplicity of implementation.
> >
> > I understand there are reasons to suggest using compat_props if
> > it makes things simpler, but I don't see why we would reject a
> > patch because the implementation is not based purely on
> > compat_props.
> >
> > I will let Vitaly to decide how to proceed, based on our
> > feedback.  I encourage him to use compat_props like you suggest,
> > but I don't plan to make this a requirement.
> >  
> 
> Like I replied to Igor in a parallel thread, I hardly see how using
> compat_props can simplify things in case we decide to keep 'hyperv=on' a
> machine type option. It doesn't seem to fit our use-case when we need a
> mechanism to alter CPU properties for the current machine type as well
> as subtract some features for the old ones. If we, however, decide that
> '-cpu' option is better, then we can try to make it work (but the
> implementation won't be straitforward either). 
lets discuss it in that thread.