Message ID | 20201119103221.1665171-6-vkuznets@redhat.com |
---|---|
State | New |
Headers | show |
Series | i386: simplify Hyper-V enlightenments enablement | expand |
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> [...] > --- > docs/hyperv.txt | 8 ++++++++ > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++ > include/hw/i386/x86.h | 7 +++++++ > target/i386/cpu.c | 14 ++++++++++++++ > 4 files changed, 59 insertions(+) > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt > index 5df00da54fc4..1a76a07f8417 100644 > --- a/docs/hyperv.txt > +++ b/docs/hyperv.txt > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification > and features are kept in leaves 0x40000100..0x40000101. > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an > +x86 machine type: > + > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... > + > +Note, new enlightenments are only added to the latest (in-develompent) machine > +type, older machine types keep the list of the supported features intact to > +safeguard migration. > > 3. Existing enlightenments > =========================== > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 5944fc44edca..57f27d56ecc6 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); > } > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + > + return x86ms->hyperv_enabled; > +} > + > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) > +{ > + X86MachineState *x86ms = X86_MACHINE(obj); > + > + x86ms->hyperv_enabled = value; > +} > + > static void x86_machine_initfn(Object *obj) > { > X86MachineState *x86ms = X86_MACHINE(obj); > @@ -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); > + > object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", > x86_machine_get_smm, x86_machine_set_smm, > NULL, NULL); > @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) > NULL, NULL); > object_class_property_set_description(oc, X86_MACHINE_ACPI, > "Enable ACPI"); > + > + object_class_property_add_bool(oc, X86_MACHINE_HYPERV, > + x86_machine_get_hyperv, x86_machine_set_hyperv); > + > + object_class_property_set_description(oc, X86_MACHINE_HYPERV, > + "Enable Hyper-V enlightenments"); > } > > static const TypeInfo x86_machine_info = { > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > index 739fac50871b..598abd1be806 100644 > --- a/include/hw/i386/x86.h > +++ b/include/hw/i386/x86.h > @@ -38,6 +38,9 @@ struct X86MachineClass { > bool save_tsc_khz; > /* Enables contiguous-apic-ID mode */ > bool compat_apic_id_mode; > + > + /* Hyper-V features enabled with 'hyperv=on' */ > + uint64_t default_hyperv_features; > }; > > struct X86MachineState { > @@ -71,10 +74,14 @@ struct X86MachineState { > * will be translated to MSI messages in the address space. > */ > AddressSpace *ioapic_as; > + > + /* Hyper-V emulation */ > + bool hyperv_enabled; > }; > > #define X86_MACHINE_SMM "smm" > #define X86_MACHINE_ACPI "acpi" > +#define X86_MACHINE_HYPERV "hyperv" > > #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") > OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 83aca942d87c..63a931679d73 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -53,6 +53,7 @@ > #include "sysemu/tcg.h" > #include "hw/qdev-properties.h" > #include "hw/i386/topology.h" > +#include "hw/i386/x86.h" > #ifndef CONFIG_USER_ONLY > #include "exec/address-spaces.h" > #include "hw/i386/apic_internal.h" > @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) > > 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; > + } 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().
Eduardo Habkost <ehabkost@redhat.com> writes: > 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> > [...] >> --- >> docs/hyperv.txt | 8 ++++++++ >> hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++ >> include/hw/i386/x86.h | 7 +++++++ >> target/i386/cpu.c | 14 ++++++++++++++ >> 4 files changed, 59 insertions(+) >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt >> index 5df00da54fc4..1a76a07f8417 100644 >> --- a/docs/hyperv.txt >> +++ b/docs/hyperv.txt >> @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor >> identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification >> and features are kept in leaves 0x40000100..0x40000101. >> >> +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an >> +x86 machine type: >> + >> + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... >> + >> +Note, new enlightenments are only added to the latest (in-develompent) machine >> +type, older machine types keep the list of the supported features intact to >> +safeguard migration. >> >> 3. Existing enlightenments >> =========================== >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index 5944fc44edca..57f27d56ecc6 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, >> visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); >> } >> >> +static bool x86_machine_get_hyperv(Object *obj, Error **errp) >> +{ >> + X86MachineState *x86ms = X86_MACHINE(obj); >> + >> + return x86ms->hyperv_enabled; >> +} >> + >> +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) >> +{ >> + X86MachineState *x86ms = X86_MACHINE(obj); >> + >> + x86ms->hyperv_enabled = value; >> +} >> + >> static void x86_machine_initfn(Object *obj) >> { >> X86MachineState *x86ms = X86_MACHINE(obj); >> @@ -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); >> + >> object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", >> x86_machine_get_smm, x86_machine_set_smm, >> NULL, NULL); >> @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) >> NULL, NULL); >> object_class_property_set_description(oc, X86_MACHINE_ACPI, >> "Enable ACPI"); >> + >> + object_class_property_add_bool(oc, X86_MACHINE_HYPERV, >> + x86_machine_get_hyperv, x86_machine_set_hyperv); >> + >> + object_class_property_set_description(oc, X86_MACHINE_HYPERV, >> + "Enable Hyper-V enlightenments"); >> } >> >> static const TypeInfo x86_machine_info = { >> diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h >> index 739fac50871b..598abd1be806 100644 >> --- a/include/hw/i386/x86.h >> +++ b/include/hw/i386/x86.h >> @@ -38,6 +38,9 @@ struct X86MachineClass { >> bool save_tsc_khz; >> /* Enables contiguous-apic-ID mode */ >> bool compat_apic_id_mode; >> + >> + /* Hyper-V features enabled with 'hyperv=on' */ >> + uint64_t default_hyperv_features; >> }; >> >> struct X86MachineState { >> @@ -71,10 +74,14 @@ struct X86MachineState { >> * will be translated to MSI messages in the address space. >> */ >> AddressSpace *ioapic_as; >> + >> + /* Hyper-V emulation */ >> + bool hyperv_enabled; >> }; >> >> #define X86_MACHINE_SMM "smm" >> #define X86_MACHINE_ACPI "acpi" >> +#define X86_MACHINE_HYPERV "hyperv" >> >> #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") >> OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 83aca942d87c..63a931679d73 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -53,6 +53,7 @@ >> #include "sysemu/tcg.h" >> #include "hw/qdev-properties.h" >> #include "hw/i386/topology.h" >> +#include "hw/i386/x86.h" >> #ifndef CONFIG_USER_ONLY >> #include "exec/address-spaces.h" >> #include "hw/i386/apic_internal.h" >> @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) >> >> 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; >> + } > > 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(). Bummer, let me try the suggestion.
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> > [...] > > --- > > docs/hyperv.txt | 8 ++++++++ > > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++ > > include/hw/i386/x86.h | 7 +++++++ > > target/i386/cpu.c | 14 ++++++++++++++ > > 4 files changed, 59 insertions(+) > > > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt > > index 5df00da54fc4..1a76a07f8417 100644 > > --- a/docs/hyperv.txt > > +++ b/docs/hyperv.txt > > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor > > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification > > and features are kept in leaves 0x40000100..0x40000101. > > > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an > > +x86 machine type: > > + > > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... > > + > > +Note, new enlightenments are only added to the latest (in-develompent) machine > > +type, older machine types keep the list of the supported features intact to > > +safeguard migration. > > > > 3. Existing enlightenments > > =========================== > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > index 5944fc44edca..57f27d56ecc6 100644 > > --- a/hw/i386/x86.c > > +++ b/hw/i386/x86.c > > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, > > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); > > } > > > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp) > > +{ > > + X86MachineState *x86ms = X86_MACHINE(obj); > > + > > + return x86ms->hyperv_enabled; > > +} > > + > > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) > > +{ > > + X86MachineState *x86ms = X86_MACHINE(obj); > > + > > + x86ms->hyperv_enabled = value; > > +} > > + > > static void x86_machine_initfn(Object *obj) > > { > > X86MachineState *x86ms = X86_MACHINE(obj); > > @@ -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). > > + > > object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", > > x86_machine_get_smm, x86_machine_set_smm, > > NULL, NULL); > > @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) > > NULL, NULL); > > object_class_property_set_description(oc, X86_MACHINE_ACPI, > > "Enable ACPI"); > > + > > + object_class_property_add_bool(oc, X86_MACHINE_HYPERV, > > + x86_machine_get_hyperv, x86_machine_set_hyperv); > > + > > + object_class_property_set_description(oc, X86_MACHINE_HYPERV, > > + "Enable Hyper-V enlightenments"); > > } > > > > static const TypeInfo x86_machine_info = { > > diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h > > index 739fac50871b..598abd1be806 100644 > > --- a/include/hw/i386/x86.h > > +++ b/include/hw/i386/x86.h > > @@ -38,6 +38,9 @@ struct X86MachineClass { > > bool save_tsc_khz; > > /* Enables contiguous-apic-ID mode */ > > bool compat_apic_id_mode; > > + > > + /* Hyper-V features enabled with 'hyperv=on' */ > > + uint64_t default_hyperv_features; > > }; > > > > struct X86MachineState { > > @@ -71,10 +74,14 @@ struct X86MachineState { > > * will be translated to MSI messages in the address space. > > */ > > AddressSpace *ioapic_as; > > + > > + /* Hyper-V emulation */ > > + bool hyperv_enabled; > > }; > > > > #define X86_MACHINE_SMM "smm" > > #define X86_MACHINE_ACPI "acpi" > > +#define X86_MACHINE_HYPERV "hyperv" > > > > #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") > > OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 83aca942d87c..63a931679d73 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -53,6 +53,7 @@ > > #include "sysemu/tcg.h" > > #include "hw/qdev-properties.h" > > #include "hw/i386/topology.h" > > +#include "hw/i386/x86.h" > > #ifndef CONFIG_USER_ONLY > > #include "exec/address-spaces.h" > > #include "hw/i386/apic_internal.h" > > @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) > > > > 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 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? for example versioned CPUs or maybe something like this: for CLI: -cpu hyperv-use-preset=on,hv-foo=off 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), DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features, HYPERV_FEAT_RELAXED, 0), DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
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> > > [...] > > > --- > > > docs/hyperv.txt | 8 ++++++++ > > > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++ > > > include/hw/i386/x86.h | 7 +++++++ > > > target/i386/cpu.c | 14 ++++++++++++++ > > > 4 files changed, 59 insertions(+) > > > > > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt > > > index 5df00da54fc4..1a76a07f8417 100644 > > > --- a/docs/hyperv.txt > > > +++ b/docs/hyperv.txt > > > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor > > > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification > > > and features are kept in leaves 0x40000100..0x40000101. > > > > > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an > > > +x86 machine type: > > > + > > > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... > > > + > > > +Note, new enlightenments are only added to the latest (in-develompent) machine > > > +type, older machine types keep the list of the supported features intact to > > > +safeguard migration. > > > > > > 3. Existing enlightenments > > > =========================== > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > > index 5944fc44edca..57f27d56ecc6 100644 > > > --- a/hw/i386/x86.c > > > +++ b/hw/i386/x86.c > > > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, > > > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); > > > } > > > > > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp) > > > +{ > > > + X86MachineState *x86ms = X86_MACHINE(obj); > > > + > > > + return x86ms->hyperv_enabled; > > > +} > > > + > > > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) > > > +{ > > > + X86MachineState *x86ms = X86_MACHINE(obj); > > > + > > > + x86ms->hyperv_enabled = value; > > > +} > > > + > > > static void x86_machine_initfn(Object *obj) > > > { > > > X86MachineState *x86ms = X86_MACHINE(obj); > > > @@ -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. 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? 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 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. 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(). 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. > DEFINE_PROP_BIT64("hv-relaxed", X86CPU, hyperv_features, > HYPERV_FEAT_RELAXED, 0), > DEFINE_PROP_BIT64("hv-vapic", X86CPU, hyperv_features,
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> > > > [...] > > > > --- > > > > docs/hyperv.txt | 8 ++++++++ > > > > hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++ > > > > include/hw/i386/x86.h | 7 +++++++ > > > > target/i386/cpu.c | 14 ++++++++++++++ > > > > 4 files changed, 59 insertions(+) > > > > > > > > diff --git a/docs/hyperv.txt b/docs/hyperv.txt > > > > index 5df00da54fc4..1a76a07f8417 100644 > > > > --- a/docs/hyperv.txt > > > > +++ b/docs/hyperv.txt > > > > @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor > > > > identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification > > > > and features are kept in leaves 0x40000100..0x40000101. > > > > > > > > +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an > > > > +x86 machine type: > > > > + > > > > + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... > > > > + > > > > +Note, new enlightenments are only added to the latest (in-develompent) machine > > > > +type, older machine types keep the list of the supported features intact to > > > > +safeguard migration. > > > > > > > > 3. Existing enlightenments > > > > =========================== > > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > > > index 5944fc44edca..57f27d56ecc6 100644 > > > > --- a/hw/i386/x86.c > > > > +++ b/hw/i386/x86.c > > > > @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, > > > > visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); > > > > } > > > > > > > > +static bool x86_machine_get_hyperv(Object *obj, Error **errp) > > > > +{ > > > > + X86MachineState *x86ms = X86_MACHINE(obj); > > > > + > > > > + return x86ms->hyperv_enabled; > > > > +} > > > > + > > > > +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) > > > > +{ > > > > + X86MachineState *x86ms = X86_MACHINE(obj); > > > > + > > > > + x86ms->hyperv_enabled = value; > > > > +} > > > > + > > > > static void x86_machine_initfn(Object *obj) > > > > { > > > > X86MachineState *x86ms = X86_MACHINE(obj); > > > > @@ -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. > 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. > 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, >
+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, > > >
> Am 21.12.2020 um 20:47 schrieb Eduardo Habkost <ehabkost@redhat.com>: > > +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. IIRC, the last one wins. Properties are applied left to right.
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.
On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov 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. This is a good point, although having accel affect CPUID bits was also a source of complexity for query-cpu-model-expansion and other QMP queries. > > 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. I agree completely that the set of bits needs to be on MachineClass. We just need to agree on the external interface. > > >> > > >> > + 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. I'm all for not wasting time supporting use cases that are not necessary in practice. We just need to document the expected behavior clearly, whatever we decide to do. > > > 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. I agree. Hyper-V is not just a set of CPU features. Also, those two approaches are not mutually exclusive. "-machine hyperv=on" can be implemented internally using "hyperv-use-preset=on" if necessary. I don't think it has to, however.
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 > > >> > > >> > + 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. there is a lot of way to implement something, in this case point is to keep it consistent with the way we handle cpu features/properties. And not to make up new semantics unless there is no other way. > > 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 :-) that was just an example > > 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. >
On Mon, 4 Jan 2021 13:29:06 -0500 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov 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. > > This is a good point, although having accel affect CPUID bits was > also a source of complexity for query-cpu-model-expansion and > other QMP queries. why was, it's still a headache (mutating CPU models depending on accelerator) > > > > > 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. > > I agree completely that the set of bits needs to be on > MachineClass. We just need to agree on the external interface. That's where I disagree, let me exaggerate for demo purpose: - let's move all CPU models feature defaults to MachineClass and forget about compat properties since in that case we can opencode changes in machine_class_init It's rather hard code integration between device models, which we try to avoid and still refactoring QEMU code to get rid of it. (sure it works until it's not and someone else need to rewrite half of QEMU to accomplish it's own task because we mixed things together) > > > > > >> > > > >> > + 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. > > I'm all for not wasting time supporting use cases that are not > necessary in practice. We just need to document the expected > behavior clearly, whatever we decide to do. documenting is good, but if it adds new semantics to how CPU features are handled users up the stack will need code it up as well and juggle with -machine + -cpu + -device cpu-foo not to mention poor developers who will have to figure out why we do set CPU properties in multiple different ways. however if we add it as CPU properties that behave the same way as other properties, all mgmt has to do is expose new property to user for usage. it even more true when building machine from QMP interface would be available, where we would want '-device foo' more or less the same way instead of special casing some of them, i.e. I'd rather have one device to configure, instead of doing it in multiple places. It's not possible in reality but for new code we should try to minimize split brain issues. > > > > > 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. > > I agree. Hyper-V is not just a set of CPU features. me too, however in this case we are talking about a set of cpu features, if there is no way to implement it as cpu properties + compat properties and requires opencodding it within machine code it might be fine but I fail to see a very good reason for doing that at this momment. > > Also, those two approaches are not mutually exclusive. > "-machine hyperv=on" can be implemented internally using > "hyperv-use-preset=on" if necessary. I don't think it has to, > however.
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.
On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote: > On Mon, 4 Jan 2021 13:29:06 -0500 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov 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. > > > > This is a good point, although having accel affect CPUID bits was > > also a source of complexity for query-cpu-model-expansion and > > other QMP queries. > > why was, it's still a headache (mutating CPU models depending on accelerator) > > > > > > > > > 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. > > > > I agree completely that the set of bits needs to be on > > MachineClass. We just need to agree on the external interface. > That's where I disagree, > let me exaggerate for demo purpose: > - let's move all CPU models feature defaults to MachineClass and forget about compat properties > since in that case we can opencode changes in machine_class_init I don't see your point here. compat_props is also part of MachineClass. > > It's rather hard code integration between device models, which we try > to avoid and still refactoring QEMU code to get rid of it. > (sure it works until it's not and someone else need to rewrite half of QEMU > to accomplish it's own task because we mixed things together) I don't see why using a X86CPU-specific API that is not based on QOM properties is hard code integration. compat_props is not the only allowed API for machines to communicate with devices. > > > > > > > > > >> > > > > >> > + 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. > > > > I'm all for not wasting time supporting use cases that are not > > necessary in practice. We just need to document the expected > > behavior clearly, whatever we decide to do. > > documenting is good, but if it adds new semantics to how CPU features are handled > users up the stack will need code it up as well and juggle with > -machine + -cpu + -device cpu-foo > not to mention poor developers who will have to figure out why we do > set CPU properties in multiple different ways. > > however if we add it as CPU properties that behave the same way as other > properties, all mgmt has to do is expose new property to user for usage. I think we need to be careful here. Sometimes just exposing the QOM properties used to implemented a feature is not the best user interface. e.g.: even if using compat_props for implementing the hyperv features preset, that doesn't automatically mean we want hyperv=on to be a -cpu option. I would even argue we shouldn't be focusing on implementation details (like we are doing right now) until the desired external interface is described clearly. > > it even more true when building machine from QMP interface would be available, > where we would want '-device foo' more or less the same way instead of > special casing some of them, i.e. I'd rather have one device to configure, > instead of doing it in multiple places. It's not possible in reality > but for new code we should try to minimize split brain issues. Is split brain a practical problem here? If the new behavior is implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know it's going to affect all CPU objects. > > > > > > > > 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. > > > > I agree. Hyper-V is not just a set of CPU features. > me too, > however in this case we are talking about a set of cpu features, > if there is no way to implement it as cpu properties + compat properties > and requires opencodding it within machine code it might be fine > but I fail to see a very good reason for doing that at this momment. The reason would be just simplicity of implementation. I understand there are reasons to suggest using compat_props if it makes things simpler, but I don't see why we would reject a patch because the implementation is not based purely on compat_props. I will let Vitaly to decide how to proceed, based on our feedback. I encourage him to use compat_props like you suggest, but I don't plan to make this a requirement. > > > > > Also, those two approaches are not mutually exclusive. > > "-machine hyperv=on" can be implemented internally using > > "hyperv-use-preset=on" if necessary. I don't think it has to, > > however. > >
Eduardo Habkost <ehabkost@redhat.com> writes: > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote: >> >> documenting is good, but if it adds new semantics to how CPU features are handled >> users up the stack will need code it up as well and juggle with >> -machine + -cpu + -device cpu-foo >> not to mention poor developers who will have to figure out why we do >> set CPU properties in multiple different ways. >> >> however if we add it as CPU properties that behave the same way as other >> properties, all mgmt has to do is expose new property to user for usage. > > I think we need to be careful here. Sometimes just exposing the > QOM properties used to implemented a feature is not the best user > interface. e.g.: even if using compat_props for implementing the > hyperv features preset, that doesn't automatically mean we want > hyperv=on to be a -cpu option. > > I would even argue we shouldn't be focusing on implementation > details (like we are doing right now) until the desired external > interface is described clearly. I agree, the interface is definitely more important than the implementation here. AFAIU we have two options suggested: 1) 'hyperv=on' option for x86 machine types. Pros: we can use it later to create non-CPU Hyper-V devices (e.g. Vmbus). Cons: two different places for the currently existing Hyper-V features enablement (-cpu and -machine), non-standard way of doing things (code-wise). 2) 'hv_default=on' -cpu option Pros: Single place to enable all Hyper-V enlightenments, we can make it mutually exclusive with other hv_* options including hv_passthrough (clear semantics). Cons: This can't be reused to create non-CPU objects in the future and so upper layers will (again) need to be modified. There's probably more, please feel free to add. >> however in this case we are talking about a set of cpu features, >> if there is no way to implement it as cpu properties + compat properties >> and requires opencodding it within machine code it might be fine >> but I fail to see a very good reason for doing that at this momment. > > The reason would be just simplicity of implementation. > > I understand there are reasons to suggest using compat_props if > it makes things simpler, but I don't see why we would reject a > patch because the implementation is not based purely on > compat_props. > > I will let Vitaly to decide how to proceed, based on our > feedback. I encourage him to use compat_props like you suggest, > but I don't plan to make this a requirement. > Like I replied to Igor in a parallel thread, I hardly see how using compat_props can simplify things in case we decide to keep 'hyperv=on' a machine type option. It doesn't seem to fit our use-case when we need a mechanism to alter CPU properties for the current machine type as well as subtract some features for the old ones. If we, however, decide that '-cpu' option is better, then we can try to make it work (but the implementation won't be straitforward either).
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.
On Tue, 5 Jan 2021 09:34:31 -0500 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote: > > On Mon, 4 Jan 2021 13:29:06 -0500 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov 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. > > > > > > This is a good point, although having accel affect CPUID bits was > > > also a source of complexity for query-cpu-model-expansion and > > > other QMP queries. > > > > why was, it's still a headache (mutating CPU models depending on accelerator) > > > > > > > > > > > > > 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. > > > > > > I agree completely that the set of bits needs to be on > > > MachineClass. We just need to agree on the external interface. > > That's where I disagree, > > let me exaggerate for demo purpose: > > - let's move all CPU models feature defaults to MachineClass and forget about compat properties > > since in that case we can opencode changes in machine_class_init > > I don't see your point here. compat_props is also part of > MachineClass. they are but compat_props are data and we typically use them for keeping old behavior for devices, all it needs is adding a line to set old property value. While class_init is typically used for altering machine specific behavior, sure it can be used to patch up device but that's a bit more ugly (need to add a field to MachineClass to key off and the somehow wire it up to affected device). > > > > It's rather hard code integration between device models, which we try > > to avoid and still refactoring QEMU code to get rid of it. > > (sure it works until it's not and someone else need to rewrite half of QEMU > > to accomplish it's own task because we mixed things together) > > I don't see why using a X86CPU-specific API that is not based on > QOM properties is hard code integration. compat_props is not the > only allowed API for machines to communicate with devices. > > > > > > > > > > > > > > >> > > > > > >> > + 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. > > > > > > I'm all for not wasting time supporting use cases that are not > > > necessary in practice. We just need to document the expected > > > behavior clearly, whatever we decide to do. > > > > documenting is good, but if it adds new semantics to how CPU features are handled > > users up the stack will need code it up as well and juggle with > > -machine + -cpu + -device cpu-foo > > not to mention poor developers who will have to figure out why we do > > set CPU properties in multiple different ways. > > > > however if we add it as CPU properties that behave the same way as other > > properties, all mgmt has to do is expose new property to user for usage. > > I think we need to be careful here. Sometimes just exposing the > QOM properties used to implemented a feature is not the best user > interface. e.g.: even if using compat_props for implementing the > hyperv features preset, that doesn't automatically mean we want > hyperv=on to be a -cpu option. > > I would even argue we shouldn't be focusing on implementation > details (like we are doing right now) until the desired external > interface is described clearly. > > > > > it even more true when building machine from QMP interface would be available, > > where we would want '-device foo' more or less the same way instead of > > special casing some of them, i.e. I'd rather have one device to configure, > > instead of doing it in multiple places. It's not possible in reality > > but for new code we should try to minimize split brain issues. > > Is split brain a practical problem here? If the new behavior is > implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know > it's going to affect all CPU objects. i was talking about user interface here, i.e.: (QMP) create_machine(hyperv=on) (QMP) device_add(cpu, hv_foo=x) vs: (QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x) i.e. in the later case cpu specific options are consolidate within device stanza and mgmt doesn't need to be aware and split cpu config in to steps. > > > > > > > > > > > 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. > > > > > > I agree. Hyper-V is not just a set of CPU features. > > me too, > > however in this case we are talking about a set of cpu features, > > if there is no way to implement it as cpu properties + compat properties > > and requires opencodding it within machine code it might be fine > > but I fail to see a very good reason for doing that at this momment. > > The reason would be just simplicity of implementation. aside other issues, cpu props + compact_props looks simpler than machine based variant. > > I understand there are reasons to suggest using compat_props if > it makes things simpler, but I don't see why we would reject a > patch because the implementation is not based purely on > compat_props. main issue is that patch introduces new semantics to cpu feature parsing. compat_props is for consistency with how we typically handle device compatibility, which is also good enough reason. > I will let Vitaly to decide how to proceed, based on our > feedback. I encourage him to use compat_props like you suggest, > but I don't plan to make this a requirement. > > > > > > > > > Also, those two approaches are not mutually exclusive. > > > "-machine hyperv=on" can be implemented internally using > > > "hyperv-use-preset=on" if necessary. I don't think it has to, > > > however. > > > > >
Igor Mammedov <imammedo@redhat.com> writes: > On Tue, 05 Jan 2021 12:50:05 +0100 > > 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 > } > ... > 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 don't currently have any devices which are not 'CPU features' (in QEMU terminology), however, we already have Vmbus and I can easily imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We *may* want to enable these 'automatically' and that's what make '-machine' option preferable. It is, however, not a *must* right now and we can indeed wait until these devices appear and be happy with 'hv_default' -cpu option for now. We will, however, need to teach upper layers about the change when/if it happens.
On Tue, 05 Jan 2021 16:10:36 +0100 Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote: > >> > >> documenting is good, but if it adds new semantics to how CPU features are handled > >> users up the stack will need code it up as well and juggle with > >> -machine + -cpu + -device cpu-foo > >> not to mention poor developers who will have to figure out why we do > >> set CPU properties in multiple different ways. > >> > >> however if we add it as CPU properties that behave the same way as other > >> properties, all mgmt has to do is expose new property to user for usage. > > > > I think we need to be careful here. Sometimes just exposing the > > QOM properties used to implemented a feature is not the best user > > interface. e.g.: even if using compat_props for implementing the > > hyperv features preset, that doesn't automatically mean we want > > hyperv=on to be a -cpu option. > > > > I would even argue we shouldn't be focusing on implementation > > details (like we are doing right now) until the desired external > > interface is described clearly. > > I agree, the interface is definitely more important than the > implementation here. AFAIU we have two options suggested: > > 1) 'hyperv=on' option for x86 machine types. > > Pros: we can use it later to create non-CPU Hyper-V devices > (e.g. Vmbus). > Cons: two different places for the currently existing Hyper-V features > enablement (-cpu and -machine), non-standard way of doing things > (code-wise). > > 2) 'hv_default=on' -cpu option > > Pros: Single place to enable all Hyper-V enlightenments, we can make it > mutually exclusive with other hv_* options including hv_passthrough > (clear semantics). > > Cons: This can't be reused to create non-CPU objects in the future and > so upper layers will (again) need to be modified. > > There's probably more, please feel free to add. #1 can be implemented on top of #2, when it becomes necessary. > >> however in this case we are talking about a set of cpu features, > >> if there is no way to implement it as cpu properties + compat properties > >> and requires opencodding it within machine code it might be fine > >> but I fail to see a very good reason for doing that at this momment. > > > > The reason would be just simplicity of implementation. > > > > I understand there are reasons to suggest using compat_props if > > it makes things simpler, but I don't see why we would reject a > > patch because the implementation is not based purely on > > compat_props. > > > > I will let Vitaly to decide how to proceed, based on our > > feedback. I encourage him to use compat_props like you suggest, > > but I don't plan to make this a requirement. > > > > Like I replied to Igor in a parallel thread, I hardly see how using > compat_props can simplify things in case we decide to keep 'hyperv=on' a > machine type option. It doesn't seem to fit our use-case when we need a > mechanism to alter CPU properties for the current machine type as well > as subtract some features for the old ones. If we, however, decide that > '-cpu' option is better, then we can try to make it work (but the > implementation won't be straitforward either). lets discuss it in that thread.
Igor Mammedov <imammedo@redhat.com> writes: ... > > i was talking about user interface here, i.e.: > (QMP) create_machine(hyperv=on) > (QMP) device_add(cpu, hv_foo=x) > vs: > (QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x) > > i.e. in the later case cpu specific options are consolidate within device stanza > and mgmt doesn't need to be aware and split cpu config in to steps. > FWIW, 'hv_foo=x' only makes sense if 'x' == 'off' as currently we enable all existing enlightenments. Also, this requires upper layer tools to know something about 'hv_foo' (to be able to disable it) and AFAICT layers above libvirt don't actually want to know such low-level details. Don't get me wrong, I'm not against 'hv_defaults=on', just trying to give the perspective so we won't need to change the interface again anytime soon.
On Tue, Jan 05, 2021 at 05:31:41PM +0100, Igor Mammedov wrote: > On Tue, 5 Jan 2021 09:34:31 -0500 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Jan 05, 2021 at 12:36:50AM +0100, Igor Mammedov wrote: > > > On Mon, 4 Jan 2021 13:29:06 -0500 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Jan 04, 2021 at 01:54:32PM +0100, Vitaly Kuznetsov 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. > > > > > > > > This is a good point, although having accel affect CPUID bits was > > > > also a source of complexity for query-cpu-model-expansion and > > > > other QMP queries. > > > > > > why was, it's still a headache (mutating CPU models depending on accelerator) > > > > > > > > > > > > > > > > > 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. > > > > > > > > I agree completely that the set of bits needs to be on > > > > MachineClass. We just need to agree on the external interface. > > > That's where I disagree, > > > let me exaggerate for demo purpose: > > > - let's move all CPU models feature defaults to MachineClass and forget about compat properties > > > since in that case we can opencode changes in machine_class_init > > > > I don't see your point here. compat_props is also part of > > MachineClass. > they are but compat_props are data and we typically use them for > keeping old behavior for devices, all it needs is adding a line > to set old property value. > While class_init is typically used for altering machine specific > behavior, sure it can be used to patch up device but that's > a bit more ugly (need to add a field to MachineClass to key off > and the somehow wire it up to affected device). I was not excluding compat_props when I said "the set of bits needs to be on MachineClass", so I don't think we disagree in this specific point. (Except that I don't think non-compat_props solution will be necessarily ugly) > > > > > > > It's rather hard code integration between device models, which we try > > > to avoid and still refactoring QEMU code to get rid of it. > > > (sure it works until it's not and someone else need to rewrite half of QEMU > > > to accomplish it's own task because we mixed things together) > > > > I don't see why using a X86CPU-specific API that is not based on > > QOM properties is hard code integration. compat_props is not the > > only allowed API for machines to communicate with devices. > > > > > > > > > > > > > > > > > > > >> > > > > > > >> > + 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. > > > > > > > > I'm all for not wasting time supporting use cases that are not > > > > necessary in practice. We just need to document the expected > > > > behavior clearly, whatever we decide to do. > > > > > > documenting is good, but if it adds new semantics to how CPU features are handled > > > users up the stack will need code it up as well and juggle with > > > -machine + -cpu + -device cpu-foo > > > not to mention poor developers who will have to figure out why we do > > > set CPU properties in multiple different ways. > > > > > > however if we add it as CPU properties that behave the same way as other > > > properties, all mgmt has to do is expose new property to user for usage. > > > > I think we need to be careful here. Sometimes just exposing the > > QOM properties used to implemented a feature is not the best user > > interface. e.g.: even if using compat_props for implementing the > > hyperv features preset, that doesn't automatically mean we want > > hyperv=on to be a -cpu option. > > > > I would even argue we shouldn't be focusing on implementation > > details (like we are doing right now) until the desired external > > interface is described clearly. > > > > > > > > it even more true when building machine from QMP interface would be available, > > > where we would want '-device foo' more or less the same way instead of > > > special casing some of them, i.e. I'd rather have one device to configure, > > > instead of doing it in multiple places. It's not possible in reality > > > but for new code we should try to minimize split brain issues. > > > > Is split brain a practical problem here? If the new behavior is > > implemented in x86_cpu_realizefn() or x86_cpu_pre_plug(), we know > > it's going to affect all CPU objects. > > i was talking about user interface here, i.e.: > (QMP) create_machine(hyperv=on) > (QMP) device_add(cpu, hv_foo=x) > vs: > (QMP) device_add(cpu, hyperv_defaults=on,=onhv_foo=x) > > i.e. in the later case cpu specific options are consolidate within device stanza > and mgmt doesn't need to be aware and split cpu config in to steps. This might make sense for this feature. I just worry that one day we might need to make a machine option to affect CPUID feature flags, requiring us to make this work somehow. (If we decide to go with "-cpu hyperv=on", we can postpone that discussion, though) > > > > > > > > > > > > > > > > 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. > > > > > > > > I agree. Hyper-V is not just a set of CPU features. > > > me too, > > > however in this case we are talking about a set of cpu features, > > > if there is no way to implement it as cpu properties + compat properties > > > and requires opencodding it within machine code it might be fine > > > but I fail to see a very good reason for doing that at this momment. > > > > The reason would be just simplicity of implementation. > aside other issues, > cpu props + compact_props looks simpler than machine based variant. Possibly the compat_props solution will be simpler if we choose the "-cpu hyperv=on" path. I don't expect it to be simpler if we choose the "-machine hyperv=on" path. Maybe that's our main source of disagreement (which will go away if we go with "-cpu hyperv=on"). Both user interface approaches (-cpu -machine) look good enough to me as long as their behavior is documented and makes sense. Even better if they have automated test cases. > > > > > I understand there are reasons to suggest using compat_props if > > it makes things simpler, but I don't see why we would reject a > > patch because the implementation is not based purely on > > compat_props. > main issue is that patch introduces new semantics to cpu feature > parsing. > compat_props is for consistency with how we typically handle device > compatibility, which is also good enough reason. Is this point about the implementation, or about the user interface? > > > I will let Vitaly to decide how to proceed, based on our > > feedback. I encourage him to use compat_props like you suggest, > > but I don't plan to make this a requirement. > > > > > > > > > > > > > Also, those two approaches are not mutually exclusive. > > > > "-machine hyperv=on" can be implemented internally using > > > > "hyperv-use-preset=on" if necessary. I don't think it has to, > > > > however. > > > > > > > > >
On Tue, 05 Jan 2021 17:31:43 +0100 Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Tue, 05 Jan 2021 12:50:05 +0100 > > > > 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 > > } > > > > ... > > > 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 don't currently have any devices which are not 'CPU features' (in > QEMU terminology), however, we already have Vmbus and I can easily > imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We > *may* want to enable these 'automatically' and that's what make > '-machine' option preferable. It is, however, not a *must* right now and > we can indeed wait until these devices appear and be happy with > 'hv_default' -cpu option for now. We will, however, need to teach upper > layers about the change when/if it happens. which makes me think we are trying to bite something that we shouldn't. Do we really need this patch (QEMU knob) to magically enable subset of features and/or devices for a specific OS flavor? It's job of upper layers to abstract low level QEMU details in to coarse grained knobs (libvirt/virt-install/virt-manager/...). For example virt-install may know that it installing a specific Windows version, and can build a tailored for that OS configuration including needed hyperv CPU features and hyperv specific devices. (if I'm not mistaken libosinfo is used to get metadata for preferred configuration, so perhaps this should become a patch for that library and its direct users). What we actually lack is a documentation for preferred configuration in docs/hyperv.txt, currently it just enumerates possible features. We can just document a recommended 'best practices' there without putting it in QEMU code and let upper layers to do their job in the stack.
Igor Mammedov <imammedo@redhat.com> writes: > On Tue, 05 Jan 2021 17:31:43 +0100 > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> Igor Mammedov <imammedo@redhat.com> writes: >> >> > On Tue, 05 Jan 2021 12:50:05 +0100 >> > >> > 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 >> > } >> > >> >> ... >> >> > 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 don't currently have any devices which are not 'CPU features' (in >> QEMU terminology), however, we already have Vmbus and I can easily >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We >> *may* want to enable these 'automatically' and that's what make >> '-machine' option preferable. It is, however, not a *must* right now and >> we can indeed wait until these devices appear and be happy with >> 'hv_default' -cpu option for now. We will, however, need to teach upper >> layers about the change when/if it happens. > > which makes me think we are trying to bite something that we shouldn't. > Do we really need this patch (QEMU knob) to magically enable subset of > features and/or devices for a specific OS flavor? > > It's job of upper layers to abstract low level QEMU details in to coarse > grained knobs (libvirt/virt-install/virt-manager/...). > For example virt-install may know that it installing a specific Windows > version, and can build a tailored for that OS configuration including > needed hyperv CPU features and hyperv specific devices. > (if I'm not mistaken libosinfo is used to get metadata for preferred > configuration, so perhaps this should become a patch for that library > and its direct users). > > What we actually lack is a documentation for preferred configuration > in docs/hyperv.txt, currently it just enumerates possible features. > We can just document a recommended 'best practices' there without > putting it in QEMU code and let upper layers to do their job in > the stack. The problem we're facing here is that when a new enlightenment is implemented it takes forever to propagate to the whole stack. We don't have any different recommendations for different Windows versions, neither does genuine Hyper-V. The 'fine grained' mechanis we have just contributes to the creation of various Frankenstein configurations (which look nothing like real Hyper-V), people just google for 'Windows KVM slow', add something to their scripts and this keeps propagating. Every time I see a configuration with only a few 'hv_*' options I ask 'why don't you enable the rest?' and I'm yet to receive an answer different from 'hm, I don't know, I copied it from somewhere and it worked'. Setting 'hv_*' options individually should be considered debug only.
On Wed, 06 Jan 2021 14:38:56 +0100 Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Tue, 05 Jan 2021 17:31:43 +0100 > > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > > >> Igor Mammedov <imammedo@redhat.com> writes: > >> > >> > On Tue, 05 Jan 2021 12:50:05 +0100 > >> > > >> > 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 > >> > } > >> > > >> > >> ... > >> > >> > 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 don't currently have any devices which are not 'CPU features' (in > >> QEMU terminology), however, we already have Vmbus and I can easily > >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We > >> *may* want to enable these 'automatically' and that's what make > >> '-machine' option preferable. It is, however, not a *must* right now and > >> we can indeed wait until these devices appear and be happy with > >> 'hv_default' -cpu option for now. We will, however, need to teach upper > >> layers about the change when/if it happens. > > > > which makes me think we are trying to bite something that we shouldn't. > > Do we really need this patch (QEMU knob) to magically enable subset of > > features and/or devices for a specific OS flavor? > > > > It's job of upper layers to abstract low level QEMU details in to coarse > > grained knobs (libvirt/virt-install/virt-manager/...). > > For example virt-install may know that it installing a specific Windows > > version, and can build a tailored for that OS configuration including > > needed hyperv CPU features and hyperv specific devices. > > (if I'm not mistaken libosinfo is used to get metadata for preferred > > configuration, so perhaps this should become a patch for that library > > and its direct users). > > > > What we actually lack is a documentation for preferred configuration > > in docs/hyperv.txt, currently it just enumerates possible features. > > We can just document a recommended 'best practices' there without > > putting it in QEMU code and let upper layers to do their job in > > the stack. > > The problem we're facing here is that when a new enlightenment is > implemented it takes forever to propagate to the whole stack. We don't It's true not only for Hyper-V, I guess it's price to pay for modular solution. > have any different recommendations for different Windows versions, > neither does genuine Hyper-V. The 'fine grained' mechanis we have just > contributes to the creation of various Frankenstein configurations > (which look nothing like real Hyper-V), people just google for 'Windows > KVM slow', add something to their scripts and this keeps propagating. That's why I mentioned lack of documentation. If someone manually configures QEMU, one should understand what they do enable and why or enlist help of virt-install and likes. > Every time I see a configuration with only a few 'hv_*' options I ask > 'why don't you enable the rest?' and I'm yet to receive an answer > different from 'hm, I don't know, I copied it from somewhere and it > worked'. If individual features are are composed by virt-install or other tools based on libosinfo data, then we don't have to maintain versioning of new default_set_features per machine type, which will only become worse if we include hv specific devices into it. Also with libosinfo approach, old machine types and old QEMU versions can also benefit from it without need to change whole stack. And no versioning is necessary since chosen config set is stored in domain XML at the moment VM is created. > Setting 'hv_*' options individually should be considered debug only. that's how cpu's features were designed, a helper knob on top is fine as long as it doesn't mess the way it used to work and preferably is build on top of existing features. PS: another wild idea how to implement it using '-machine hyperv=on', based on compat props idea: // replaces bit set in your version hv_default_set[] = "hv_feat1", "hv_feat2", ... }; // probably should be done before -cpu is parsed then if machine hyperv=on foreach in hv_default_set[] object_register_sugar_prop(hv_default_set[i], "on") PS2: my preferred approach is still -cpu hyperv=on, since it doesn't depend on order CLI is currently parsed (which is fragile thing), but rather on what user asked us to do with CPU.
On Wed, Jan 06, 2021 at 02:38:56PM +0100, Vitaly Kuznetsov wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > On Tue, 05 Jan 2021 17:31:43 +0100 > > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > > >> Igor Mammedov <imammedo@redhat.com> writes: > >> > >> > On Tue, 05 Jan 2021 12:50:05 +0100 > >> > > >> > 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 > >> > } > >> > > >> > >> ... > >> > >> > 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 don't currently have any devices which are not 'CPU features' (in > >> QEMU terminology), however, we already have Vmbus and I can easily > >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We > >> *may* want to enable these 'automatically' and that's what make > >> '-machine' option preferable. It is, however, not a *must* right now and > >> we can indeed wait until these devices appear and be happy with > >> 'hv_default' -cpu option for now. We will, however, need to teach upper > >> layers about the change when/if it happens. > > > > which makes me think we are trying to bite something that we shouldn't. > > Do we really need this patch (QEMU knob) to magically enable subset of > > features and/or devices for a specific OS flavor? I think we really want this, yes. It's not for a specific OS flavor, it is just a machine feature. > > > > It's job of upper layers to abstract low level QEMU details in to coarse > > grained knobs (libvirt/virt-install/virt-manager/...). > > For example virt-install may know that it installing a specific Windows > > version, and can build a tailored for that OS configuration including > > needed hyperv CPU features and hyperv specific devices. > > (if I'm not mistaken libosinfo is used to get metadata for preferred > > configuration, so perhaps this should become a patch for that library > > and its direct users). virt-install/libosinfo/etc can be used to enable a feature automatically, but the coarse grained knob may be provided by QEMU. > > > > What we actually lack is a documentation for preferred configuration > > in docs/hyperv.txt, currently it just enumerates possible features. > > We can just document a recommended 'best practices' there without > > putting it in QEMU code and let upper layers to do their job in > > the stack. > > The problem we're facing here is that when a new enlightenment is > implemented it takes forever to propagate to the whole stack. We don't > have any different recommendations for different Windows versions, > neither does genuine Hyper-V. The 'fine grained' mechanis we have just > contributes to the creation of various Frankenstein configurations > (which look nothing like real Hyper-V), people just google for 'Windows > KVM slow', add something to their scripts and this keeps propagating. Exactly. Requiring new code to be added to all other components in the stack every time we add a low level feature to KVM or QEMU is not working. It's even worse when we require users to manually update their configurations with low level bits. > > Every time I see a configuration with only a few 'hv_*' options I ask > 'why don't you enable the rest?' and I'm yet to receive an answer > different from 'hm, I don't know, I copied it from somewhere and it > worked'. > > Setting 'hv_*' options individually should be considered debug only. They can also be useful in production to work around unexpected issues (not just debugging). I don't think we should prevent other layers from controlling low level knobs. We just shouldn't make the low level knobs necessary for making the feature work.
On Wed, Jan 06, 2021 at 05:45:42PM +0100, Igor Mammedov wrote: > On Wed, 06 Jan 2021 14:38:56 +0100 > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > > Igor Mammedov <imammedo@redhat.com> writes: > > > > > On Tue, 05 Jan 2021 17:31:43 +0100 > > > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > > > > >> Igor Mammedov <imammedo@redhat.com> writes: > > >> > > >> > On Tue, 05 Jan 2021 12:50:05 +0100 > > >> > > > >> > 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 > > >> > } > > >> > > > >> > > >> ... > > >> > > >> > 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 don't currently have any devices which are not 'CPU features' (in > > >> QEMU terminology), however, we already have Vmbus and I can easily > > >> imagine us implementing e.g. hartbeat/kvp/vss/... devices on top. We > > >> *may* want to enable these 'automatically' and that's what make > > >> '-machine' option preferable. It is, however, not a *must* right now and > > >> we can indeed wait until these devices appear and be happy with > > >> 'hv_default' -cpu option for now. We will, however, need to teach upper > > >> layers about the change when/if it happens. > > > > > > which makes me think we are trying to bite something that we shouldn't. > > > Do we really need this patch (QEMU knob) to magically enable subset of > > > features and/or devices for a specific OS flavor? > > > > > > It's job of upper layers to abstract low level QEMU details in to coarse > > > grained knobs (libvirt/virt-install/virt-manager/...). > > > For example virt-install may know that it installing a specific Windows > > > version, and can build a tailored for that OS configuration including > > > needed hyperv CPU features and hyperv specific devices. > > > (if I'm not mistaken libosinfo is used to get metadata for preferred > > > configuration, so perhaps this should become a patch for that library > > > and its direct users). > > > > > > What we actually lack is a documentation for preferred configuration > > > in docs/hyperv.txt, currently it just enumerates possible features. > > > We can just document a recommended 'best practices' there without > > > putting it in QEMU code and let upper layers to do their job in > > > the stack. > > > > The problem we're facing here is that when a new enlightenment is > > implemented it takes forever to propagate to the whole stack. We don't > It's true not only for Hyper-V, I guess it's price to pay for modular solution. Yes, this discussion applies to other features as well. > > > have any different recommendations for different Windows versions, > > neither does genuine Hyper-V. The 'fine grained' mechanis we have just > > contributes to the creation of various Frankenstein configurations > > (which look nothing like real Hyper-V), people just google for 'Windows > > KVM slow', add something to their scripts and this keeps propagating. > That's why I mentioned lack of documentation. > If someone manually configures QEMU, one should understand what they do > enable and why or enlist help of virt-install and likes. Why? QEMU's lack of usability is an unfortunate accident, not a desirable goal. > > > Every time I see a configuration with only a few 'hv_*' options I ask > > 'why don't you enable the rest?' and I'm yet to receive an answer > > different from 'hm, I don't know, I copied it from somewhere and it > > worked'. > > If individual features are are composed by virt-install or other tools > based on libosinfo data, then we don't have to maintain versioning > of new default_set_features per machine type, which will only become > worse if we include hv specific devices into it. Versioning is extra work for us QEMU developers, but it has a purpose. It saves everybody else's valuable time. > > Also with libosinfo approach, old machine types and old QEMU versions > can also benefit from it without need to change whole stack. Except that you need to update the whole stack (QEMU + libvirt + libosinfo + the glue code between libosinfo and libvirt) every time a new feature is available. This is unnecessary overhead, and this is not working. > And no versioning is necessary since chosen config set is stored in > domain XML at the moment VM is created. I don't even think that is a good thing. I would agree completely with you if the people maintaining the upper layers were asking us to just let them manage low level details of guest ABI. They are not. > > > Setting 'hv_*' options individually should be considered debug only. > that's how cpu's features were designed, a helper knob on top is fine > as long as it doesn't mess the way it used to work and preferably is > build on top of existing features. > > PS: > another wild idea how to implement it using '-machine hyperv=on', > based on compat props idea: > > // replaces bit set in your version > hv_default_set[] = > "hv_feat1", "hv_feat2", > ... > }; > > // probably should be done before -cpu is parsed > then if machine hyperv=on > foreach in hv_default_set[] > object_register_sugar_prop(hv_default_set[i], "on") This sounds interesting. > > PS2: > my preferred approach is still -cpu hyperv=on, since it doesn't > depend on order CLI is currently parsed (which is fragile thing), > but rather on what user asked us to do with CPU.
Igor Mammedov wrote: > my preferred approach is still -cpu hyperv=on, since it doesn't > depend on order CLI is currently parsed (which is fragile thing), > but rather on what user asked us to do with CPU. I think I'm OK with this solution for the time being. When non-CPU devices arrive and if we decide that it is a good idea to have them enabled by default, we can make -machine hyperv=on option implying 'hv_default' CPU option. The real benefit I see from -cpu option is simplification of debug configurations (to find out what would happen if certain enlightenments are disabled) and making it possible to use 'hv_default' with older kernels lacking some enlightenments (by disabling them). Not that this is impossible with -machine option, just not very straitforward ('-cpu host,hv-default,hv-evmcs=off' vs '-machine q35,hyperv=on -cpu host,hv-evmcs=off'). I'll send out v3 shortly and I'll include patches from "i386: KVM: expand Hyper-V features early" which were waiting for Linux-5.11 merge window. Eduardo Habkost <ehabkost@redhat.com> writes: >> > On Tue, 05 Jan 2021 17:31:43 +0100 >> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Every time I see a configuration with only a few 'hv_*' options I ask >> 'why don't you enable the rest?' and I'm yet to receive an answer >> different from 'hm, I don't know, I copied it from somewhere and it >> worked'. >> >> Setting 'hv_*' options individually should be considered debug only. > > They can also be useful in production to work around > unexpected issues (not just debugging). > Right, by 'debugging' I meant 'dealing with issues' :-) > I don't think we should prevent other layers from controlling low > level knobs. We just shouldn't make the low level knobs > necessary for making the feature work. Let me give an exaggerated example. Why do we have named cpu models (e.g. 'Skylake')? We could've exposed basic CPU model and all the knobs to upper layers of the stack to deal with. I don't even want to imagine the chaos this would've created. Low level knobs are necessary when issues arise (e.g. it's easy to ask 'could you please try "-cpu Skylake,-vmx" and see what happens?') but mandating that upper layers (btw, all of them -- we don't have 'one libvirt to rule them all') have to set them is non-practical IMO.
diff --git a/docs/hyperv.txt b/docs/hyperv.txt index 5df00da54fc4..1a76a07f8417 100644 --- a/docs/hyperv.txt +++ b/docs/hyperv.txt @@ -29,6 +29,14 @@ When any set of the Hyper-V enlightenments is enabled, QEMU changes hypervisor identification (CPUID 0x40000000..0x4000000A) to Hyper-V. KVM identification and features are kept in leaves 0x40000100..0x40000101. +Hyper-V enlightenments can be enabled in bulk by specifying 'hyperv=on' to an +x86 machine type: + + qemu-system-x86_64 -machine q35,accel=kvm,kernel-irqchip=split,hyperv=on ... + +Note, new enlightenments are only added to the latest (in-develompent) machine +type, older machine types keep the list of the supported features intact to +safeguard migration. 3. Existing enlightenments =========================== diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 5944fc44edca..57f27d56ecc6 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -1171,6 +1171,20 @@ static void x86_machine_set_acpi(Object *obj, Visitor *v, const char *name, visit_type_OnOffAuto(v, name, &x86ms->acpi, errp); } +static bool x86_machine_get_hyperv(Object *obj, Error **errp) +{ + X86MachineState *x86ms = X86_MACHINE(obj); + + return x86ms->hyperv_enabled; +} + +static void x86_machine_set_hyperv(Object *obj, bool value, Error **errp) +{ + X86MachineState *x86ms = X86_MACHINE(obj); + + x86ms->hyperv_enabled = value; +} + static void x86_machine_initfn(Object *obj) { X86MachineState *x86ms = X86_MACHINE(obj); @@ -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); + object_class_property_add(oc, X86_MACHINE_SMM, "OnOffAuto", x86_machine_get_smm, x86_machine_set_smm, NULL, NULL); @@ -1205,6 +1229,12 @@ static void x86_machine_class_init(ObjectClass *oc, void *data) NULL, NULL); object_class_property_set_description(oc, X86_MACHINE_ACPI, "Enable ACPI"); + + object_class_property_add_bool(oc, X86_MACHINE_HYPERV, + x86_machine_get_hyperv, x86_machine_set_hyperv); + + object_class_property_set_description(oc, X86_MACHINE_HYPERV, + "Enable Hyper-V enlightenments"); } static const TypeInfo x86_machine_info = { diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h index 739fac50871b..598abd1be806 100644 --- a/include/hw/i386/x86.h +++ b/include/hw/i386/x86.h @@ -38,6 +38,9 @@ struct X86MachineClass { bool save_tsc_khz; /* Enables contiguous-apic-ID mode */ bool compat_apic_id_mode; + + /* Hyper-V features enabled with 'hyperv=on' */ + uint64_t default_hyperv_features; }; struct X86MachineState { @@ -71,10 +74,14 @@ struct X86MachineState { * will be translated to MSI messages in the address space. */ AddressSpace *ioapic_as; + + /* Hyper-V emulation */ + bool hyperv_enabled; }; #define X86_MACHINE_SMM "smm" #define X86_MACHINE_ACPI "acpi" +#define X86_MACHINE_HYPERV "hyperv" #define TYPE_X86_MACHINE MACHINE_TYPE_NAME("x86") OBJECT_DECLARE_TYPE(X86MachineState, X86MachineClass, X86_MACHINE) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 83aca942d87c..63a931679d73 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -53,6 +53,7 @@ #include "sysemu/tcg.h" #include "hw/qdev-properties.h" #include "hw/i386/topology.h" +#include "hw/i386/x86.h" #ifndef CONFIG_USER_ONLY #include "exec/address-spaces.h" #include "hw/i386/apic_internal.h" @@ -6511,8 +6512,21 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) 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; + } + /* Hyper-V vendor id */ if (!cpu->hyperv_vendor) { memcpy(cpu->hyperv_vendor_id, "Microsoft Hv", 12);