[Qemu-devel] [PATCH for-2.12 v2 0/2] i386/hyperv: fully control Hyper-V features in CPUID

[Qemu-devel] [PATCH for-2.12 v2 0/2] i386/hyperv: fully control Hyper-V features in CPUID

[Roman Kagan]

In order to guarantee compatibility on migration, QEMU should have
complete control over the features it announces to the guest via CPUID.

However, a number of Hyper-V-related features happen to depend on the
support in the underlying KVM, with no regard to QEMU configuration.

Make QEMU regain control over what Hyper-V features it announces to the
guest.

Note #1: the patches are also being proposed[*] for stable-2.11, even
though one of them introduces a new cpu property.  This is done to
minimize the number of published QEMU releases where the behavior of the
features is unpredictable, with potentially fatal consequences for the
guest.

Note #2: there are other problems in the surrounding code, like ugly
error reporting or inconsistent population of MSRs.  I think this can be
put off to post-2.12.

[*] for the stable branch the second patch will have error returns
replaced with warnings; I'll post a separate series.

v1 -> v2:
 - indicate what flag requested the feature that can't be enabled in the
   error message
 - fix a typo in the error message for VP_RUNTIME

Roman Kagan (2):
  i386/hyperv: add hv-frequencies cpu property
  i386/hyperv: error out if features requested but unsupported

 target/i386/cpu.h |  1 +
 target/i386/cpu.c |  1 +
 target/i386/kvm.c | 45 +++++++++++++++++++++++++++++++++++++--------
 3 files changed, 39 insertions(+), 8 deletions(-)

-- 
2.14.3

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

[Roman Kagan]

In order to guarantee compatibility on migration, QEMU should have
complete control over the features it announces to the guest via CPUID.

However, the 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.

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

While at this, drop the redundant check of the cpu tsc frequency, and
decouple this feature from hv-time.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
 - indicate what flag requested the feature that can't be enabled in the
   error message

 target/i386/cpu.h |  1 +
 target/i386/cpu.c |  1 +
 target/i386/kvm.c | 13 +++++++++----
 3 files changed, 11 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..b35623ae24 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -648,11 +648,16 @@ 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 "
+                    "(requested by 'hv-frequencies' cpu flag) "
+                    "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

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

[Eduardo Habkost]

On Wed, Mar 28, 2018 at 06:30:23PM +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 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.
> 
> Introduce "hv-frequencies" cpu property (off by default) which gives
> QEMU full control over whether these MSRs are announced.
> 
> While at this, drop the redundant check of the cpu tsc frequency, and
> decouple this feature from hv-time.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> v1 -> v2:
>  - indicate what flag requested the feature that can't be enabled in the
>    error message
> 
>  target/i386/cpu.h |  1 +
>  target/i386/cpu.c |  1 +
>  target/i386/kvm.c | 13 +++++++++----
>  3 files changed, 11 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..b35623ae24 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -648,11 +648,16 @@ 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 "
> +                    "(requested by 'hv-frequencies' cpu flag) "
> +                    "are not supported by kernel\n");
> +            return -ENOSYS;

I would like to move this to x86_cpu_filter_features(), but while
we don't refactor the Hyper-V CPUID code, this is good enough for
now.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>          }
> +        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

[Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Roman Kagan]

In order to guarantee compatibility on migration, QEMU should have
complete control over the features it announces to the guest via CPUID.

However, for a number of Hyper-V-related cpu properties, if the
corresponding feature is not supported by the underlying KVM, the
propery is silently ignored and the feature is not announced to the
guest.

Refuse to start with an error instead.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v1 -> v2:
 - indicate what flag requested the feature that can't be enabled in the
   error message
 - fix a typo in the error message for VP_RUNTIME

 target/i386/kvm.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index b35623ae24..113926aff2 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -659,17 +659,41 @@ static int hyperv_handle_properties(CPUState *cs)
         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) {
+    if (cpu->hyperv_crash) {
+        if (!has_msr_hv_crash) {
+            fprintf(stderr, "Hyper-V crash MSRs "
+                    "(requested by 'hv-crash' cpu flag) "
+                    "are not supported by kernel\n");
+            return -ENOSYS;
+        }
         env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
     }
     env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
-    if (cpu->hyperv_reset && has_msr_hv_reset) {
+    if (cpu->hyperv_reset) {
+        if (!has_msr_hv_reset) {
+            fprintf(stderr, "Hyper-V reset MSR "
+                    "(requested by 'hv-reset' cpu flag) "
+                    "is not supported by kernel\n");
+            return -ENOSYS;
+        }
         env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE;
     }
-    if (cpu->hyperv_vpindex && has_msr_hv_vpindex) {
+    if (cpu->hyperv_vpindex) {
+        if (!has_msr_hv_vpindex) {
+            fprintf(stderr, "Hyper-V VP_INDEX MSR "
+                    "(requested by 'hv-vpindex' cpu flag) "
+                    "is not supported by kernel\n");
+            return -ENOSYS;
+        }
         env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE;
     }
-    if (cpu->hyperv_runtime && has_msr_hv_runtime) {
+    if (cpu->hyperv_runtime) {
+        if (!has_msr_hv_runtime) {
+            fprintf(stderr, "Hyper-V VP_RUNTIME MSR "
+                    "(requested by 'hv-runtime' cpu flag) "
+                    "is not supported by kernel\n");
+            return -ENOSYS;
+        }
         env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
     }
     if (cpu->hyperv_synic) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Eduardo Habkost]

On Wed, Mar 28, 2018 at 06:30:24PM +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, for a number of Hyper-V-related cpu properties, if the
> corresponding feature is not supported by the underlying KVM, the
> propery is silently ignored and the feature is not announced to the
> guest.
> 
> Refuse to start with an error instead.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

Something I didn't consider before:

Will this block migration before it even starts, or will crash
the VM only after all migration data was sent to the destination?

I didn't test it, but kvm_arch_init_vcpu() seems to be too late
to block an invalid/unsupport configuration.

Maybe we can simply call hyperv_handle_properties() earlier,
inside x86_cpu_realizefn()?

(I know it's very late for this kind of intrusive change in
v2.12, but I still think it's a good idea to fix this as soon as
possible.)


> ---
> v1 -> v2:
>  - indicate what flag requested the feature that can't be enabled in the
>    error message
>  - fix a typo in the error message for VP_RUNTIME
> 
>  target/i386/kvm.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index b35623ae24..113926aff2 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -659,17 +659,41 @@ static int hyperv_handle_properties(CPUState *cs)
>          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) {
> +    if (cpu->hyperv_crash) {
> +        if (!has_msr_hv_crash) {
> +            fprintf(stderr, "Hyper-V crash MSRs "
> +                    "(requested by 'hv-crash' cpu flag) "
> +                    "are not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE;
>      }
>      env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE;
> -    if (cpu->hyperv_reset && has_msr_hv_reset) {
> +    if (cpu->hyperv_reset) {
> +        if (!has_msr_hv_reset) {
> +            fprintf(stderr, "Hyper-V reset MSR "
> +                    "(requested by 'hv-reset' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE;
>      }
> -    if (cpu->hyperv_vpindex && has_msr_hv_vpindex) {
> +    if (cpu->hyperv_vpindex) {
> +        if (!has_msr_hv_vpindex) {
> +            fprintf(stderr, "Hyper-V VP_INDEX MSR "
> +                    "(requested by 'hv-vpindex' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE;
>      }
> -    if (cpu->hyperv_runtime && has_msr_hv_runtime) {
> +    if (cpu->hyperv_runtime) {
> +        if (!has_msr_hv_runtime) {
> +            fprintf(stderr, "Hyper-V VP_RUNTIME MSR "
> +                    "(requested by 'hv-runtime' cpu flag) "
> +                    "is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
>      }
>      if (cpu->hyperv_synic) {
> -- 
> 2.14.3
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Roman Kagan]

On Wed, Mar 28, 2018 at 03:53:31PM -0300, Eduardo Habkost wrote:
> On Wed, Mar 28, 2018 at 06:30:24PM +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, for a number of Hyper-V-related cpu properties, if the
> > corresponding feature is not supported by the underlying KVM, the
> > propery is silently ignored and the feature is not announced to the
> > guest.
> > 
> > Refuse to start with an error instead.
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> Something I didn't consider before:
> 
> Will this block migration before it even starts, or will crash
> the VM only after all migration data was sent to the destination?
> 
> I didn't test it, but kvm_arch_init_vcpu() seems to be too late
> to block an invalid/unsupport configuration.

I just did a simple test, force-failing one of the checks and starting
QEMU with -incoming defer.  It refused to start with the expected error
message.  IOW in the migration case the destination will abort before
the source have started to send any data.

> Maybe we can simply call hyperv_handle_properties() earlier,
> inside x86_cpu_realizefn()?

Now it's

  ...
  x86_cpu_realizefn
    qemu_init_vcpu
      qemu_kvm_start_vcpu
        qemu_thread_create(qemu_kvm_cpu_thread_fn)
	  [in vcpu thread]
	  qemu_kvm_cpu_thread_fn
	    kvm_init_vcpu
	      kvm_arch_init_vcpu
	        hyperv_handle_properties
		  [error return]
		[error return]
	      [error return]
	    exit(1)

> (I know it's very late for this kind of intrusive change in
> v2.12, but I still think it's a good idea to fix this as soon as
> possible.)

I agree that the current hyperv flag handling begs for cleanup but I
think this can wait for post-2.12.

> > ---
> > v1 -> v2:
> >  - indicate what flag requested the feature that can't be enabled in the
> >    error message
> >  - fix a typo in the error message for VP_RUNTIME

I just noticed that I missed hv-time being silently cleared, too (just
in a slightly different pattern), so I'll have to respin.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Eduardo Habkost]

On Thu, Mar 29, 2018 at 12:47:42PM +0300, Roman Kagan wrote:
> On Wed, Mar 28, 2018 at 03:53:31PM -0300, Eduardo Habkost wrote:
> > On Wed, Mar 28, 2018 at 06:30:24PM +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, for a number of Hyper-V-related cpu properties, if the
> > > corresponding feature is not supported by the underlying KVM, the
> > > propery is silently ignored and the feature is not announced to the
> > > guest.
> > > 
> > > Refuse to start with an error instead.
> > > 
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > 
> > Something I didn't consider before:
> > 
> > Will this block migration before it even starts, or will crash
> > the VM only after all migration data was sent to the destination?
> > 
> > I didn't test it, but kvm_arch_init_vcpu() seems to be too late
> > to block an invalid/unsupport configuration.
> 
> I just did a simple test, force-failing one of the checks and starting
> QEMU with -incoming defer.  It refused to start with the expected error
> message.  IOW in the migration case the destination will abort before
> the source have started to send any data.
> 
> > Maybe we can simply call hyperv_handle_properties() earlier,
> > inside x86_cpu_realizefn()?
> 
> Now it's
> 
>   ...
>   x86_cpu_realizefn
>     qemu_init_vcpu
>       qemu_kvm_start_vcpu
>         qemu_thread_create(qemu_kvm_cpu_thread_fn)
> 	  [in vcpu thread]
> 	  qemu_kvm_cpu_thread_fn

Oh, this is the part that I missed.  I thought the vCPU thread
wouldn't start until migration was finished.

This means the patch is OK as-is.  Sorry for the confusion.

> 	    kvm_init_vcpu
> 	      kvm_arch_init_vcpu
> 	        hyperv_handle_properties
> 		  [error return]
> 		[error return]
> 	      [error return]
> 	    exit(1)
> 
> > (I know it's very late for this kind of intrusive change in
> > v2.12, but I still think it's a good idea to fix this as soon as
> > possible.)
> 
> I agree that the current hyperv flag handling begs for cleanup but I
> think this can wait for post-2.12.
> 
> > > ---
> > > v1 -> v2:
> > >  - indicate what flag requested the feature that can't be enabled in the
> > >    error message
> > >  - fix a typo in the error message for VP_RUNTIME
> 
> I just noticed that I missed hv-time being silently cleared, too (just
> in a slightly different pattern), so I'll have to respin.
> 
> Thanks,
> Roman.

-- 
Eduardo