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

[Vitaly Kuznetsov]

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

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.

-- 
Vitaly