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-sta...@nongnu.org
> Signed-off-by: Roman Kagan <rka...@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
