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

[Vitaly Kuznetsov]

Igor Mammedov <imammedo@redhat.com> writes:

>> >  
>> > +    /* Hyper-V features enabled with 'hyperv=on' */
>> > +    x86mc->default_hyperv_features = BIT(HYPERV_FEAT_RELAXED) |
>> > +        BIT(HYPERV_FEAT_VAPIC) | BIT(HYPERV_FEAT_TIME) |
>> > +        BIT(HYPERV_FEAT_CRASH) | BIT(HYPERV_FEAT_RESET) |
>> > +        BIT(HYPERV_FEAT_VPINDEX) | BIT(HYPERV_FEAT_RUNTIME) |
>> > +        BIT(HYPERV_FEAT_SYNIC) | BIT(HYPERV_FEAT_STIMER) |
>> > +        BIT(HYPERV_FEAT_FREQUENCIES) | BIT(HYPERV_FEAT_REENLIGHTENMENT) |
>> > +        BIT(HYPERV_FEAT_TLBFLUSH) | BIT(HYPERV_FEAT_EVMCS) |
>> > +        BIT(HYPERV_FEAT_IPI) | BIT(HYPERV_FEAT_STIMER_DIRECT);
> I'd argue that feature bits do not belong to machine code at all.
> If we have to involve machine at all then it should be a set property/value 
> pairs
> that machine will set on CPU object (I'm not convinced that doing it
> from machine code is good idea though).
>

These are 'features' and not feature bits. 'Bits' here are just our
internal (to QEMU) representation of which features are enable and which
are not, we could've just used booleans instead. These feature, when
enabled, will result in some CPUID changes (not 1:1) but I don't see how
it's different from
  
" -machine q35,accel=kvm "

which also results in CPUID changes.

The main reason for putting this to x86 machine type is versioning, as
we go along we will (hopefully) be implementing more and more Hyper-V
features but we want to provide 'one knob to rule them all' but do it in
a way that will allow migration. We already have 'hv_passthrough' for
CPU.

>> >  
>> > +    if (x86ms->hyperv_enabled) {
>> > +        feat = x86mc->default_hyperv_features;
>> > +        /* Enlightened VMCS is only available on Intel/VMX */
>> > +        if (!cpu_has_vmx(&cpu->env)) {
>> > +            feat &= ~BIT(HYPERV_FEAT_EVMCS);
>> > +        }
>> > +
>> > +        cpu->hyperv_features |= feat;
> that will ignore features user explicitly doesn't want,
> ex:
>  -machine hyperv=on -cpu foo,hv-foo=off
>

Existing 'hv_passthrough' mode can also affect the result. Personally, I
don't see where 'hv-foo=off' is needed outside of debugging and these
use-cases can probably be covered by explicitly listing required
features but I'm not against making this work, shouldn't be hard.

> not sure we would like to introduce such invariant,
> in normal qom property handling the latest set property should have effect
> (all other invariants we have in x86 cpu property semantics are comming from 
> legacy handling
> and I plan to deprecate them (it will affect x86 and sparc cpus) so CPUs will 
> behave like
> any other QOM object when it come to property handling)
>  
> anyways it's confusing a bit to have cpu flags to come from 2 different places
>
> -cpu hyperv-use-preset=on,hv-foo=off
>
> looks less confusing and will heave expected effect
>

Honestly, 'hyperv-use-preset' is confusing even to me :-)

What if we for a second stop thinking about Hyper-V features being CPU
features only, e.g. if we want to create Dynamic Memory or PTP or any
other Hyper-V specific device in a simple way? We'll have to put these
under machine type.

-- 
Vitaly