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

[Vitaly Kuznetsov]

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.

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

-- 
Vitaly