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

[Eduardo Habkost]

+s390 maintainers, a question about feature groups below:

On Mon, Dec 21, 2020 at 02:24:18PM +0100, Igor Mammedov wrote:
> On Fri, 18 Dec 2020 13:07:21 -0500
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 18, 2020 at 06:13:40PM +0100, Igor Mammedov wrote:
> > > On Wed, 16 Dec 2020 15:52:02 -0500
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Nov 19, 2020 at 11:32:21AM +0100, Vitaly Kuznetsov wrote:  
> > > > > Enabling Hyper-V emulation for a Windows VM is a tiring experience as 
> > > > > it
> > > > > requires listing all currently supported enlightenments ("hv_*" CPU
> > > > > features) explicitly. We do have a 'hv_passthrough' mode enabling
> > > > > everything but it can't be used in production as it prevents 
> > > > > migration.
> > > > > 
> > > > > Introduce a simple 'hyperv=on' option for all x86 machine types 
> > > > > enabling
> > > > > all currently supported Hyper-V enlightenments. Later, when new
> > > > > enlightenments get implemented, we will be adding them to newer 
> > > > > machine
> > > > > types only (by disabling them for legacy machine types) thus 
> > > > > preserving
> > > > > migration.
> > > > > 
> > > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>    
[...]  
> > > > > @@ -1194,6 +1208,16 @@ static void x86_machine_class_init(ObjectClass 
> > > > > *oc, void *data)
> > > > >      x86mc->save_tsc_khz = true;
> > > > >      nc->nmi_monitor_handler = x86_nmi;
> > > > >  
> > > > > +    /* 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).  
> > 
> > The set of default hyperv features needs be defined by the
> > machine type somehow, we can't avoid that.
> > 
> > You are correct that the policy could be implemented using
> > compat_props, but I don't think we should block a patch just
> > because we're not using a pure QOM property-based interface to
> > implement that.
> I'm fine with 1-4/5 patches but not with this one.
> With this patch I don't agree with inventing
> special semantics to property handling when it could
> be done in a typical and consistent way (especially for
> the sake of convenience).
> 
> 
> > We need the external interface to be good, though:
> > 
> > >   
> > [...]
> > > > >  static void x86_cpu_hyperv_realize(X86CPU *cpu)
> > > > >  {
> > > > > +    X86MachineState *x86ms = X86_MACHINE(qdev_get_machine());
> > > > > +    X86MachineClass *x86mc = X86_MACHINE_GET_CLASS(x86ms);
> > > > > +    uint64_t feat;
> > > > >      size_t len;
> > > > >  
> > > > > +    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  
> > 
> > Oops, good point.
> > 
> > 
> > > 
> > > 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
> > >   
> > > > > +    }    
> > > > 
> > > > I had to dequeue this because it doesn't compile with
> > > > CONFIG_USER_ONLY:
> > > > 
> > > > https://gitlab.com/ehabkost/qemu/-/jobs/916651017
> > > > 
> > > > The easiest solution would be to wrap the new code in #ifndef
> > > > CONFIG_USER_ONLY, but maybe we should try to move all
> > > > X86Machine-specific code from cpu.c to
> > > > hw/i386/x86.c:x86_cpu_pre_plug().  
> > > this looks to me like a preset of feature flags that belongs to CPU,
> > > and machine code here only as a way to version subset of CPU features.
> > > 
> > > Is there a way to implement it without modifying machine?  
> > 
> > Maybe there is, but why modifying machine is a problem?
> 
> 1. it doesn't let do the job properly (realize time is too late)
> 2. unnecessarily pushes CPU specific logic to machine code,
>    it just doesn't belong there.
>    Sure we can do that here, then some where else and in the end
>    code becomes unmanageable mess.
>  
> > I agree the interface needs to be clear and consistent, though.
> > Maybe making it a -cpu option would make this clearer and more
> > consistent.
> > 
> > > 
> > > for example versioned CPUs or maybe something like this:
> > > 
> > > for CLI:
> > > -cpu hyperv-use-preset=on,hv-foo=off  
> > 
> > In either case, we must clearly define what should happen if the
> > preset is (HYPERV_FEAT_X | HYPERV_FEAT_Y), and the command line
> > has:
> > 
> >   -cpu foo,hv-A=on,hv-X=off,hyperv-use-preset=on,hv-B=on,hv-Y=off
> 
> current x86 cpu code (it doesn't have typical properties handling
> for keeping legacy semantics), it will basically reorder all features
> with 'off' value to the end, so hv-X=off will still have an effect.
> 
> However I plan to deprecate those reordering semantics (x86/sparc cpus),
> to make it consistent with typical property handling
> (last set value overwrites any previously set one).
> 
> That will let us drop custom parsing of -cpu (quite a bit of code) and
> more importantly make it consistent with -device/device_add cpu-foo.

Right.

> 
> 
> > or:
> > 
> >   -machine hyperv=on -cpu foo,hv-A=on,hv-X=off,hv-B=on,hv-X=off
> > 
> > Personally, I don't care what the rules are, as long as: 1) they
> > are clearly defined and documented; 2) they support the use cases
> > we need to support.
> 
> I'd like to stick with typical property handling rules, and resort to
> inventing/using other invariant only if there is no other choice.

What would be the typical handling rules, in this case?  I don't
remember other cases in x86 where a single property affects
multiple feature flags.

We have something similar on s390x, though.  So, a question to
s390x maintainers:

If "G" is a feature group including the features X, Y, Z, what is
the result of:

   -cpu foo,X=off,G=on,Y=off

Would X be enabled?  Would Y be enabled?

I would expect X to be enabled and Y to be disabled, but I'd like
to confirm.


> 
> 
> > An automated test case to make sure we don't break the rules
> > would be really welcome.
> > 
> > > 
> > >    diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index 8d1a90c6cf..8828dcde8e 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -35,6 +35,7 @@ GlobalProperty hw_compat_5_0[] = {
> > >      { "vmport", "x-signal-unsupported-cmd", "off" },
> > >      { "vmport", "x-report-vmx-type", "off" },
> > >      { "vmport", "x-cmds-v2", "off" },
> > > +    { "cpu-foo", "hv-preset", "0xXXXX" }, // use compat props to keep 
> > > old defaults
> > > +                                          // it will be set before we 
> > > return from object_new(cpu_type)
> > >  };
> > >  const size_t hw_compat_5_0_len = G_N_ELEMENTS(hw_compat_5_0);
> > >  
> > > diff --git a/slirp b/slirp
> > > --- a/slirp
> > > +++ b/slirp
> > > @@ -1 +1 @@
> > > -Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275
> > > +Subproject commit ce94eba2042d52a0ba3d9e252ebce86715e94275-dirty
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 588f32e136..f0b511ce27 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -7190,6 +7190,8 @@ static Property x86_cpu_properties[] = {
> > >  
> > >      DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts,
> > >                         HYPERV_SPINLOCK_NEVER_RETRY),
> > > +    DEFINE_PROP_UNIT64("hyperv-preset", X86CPU, hyperv_features_def, 
> > > 0xYYYYY),
> > > + // prop_info should define custom setter/getter that will copy 
> > > hyperv_features_def into hyperv_features
> > > + // moment "hyperv-use-preset=on" is processed, it will overwrite any 
> > > previously set
> > > + // hv-foo but that's fine because user asked for it explictly
> > > +    DEFINE_PROP("hyperv-use-preset", X86CPU, hyperv_use_preset, 
> > > prop_info, bool),  
> > 
> > We don't need to use custom getters/setters with DEFINE_PROP, if
> > we can use object_class_property_add_bool().
> of cause, I've used DEFINE_PROP just as a possible example.
> 
> > I dislike custom getters/setters in either case, but maybe we
> > don't have a choice.  Depending on the rules we agree upon above,
> > custom setters could become avoidable, or they could become a
> > necessity.
> 
> I do dislike them too, but sometimes custom setters are convenient
> as they allow to check if value is valid and let us implement non
> trivial handling (like in this case) at property setting time.
> (doing overwites)
> 
> > >      DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features,
> > >                        HYPERV_FEAT_RELAXED, 0),
> > >      DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,  
> > 
> 

-- 
Eduardo