+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 <ehabk...@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 <ehabk...@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 <vkuzn...@redhat.com>
> > > > > Reviewed-by: Eduardo Habkost <ehabk...@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
