[v3,18/19] i386: provide simple 'hv-default=on' option

Commit Message

Vitaly Kuznetsov Jan. 7, 2021, 3:14 p.m. UTC

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 'hv-passthrough' mode enabling
everything but it can't be used in production as it prevents migration.

Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
Hyper-V enlightenments. Later, when new enlightenments get implemented,
compat_props mechanism will be used to disable them for legacy machine types,
this will keep 'hv-default=on' configurations migratable.

Signed-off-by: Vitaly Kuznetsov <[e-mail]>
---
 docs/hyperv.txt   | 16 +++++++++++++---
 target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
 target/i386/cpu.h |  5 +++++
 3 files changed, 56 insertions(+), 3 deletions(-)

Comments

Igor Mammedov Jan. 15, 2021, 2:11 a.m. UTC | #1

On Thu,  7 Jan 2021 16:14:49 +0100
Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> everything but it can't be used in production as it prevents migration.
> 
> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> compat_props mechanism will be used to disable them for legacy machine types,
> this will keep 'hv-default=on' configurations migratable.
> 
> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> ---
>  docs/hyperv.txt   | 16 +++++++++++++---
>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
>  target/i386/cpu.h |  5 +++++
>  3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> index 5df00da54fc4..a54c066cab09 100644
> --- a/docs/hyperv.txt
> +++ b/docs/hyperv.txt
> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
>  
>  2. Setup
>  =========
> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> +All currently supported Hyper-V enlightenments can be enabled by specifying
> +'hv-default=on' CPU flag:
>  
> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> +
> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> +e.g:
> +
> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...

I'd put here not '...' but rather recommended list of flags, and update
it every time when new feature added if necessary.

(not to mention that if we had it to begin with, then new 'hv-default' won't
be necessary, I still see it as functionality duplication but I will not oppose it)


> +It is also possible to disable individual enlightenments from the default list,
> +this can be used for debugging purposes:
> +
> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
>  
>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
>  check that the supplied configuration is sane.
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 48007a876e32..99338de00f78 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
>  }
>  
> +static bool x86_hv_default_get(Object *obj, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +
> +    return cpu->hyperv_default;
> +}
> +
> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +
> +    cpu->hyperv_default = value;
> +
> +    if (value) {
> +        cpu->hyperv_features |= cpu->hyperv_default_features;

s/|="/=/ please,
i.e. no option overrides whatever was specified before to keep semantics consistent.

> +    }
> +}
> +
>  /* Generic getter for "feature-words" and "filtered-features" properties */
>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
>                                        const char *name, void *opaque,
> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
>  
>      if (xcc->model) {
>          x86_cpu_load_model(cpu, xcc->model);
>      }
> +
> +    /* Hyper-V features enabled with 'hv-default=on' */
> +    cpu->hyperv_default_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_IPI) |
> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
> +
> +    /* Enlightened VMCS is only available on Intel/VMX */
> +    if (kvm_hv_evmcs_available()) {
> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
> +    }
what if VVM is migrated to another host without evmcs,
will it change CPUID?

>  }
>  
>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
> @@ -7285,6 +7319,10 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>                                x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
>  #endif
>  
> +    object_class_property_add_bool(oc, "hv-default",
> +                              x86_hv_default_get,
> +                              x86_hv_default_set);
> +
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          int bitnr;
>          for (bitnr = 0; bitnr < 64; bitnr++) {
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 6220cb2cabb9..8a484becb6b9 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1657,6 +1657,11 @@ struct X86CPU {
>      bool hyperv_synic_kvm_only;
>      uint64_t hyperv_features;
>      bool hyperv_passthrough;
> +
> +    /* 'hv-default' enablement */
> +    uint64_t hyperv_default_features;
> +    bool hyperv_default;
> +
>      OnOffAuto hyperv_no_nonarch_cs;
>      uint32_t hyperv_vendor_id[3];
>      uint32_t hyperv_interface_id[4];

Vitaly Kuznetsov Jan. 15, 2021, 9:20 a.m. UTC | #2

Igor Mammedov <[e-mail]> writes:

> On Thu,  7 Jan 2021 16:14:49 +0100
> Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
>> everything but it can't be used in production as it prevents migration.
>> 
>> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
>> Hyper-V enlightenments. Later, when new enlightenments get implemented,
>> compat_props mechanism will be used to disable them for legacy machine types,
>> this will keep 'hv-default=on' configurations migratable.
>> 
>> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
>> ---
>>  docs/hyperv.txt   | 16 +++++++++++++---
>>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
>>  target/i386/cpu.h |  5 +++++
>>  3 files changed, 56 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> index 5df00da54fc4..a54c066cab09 100644
>> --- a/docs/hyperv.txt
>> +++ b/docs/hyperv.txt
>> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
>>  
>>  2. Setup
>>  =========
>> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
>> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
>> +All currently supported Hyper-V enlightenments can be enabled by specifying
>> +'hv-default=on' CPU flag:
>>  
>> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
>> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
>> +
>> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
>> +e.g:
>> +
>> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...
>
> I'd put here not '...' but rather recommended list of flags, and update
> it every time when new feature added if necessary.
>

This is an example of fine-grained enablement, there is no point to put
all the existing flags there (hv-default is the only recommended way
now, the rest is 'expert'/'debugging').

> (not to mention that if we had it to begin with, then new 'hv-default' won't
> be necessary, I still see it as functionality duplication but I will not oppose it)
>

Unfortunately, upper layer tools don't read this doc and update
themselves to enable new features when they appear. Similarly, if when
these tools use '-machine q35' they get all the new features we add
automatically, right?

>
>> +It is also possible to disable individual enlightenments from the default list,
>> +this can be used for debugging purposes:
>> +
>> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
>>  
>>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
>>  check that the supplied configuration is sane.
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 48007a876e32..99338de00f78 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
>>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
>>  }
>>  
>> +static bool x86_hv_default_get(Object *obj, Error **errp)
>> +{
>> +    X86CPU *cpu = X86_CPU(obj);
>> +
>> +    return cpu->hyperv_default;
>> +}
>> +
>> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
>> +{
>> +    X86CPU *cpu = X86_CPU(obj);
>> +
>> +    cpu->hyperv_default = value;
>> +
>> +    if (value) {
>> +        cpu->hyperv_features |= cpu->hyperv_default_features;
>
> s/|="/=/ please,
> i.e. no option overrides whatever was specified before to keep semantics consistent.
>

Hm,

this doesn't matter for the most recent machine type as
hyperv_default_features has all the features but imagine you're running
an older machine type which doesn't have 'hv_feature'. Now your
suggestion is 

if I do:

'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"

but if I do

'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
(as hv_default enablement will overwrite everything)

How is this consistent?

>> +    }
>> +}
>> +
>>  /* Generic getter for "feature-words" and "filtered-features" properties */
>>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
>>                                        const char *name, void *opaque,
>> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
>>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
>>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
>>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
>> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
>>  
>>      if (xcc->model) {
>>          x86_cpu_load_model(cpu, xcc->model);
>>      }
>> +
>> +    /* Hyper-V features enabled with 'hv-default=on' */
>> +    cpu->hyperv_default_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_IPI) |
>> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
>> +
>> +    /* Enlightened VMCS is only available on Intel/VMX */
>> +    if (kvm_hv_evmcs_available()) {
>> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
>> +    }
> what if VVM is migrated to another host without evmcs,
> will it change CPUID?
>

Evmcs is tightly coupled with VMX, we can't migrate when it's not
there.

>>  }
>>  
>>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
>> @@ -7285,6 +7319,10 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>>                                x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
>>  #endif
>>  
>> +    object_class_property_add_bool(oc, "hv-default",
>> +                              x86_hv_default_get,
>> +                              x86_hv_default_set);
>> +
>>      for (w = 0; w < FEATURE_WORDS; w++) {
>>          int bitnr;
>>          for (bitnr = 0; bitnr < 64; bitnr++) {
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 6220cb2cabb9..8a484becb6b9 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1657,6 +1657,11 @@ struct X86CPU {
>>      bool hyperv_synic_kvm_only;
>>      uint64_t hyperv_features;
>>      bool hyperv_passthrough;
>> +
>> +    /* 'hv-default' enablement */
>> +    uint64_t hyperv_default_features;
>> +    bool hyperv_default;
>> +
>>      OnOffAuto hyperv_no_nonarch_cs;
>>      uint32_t hyperv_vendor_id[3];
>>      uint32_t hyperv_interface_id[4];
>

Igor Mammedov Jan. 20, 2021, 1:13 p.m. UTC | #3

On Fri, 15 Jan 2021 10:20:23 +0100
Vitaly Kuznetsov <[e-mail]> wrote:

> Igor Mammedov <[e-mail]> writes:
> 
> > On Thu,  7 Jan 2021 16:14:49 +0100
> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> >> everything but it can't be used in production as it prevents migration.
> >> 
> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> >> compat_props mechanism will be used to disable them for legacy machine types,
> >> this will keep 'hv-default=on' configurations migratable.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> >> ---
> >>  docs/hyperv.txt   | 16 +++++++++++++---
> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>  target/i386/cpu.h |  5 +++++
> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> >> index 5df00da54fc4..a54c066cab09 100644
> >> --- a/docs/hyperv.txt
> >> +++ b/docs/hyperv.txt
> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> >>  
> >>  2. Setup
> >>  =========
> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> >> +'hv-default=on' CPU flag:
> >>  
> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> >> +
> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> >> +e.g:
> >> +
> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...  
> >
> > I'd put here not '...' but rather recommended list of flags, and update
> > it every time when new feature added if necessary.
> >  

1)
 
> This is an example of fine-grained enablement, there is no point to put
> all the existing flags there (hv-default is the only recommended way
> now, the rest is 'expert'/'debugging').
so users are kept in dark what hv-default disables/enables (and it might depend
on machine version on top that). Doesn't look like a good documentation to me
(sure everyone can go and read source code for it and try to figure out how
it's supposed to work)

>
> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > be necessary, I still see it as functionality duplication but I will not oppose it)
> >  
> 
> Unfortunately, upper layer tools don't read this doc and update
> themselves to enable new features when they appear.
rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
why stop there, just merge with yet another upper layer, it would save us a lot
on communication protocols and simplify VM creation even more,
and no one will have to read docs and write anything new on top.)
There should be limit somewhere, where QEMU job ends and others pile hw abstraction
layers on top of it.

> Similarly, if when these tools use '-machine q35' they get all the new features we add
> automatically, right?
it depends, in case of CPUs, new features usually 'off' by default
for existing models. In case of bugs, features sometimes could be
flipped and versioned machines were used to keep broken CPU models
on old machine types.

   
> >> +It is also possible to disable individual enlightenments from the default list,
> >> +this can be used for debugging purposes:
> >> +
> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> >>  
> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> >>  check that the supplied configuration is sane.
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index 48007a876e32..99338de00f78 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> >>  }
> >>  
> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> >> +{
> >> +    X86CPU *cpu = X86_CPU(obj);
> >> +
> >> +    return cpu->hyperv_default;
> >> +}
> >> +
> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> >> +{
> >> +    X86CPU *cpu = X86_CPU(obj);
> >> +
> >> +    cpu->hyperv_default = value;
> >> +
> >> +    if (value) {
> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;  
> >
> > s/|="/=/ please,
> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> >  
> 
> Hm,
> 

> this doesn't matter for the most recent machine type as
> hyperv_default_features has all the features but imagine you're running
> an older machine type which doesn't have 'hv_feature'. Now your
normally one shouldn't use new feature with old machine type as it makes
VM non-migratable to older QEMU that has this machine type but not this feature.

nitpicking:
  according to (1) user should not use 'hv_feature' on old machine since
  hv_default should cover all their needs (well they don't know what hv_default actually is).

> suggestion is 
> 
> if I do:
> 
> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> 
> but if I do
> 
> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> (as hv_default enablement will overwrite everything)
> 
> How is this consistent?
usual semantics for properties, is that the latest property overwrites,
the previous property value parsed from left to right.
(i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
if one needs more than that one should add more related features after that.


> >> +    }
> >> +}
> >> +
> >>  /* Generic getter for "feature-words" and "filtered-features" properties */
> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> >>                                        const char *name, void *opaque,
> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
> >>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
> >>  
> >>      if (xcc->model) {
> >>          x86_cpu_load_model(cpu, xcc->model);
> >>      }
> >> +
> >> +    /* Hyper-V features enabled with 'hv-default=on' */
> >> +    cpu->hyperv_default_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_IPI) |
> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
> >> +
> >> +    /* Enlightened VMCS is only available on Intel/VMX */
> >> +    if (kvm_hv_evmcs_available()) {
> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
> >> +    }  
> > what if VVM is migrated to another host without evmcs,
> > will it change CPUID?
> >  
> 
> Evmcs is tightly coupled with VMX, we can't migrate when it's not
> there.

Are you saying mgmt will check and refuse to migrate to such host?

> 
> >>  }
> >>  
> >>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
> >> @@ -7285,6 +7319,10 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> >>                                x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
> >>  #endif
> >>  
> >> +    object_class_property_add_bool(oc, "hv-default",
> >> +                              x86_hv_default_get,
> >> +                              x86_hv_default_set);
> >> +
> >>      for (w = 0; w < FEATURE_WORDS; w++) {
> >>          int bitnr;
> >>          for (bitnr = 0; bitnr < 64; bitnr++) {
> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >> index 6220cb2cabb9..8a484becb6b9 100644
> >> --- a/target/i386/cpu.h
> >> +++ b/target/i386/cpu.h
> >> @@ -1657,6 +1657,11 @@ struct X86CPU {
> >>      bool hyperv_synic_kvm_only;
> >>      uint64_t hyperv_features;
> >>      bool hyperv_passthrough;
> >> +
> >> +    /* 'hv-default' enablement */
> >> +    uint64_t hyperv_default_features;
> >> +    bool hyperv_default;
> >> +
> >>      OnOffAuto hyperv_no_nonarch_cs;
> >>      uint32_t hyperv_vendor_id[3];
> >>      uint32_t hyperv_interface_id[4];  
> >  
>

Vitaly Kuznetsov Jan. 20, 2021, 2:38 p.m. UTC | #4

Igor Mammedov <[e-mail]> writes:

> On Fri, 15 Jan 2021 10:20:23 +0100
> Vitaly Kuznetsov <[e-mail]> wrote:
>
>> Igor Mammedov <[e-mail]> writes:
>> 
>> > On Thu,  7 Jan 2021 16:14:49 +0100
>> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
>> >> everything but it can't be used in production as it prevents migration.
>> >> 
>> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
>> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
>> >> compat_props mechanism will be used to disable them for legacy machine types,
>> >> this will keep 'hv-default=on' configurations migratable.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
>> >> ---
>> >>  docs/hyperv.txt   | 16 +++++++++++++---
>> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
>> >>  target/i386/cpu.h |  5 +++++
>> >>  3 files changed, 56 insertions(+), 3 deletions(-)
>> >> 
>> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> >> index 5df00da54fc4..a54c066cab09 100644
>> >> --- a/docs/hyperv.txt
>> >> +++ b/docs/hyperv.txt
>> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
>> >>  
>> >>  2. Setup
>> >>  =========
>> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
>> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
>> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
>> >> +'hv-default=on' CPU flag:
>> >>  
>> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
>> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
>> >> +
>> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
>> >> +e.g:
>> >> +
>> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...  
>> >
>> > I'd put here not '...' but rather recommended list of flags, and update
>> > it every time when new feature added if necessary.
>> >  
>
> 1)
>  
>> This is an example of fine-grained enablement, there is no point to put
>> all the existing flags there (hv-default is the only recommended way
>> now, the rest is 'expert'/'debugging').
> so users are kept in dark what hv-default disables/enables (and it might depend
> on machine version on top that). Doesn't look like a good documentation to me
> (sure everyone can go and read source code for it and try to figure out how
> it's supposed to work)

'hv-default' enables *all* currently supported enlightenments. When
using with an old machine type, it will enable *all* Hyper-V
enlightenmnets which were supported when the corresponding machine type
was released. I don't think we document all other cases when a machine
type is modified (i.e. where can I read how pc-q35-5.1 is different from
pc-q35-5.0 if I refuse to read the source code?)

>
>>
>> > (not to mention that if we had it to begin with, then new 'hv-default' won't
>> > be necessary, I still see it as functionality duplication but I will not oppose it)
>> >  
>> 
>> Unfortunately, upper layer tools don't read this doc and update
>> themselves to enable new features when they appear.
> rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> why stop there, just merge with yet another upper layer, it would save us a lot
> on communication protocols and simplify VM creation even more,
> and no one will have to read docs and write anything new on top.)
> There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> layers on top of it.

We have '-machine q35' and we don't require to list all the devices from
it. We have '-cpu Skylake-Server' and we don't require to configure all
the features manually. Why can't we have similar enablement for Hyper-V
emulation where we can't even see a real need for anything but 'enable
everything' option?

There is no 'one libvirt to rule them all' (fortunately or
unfortunately). And sometimes QEMU is the uppermost layer and there's no
'libvirt' on top of it, this is also a perfectly valid use-case.

>
>> Similarly, if when these tools use '-machine q35' they get all the new features we add
>> automatically, right?
> it depends, in case of CPUs, new features usually 'off' by default
> for existing models. In case of bugs, features sometimes could be
> flipped and versioned machines were used to keep broken CPU models
> on old machine types.
>

That's why I was saying that Hyper-V enlightenments hardly resemble
'hardware' CPU features.

>    
>> >> +It is also possible to disable individual enlightenments from the default list,
>> >> +this can be used for debugging purposes:
>> >> +
>> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
>> >>  
>> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
>> >>  check that the supplied configuration is sane.
>> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> >> index 48007a876e32..99338de00f78 100644
>> >> --- a/target/i386/cpu.c
>> >> +++ b/target/i386/cpu.c
>> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
>> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
>> >>  }
>> >>  
>> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
>> >> +{
>> >> +    X86CPU *cpu = X86_CPU(obj);
>> >> +
>> >> +    return cpu->hyperv_default;
>> >> +}
>> >> +
>> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
>> >> +{
>> >> +    X86CPU *cpu = X86_CPU(obj);
>> >> +
>> >> +    cpu->hyperv_default = value;
>> >> +
>> >> +    if (value) {
>> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;  
>> >
>> > s/|="/=/ please,
>> > i.e. no option overrides whatever was specified before to keep semantics consistent.
>> >  
>> 
>> Hm,
>> 
>
>> this doesn't matter for the most recent machine type as
>> hyperv_default_features has all the features but imagine you're running
>> an older machine type which doesn't have 'hv_feature'. Now your
> normally one shouldn't use new feature with old machine type as it makes
> VM non-migratable to older QEMU that has this machine type but not this feature.
>
> nitpicking:
>   according to (1) user should not use 'hv_feature' on old machine since
>   hv_default should cover all their needs (well they don't know what
> hv_default actually is).

Normally yes but I can imagine sticking to some old machine type for
other-than-hyperv-enlightenments purposes and still wanting to add a
newly introduced enlightenment. Migration is not always a must.

>
>> suggestion is 
>> 
>> if I do:
>> 
>> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
>> 
>> but if I do
>> 
>> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
>> (as hv_default enablement will overwrite everything)
>> 
>> How is this consistent?
> usual semantics for properties, is that the latest property overwrites,
> the previous property value parsed from left to right.
> (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> if one needs more than that one should add more related features after that.
>

This semantics probably doesn't apply to 'hv-default' case IMO as my
brain refuses to accept the fact that

'hv_default,hv_feature' != 'hv_feature,hv_default'

which should express the same desire 'the default set PLUS the feature I
want'.

I think I prefer sanity over purity in this case.

>
>> >> +    }
>> >> +}
>> >> +
>> >>  /* Generic getter for "feature-words" and "filtered-features" properties */
>> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
>> >>                                        const char *name, void *opaque,
>> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
>> >>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
>> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
>> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
>> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
>> >>  
>> >>      if (xcc->model) {
>> >>          x86_cpu_load_model(cpu, xcc->model);
>> >>      }
>> >> +
>> >> +    /* Hyper-V features enabled with 'hv-default=on' */
>> >> +    cpu->hyperv_default_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_IPI) |
>> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
>> >> +
>> >> +    /* Enlightened VMCS is only available on Intel/VMX */
>> >> +    if (kvm_hv_evmcs_available()) {
>> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
>> >> +    }  
>> > what if VVM is migrated to another host without evmcs,
>> > will it change CPUID?
>> >  
>> 
>> Evmcs is tightly coupled with VMX, we can't migrate when it's not
>> there.
>
> Are you saying mgmt will check and refuse to migrate to such host?
>

Is it possible to migrate a VM from a VMX-enabled host to a VMX-disabled
one if VMX feature was exposed to the VM? Probably not, you will fail to
create a VM on the destination host. Evmcs doesn't change anything in
this regard, there are no hosts where VMX is available but EVMCS is not.

>> 
>> >>  }
>> >>  
>> >>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
>> >> @@ -7285,6 +7319,10 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>> >>                                x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
>> >>  #endif
>> >>  
>> >> +    object_class_property_add_bool(oc, "hv-default",
>> >> +                              x86_hv_default_get,
>> >> +                              x86_hv_default_set);
>> >> +
>> >>      for (w = 0; w < FEATURE_WORDS; w++) {
>> >>          int bitnr;
>> >>          for (bitnr = 0; bitnr < 64; bitnr++) {
>> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> >> index 6220cb2cabb9..8a484becb6b9 100644
>> >> --- a/target/i386/cpu.h
>> >> +++ b/target/i386/cpu.h
>> >> @@ -1657,6 +1657,11 @@ struct X86CPU {
>> >>      bool hyperv_synic_kvm_only;
>> >>      uint64_t hyperv_features;
>> >>      bool hyperv_passthrough;
>> >> +
>> >> +    /* 'hv-default' enablement */
>> >> +    uint64_t hyperv_default_features;
>> >> +    bool hyperv_default;
>> >> +
>> >>      OnOffAuto hyperv_no_nonarch_cs;
>> >>      uint32_t hyperv_vendor_id[3];
>> >>      uint32_t hyperv_interface_id[4];  
>> >  
>> 
>

Igor Mammedov Jan. 20, 2021, 7:08 p.m. UTC | #5

On Wed, 20 Jan 2021 15:38:33 +0100
Vitaly Kuznetsov <[e-mail]> wrote:

> Igor Mammedov <[e-mail]> writes:
> 
> > On Fri, 15 Jan 2021 10:20:23 +0100
> > Vitaly Kuznetsov <[e-mail]> wrote:
> >  
> >> Igor Mammedov <[e-mail]> writes:
> >>   
> >> > On Thu,  7 Jan 2021 16:14:49 +0100
> >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> >> >> everything but it can't be used in production as it prevents migration.
> >> >> 
> >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> >> >> compat_props mechanism will be used to disable them for legacy machine types,
> >> >> this will keep 'hv-default=on' configurations migratable.
> >> >> 
> >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> >> >> ---
> >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> >> >>  target/i386/cpu.h |  5 +++++
> >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> >> >> 
> >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> >> >> index 5df00da54fc4..a54c066cab09 100644
> >> >> --- a/docs/hyperv.txt
> >> >> +++ b/docs/hyperv.txt
> >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> >> >>  
> >> >>  2. Setup
> >> >>  =========
> >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> >> >> +'hv-default=on' CPU flag:
> >> >>  
> >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> >> >> +
> >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> >> >> +e.g:
> >> >> +
> >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...    
> >> >
> >> > I'd put here not '...' but rather recommended list of flags, and update
> >> > it every time when new feature added if necessary.
> >> >    
> >
> > 1)
> >    
> >> This is an example of fine-grained enablement, there is no point to put
> >> all the existing flags there (hv-default is the only recommended way
> >> now, the rest is 'expert'/'debugging').  
> > so users are kept in dark what hv-default disables/enables (and it might depend
> > on machine version on top that). Doesn't look like a good documentation to me
> > (sure everyone can go and read source code for it and try to figure out how
> > it's supposed to work)  
> 
> 'hv-default' enables *all* currently supported enlightenments. When
> using with an old machine type, it will enable *all* Hyper-V
> enlightenmnets which were supported when the corresponding machine type
> was released. I don't think we document all other cases when a machine
> type is modified (i.e. where can I read how pc-q35-5.1 is different from
> pc-q35-5.0 if I refuse to read the source code?)
> 
> >  
> >>  
> >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> >> > be necessary, I still see it as functionality duplication but I will not oppose it)
> >> >    
> >> 
> >> Unfortunately, upper layer tools don't read this doc and update
> >> themselves to enable new features when they appear.  
> > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> > why stop there, just merge with yet another upper layer, it would save us a lot
> > on communication protocols and simplify VM creation even more,
> > and no one will have to read docs and write anything new on top.)
> > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> > layers on top of it.  
> 
> We have '-machine q35' and we don't require to list all the devices from
> it. We have '-cpu Skylake-Server' and we don't require to configure all
> the features manually. Why can't we have similar enablement for Hyper-V
> emulation where we can't even see a real need for anything but 'enable
> everything' option?
> 
> There is no 'one libvirt to rule them all' (fortunately or
> unfortunately). And sometimes QEMU is the uppermost layer and there's no
> 'libvirt' on top of it, this is also a perfectly valid use-case.
> 
> >  
> >> Similarly, if when these tools use '-machine q35' they get all the new features we add
> >> automatically, right?  
> > it depends, in case of CPUs, new features usually 'off' by default
> > for existing models. In case of bugs, features sometimes could be
> > flipped and versioned machines were used to keep broken CPU models
> > on old machine types.
> >  
> 
> That's why I was saying that Hyper-V enlightenments hardly resemble
> 'hardware' CPU features.
Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
and I prefer to treat them the same way as any other CPUID bits.

> 
> >      
> >> >> +It is also possible to disable individual enlightenments from the default list,
> >> >> +this can be used for debugging purposes:
> >> >> +
> >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> >> >>  
> >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> >> >>  check that the supplied configuration is sane.
> >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> >> index 48007a876e32..99338de00f78 100644
> >> >> --- a/target/i386/cpu.c
> >> >> +++ b/target/i386/cpu.c
> >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> >> >>  }
> >> >>  
> >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> >> >> +{
> >> >> +    X86CPU *cpu = X86_CPU(obj);
> >> >> +
> >> >> +    return cpu->hyperv_default;
> >> >> +}
> >> >> +
> >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> >> >> +{
> >> >> +    X86CPU *cpu = X86_CPU(obj);
> >> >> +
> >> >> +    cpu->hyperv_default = value;
> >> >> +
> >> >> +    if (value) {
> >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;    
> >> >
> >> > s/|="/=/ please,
> >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> >> >    
> >> 
> >> Hm,
> >>   
> >  
> >> this doesn't matter for the most recent machine type as
> >> hyperv_default_features has all the features but imagine you're running
> >> an older machine type which doesn't have 'hv_feature'. Now your  
> > normally one shouldn't use new feature with old machine type as it makes
> > VM non-migratable to older QEMU that has this machine type but not this feature.
> >
> > nitpicking:
> >   according to (1) user should not use 'hv_feature' on old machine since
> >   hv_default should cover all their needs (well they don't know what
> > hv_default actually is).  
> 
> Normally yes but I can imagine sticking to some old machine type for
> other-than-hyperv-enlightenments purposes and still wanting to add a
> newly introduced enlightenment. Migration is not always a must.
> 
> >  
> >> suggestion is 
> >> 
> >> if I do:
> >> 
> >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> >> 
> >> but if I do
> >> 
> >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> >> (as hv_default enablement will overwrite everything)
> >> 
> >> How is this consistent?  
> > usual semantics for properties, is that the latest property overwrites,
> > the previous property value parsed from left to right.
> > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> > if one needs more than that one should add more related features after that.
> >  
> 
> This semantics probably doesn't apply to 'hv-default' case IMO as my
> brain refuses to accept the fact that
it's difficult probably because 'hv-default' is 'alias' property 
that covers all individual hv-foo features in one go and that individual
features are exposed to user, but otherwise it is just a property that
sets CPUID features or like any other property, and should be treated like such.

> 'hv_default,hv_feature' != 'hv_feature,hv_default'
>
> which should express the same desire 'the default set PLUS the feature I
> want'.
if hv_default were touching different data, I'd agree.
But in the end hv_default boils down to the same CPUID bits as individual
features:

  hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
         !=
  hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)

 
> I think I prefer sanity over purity in this case.
what is sanity to one could be insanity for another,
so I pointed out the way properties expected to work today.

But you are adding new semantic ('combine') to property/features parsing
(instead of current 'set' policy), and users will have to be aware of
this new behavior and add/maintain code for this special case.
(maybe I worry in vain, and no one will read docs and know about this
new property anyways)

That will also push x86 CPUs consolidation farther away from other targets,
where there aren't any special casing for features parsing, just simple
left to right parsing with the latest property having overwriting previously
set value.
We are trying hard to reduce special cases and unify interfaces for same
components to simplify qemu and make it predictable/easier for users.


> >> >> +    }
> >> >> +}
> >> >> +
> >> >>  /* Generic getter for "feature-words" and "filtered-features" properties */
> >> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> >> >>                                        const char *name, void *opaque,
> >> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
> >> >>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
> >> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
> >> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
> >> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
> >> >>  
> >> >>      if (xcc->model) {
> >> >>          x86_cpu_load_model(cpu, xcc->model);
> >> >>      }
> >> >> +
> >> >> +    /* Hyper-V features enabled with 'hv-default=on' */
> >> >> +    cpu->hyperv_default_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_IPI) |
> >> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
> >> >> +
> >> >> +    /* Enlightened VMCS is only available on Intel/VMX */
> >> >> +    if (kvm_hv_evmcs_available()) {
> >> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
> >> >> +    }    
> >> > what if VVM is migrated to another host without evmcs,
> >> > will it change CPUID?
> >> >    
> >> 
> >> Evmcs is tightly coupled with VMX, we can't migrate when it's not
> >> there.  
> >
> > Are you saying mgmt will check and refuse to migrate to such host?
> >  
> 
> Is it possible to migrate a VM from a VMX-enabled host to a VMX-disabled
> one if VMX feature was exposed to the VM? Probably not, you will fail to
> create a VM on the destination host. Evmcs doesn't change anything in
> this regard, there are no hosts where VMX is available but EVMCS is not.

I'm not sure how evmcs should be handled,
can you point out what in this series makes sure that migration fails or
makes qemu not able to start in case kvm_hv_evmcs_available() returns false.

So far I read snippet above as a problem:
1:
  host supports evmcs:
  and exposes HYPERV_FEAT_EVMCS in CPUID
2: we migrate to host without evmcs
2.1 start target QEMU, it happily creates vCPUs without HYPERV_FEAT_EVMCS in CPUID
2.2 if I'm not mistaken CPUID is not part of migration stream,
    nothing could check and fail migration
2.3 guest runs fine till it tries to use non existing feature, ...


> >> >>  }
> >> >>  
> >> >>  static int64_t x86_cpu_get_arch_id(CPUState *cs)
> >> >> @@ -7285,6 +7319,10 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> >> >>                                x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
> >> >>  #endif
> >> >>  
> >> >> +    object_class_property_add_bool(oc, "hv-default",
> >> >> +                              x86_hv_default_get,
> >> >> +                              x86_hv_default_set);
> >> >> +
> >> >>      for (w = 0; w < FEATURE_WORDS; w++) {
> >> >>          int bitnr;
> >> >>          for (bitnr = 0; bitnr < 64; bitnr++) {
> >> >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> >> >> index 6220cb2cabb9..8a484becb6b9 100644
> >> >> --- a/target/i386/cpu.h
> >> >> +++ b/target/i386/cpu.h
> >> >> @@ -1657,6 +1657,11 @@ struct X86CPU {
> >> >>      bool hyperv_synic_kvm_only;
> >> >>      uint64_t hyperv_features;
> >> >>      bool hyperv_passthrough;
> >> >> +
> >> >> +    /* 'hv-default' enablement */
> >> >> +    uint64_t hyperv_default_features;
> >> >> +    bool hyperv_default;
> >> >> +
> >> >>      OnOffAuto hyperv_no_nonarch_cs;
> >> >>      uint32_t hyperv_vendor_id[3];
> >> >>      uint32_t hyperv_interface_id[4];    
> >> >    
> >>   
> >  
>

Eduardo Habkost Jan. 20, 2021, 7:55 p.m. UTC | #6

On Wed, Jan 20, 2021 at 02:13:12PM +0100, Igor Mammedov wrote:
> On Fri, 15 Jan 2021 10:20:23 +0100
> Vitaly Kuznetsov <[e-mail]> wrote:
> 
> > Igor Mammedov <[e-mail]> writes:
> > 
> > > On Thu,  7 Jan 2021 16:14:49 +0100
> > > Vitaly Kuznetsov <[e-mail]> wrote:
[...]
> > >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> > >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> > >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> > >> +'hv-default=on' CPU flag:
> > >>  
> > >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> > >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > >> +
> > >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> > >> +e.g:
> > >> +
> > >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...  
> > >
> > > I'd put here not '...' but rather recommended list of flags, and update
> > > it every time when new feature added if necessary.
> > >  
> 
> 1)
>  
> > This is an example of fine-grained enablement, there is no point to put
> > all the existing flags there (hv-default is the only recommended way
> > now, the rest is 'expert'/'debugging').
> so users are kept in dark what hv-default disables/enables (and it might depend
> on machine version on top that). Doesn't look like a good documentation to me
> (sure everyone can go and read source code for it and try to figure out how
> it's supposed to work)

Why is this a problem?  This is not different from CPU feature
flags hidden by CPU model names.  Or virtio feature bits hidden
by virtio devices and machine type compat code.


> > > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > > be necessary, I still see it as functionality duplication but I will not oppose it)
> > >  
> > 
> > Unfortunately, upper layer tools don't read this doc and update
> > themselves to enable new features when they appear.
> rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> why stop there, just merge with yet another upper layer, it would save us a lot
> on communication protocols and simplify VM creation even more,
> and no one will have to read docs and write anything new on top.)
> There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> layers on top of it.

If there should be a limit somewhere, I'd say low level hyperv
flags are a very long way from crossing that limit.  It is
completely reasonable to provide a higher level knob for them in
QEMU.  Vitaly is trying to solve a real problem here, because the
existing solution is not working.

We only need to offer more complex and lower level interfaces if
we have to.  I don't think we have real world examples where
libvirt or management software developers/users are asking for
low level hyperv feature flag knobs, do we?

> 
> > Similarly, if when these tools use '-machine q35' they get all the new features we add
> > automatically, right?
> it depends, in case of CPUs, new features usually 'off' by default
> for existing models. In case of bugs, features sometimes could be
> flipped and versioned machines were used to keep broken CPU models
> on old machine types.
> 
>    
[...]

Eduardo Habkost Jan. 20, 2021, 8:49 p.m. UTC | #7

On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:
> On Wed, 20 Jan 2021 15:38:33 +0100
> Vitaly Kuznetsov <[e-mail]> wrote:
> 
> > Igor Mammedov <[e-mail]> writes:
> > 
> > > On Fri, 15 Jan 2021 10:20:23 +0100
> > > Vitaly Kuznetsov <[e-mail]> wrote:
> > >  
> > >> Igor Mammedov <[e-mail]> writes:
> > >>   
> > >> > On Thu,  7 Jan 2021 16:14:49 +0100
> > >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> > >> >> everything but it can't be used in production as it prevents migration.
> > >> >> 
> > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> > >> >> compat_props mechanism will be used to disable them for legacy machine types,
> > >> >> this will keep 'hv-default=on' configurations migratable.
> > >> >> 
> > >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> > >> >> ---
> > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> > >> >>  target/i386/cpu.h |  5 +++++
> > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> > >> >> 
> > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > >> >> index 5df00da54fc4..a54c066cab09 100644
> > >> >> --- a/docs/hyperv.txt
> > >> >> +++ b/docs/hyperv.txt
> > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> > >> >>  
> > >> >>  2. Setup
> > >> >>  =========
> > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> > >> >> +'hv-default=on' CPU flag:
> > >> >>  
> > >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > >> >> +
> > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> > >> >> +e.g:
> > >> >> +
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...    
> > >> >
> > >> > I'd put here not '...' but rather recommended list of flags, and update
> > >> > it every time when new feature added if necessary.
> > >> >    
> > >
> > > 1)
> > >    
> > >> This is an example of fine-grained enablement, there is no point to put
> > >> all the existing flags there (hv-default is the only recommended way
> > >> now, the rest is 'expert'/'debugging').  
> > > so users are kept in dark what hv-default disables/enables (and it might depend
> > > on machine version on top that). Doesn't look like a good documentation to me
> > > (sure everyone can go and read source code for it and try to figure out how
> > > it's supposed to work)  
> > 
> > 'hv-default' enables *all* currently supported enlightenments. When
> > using with an old machine type, it will enable *all* Hyper-V
> > enlightenmnets which were supported when the corresponding machine type
> > was released. I don't think we document all other cases when a machine
> > type is modified (i.e. where can I read how pc-q35-5.1 is different from
> > pc-q35-5.0 if I refuse to read the source code?)
> > 
> > >  
> > >>  
> > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > >> > be necessary, I still see it as functionality duplication but I will not oppose it)
> > >> >    
> > >> 
> > >> Unfortunately, upper layer tools don't read this doc and update
> > >> themselves to enable new features when they appear.  
> > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> > > why stop there, just merge with yet another upper layer, it would save us a lot
> > > on communication protocols and simplify VM creation even more,
> > > and no one will have to read docs and write anything new on top.)
> > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> > > layers on top of it.  
> > 
> > We have '-machine q35' and we don't require to list all the devices from
> > it. We have '-cpu Skylake-Server' and we don't require to configure all
> > the features manually. Why can't we have similar enablement for Hyper-V
> > emulation where we can't even see a real need for anything but 'enable
> > everything' option?
> > 
> > There is no 'one libvirt to rule them all' (fortunately or
> > unfortunately). And sometimes QEMU is the uppermost layer and there's no
> > 'libvirt' on top of it, this is also a perfectly valid use-case.
> > 
> > >  
> > >> Similarly, if when these tools use '-machine q35' they get all the new features we add
> > >> automatically, right?  
> > > it depends, in case of CPUs, new features usually 'off' by default
> > > for existing models. In case of bugs, features sometimes could be
> > > flipped and versioned machines were used to keep broken CPU models
> > > on old machine types.
> > >  
> > 
> > That's why I was saying that Hyper-V enlightenments hardly resemble
> > 'hardware' CPU features.
> Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
> and I prefer to treat them the same way as any other CPUID bits.
> 
> > 
> > >      
> > >> >> +It is also possible to disable individual enlightenments from the default list,
> > >> >> +this can be used for debugging purposes:
> > >> >> +
> > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> > >> >>  
> > >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> > >> >>  check that the supplied configuration is sane.
> > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > >> >> index 48007a876e32..99338de00f78 100644
> > >> >> --- a/target/i386/cpu.c
> > >> >> +++ b/target/i386/cpu.c
> > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > >> >>  }
> > >> >>  
> > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> > >> >> +{
> > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > >> >> +
> > >> >> +    return cpu->hyperv_default;
> > >> >> +}
> > >> >> +
> > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> > >> >> +{
> > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > >> >> +
> > >> >> +    cpu->hyperv_default = value;
> > >> >> +
> > >> >> +    if (value) {
> > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;    
> > >> >
> > >> > s/|="/=/ please,
> > >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> > >> >    
> > >> 
> > >> Hm,
> > >>   
> > >  
> > >> this doesn't matter for the most recent machine type as
> > >> hyperv_default_features has all the features but imagine you're running
> > >> an older machine type which doesn't have 'hv_feature'. Now your  
> > > normally one shouldn't use new feature with old machine type as it makes
> > > VM non-migratable to older QEMU that has this machine type but not this feature.
> > >
> > > nitpicking:
> > >   according to (1) user should not use 'hv_feature' on old machine since
> > >   hv_default should cover all their needs (well they don't know what
> > > hv_default actually is).  
> > 
> > Normally yes but I can imagine sticking to some old machine type for
> > other-than-hyperv-enlightenments purposes and still wanting to add a
> > newly introduced enlightenment. Migration is not always a must.
> > 
> > >  
> > >> suggestion is 
> > >> 
> > >> if I do:
> > >> 
> > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> > >> 
> > >> but if I do
> > >> 
> > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> > >> (as hv_default enablement will overwrite everything)
> > >> 
> > >> How is this consistent?  
> > > usual semantics for properties, is that the latest property overwrites,
> > > the previous property value parsed from left to right.
> > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> > > if one needs more than that one should add more related features after that.
> > >  
> > 
> > This semantics probably doesn't apply to 'hv-default' case IMO as my
> > brain refuses to accept the fact that
> it's difficult probably because 'hv-default' is 'alias' property 
> that covers all individual hv-foo features in one go and that individual
> features are exposed to user, but otherwise it is just a property that
> sets CPUID features or like any other property, and should be treated like such.
> 
> > 'hv_default,hv_feature' != 'hv_feature,hv_default'
> >
> > which should express the same desire 'the default set PLUS the feature I
> > want'.
> if hv_default were touching different data, I'd agree.
> But in the end hv_default boils down to the same CPUID bits as individual
> features:
> 
>   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
>          !=
>   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)

I don't know why you chose to define "hv_default" as
hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
doesn't need to be touched by hv_default at all.


> 
>  
> > I think I prefer sanity over purity in this case.
> what is sanity to one could be insanity for another,
> so I pointed out the way properties expected to work today.
> 
> But you are adding new semantic ('combine') to property/features parsing
> (instead of current 'set' policy), and users will have to be aware of
> this new behavior and add/maintain code for this special case.
> (maybe I worry in vain, and no one will read docs and know about this
> new property anyways)
> 
> That will also push x86 CPUs consolidation farther away from other targets,
> where there aren't any special casing for features parsing, just simple
> left to right parsing with the latest property having overwriting previously
> set value.
> We are trying hard to reduce special cases and unify interfaces for same
> components to simplify qemu and make it predictable/easier for users.
> 

What you are proposing diverges from other targets, actually.
See target/s390x/cpu_models.c:set_feature_group() for example.
Enabling a feature group in s390x only enables a set of feature
bits, and doesn't touch the rest.

In other words, if hv_default includes hv_f1+hv_f2 (and not hv_f3
or hv_f4), this means:

   hv_default,hv_f3=on,hv_f4=off => (hv_f1=on,hv_f2=on),hv_f3=on,hv_f4=off
          ==
   hv_f3=on,hv_f4=off,hv_default => hv_f3=on,hv_f4=off,(hv_f2=on,hv_f2=on)

That would also mean:

   hv_default,hv_f1=on,hv_f2=off => (hv_f1=on,hv_f2=on),hv_f1=on,hv_f2=off
          !=
   hv_f1=on,hv_f2=off,hv_default => hv_f1=on,hv_f2=off,(hv_f2=on,hv_f2=on)

That's the behavior implemented by Vitaly.

> [...]

Vitaly Kuznetsov Jan. 21, 2021, 8:45 a.m. UTC | #8

Igor Mammedov <[e-mail]> writes:

> On Wed, 20 Jan 2021 15:38:33 +0100
> Vitaly Kuznetsov <[e-mail]> wrote:
>
>> Igor Mammedov <[e-mail]> writes:
>> 
>> > On Fri, 15 Jan 2021 10:20:23 +0100
>> > Vitaly Kuznetsov <[e-mail]> wrote:
>> >  
>> >> suggestion is 
>> >> 
>> >> if I do:
>> >> 
>> >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
>> >> 
>> >> but if I do
>> >> 
>> >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
>> >> (as hv_default enablement will overwrite everything)
>> >> 
>> >> How is this consistent?  
>> > usual semantics for properties, is that the latest property overwrites,
>> > the previous property value parsed from left to right.
>> > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
>> > if one needs more than that one should add more related features after that.
>> >  
>> 
>> This semantics probably doesn't apply to 'hv-default' case IMO as my
>> brain refuses to accept the fact that
> it's difficult probably because 'hv-default' is 'alias' property 
> that covers all individual hv-foo features in one go and that individual
> features are exposed to user, but otherwise it is just a property that
> sets CPUID features or like any other property, and should be treated
> like such.
>
>> 'hv_default,hv_feature' != 'hv_feature,hv_default'
>>
>> which should express the same desire 'the default set PLUS the feature I
>> want'.
> if hv_default were touching different data, I'd agree.
> But in the end hv_default boils down to the same CPUID bits as individual
> features:
>
>   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
>          !=
>   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)
>

In your case I treat 'hv_default' as 'hv_f1=on' and it says nothing
about 'hv_f2' - neither it is enabled, nor it is disabled because when
the corresponding machine type was released it just wasn't there.

>  
>> I think I prefer sanity over purity in this case.
> what is sanity to one could be insanity for another,
> so I pointed out the way properties expected to work today.
>
> But you are adding new semantic ('combine') to property/features parsing
> (instead of current 'set' policy), and users will have to be aware of
> this new behavior and add/maintain code for this special case.
> (maybe I worry in vain, and no one will read docs and know about this
> new property anyways)
>
> That will also push x86 CPUs consolidation farther away from other targets,
> where there aren't any special casing for features parsing, just simple
> left to right parsing with the latest property having overwriting previously
> set value.

In case this is somewhat important I suggest we get back to adding
'hyperv=on' machine type option and not do the 'aliasing' with
'hv_default'. I think it would be possible to support

'-M q35,hyper=on -cpu host,hv-stimer-direct=off' 

even if we need to add a custom handler for Hyper-V feature setting
instead of just using bits in u64 as we need to remember both what was
enabled and what was disabled to combine this with machine type property
correctly.

> We are trying hard to reduce special cases and unify interfaces for same
> components to simplify qemu and make it predictable/easier for users.
>

That's exactly the reason why we need simpler Hyper-V feature
enablement! :-)

>
>> >> >> +    }
>> >> >> +}
>> >> >> +
>> >> >>  /* Generic getter for "feature-words" and "filtered-features" properties */
>> >> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
>> >> >>                                        const char *name, void *opaque,
>> >> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
>> >> >>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
>> >> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
>> >> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
>> >> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
>> >> >>  
>> >> >>      if (xcc->model) {
>> >> >>          x86_cpu_load_model(cpu, xcc->model);
>> >> >>      }
>> >> >> +
>> >> >> +    /* Hyper-V features enabled with 'hv-default=on' */
>> >> >> +    cpu->hyperv_default_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_IPI) |
>> >> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
>> >> >> +
>> >> >> +    /* Enlightened VMCS is only available on Intel/VMX */
>> >> >> +    if (kvm_hv_evmcs_available()) {
>> >> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
>> >> >> +    }    
>> >> > what if VVM is migrated to another host without evmcs,
>> >> > will it change CPUID?
>> >> >    
>> >> 
>> >> Evmcs is tightly coupled with VMX, we can't migrate when it's not
>> >> there.  
>> >
>> > Are you saying mgmt will check and refuse to migrate to such host?
>> >  
>> 
>> Is it possible to migrate a VM from a VMX-enabled host to a VMX-disabled
>> one if VMX feature was exposed to the VM? Probably not, you will fail to
>> create a VM on the destination host. Evmcs doesn't change anything in
>> this regard, there are no hosts where VMX is available but EVMCS is not.
>
> I'm not sure how evmcs should be handled,
> can you point out what in this series makes sure that migration fails or
> makes qemu not able to start in case kvm_hv_evmcs_available() returns false.
>
> So far I read snippet above as a problem:
> 1:
>   host supports evmcs:
>   and exposes HYPERV_FEAT_EVMCS in CPUID

Host with EVMCS is Intel

> 2: we migrate to host without evmcs

Host without EVMCS is AMD, there are no other options. It is a pure
software feature available for KVM-intel. And if your KVM is so old that
it doesn't know anything about EVMCS, a bunch of other options from
'hv-default' will not start as well.

> 2.1 start target QEMU, it happily creates vCPUs without
> HYPERV_FEAT_EVMCS in CPUID

No, it doesn't as on host1 we had at least VMX CPU feature enabled (or a
CPU model implying it) to make this all work.

> 2.2 if I'm not mistaken CPUID is not part of migration stream,
>     nothing could check and fail migration
> 2.3 guest runs fine till it tries to use non existing feature, ..

I'm also very sceptical about possibilities for migration
Windows/Hyper-V VMs from Intel to AMD. Hyper-V doesn't even boot if you
don't have fresh-enough CPU so the common denominator for Intel/AMD
would definitely not work.

Igor Mammedov Jan. 21, 2021, 1:27 p.m. UTC | #9

On Wed, 20 Jan 2021 15:49:09 -0500
Eduardo Habkost <[e-mail]> wrote:

> On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:
> > On Wed, 20 Jan 2021 15:38:33 +0100
> > Vitaly Kuznetsov <[e-mail]> wrote:
> >   
> > > Igor Mammedov <[e-mail]> writes:
> > >   
> > > > On Fri, 15 Jan 2021 10:20:23 +0100
> > > > Vitaly Kuznetsov <[e-mail]> wrote:
> > > >    
> > > >> Igor Mammedov <[e-mail]> writes:
> > > >>     
> > > >> > On Thu,  7 Jan 2021 16:14:49 +0100
> > > >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> > > >> >> everything but it can't be used in production as it prevents migration.
> > > >> >> 
> > > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> > > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> > > >> >> compat_props mechanism will be used to disable them for legacy machine types,
> > > >> >> this will keep 'hv-default=on' configurations migratable.
> > > >> >> 
> > > >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> > > >> >> ---
> > > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> > > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> > > >> >>  target/i386/cpu.h |  5 +++++
> > > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> > > >> >> 
> > > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > > >> >> index 5df00da54fc4..a54c066cab09 100644
> > > >> >> --- a/docs/hyperv.txt
> > > >> >> +++ b/docs/hyperv.txt
> > > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> > > >> >>  
> > > >> >>  2. Setup
> > > >> >>  =========
> > > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> > > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> > > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> > > >> >> +'hv-default=on' CPU flag:
> > > >> >>  
> > > >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > > >> >> +
> > > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> > > >> >> +e.g:
> > > >> >> +
> > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...      
> > > >> >
> > > >> > I'd put here not '...' but rather recommended list of flags, and update
> > > >> > it every time when new feature added if necessary.
> > > >> >      
> > > >
> > > > 1)
> > > >      
> > > >> This is an example of fine-grained enablement, there is no point to put
> > > >> all the existing flags there (hv-default is the only recommended way
> > > >> now, the rest is 'expert'/'debugging').    
> > > > so users are kept in dark what hv-default disables/enables (and it might depend
> > > > on machine version on top that). Doesn't look like a good documentation to me
> > > > (sure everyone can go and read source code for it and try to figure out how
> > > > it's supposed to work)    
> > > 
> > > 'hv-default' enables *all* currently supported enlightenments. When
> > > using with an old machine type, it will enable *all* Hyper-V
> > > enlightenmnets which were supported when the corresponding machine type
> > > was released. I don't think we document all other cases when a machine
> > > type is modified (i.e. where can I read how pc-q35-5.1 is different from
> > > pc-q35-5.0 if I refuse to read the source code?)
> > >   
> > > >    
> > > >>    
> > > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > > >> > be necessary, I still see it as functionality duplication but I will not oppose it)
> > > >> >      
> > > >> 
> > > >> Unfortunately, upper layer tools don't read this doc and update
> > > >> themselves to enable new features when they appear.    
> > > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> > > > why stop there, just merge with yet another upper layer, it would save us a lot
> > > > on communication protocols and simplify VM creation even more,
> > > > and no one will have to read docs and write anything new on top.)
> > > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> > > > layers on top of it.    
> > > 
> > > We have '-machine q35' and we don't require to list all the devices from
> > > it. We have '-cpu Skylake-Server' and we don't require to configure all
> > > the features manually. Why can't we have similar enablement for Hyper-V
> > > emulation where we can't even see a real need for anything but 'enable
> > > everything' option?
> > > 
> > > There is no 'one libvirt to rule them all' (fortunately or
> > > unfortunately). And sometimes QEMU is the uppermost layer and there's no
> > > 'libvirt' on top of it, this is also a perfectly valid use-case.
> > >   
> > > >    
> > > >> Similarly, if when these tools use '-machine q35' they get all the new features we add
> > > >> automatically, right?    
> > > > it depends, in case of CPUs, new features usually 'off' by default
> > > > for existing models. In case of bugs, features sometimes could be
> > > > flipped and versioned machines were used to keep broken CPU models
> > > > on old machine types.
> > > >    
> > > 
> > > That's why I was saying that Hyper-V enlightenments hardly resemble
> > > 'hardware' CPU features.  
> > Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
> > and I prefer to treat them the same way as any other CPUID bits.
> >   
> > >   
> > > >        
> > > >> >> +It is also possible to disable individual enlightenments from the default list,
> > > >> >> +this can be used for debugging purposes:
> > > >> >> +
> > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> > > >> >>  
> > > >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> > > >> >>  check that the supplied configuration is sane.
> > > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > >> >> index 48007a876e32..99338de00f78 100644
> > > >> >> --- a/target/i386/cpu.c
> > > >> >> +++ b/target/i386/cpu.c
> > > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> > > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > > >> >>  }
> > > >> >>  
> > > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> > > >> >> +{
> > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > >> >> +
> > > >> >> +    return cpu->hyperv_default;
> > > >> >> +}
> > > >> >> +
> > > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> > > >> >> +{
> > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > >> >> +
> > > >> >> +    cpu->hyperv_default = value;
> > > >> >> +
> > > >> >> +    if (value) {
> > > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;      
> > > >> >
> > > >> > s/|="/=/ please,
> > > >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> > > >> >      
> > > >> 
> > > >> Hm,
> > > >>     
> > > >    
> > > >> this doesn't matter for the most recent machine type as
> > > >> hyperv_default_features has all the features but imagine you're running
> > > >> an older machine type which doesn't have 'hv_feature'. Now your    
> > > > normally one shouldn't use new feature with old machine type as it makes
> > > > VM non-migratable to older QEMU that has this machine type but not this feature.
> > > >
> > > > nitpicking:
> > > >   according to (1) user should not use 'hv_feature' on old machine since
> > > >   hv_default should cover all their needs (well they don't know what
> > > > hv_default actually is).    
> > > 
> > > Normally yes but I can imagine sticking to some old machine type for
> > > other-than-hyperv-enlightenments purposes and still wanting to add a
> > > newly introduced enlightenment. Migration is not always a must.
> > >   
> > > >    
> > > >> suggestion is 
> > > >> 
> > > >> if I do:
> > > >> 
> > > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> > > >> 
> > > >> but if I do
> > > >> 
> > > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> > > >> (as hv_default enablement will overwrite everything)
> > > >> 
> > > >> How is this consistent?    
> > > > usual semantics for properties, is that the latest property overwrites,
> > > > the previous property value parsed from left to right.
> > > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> > > > if one needs more than that one should add more related features after that.
> > > >    
> > > 
> > > This semantics probably doesn't apply to 'hv-default' case IMO as my
> > > brain refuses to accept the fact that  
> > it's difficult probably because 'hv-default' is 'alias' property 
> > that covers all individual hv-foo features in one go and that individual
> > features are exposed to user, but otherwise it is just a property that
> > sets CPUID features or like any other property, and should be treated like such.
> >   
> > > 'hv_default,hv_feature' != 'hv_feature,hv_default'
> > >
> > > which should express the same desire 'the default set PLUS the feature I
> > > want'.  
> > if hv_default were touching different data, I'd agree.
> > But in the end hv_default boils down to the same CPUID bits as individual
> > features:
> > 
> >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
> >          !=
> >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)  
> 
> I don't know why you chose to define "hv_default" as
> hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
> doesn't need to be touched by hv_default at all.

Essentially I was thinking about hv_default=on as setting default value
of hv CPUID leaf i.e. like doc claims, 'all' hv_* features (including
turned off and unused bits) which always sets leaf to its default state.

Now lets consider following possible situation
using combine' approach (leaf |= some_bits):

QEMU-6.0: initially we have all possible features enabled
                hv_default = (hv_f1=on,hv_f2=on)

hv_f2=on,hv_default=on == hv_f1=on,hv_f2=on

QEMU-6.1: disabled hv_f2=off that was causing problems

hv_default = (hv_f1=on,hv_f2=off)

however due to ORing hv_default doesn't fix issue for the same CLI
(i.e. it doesn't have expected effect)

hv_f2=on,hv_default=on => hv_f1=on,hv_f2=on

if one would use usual 'set' semantics (leaf = all_bits),
then new hv_default value will have desired effect despite of botched CLI,
just by virtue of property following typical 'last set' semantics:

 => hv_f1=on,hv_f2=off

If we assume that we 'never ever' will need to disable feature bits
than it doesn't matter which approach to use, however a look at
pc_compat arrays shows that features are being enabled/disabled
all the time.

PS:
I'd rename hv_default => hv_set_default,
since we would need hv_default[_value] property later on to set compat value
based on machine type version.
    
> > > I think I prefer sanity over purity in this case.  
> > what is sanity to one could be insanity for another,
> > so I pointed out the way properties expected to work today.
> > 
> > But you are adding new semantic ('combine') to property/features parsing
> > (instead of current 'set' policy), and users will have to be aware of
> > this new behavior and add/maintain code for this special case.
> > (maybe I worry in vain, and no one will read docs and know about this
> > new property anyways)
> > 
> > That will also push x86 CPUs consolidation farther away from other targets,
> > where there aren't any special casing for features parsing, just simple
> > left to right parsing with the latest property having overwriting previously
> > set value.
> > We are trying hard to reduce special cases and unify interfaces for same
> > components to simplify qemu and make it predictable/easier for users.
> >   
> 
> What you are proposing diverges from other targets, actually.
> See target/s390x/cpu_models.c:set_feature_group() for example.
> Enabling a feature group in s390x only enables a set of feature
> bits, and doesn't touch the rest.
Looking at code, it has the same issue as I described above


> In other words, if hv_default includes hv_f1+hv_f2 (and not hv_f3
> or hv_f4), this means:
> 
>    hv_default,hv_f3=on,hv_f4=off => (hv_f1=on,hv_f2=on),hv_f3=on,hv_f4=off
>           ==
>    hv_f3=on,hv_f4=off,hv_default => hv_f3=on,hv_f4=off,(hv_f2=on,hv_f2=on)
> 
> That would also mean:
> 
>    hv_default,hv_f1=on,hv_f2=off => (hv_f1=on,hv_f2=on),hv_f1=on,hv_f2=off
>           !=
>    hv_f1=on,hv_f2=off,hv_default => hv_f1=on,hv_f2=off,(hv_f2=on,hv_f2=on)
> 
> That's the behavior implemented by Vitaly.
> 
> > [...]  
>

Igor Mammedov Jan. 21, 2021, 1:49 p.m. UTC | #10

On Thu, 21 Jan 2021 09:45:33 +0100
Vitaly Kuznetsov <[e-mail]> wrote:

> Igor Mammedov <[e-mail]> writes:
> 
> > On Wed, 20 Jan 2021 15:38:33 +0100
> > Vitaly Kuznetsov <[e-mail]> wrote:
> >  
> >> Igor Mammedov <[e-mail]> writes:
> >>   
> >> > On Fri, 15 Jan 2021 10:20:23 +0100
> >> > Vitaly Kuznetsov <[e-mail]> wrote:
> >> >    
> >> >> suggestion is 
> >> >> 
> >> >> if I do:
> >> >> 
> >> >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> >> >> 
> >> >> but if I do
> >> >> 
> >> >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> >> >> (as hv_default enablement will overwrite everything)
> >> >> 
> >> >> How is this consistent?    
> >> > usual semantics for properties, is that the latest property overwrites,
> >> > the previous property value parsed from left to right.
> >> > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> >> > if one needs more than that one should add more related features after that.
> >> >    
> >> 
> >> This semantics probably doesn't apply to 'hv-default' case IMO as my
> >> brain refuses to accept the fact that  
> > it's difficult probably because 'hv-default' is 'alias' property 
> > that covers all individual hv-foo features in one go and that individual
> > features are exposed to user, but otherwise it is just a property that
> > sets CPUID features or like any other property, and should be treated
> > like such.
> >  
> >> 'hv_default,hv_feature' != 'hv_feature,hv_default'
> >>
> >> which should express the same desire 'the default set PLUS the feature I
> >> want'.  
> > if hv_default were touching different data, I'd agree.
> > But in the end hv_default boils down to the same CPUID bits as individual
> > features:
> >
> >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
> >          !=
> >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)
> >  
> 
> In your case I treat 'hv_default' as 'hv_f1=on' and it says nothing
> about 'hv_f2' - neither it is enabled, nor it is disabled because when
> the corresponding machine type was released it just wasn't there.
> 
> >    
> >> I think I prefer sanity over purity in this case.  
> > what is sanity to one could be insanity for another,
> > so I pointed out the way properties expected to work today.
> >
> > But you are adding new semantic ('combine') to property/features parsing
> > (instead of current 'set' policy), and users will have to be aware of
> > this new behavior and add/maintain code for this special case.
> > (maybe I worry in vain, and no one will read docs and know about this
> > new property anyways)
> >
> > That will also push x86 CPUs consolidation farther away from other targets,
> > where there aren't any special casing for features parsing, just simple
> > left to right parsing with the latest property having overwriting previously
> > set value.  
> 
> In case this is somewhat important I suggest we get back to adding
> 'hyperv=on' machine type option and not do the 'aliasing' with
> 'hv_default'. I think it would be possible to support
> 
> '-M q35,hyper=on -cpu host,hv-stimer-direct=off' 
> 
> even if we need to add a custom handler for Hyper-V feature setting
> instead of just using bits in u64 as we need to remember both what was
> enabled and what was disabled to combine this with machine type property
> correctly.
> 
> > We are trying hard to reduce special cases and unify interfaces for same
> > components to simplify qemu and make it predictable/easier for users.
> >  
> 
> That's exactly the reason why we need simpler Hyper-V feature
> enablement! :-)
> 
> >  
> >> >> >> +    }
> >> >> >> +}
> >> >> >> +
> >> >> >>  /* Generic getter for "feature-words" and "filtered-features" properties */
> >> >> >>  static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
> >> >> >>                                        const char *name, void *opaque,
> >> >> >> @@ -6955,10 +6973,26 @@ static void x86_cpu_initfn(Object *obj)
> >> >> >>      object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
> >> >> >>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
> >> >> >>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
> >> >> >> +    object_property_add_alias(obj, "hv_default", obj, "hv-default");
> >> >> >>  
> >> >> >>      if (xcc->model) {
> >> >> >>          x86_cpu_load_model(cpu, xcc->model);
> >> >> >>      }
> >> >> >> +
> >> >> >> +    /* Hyper-V features enabled with 'hv-default=on' */
> >> >> >> +    cpu->hyperv_default_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_IPI) |
> >> >> >> +        BIT(HYPERV_FEAT_STIMER_DIRECT);
> >> >> >> +
> >> >> >> +    /* Enlightened VMCS is only available on Intel/VMX */
> >> >> >> +    if (kvm_hv_evmcs_available()) {
> >> >> >> +        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
> >> >> >> +    }      
> >> >> > what if VVM is migrated to another host without evmcs,
> >> >> > will it change CPUID?
> >> >> >      
> >> >> 
> >> >> Evmcs is tightly coupled with VMX, we can't migrate when it's not
> >> >> there.    
> >> >
> >> > Are you saying mgmt will check and refuse to migrate to such host?
> >> >    
> >> 
> >> Is it possible to migrate a VM from a VMX-enabled host to a VMX-disabled
> >> one if VMX feature was exposed to the VM? Probably not, you will fail to
> >> create a VM on the destination host. Evmcs doesn't change anything in
> >> this regard, there are no hosts where VMX is available but EVMCS is not.  
> >
> > I'm not sure how evmcs should be handled,
> > can you point out what in this series makes sure that migration fails or
> > makes qemu not able to start in case kvm_hv_evmcs_available() returns false.
> >
> > So far I read snippet above as a problem:
> > 1:
> >   host supports evmcs:
> >   and exposes HYPERV_FEAT_EVMCS in CPUID  
> 
> Host with EVMCS is Intel
> 
> > 2: we migrate to host without evmcs  
> 
> Host without EVMCS is AMD, there are no other options. It is a pure
> software feature available for KVM-intel. And if your KVM is so old that
> it doesn't know anything about EVMCS, a bunch of other options from
> 'hv-default' will not start as well.
> > 2.1 start target QEMU, it happily creates vCPUs without
> > HYPERV_FEAT_EVMCS in CPUID  
> 
> No, it doesn't as on host1 we had at least VMX CPU feature enabled (or a
> CPU model implying it) to make this all work.
> 
> > 2.2 if I'm not mistaken CPUID is not part of migration stream,
> >     nothing could check and fail migration
> > 2.3 guest runs fine till it tries to use non existing feature, ..  
> 
> I'm also very sceptical about possibilities for migration
> Windows/Hyper-V VMs from Intel to AMD. Hyper-V doesn't even boot if you
> don't have fresh-enough CPU so the common denominator for Intel/AMD
> would definitely not work. 

Like you said host doesn't have to be AMD, just old enough kernel will
do the job. What exactly will prevent migration 'successfully' completing?

The way it's currently written migration stream won't prevent it.

One way that might solve issue is to add subsection that's enabled when
kvm_hv_evmcs_available() == true, and check on target that the feature
is available or fail migration.

Maybe Eduardo or David can add more how to deal with it if needed.

Igor Mammedov Jan. 21, 2021, 4:23 p.m. UTC | #11

On Thu, 21 Jan 2021 14:27:04 +0100
Igor Mammedov <[e-mail]> wrote:

> On Wed, 20 Jan 2021 15:49:09 -0500
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:  
> > > On Wed, 20 Jan 2021 15:38:33 +0100
> > > Vitaly Kuznetsov <[e-mail]> wrote:
> > >     
> > > > Igor Mammedov <[e-mail]> writes:
> > > >     
> > > > > On Fri, 15 Jan 2021 10:20:23 +0100
> > > > > Vitaly Kuznetsov <[e-mail]> wrote:
> > > > >      
> > > > >> Igor Mammedov <[e-mail]> writes:
> > > > >>       
> > > > >> > On Thu,  7 Jan 2021 16:14:49 +0100
> > > > >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> > > > >> >> everything but it can't be used in production as it prevents migration.
> > > > >> >> 
> > > > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> > > > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> > > > >> >> compat_props mechanism will be used to disable them for legacy machine types,
> > > > >> >> this will keep 'hv-default=on' configurations migratable.
> > > > >> >> 
> > > > >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> > > > >> >> ---
> > > > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> > > > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> > > > >> >>  target/i386/cpu.h |  5 +++++
> > > > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> > > > >> >> 
> > > > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > > > >> >> index 5df00da54fc4..a54c066cab09 100644
> > > > >> >> --- a/docs/hyperv.txt
> > > > >> >> +++ b/docs/hyperv.txt
> > > > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> > > > >> >>  
> > > > >> >>  2. Setup
> > > > >> >>  =========
> > > > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> > > > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> > > > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> > > > >> >> +'hv-default=on' CPU flag:
> > > > >> >>  
> > > > >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > > > >> >> +
> > > > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> > > > >> >> +e.g:
> > > > >> >> +
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...        
> > > > >> >
> > > > >> > I'd put here not '...' but rather recommended list of flags, and update
> > > > >> > it every time when new feature added if necessary.
> > > > >> >        
> > > > >
> > > > > 1)
> > > > >        
> > > > >> This is an example of fine-grained enablement, there is no point to put
> > > > >> all the existing flags there (hv-default is the only recommended way
> > > > >> now, the rest is 'expert'/'debugging').      
> > > > > so users are kept in dark what hv-default disables/enables (and it might depend
> > > > > on machine version on top that). Doesn't look like a good documentation to me
> > > > > (sure everyone can go and read source code for it and try to figure out how
> > > > > it's supposed to work)      
> > > > 
> > > > 'hv-default' enables *all* currently supported enlightenments. When
> > > > using with an old machine type, it will enable *all* Hyper-V
> > > > enlightenmnets which were supported when the corresponding machine type
> > > > was released. I don't think we document all other cases when a machine
> > > > type is modified (i.e. where can I read how pc-q35-5.1 is different from
> > > > pc-q35-5.0 if I refuse to read the source code?)
> > > >     
> > > > >      
> > > > >>      
> > > > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > > > >> > be necessary, I still see it as functionality duplication but I will not oppose it)
> > > > >> >        
> > > > >> 
> > > > >> Unfortunately, upper layer tools don't read this doc and update
> > > > >> themselves to enable new features when they appear.      
> > > > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> > > > > why stop there, just merge with yet another upper layer, it would save us a lot
> > > > > on communication protocols and simplify VM creation even more,
> > > > > and no one will have to read docs and write anything new on top.)
> > > > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> > > > > layers on top of it.      
> > > > 
> > > > We have '-machine q35' and we don't require to list all the devices from
> > > > it. We have '-cpu Skylake-Server' and we don't require to configure all
> > > > the features manually. Why can't we have similar enablement for Hyper-V
> > > > emulation where we can't even see a real need for anything but 'enable
> > > > everything' option?
> > > > 
> > > > There is no 'one libvirt to rule them all' (fortunately or
> > > > unfortunately). And sometimes QEMU is the uppermost layer and there's no
> > > > 'libvirt' on top of it, this is also a perfectly valid use-case.
> > > >     
> > > > >      
> > > > >> Similarly, if when these tools use '-machine q35' they get all the new features we add
> > > > >> automatically, right?      
> > > > > it depends, in case of CPUs, new features usually 'off' by default
> > > > > for existing models. In case of bugs, features sometimes could be
> > > > > flipped and versioned machines were used to keep broken CPU models
> > > > > on old machine types.
> > > > >      
> > > > 
> > > > That's why I was saying that Hyper-V enlightenments hardly resemble
> > > > 'hardware' CPU features.    
> > > Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
> > > and I prefer to treat them the same way as any other CPUID bits.
> > >     
> > > >     
> > > > >          
> > > > >> >> +It is also possible to disable individual enlightenments from the default list,
> > > > >> >> +this can be used for debugging purposes:
> > > > >> >> +
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> > > > >> >>  
> > > > >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> > > > >> >>  check that the supplied configuration is sane.
> > > > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > >> >> index 48007a876e32..99338de00f78 100644
> > > > >> >> --- a/target/i386/cpu.c
> > > > >> >> +++ b/target/i386/cpu.c
> > > > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> > > > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > > > >> >>  }
> > > > >> >>  
> > > > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> > > > >> >> +{
> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > > >> >> +
> > > > >> >> +    return cpu->hyperv_default;
> > > > >> >> +}
> > > > >> >> +
> > > > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> > > > >> >> +{
> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > > >> >> +
> > > > >> >> +    cpu->hyperv_default = value;
> > > > >> >> +
> > > > >> >> +    if (value) {
> > > > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;        
> > > > >> >
> > > > >> > s/|="/=/ please,
> > > > >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> > > > >> >        
> > > > >> 
> > > > >> Hm,
> > > > >>       
> > > > >      
> > > > >> this doesn't matter for the most recent machine type as
> > > > >> hyperv_default_features has all the features but imagine you're running
> > > > >> an older machine type which doesn't have 'hv_feature'. Now your      
> > > > > normally one shouldn't use new feature with old machine type as it makes
> > > > > VM non-migratable to older QEMU that has this machine type but not this feature.
> > > > >
> > > > > nitpicking:
> > > > >   according to (1) user should not use 'hv_feature' on old machine since
> > > > >   hv_default should cover all their needs (well they don't know what
> > > > > hv_default actually is).      
> > > > 
> > > > Normally yes but I can imagine sticking to some old machine type for
> > > > other-than-hyperv-enlightenments purposes and still wanting to add a
> > > > newly introduced enlightenment. Migration is not always a must.
> > > >     
> > > > >      
> > > > >> suggestion is 
> > > > >> 
> > > > >> if I do:
> > > > >> 
> > > > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> > > > >> 
> > > > >> but if I do
> > > > >> 
> > > > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> > > > >> (as hv_default enablement will overwrite everything)
> > > > >> 
> > > > >> How is this consistent?      
> > > > > usual semantics for properties, is that the latest property overwrites,
> > > > > the previous property value parsed from left to right.
> > > > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> > > > > if one needs more than that one should add more related features after that.
> > > > >      
> > > > 
> > > > This semantics probably doesn't apply to 'hv-default' case IMO as my
> > > > brain refuses to accept the fact that    
> > > it's difficult probably because 'hv-default' is 'alias' property 
> > > that covers all individual hv-foo features in one go and that individual
> > > features are exposed to user, but otherwise it is just a property that
> > > sets CPUID features or like any other property, and should be treated like such.
> > >     
> > > > 'hv_default,hv_feature' != 'hv_feature,hv_default'
> > > >
> > > > which should express the same desire 'the default set PLUS the feature I
> > > > want'.    
> > > if hv_default were touching different data, I'd agree.
> > > But in the end hv_default boils down to the same CPUID bits as individual
> > > features:
> > > 
> > >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
> > >          !=
> > >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)    
> > 
> > I don't know why you chose to define "hv_default" as
> > hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
> > doesn't need to be touched by hv_default at all.  
> 
> Essentially I was thinking about hv_default=on as setting default value
> of hv CPUID leaf i.e. like doc claims, 'all' hv_* features (including
> turned off and unused bits) which always sets leaf to its default state.
> 
> Now lets consider following possible situation
> using combine' approach (leaf |= some_bits):
> 
> QEMU-6.0: initially we have all possible features enabled
>                 hv_default = (hv_f1=on,hv_f2=on)
> 
> hv_f2=on,hv_default=on == hv_f1=on,hv_f2=on
> 
> QEMU-6.1: disabled hv_f2=off that was causing problems
> 
> hv_default = (hv_f1=on,hv_f2=off)
> 
> however due to ORing hv_default doesn't fix issue for the same CLI
> (i.e. it doesn't have expected effect)
> 
> hv_f2=on,hv_default=on => hv_f1=on,hv_f2=on
> 
> if one would use usual 'set' semantics (leaf = all_bits),
> then new hv_default value will have desired effect despite of botched CLI,
> just by virtue of property following typical 'last set' semantics:
> 
>  => hv_f1=on,hv_f2=off  
> 
> If we assume that we 'never ever' will need to disable feature bits
> than it doesn't matter which approach to use, however a look at
> pc_compat arrays shows that features are being enabled/disabled
> all the time.

Also there should be a good reason for adding new semantics and
deviating from typical property behavior.

Vitaly Kuznetsov Jan. 21, 2021, 4:51 p.m. UTC | #12

Igor Mammedov <[e-mail]> writes:

> On Thu, 21 Jan 2021 09:45:33 +0100
> Vitaly Kuznetsov <[e-mail]> wrote:
>
>> >
>> > So far I read snippet above as a problem:
>> > 1:
>> >   host supports evmcs:
>> >   and exposes HYPERV_FEAT_EVMCS in CPUID  
>> 
>> Host with EVMCS is Intel
>> 
>> > 2: we migrate to host without evmcs  
>> 
>> Host without EVMCS is AMD, there are no other options. It is a pure
>> software feature available for KVM-intel. And if your KVM is so old that
>> it doesn't know anything about EVMCS, a bunch of other options from
>> 'hv-default' will not start as well.
>> > 2.1 start target QEMU, it happily creates vCPUs without
>> > HYPERV_FEAT_EVMCS in CPUID  
>> 
>> No, it doesn't as on host1 we had at least VMX CPU feature enabled (or a
>> CPU model implying it) to make this all work.
>> 
>> > 2.2 if I'm not mistaken CPUID is not part of migration stream,
>> >     nothing could check and fail migration
>> > 2.3 guest runs fine till it tries to use non existing feature, ..  
>> 
>> I'm also very sceptical about possibilities for migration
>> Windows/Hyper-V VMs from Intel to AMD. Hyper-V doesn't even boot if you
>> don't have fresh-enough CPU so the common denominator for Intel/AMD
>> would definitely not work. 
>
> Like you said host doesn't have to be AMD, just old enough kernel will
> do the job. What exactly will prevent migration 'successfully' completing?
>

First, you can't start a VM with 'hv-default' with an old-enough kernel
because it won't have many other 'hv-' enlightenments
implemented. 'hv-default' will only work for a 'recent enough' kernel
(>= 5.0 when hv-stimer-direct was implemented). 

You can probably try doing '-cpu xxx,hv_default,hv-stimer-direct=off' to
trigger the problem but then KVM should also support nested state
migration to actually migrate a VM using VMX and EVMCS support for it
also emerged in 5.0. I believe that trying to call KVM_SET_NESTED_STATE
(which only appeared in 4.19 btw) on something in between will fail.

> The way it's currently written migration stream won't prevent it.
>
> One way that might solve issue is to add subsection that's enabled when
> kvm_hv_evmcs_available() == true, and check on target that the feature
> is available or fail migration.

Yes, we can but I don't think there's a real issue worth fighting
for. Nested migration was so broken in upstream KVM untill recently that
I don't see why 'old kernel' can be a problem at all. And, again, Intel
to AMD migration is likely off question.

>
> Maybe Eduardo or David can add more how to deal with it if needed.
>

Eduardo Habkost Jan. 21, 2021, 5:08 p.m. UTC | #13

On Thu, Jan 21, 2021 at 02:27:04PM +0100, Igor Mammedov wrote:
> On Wed, 20 Jan 2021 15:49:09 -0500
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:
> > > On Wed, 20 Jan 2021 15:38:33 +0100
> > > Vitaly Kuznetsov <[e-mail]> wrote:
> > >   
> > > > Igor Mammedov <[e-mail]> writes:
> > > >   
> > > > > On Fri, 15 Jan 2021 10:20:23 +0100
> > > > > Vitaly Kuznetsov <[e-mail]> wrote:
> > > > >    
> > > > >> Igor Mammedov <[e-mail]> writes:
> > > > >>     
> > > > >> > On Thu,  7 Jan 2021 16:14:49 +0100
> > > > >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
> > > > >> >> everything but it can't be used in production as it prevents migration.
> > > > >> >> 
> > > > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
> > > > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
> > > > >> >> compat_props mechanism will be used to disable them for legacy machine types,
> > > > >> >> this will keep 'hv-default=on' configurations migratable.
> > > > >> >> 
> > > > >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
> > > > >> >> ---
> > > > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
> > > > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> > > > >> >>  target/i386/cpu.h |  5 +++++
> > > > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
> > > > >> >> 
> > > > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
> > > > >> >> index 5df00da54fc4..a54c066cab09 100644
> > > > >> >> --- a/docs/hyperv.txt
> > > > >> >> +++ b/docs/hyperv.txt
> > > > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
> > > > >> >>  
> > > > >> >>  2. Setup
> > > > >> >>  =========
> > > > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
> > > > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
> > > > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
> > > > >> >> +'hv-default=on' CPU flag:
> > > > >> >>  
> > > > >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
> > > > >> >> +
> > > > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
> > > > >> >> +e.g:
> > > > >> >> +
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...      
> > > > >> >
> > > > >> > I'd put here not '...' but rather recommended list of flags, and update
> > > > >> > it every time when new feature added if necessary.
> > > > >> >      
> > > > >
> > > > > 1)
> > > > >      
> > > > >> This is an example of fine-grained enablement, there is no point to put
> > > > >> all the existing flags there (hv-default is the only recommended way
> > > > >> now, the rest is 'expert'/'debugging').    
> > > > > so users are kept in dark what hv-default disables/enables (and it might depend
> > > > > on machine version on top that). Doesn't look like a good documentation to me
> > > > > (sure everyone can go and read source code for it and try to figure out how
> > > > > it's supposed to work)    
> > > > 
> > > > 'hv-default' enables *all* currently supported enlightenments. When
> > > > using with an old machine type, it will enable *all* Hyper-V
> > > > enlightenmnets which were supported when the corresponding machine type
> > > > was released. I don't think we document all other cases when a machine
> > > > type is modified (i.e. where can I read how pc-q35-5.1 is different from
> > > > pc-q35-5.0 if I refuse to read the source code?)
> > > >   
> > > > >    
> > > > >>    
> > > > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
> > > > >> > be necessary, I still see it as functionality duplication but I will not oppose it)
> > > > >> >      
> > > > >> 
> > > > >> Unfortunately, upper layer tools don't read this doc and update
> > > > >> themselves to enable new features when they appear.    
> > > > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
> > > > > why stop there, just merge with yet another upper layer, it would save us a lot
> > > > > on communication protocols and simplify VM creation even more,
> > > > > and no one will have to read docs and write anything new on top.)
> > > > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
> > > > > layers on top of it.    
> > > > 
> > > > We have '-machine q35' and we don't require to list all the devices from
> > > > it. We have '-cpu Skylake-Server' and we don't require to configure all
> > > > the features manually. Why can't we have similar enablement for Hyper-V
> > > > emulation where we can't even see a real need for anything but 'enable
> > > > everything' option?
> > > > 
> > > > There is no 'one libvirt to rule them all' (fortunately or
> > > > unfortunately). And sometimes QEMU is the uppermost layer and there's no
> > > > 'libvirt' on top of it, this is also a perfectly valid use-case.
> > > >   
> > > > >    
> > > > >> Similarly, if when these tools use '-machine q35' they get all the new features we add
> > > > >> automatically, right?    
> > > > > it depends, in case of CPUs, new features usually 'off' by default
> > > > > for existing models. In case of bugs, features sometimes could be
> > > > > flipped and versioned machines were used to keep broken CPU models
> > > > > on old machine types.
> > > > >    
> > > > 
> > > > That's why I was saying that Hyper-V enlightenments hardly resemble
> > > > 'hardware' CPU features.  
> > > Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
> > > and I prefer to treat them the same way as any other CPUID bits.
> > >   
> > > >   
> > > > >        
> > > > >> >> +It is also possible to disable individual enlightenments from the default list,
> > > > >> >> +this can be used for debugging purposes:
> > > > >> >> +
> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
> > > > >> >>  
> > > > >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
> > > > >> >>  check that the supplied configuration is sane.
> > > > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > >> >> index 48007a876e32..99338de00f78 100644
> > > > >> >> --- a/target/i386/cpu.c
> > > > >> >> +++ b/target/i386/cpu.c
> > > > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
> > > > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
> > > > >> >>  }
> > > > >> >>  
> > > > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
> > > > >> >> +{
> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > > >> >> +
> > > > >> >> +    return cpu->hyperv_default;
> > > > >> >> +}
> > > > >> >> +
> > > > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
> > > > >> >> +{
> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
> > > > >> >> +
> > > > >> >> +    cpu->hyperv_default = value;
> > > > >> >> +
> > > > >> >> +    if (value) {
> > > > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;      
> > > > >> >
> > > > >> > s/|="/=/ please,
> > > > >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
> > > > >> >      
> > > > >> 
> > > > >> Hm,
> > > > >>     
> > > > >    
> > > > >> this doesn't matter for the most recent machine type as
> > > > >> hyperv_default_features has all the features but imagine you're running
> > > > >> an older machine type which doesn't have 'hv_feature'. Now your    
> > > > > normally one shouldn't use new feature with old machine type as it makes
> > > > > VM non-migratable to older QEMU that has this machine type but not this feature.
> > > > >
> > > > > nitpicking:
> > > > >   according to (1) user should not use 'hv_feature' on old machine since
> > > > >   hv_default should cover all their needs (well they don't know what
> > > > > hv_default actually is).    
> > > > 
> > > > Normally yes but I can imagine sticking to some old machine type for
> > > > other-than-hyperv-enlightenments purposes and still wanting to add a
> > > > newly introduced enlightenment. Migration is not always a must.
> > > >   
> > > > >    
> > > > >> suggestion is 
> > > > >> 
> > > > >> if I do:
> > > > >> 
> > > > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
> > > > >> 
> > > > >> but if I do
> > > > >> 
> > > > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
> > > > >> (as hv_default enablement will overwrite everything)
> > > > >> 
> > > > >> How is this consistent?    
> > > > > usual semantics for properties, is that the latest property overwrites,
> > > > > the previous property value parsed from left to right.
> > > > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
> > > > > if one needs more than that one should add more related features after that.
> > > > >    
> > > > 
> > > > This semantics probably doesn't apply to 'hv-default' case IMO as my
> > > > brain refuses to accept the fact that  
> > > it's difficult probably because 'hv-default' is 'alias' property 
> > > that covers all individual hv-foo features in one go and that individual
> > > features are exposed to user, but otherwise it is just a property that
> > > sets CPUID features or like any other property, and should be treated like such.
> > >   
> > > > 'hv_default,hv_feature' != 'hv_feature,hv_default'
> > > >
> > > > which should express the same desire 'the default set PLUS the feature I
> > > > want'.  
> > > if hv_default were touching different data, I'd agree.
> > > But in the end hv_default boils down to the same CPUID bits as individual
> > > features:
> > > 
> > >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
> > >          !=
> > >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)  
> > 
> > I don't know why you chose to define "hv_default" as
> > hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
> > doesn't need to be touched by hv_default at all.
> 
> Essentially I was thinking about hv_default=on as setting default value
> of hv CPUID leaf i.e. like doc claims, 'all' hv_* features (including
> turned off and unused bits) which always sets leaf to its default state.
> 
> Now lets consider following possible situation
> using combine' approach (leaf |= some_bits):
> 
> QEMU-6.0: initially we have all possible features enabled
>                 hv_default = (hv_f1=on,hv_f2=on)
> 
> hv_f2=on,hv_default=on == hv_f1=on,hv_f2=on
> 
> QEMU-6.1: disabled hv_f2=off that was causing problems
> 
> hv_default = (hv_f1=on,hv_f2=off)

Why would we choose to do that?

If we decide f2 shouldn't be part of the default, we'll redefine
hv_default as:

  hv_default = (hv_f1=on)

> 
> however due to ORing hv_default doesn't fix issue for the same CLI
> (i.e. it doesn't have expected effect)
> 
> hv_f2=on,hv_default=on => hv_f1=on,hv_f2=on
> 
> if one would use usual 'set' semantics (leaf = all_bits),
> then new hv_default value will have desired effect despite of botched CLI,
> just by virtue of property following typical 'last set' semantics:
> 
>  => hv_f1=on,hv_f2=off
> 
> If we assume that we 'never ever' will need to disable feature bits
> than it doesn't matter which approach to use, however a look at
> pc_compat arrays shows that features are being enabled/disabled
> all the time.

I'm pretty sure that "hv_default=on will also disable features
that appear in the command line" will not be a requirement.


> 
> PS:
> I'd rename hv_default => hv_set_default,
> since we would need hv_default[_value] property later on to set compat value
> based on machine type version.
>     
> > > > I think I prefer sanity over purity in this case.  
> > > what is sanity to one could be insanity for another,
> > > so I pointed out the way properties expected to work today.
> > > 
> > > But you are adding new semantic ('combine') to property/features parsing
> > > (instead of current 'set' policy), and users will have to be aware of
> > > this new behavior and add/maintain code for this special case.
> > > (maybe I worry in vain, and no one will read docs and know about this
> > > new property anyways)
> > > 
> > > That will also push x86 CPUs consolidation farther away from other targets,
> > > where there aren't any special casing for features parsing, just simple
> > > left to right parsing with the latest property having overwriting previously
> > > set value.
> > > We are trying hard to reduce special cases and unify interfaces for same
> > > components to simplify qemu and make it predictable/easier for users.
> > >   
> > 
> > What you are proposing diverges from other targets, actually.
> > See target/s390x/cpu_models.c:set_feature_group() for example.
> > Enabling a feature group in s390x only enables a set of feature
> > bits, and doesn't touch the rest.
> Looking at code, it has the same issue as I described above

I don't see why that's an issue.  This is how feature groups were
designed, and it works.


> 
> 
> > In other words, if hv_default includes hv_f1+hv_f2 (and not hv_f3
> > or hv_f4), this means:
> > 
> >    hv_default,hv_f3=on,hv_f4=off => (hv_f1=on,hv_f2=on),hv_f3=on,hv_f4=off
> >           ==
> >    hv_f3=on,hv_f4=off,hv_default => hv_f3=on,hv_f4=off,(hv_f2=on,hv_f2=on)
> > 
> > That would also mean:
> > 
> >    hv_default,hv_f1=on,hv_f2=off => (hv_f1=on,hv_f2=on),hv_f1=on,hv_f2=off
> >           !=
> >    hv_f1=on,hv_f2=off,hv_default => hv_f1=on,hv_f2=off,(hv_f2=on,hv_f2=on)
> > 
> > That's the behavior implemented by Vitaly.
> > 
> > > [...]  
> > 
>

David Edmondson Jan. 25, 2021, 1:42 p.m. UTC | #14

On Thursday, 2021-01-21 at 12:08:02 -05, Eduardo Habkost wrote:

> On Thu, Jan 21, 2021 at 02:27:04PM +0100, Igor Mammedov wrote:
>> On Wed, 20 Jan 2021 15:49:09 -0500
>> Eduardo Habkost <[e-mail]> wrote:
>> 
>> > On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote:
>> > > On Wed, 20 Jan 2021 15:38:33 +0100
>> > > Vitaly Kuznetsov <[e-mail]> wrote:
>> > >   
>> > > > Igor Mammedov <[e-mail]> writes:
>> > > >   
>> > > > > On Fri, 15 Jan 2021 10:20:23 +0100
>> > > > > Vitaly Kuznetsov <[e-mail]> wrote:
>> > > > >    
>> > > > >> Igor Mammedov <[e-mail]> writes:
>> > > > >>     
>> > > > >> > On Thu,  7 Jan 2021 16:14:49 +0100
>> > > > >> > Vitaly Kuznetsov <[e-mail]> 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 'hv-passthrough' mode enabling
>> > > > >> >> everything but it can't be used in production as it prevents migration.
>> > > > >> >> 
>> > > > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported
>> > > > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented,
>> > > > >> >> compat_props mechanism will be used to disable them for legacy machine types,
>> > > > >> >> this will keep 'hv-default=on' configurations migratable.
>> > > > >> >> 
>> > > > >> >> Signed-off-by: Vitaly Kuznetsov <[e-mail]>
>> > > > >> >> ---
>> > > > >> >>  docs/hyperv.txt   | 16 +++++++++++++---
>> > > > >> >>  target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
>> > > > >> >>  target/i386/cpu.h |  5 +++++
>> > > > >> >>  3 files changed, 56 insertions(+), 3 deletions(-)
>> > > > >> >> 
>> > > > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt
>> > > > >> >> index 5df00da54fc4..a54c066cab09 100644
>> > > > >> >> --- a/docs/hyperv.txt
>> > > > >> >> +++ b/docs/hyperv.txt
>> > > > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features.
>> > > > >> >>  
>> > > > >> >>  2. Setup
>> > > > >> >>  =========
>> > > > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
>> > > > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g:
>> > > > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying
>> > > > >> >> +'hv-default=on' CPU flag:
>> > > > >> >>  
>> > > > >> >> -  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
>> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
>> > > > >> >> +
>> > > > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags,
>> > > > >> >> +e.g:
>> > > > >> >> +
>> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...      
>> > > > >> >
>> > > > >> > I'd put here not '...' but rather recommended list of flags, and update
>> > > > >> > it every time when new feature added if necessary.
>> > > > >> >      
>> > > > >
>> > > > > 1)
>> > > > >      
>> > > > >> This is an example of fine-grained enablement, there is no point to put
>> > > > >> all the existing flags there (hv-default is the only recommended way
>> > > > >> now, the rest is 'expert'/'debugging').    
>> > > > > so users are kept in dark what hv-default disables/enables (and it might depend
>> > > > > on machine version on top that). Doesn't look like a good documentation to me
>> > > > > (sure everyone can go and read source code for it and try to figure out how
>> > > > > it's supposed to work)    
>> > > > 
>> > > > 'hv-default' enables *all* currently supported enlightenments. When
>> > > > using with an old machine type, it will enable *all* Hyper-V
>> > > > enlightenmnets which were supported when the corresponding machine type
>> > > > was released. I don't think we document all other cases when a machine
>> > > > type is modified (i.e. where can I read how pc-q35-5.1 is different from
>> > > > pc-q35-5.0 if I refuse to read the source code?)
>> > > >   
>> > > > >    
>> > > > >>    
>> > > > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't
>> > > > >> > be necessary, I still see it as functionality duplication but I will not oppose it)
>> > > > >> >      
>> > > > >> 
>> > > > >> Unfortunately, upper layer tools don't read this doc and update
>> > > > >> themselves to enable new features when they appear.    
>> > > > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level.
>> > > > > why stop there, just merge with yet another upper layer, it would save us a lot
>> > > > > on communication protocols and simplify VM creation even more,
>> > > > > and no one will have to read docs and write anything new on top.)
>> > > > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction
>> > > > > layers on top of it.    
>> > > > 
>> > > > We have '-machine q35' and we don't require to list all the devices from
>> > > > it. We have '-cpu Skylake-Server' and we don't require to configure all
>> > > > the features manually. Why can't we have similar enablement for Hyper-V
>> > > > emulation where we can't even see a real need for anything but 'enable
>> > > > everything' option?
>> > > > 
>> > > > There is no 'one libvirt to rule them all' (fortunately or
>> > > > unfortunately). And sometimes QEMU is the uppermost layer and there's no
>> > > > 'libvirt' on top of it, this is also a perfectly valid use-case.
>> > > >   
>> > > > >    
>> > > > >> Similarly, if when these tools use '-machine q35' they get all the new features we add
>> > > > >> automatically, right?    
>> > > > > it depends, in case of CPUs, new features usually 'off' by default
>> > > > > for existing models. In case of bugs, features sometimes could be
>> > > > > flipped and versioned machines were used to keep broken CPU models
>> > > > > on old machine types.
>> > > > >    
>> > > > 
>> > > > That's why I was saying that Hyper-V enlightenments hardly resemble
>> > > > 'hardware' CPU features.  
>> > > Well, Microsoft chose to implement them as hardware concept (CPUID leaf),
>> > > and I prefer to treat them the same way as any other CPUID bits.
>> > >   
>> > > >   
>> > > > >        
>> > > > >> >> +It is also possible to disable individual enlightenments from the default list,
>> > > > >> >> +this can be used for debugging purposes:
>> > > > >> >> +
>> > > > >> >> +  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
>> > > > >> >>  
>> > > > >> >>  Sometimes there are dependencies between enlightenments, QEMU is supposed to
>> > > > >> >>  check that the supplied configuration is sane.
>> > > > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> > > > >> >> index 48007a876e32..99338de00f78 100644
>> > > > >> >> --- a/target/i386/cpu.c
>> > > > >> >> +++ b/target/i386/cpu.c
>> > > > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
>> > > > >> >>      cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
>> > > > >> >>  }
>> > > > >> >>  
>> > > > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp)
>> > > > >> >> +{
>> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
>> > > > >> >> +
>> > > > >> >> +    return cpu->hyperv_default;
>> > > > >> >> +}
>> > > > >> >> +
>> > > > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp)
>> > > > >> >> +{
>> > > > >> >> +    X86CPU *cpu = X86_CPU(obj);
>> > > > >> >> +
>> > > > >> >> +    cpu->hyperv_default = value;
>> > > > >> >> +
>> > > > >> >> +    if (value) {
>> > > > >> >> +        cpu->hyperv_features |= cpu->hyperv_default_features;      
>> > > > >> >
>> > > > >> > s/|="/=/ please,
>> > > > >> > i.e. no option overrides whatever was specified before to keep semantics consistent.
>> > > > >> >      
>> > > > >> 
>> > > > >> Hm,
>> > > > >>     
>> > > > >    
>> > > > >> this doesn't matter for the most recent machine type as
>> > > > >> hyperv_default_features has all the features but imagine you're running
>> > > > >> an older machine type which doesn't have 'hv_feature'. Now your    
>> > > > > normally one shouldn't use new feature with old machine type as it makes
>> > > > > VM non-migratable to older QEMU that has this machine type but not this feature.
>> > > > >
>> > > > > nitpicking:
>> > > > >   according to (1) user should not use 'hv_feature' on old machine since
>> > > > >   hv_default should cover all their needs (well they don't know what
>> > > > > hv_default actually is).    
>> > > > 
>> > > > Normally yes but I can imagine sticking to some old machine type for
>> > > > other-than-hyperv-enlightenments purposes and still wanting to add a
>> > > > newly introduced enlightenment. Migration is not always a must.
>> > > >   
>> > > > >    
>> > > > >> suggestion is 
>> > > > >> 
>> > > > >> if I do:
>> > > > >> 
>> > > > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature"
>> > > > >> 
>> > > > >> but if I do
>> > > > >> 
>> > > > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features'
>> > > > >> (as hv_default enablement will overwrite everything)
>> > > > >> 
>> > > > >> How is this consistent?    
>> > > > > usual semantics for properties, is that the latest property overwrites,
>> > > > > the previous property value parsed from left to right.
>> > > > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset,
>> > > > > if one needs more than that one should add more related features after that.
>> > > > >    
>> > > > 
>> > > > This semantics probably doesn't apply to 'hv-default' case IMO as my
>> > > > brain refuses to accept the fact that  
>> > > it's difficult probably because 'hv-default' is 'alias' property 
>> > > that covers all individual hv-foo features in one go and that individual
>> > > features are exposed to user, but otherwise it is just a property that
>> > > sets CPUID features or like any other property, and should be treated like such.
>> > >   
>> > > > 'hv_default,hv_feature' != 'hv_feature,hv_default'
>> > > >
>> > > > which should express the same desire 'the default set PLUS the feature I
>> > > > want'.  
>> > > if hv_default were touching different data, I'd agree.
>> > > But in the end hv_default boils down to the same CPUID bits as individual
>> > > features:
>> > > 
>> > >   hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on
>> > >          !=
>> > >   hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off)  
>> > 
>> > I don't know why you chose to define "hv_default" as
>> > hv_f1=on,hv_f2=off.  If hv_f2 is not enabled by hv_default, it
>> > doesn't need to be touched by hv_default at all.
>> 
>> Essentially I was thinking about hv_default=on as setting default value
>> of hv CPUID leaf i.e. like doc claims, 'all' hv_* features (including
>> turned off and unused bits) which always sets leaf to its default state.
>> 
>> Now lets consider following possible situation
>> using combine' approach (leaf |= some_bits):
>> 
>> QEMU-6.0: initially we have all possible features enabled
>>                 hv_default = (hv_f1=on,hv_f2=on)
>> 
>> hv_f2=on,hv_default=on == hv_f1=on,hv_f2=on
>> 
>> QEMU-6.1: disabled hv_f2=off that was causing problems
>> 
>> hv_default = (hv_f1=on,hv_f2=off)
>
> Why would we choose to do that?
>
> If we decide f2 shouldn't be part of the default, we'll redefine
> hv_default as:
>
>   hv_default = (hv_f1=on)
>
>> 
>> however due to ORing hv_default doesn't fix issue for the same CLI
>> (i.e. it doesn't have expected effect)
>> 
>> hv_f2=on,hv_default=on => hv_f1=on,hv_f2=on
>> 
>> if one would use usual 'set' semantics (leaf = all_bits),
>> then new hv_default value will have desired effect despite of botched CLI,
>> just by virtue of property following typical 'last set' semantics:
>> 
>>  => hv_f1=on,hv_f2=off
>> 
>> If we assume that we 'never ever' will need to disable feature bits
>> than it doesn't matter which approach to use, however a look at
>> pc_compat arrays shows that features are being enabled/disabled
>> all the time.
>
> I'm pretty sure that "hv_default=on will also disable features
> that appear in the command line" will not be a requirement.

Is there a definitive conclusion to this?

In reading the thread I realised that the patch I sent adding
"kvm-no-defaults" may fall foul of the "process properties in order"
rule.

That is..

kvm_clock=on,kvm_no_defaults=on
  ==
kvm_no_defaults=on,kvm_clock=on

...which would not be the case if the properties are processed strictly
in order (because in the first case kvm_no_defaults would disable
kvm_clock).

>> 
>> PS:
>> I'd rename hv_default => hv_set_default,
>> since we would need hv_default[_value] property later on to set compat value
>> based on machine type version.
>>     
>> > > > I think I prefer sanity over purity in this case.  
>> > > what is sanity to one could be insanity for another,
>> > > so I pointed out the way properties expected to work today.
>> > > 
>> > > But you are adding new semantic ('combine') to property/features parsing
>> > > (instead of current 'set' policy), and users will have to be aware of
>> > > this new behavior and add/maintain code for this special case.
>> > > (maybe I worry in vain, and no one will read docs and know about this
>> > > new property anyways)
>> > > 
>> > > That will also push x86 CPUs consolidation farther away from other targets,
>> > > where there aren't any special casing for features parsing, just simple
>> > > left to right parsing with the latest property having overwriting previously
>> > > set value.
>> > > We are trying hard to reduce special cases and unify interfaces for same
>> > > components to simplify qemu and make it predictable/easier for users.
>> > >   
>> > 
>> > What you are proposing diverges from other targets, actually.
>> > See target/s390x/cpu_models.c:set_feature_group() for example.
>> > Enabling a feature group in s390x only enables a set of feature
>> > bits, and doesn't touch the rest.
>> Looking at code, it has the same issue as I described above
>
> I don't see why that's an issue.  This is how feature groups were
> designed, and it works.
>
>
>> 
>> 
>> > In other words, if hv_default includes hv_f1+hv_f2 (and not hv_f3
>> > or hv_f4), this means:
>> > 
>> >    hv_default,hv_f3=on,hv_f4=off => (hv_f1=on,hv_f2=on),hv_f3=on,hv_f4=off
>> >           ==
>> >    hv_f3=on,hv_f4=off,hv_default => hv_f3=on,hv_f4=off,(hv_f2=on,hv_f2=on)
>> > 
>> > That would also mean:
>> > 
>> >    hv_default,hv_f1=on,hv_f2=off => (hv_f1=on,hv_f2=on),hv_f1=on,hv_f2=off
>> >           !=
>> >    hv_f1=on,hv_f2=off,hv_default => hv_f1=on,hv_f2=off,(hv_f2=on,hv_f2=on)
>> > 
>> > That's the behavior implemented by Vitaly.
>> > 
>> > > [...]  
>> > 
>> 
>
> -- 
> Eduardo

dme.

Patch

diff --git a/docs/hyperv.txt b/docs/hyperv.txt
index 5df00da54fc4..a54c066cab09 100644
--- a/docs/hyperv.txt
+++ b/docs/hyperv.txt
@@ -17,10 +17,20 @@  compatible hypervisor and use Hyper-V specific features.
 
 2. Setup
 =========
-No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In
-QEMU, individual enlightenments can be enabled through CPU flags, e.g:
+All currently supported Hyper-V enlightenments can be enabled by specifying
+'hv-default=on' CPU flag:
 
-  qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ...
+  qemu-system-x86_64 --enable-kvm --cpu host,hv-default ...
+
+Alternatively, it is possible to do fine-grained enablement through CPU flags,
+e.g:
+
+  qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ...
+
+It is also possible to disable individual enlightenments from the default list,
+this can be used for debugging purposes:
+
+  qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ...
 
 Sometimes there are dependencies between enlightenments, QEMU is supposed to
 check that the supplied configuration is sane.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 48007a876e32..99338de00f78 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4552,6 +4552,24 @@  static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name,
     cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000;
 }
 
+static bool x86_hv_default_get(Object *obj, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+
+    return cpu->hyperv_default;
+}
+
+static void x86_hv_default_set(Object *obj, bool value, Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+
+    cpu->hyperv_default = value;
+
+    if (value) {
+        cpu->hyperv_features |= cpu->hyperv_default_features;
+    }
+}
+
 /* Generic getter for "feature-words" and "filtered-features" properties */
 static void x86_cpu_get_feature_words(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
@@ -6955,10 +6973,26 @@  static void x86_cpu_initfn(Object *obj)
     object_property_add_alias(obj, "pause_filter", obj, "pause-filter");
     object_property_add_alias(obj, "sse4_1", obj, "sse4.1");
     object_property_add_alias(obj, "sse4_2", obj, "sse4.2");
+    object_property_add_alias(obj, "hv_default", obj, "hv-default");
 
     if (xcc->model) {
         x86_cpu_load_model(cpu, xcc->model);
     }
+
+    /* Hyper-V features enabled with 'hv-default=on' */
+    cpu->hyperv_default_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_IPI) |
+        BIT(HYPERV_FEAT_STIMER_DIRECT);
+
+    /* Enlightened VMCS is only available on Intel/VMX */
+    if (kvm_hv_evmcs_available()) {
+        cpu->hyperv_default_features |= BIT(HYPERV_FEAT_EVMCS);
+    }
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
@@ -7285,6 +7319,10 @@  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
                               x86_cpu_get_crash_info_qom, NULL, NULL, NULL);
 #endif
 
+    object_class_property_add_bool(oc, "hv-default",
+                              x86_hv_default_get,
+                              x86_hv_default_set);
+
     for (w = 0; w < FEATURE_WORDS; w++) {
         int bitnr;
         for (bitnr = 0; bitnr < 64; bitnr++) {
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6220cb2cabb9..8a484becb6b9 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1657,6 +1657,11 @@  struct X86CPU {
     bool hyperv_synic_kvm_only;
     uint64_t hyperv_features;
     bool hyperv_passthrough;
+
+    /* 'hv-default' enablement */
+    uint64_t hyperv_default_features;
+    bool hyperv_default;
+
     OnOffAuto hyperv_no_nonarch_cs;
     uint32_t hyperv_vendor_id[3];
     uint32_t hyperv_interface_id[4];