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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 05 Jan 2021 12:50:05 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Mon, 04 Jan 2021 13:54:32 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> 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.  
> >
> > for versioning device models (cpu included), we typically set some default in
> > device's ininfn, and if later on we need to change it to another default
> > we use compat properties to keep old default to old machine types.
> >
> > For example using it with CPU look at pc_compat_3_1
> >  
> 
> The tricky part for Hyper-V enlightenments is that we have to keep them
> all off as the default when it wasn't explicitly requested as they're
> only needed for Windows guests so one way or another we need a new knob
> to enable the default-good-set.
> 
> >> 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.  
> > ideally it would be a property of device that implements the feature
> > and machine might enable it depending on its own properties defaults,
> > but if you change the default behavior of the device model, you do
> > it in device model and use compat properties infrastructure to keep
> > old machine types happy.  
> 
> This would work nicely if we were to enable some of the Hyper-V
> enlightenments by default for new machine types and then turn them off
> with compat properties. We are in a different situation though, we want
> one knob which will tell us 'enable the default good set' and then we
> need to subtract something from this set because e.g. our machine type
> is old. In case the knob is, as you suggest, in CPU properties
> ('hv_default=on' or something like that) we'll have to play dirty games
> in machine init funtion again: go to CPU device options and check if
> 'hv_default=on' was requested. If yes, then we enable all Hyper-V
> enlightenments and do the subtraction according to machine version. And
> what's even more weird,

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
}

and cpu property 'hv_default=on' should apply hv specific default set to
CPU when it's provided by user.
Simple as that, the scope of the patch was on CPU features not other devices,
and that keeps property semantics simple and clean
   -cpu foo,hv_default=on,hv_foo1=on,hv_foo2=off
properties just any other are parsed and applied from left to right and no extra code
is necessary.

if we try to do the same at cpu.realize() time like this patch (cpu_pre_plug is the same)
based on '-machine hyperv=on', it won't work because realize() time is too late and
we end up loosing 'hv_foo2=off' and/or 'hv_foo1=on' which creates different semantics, and I wouldn't
write off 'hv_foo2=off' as useless, as it might unbreak some guests which don't like
'hv_foo2' for some reason.

to make '-machine hyperv=on' work nice with user set properties we either:
 - at realize time, need to know if 'hv_foo2=off' was set by user and re-apply it on top of
   hv_default=on (there is no API for that at the moment), and maybe we need to know order
   in which properties are specified. which is messy and complex.

 - potentially '-machine hyperv=on' may push 'cpu.hv_default=on' global property
   like '-cpu foo,features' does. We need just need to make up mind on the order.
   I'd go for 
      1. '-machine hyperv=on' => add_global('cpu.hv_default=on')
      2. parse '-cpu' and/or -device cpu,features

In the end I'm not against '-machine hyperv=on', but it complicates things
compared to just

 cpu.default_hyperv_cpu_features + cpu.hv_default + -cpu foo,hv_default=on when desired
 + using compat props for versioning.

And if -machine hyperv=on is 'must have' thing, it's fine as long as doesn't
create special cases for property parsing semantics. 

> that we'll have to use 'hv_default=on' CPU flag
> as an indication to create non-CPU devices. Much easier if the knob is a
> property of machine type itself.
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 can, of course, create a parallel (to the existing one) set of
> Hyper-V properties which are going to be enabled by default (and
> blacklisted by compat properties) and then later when CPU object is
> created we'll set CPU properties according to these 'default' properties
> but I hardly see a benefit in complicating stuff that much.
> Also, compat properties are not the only thing we take into
> consideration when creating an old machine type today. E.g.:
> 
> static void pc_q35_3_1_machine_options(MachineClass *m)
> {
>     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> 
>     pc_q35_4_0_machine_options(m);
>     m->default_kernel_irqchip_split = false;
>     pcmc->do_not_add_smb_acpi = true;
>     m->smbus_no_migration_support = true;
>     m->alias = NULL;
>     pcmc->pvh_enabled = false;
>     compat_props_add(m->compat_props, hw_compat_3_1, hw_compat_3_1_len);
>     compat_props_add(m->compat_props, pc_compat_3_1, pc_compat_3_1_len);
> }
> 
> and that's exactly what I was thinking about for Hyper-V enlightenments:
> when a new one is introduced we'll turn it off by default for new
> machine types, no matter if it's going to be a CPU property or a new
> device.

you lost me here, I'm not sure what are you talking about.