Re: [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Mar 23, 2018 at 03:58:07PM +0300, Roman Kagan wrote:
> In order to guarantee compatibility on migration, QEMU should have
> complete control over the features it announces to the guest via CPUID.
> 
> However, the declared availability of Hyper-V frequency MSRs
> (HV_X64_MSR_TSC_FREQUENCY and HV_X64_MSR_APIC_FREQUENCY) depends solely
> on the support for them in the underlying KVM.

So this problem was introduced fairly recently (in v2.11).  This
makes the decision to break compatibility of (some[1]) existing
configurations that didn't specify "hv-frequency" a bit easier.

> Introduce "hv-frequencies" cpu property (off by default) which gives
> QEMU full control over whether these MSRs are announced.
> 

So, as we have two possible results when running QEMU-2.11, we
need to make a guess and choose which half of our users will be
affected:

a) People running "-machine pc-2.11 -cpu ...,+hv-time" on Linux 4.14+
   (including commit 72c139bacfa3), that have hv-frequencies
   enabled automatically.
b) People running "-machine pc-2.11 -cpu ...,+hv-time" on Linux
   4.13 and older (without commit 72c139bacfa3), that have
   hv-frequencies disabled.

If we set hv-frequencies=off by default on pc-2.11 (this patch),
we will inconvenience group (a).  The consequence for them is
having hv-frequencies disabled suddenly on CPUID after updating
QEMU.  The MSRs will still be available to the guest, however (so
the guest won't crash), and they can add hv-frequencies=on to
their configuration manually.

If we set hv-frequencies=on by default on pc-2.11, we will
inconvenience group (b).  The consequence for them is having the
VM not being runnable anymore until they change the machine-type
or add hv-frequencies=off to their configuration.

So it looks like this patch is the safest solution, but I will
get back to the "[PATCH v3 2/2] i386/kvm: lower requirements for
Hyper-V frequency MSRs exposure" thread to be sure we are not
missing anything.


> While at this, drop the redundant check of the cpu tsc frequency, and
> decouple this feature from hv-time.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> Note: this patch introduces a new cpu property, which is not what we
> normally do in stable branches.  However, this appears to be the minimal
> effort/churn approach to reduce the number of published QEMU releases
> where the behavior of the feature is unpredictable, with potentially
> fatal consequences for the guest.
> 
>  target/i386/cpu.h |  1 +
>  target/i386/cpu.c |  1 +
>  target/i386/kvm.c | 12 ++++++++----
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 78db1b833a..1b219fafc4 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1296,6 +1296,7 @@ struct X86CPU {
>      bool hyperv_runtime;
>      bool hyperv_synic;
>      bool hyperv_stimer;
> +    bool hyperv_frequencies;
>      bool check_cpuid;
>      bool enforce_cpuid;
>      bool expose_kvm;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 555ae79d29..1a6b082b6f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4761,6 +4761,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
>      DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
>      DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
> +    DEFINE_PROP_BOOL("hv-frequencies", X86CPU, hyperv_frequencies, false),
>      DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
>      DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index d23fff12f5..fb20ff18c2 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -648,11 +648,15 @@ static int hyperv_handle_properties(CPUState *cs)
>          env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE;
>          env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE;
>          env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE;
> -
> -        if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) {
> -            env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
> -            env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
> +    }
> +    if (cpu->hyperv_frequencies) {
> +        if (!has_msr_hv_frequencies) {
> +            fprintf(stderr,
> +                    "Hyper-V frequency MSRs are not supported by kernel\n");
> +            return -ENOSYS;
>          }
> +        env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS;
> +        env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE;
>      }
>      if (cpu->hyperv_crash && has_msr_hv_crash) {
>          env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
> -- 
> 2.14.3
> 

-- 
Eduardo