diff mbox series

[for-2.12,2/2] i386/hyperv: error out if features requested but unsupported

Message ID 20180323125808.4479-3-rkagan@virtuozzo.com
State New
Headers show
Series i386/hyperv: fully control Hyper-V features in CPUID | expand

Commit Message

Roman Kagan March 23, 2018, 12:58 p.m. UTC 
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.

Cc: qemu-stable@nongnu.org
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 target/i386/kvm.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Eduardo Habkost March 23, 2018, 7:56 p.m. UTC | #1 
On Fri, Mar 23, 2018 at 03:58:08PM +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.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>

I wonder if we should make these just warnings on -stable, and
make them fatal errors only on 2.12.  I wouldn't want to make
existing running VMs not runnable on a stable update.


> ---
>  target/i386/kvm.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index fb20ff18c2..c9c359241c 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -658,17 +658,34 @@ 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 are not supported by kernel\n");

I would mention the corresponding "hv-..." -cpu option
explicitly, for clarity.

> +            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 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 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_INDEX is not supported by kernel\n");
> +            return -ENOSYS;
> +        }
>          env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
>      }
>      if (cpu->hyperv_synic) {
> -- 
> 2.14.3
>
Roman Kagan March 26, 2018, 3:06 p.m. UTC | #2 
On Fri, Mar 23, 2018 at 04:56:21PM -0300, Eduardo Habkost wrote:
> On Fri, Mar 23, 2018 at 03:58:08PM +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.
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> I wonder if we should make these just warnings on -stable, and
> make them fatal errors only on 2.12.  I wouldn't want to make
> existing running VMs not runnable on a stable update.

OK let's follow your approach and consider who would suffer more:

a) users who started a VM on a newer kernel and then migrated to an
   older one, and got a guest crash on an unsupported MSR (I'm not even
   sure they'd be able to see the warnings)

b) users who had a VM configuration with features which didn't actually
   work (but that wasn't apparently a problem for them), and suddenly
   couldn't start their VM after QEMU upgrade (the problem is not only
   cold-restarting per se, but also live migration to the upgraded
   version: dunno if the management layer would allow to adjust the VM
   config to migrate successfully).

I don't have a strong opinion on this, will follow whatever direction
you advise.


> >  target/i386/kvm.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index fb20ff18c2..c9c359241c 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -658,17 +658,34 @@ 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 are not supported by kernel\n");
> 
> I would mention the corresponding "hv-..." -cpu option
> explicitly, for clarity.

This sounds like a good idea, but I wonder if it should better be done
uniformly for all hv_* options, together with transitioning to
error_report(), and whether we want to do this at this point in the
release cycle.

Thanks,
Roman.
Eduardo Habkost March 28, 2018, 2:18 p.m. UTC | #3 
On Mon, Mar 26, 2018 at 06:06:16PM +0300, Roman Kagan wrote:
> On Fri, Mar 23, 2018 at 04:56:21PM -0300, Eduardo Habkost wrote:
> > On Fri, Mar 23, 2018 at 03:58:08PM +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.
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > 
> > I wonder if we should make these just warnings on -stable, and
> > make them fatal errors only on 2.12.  I wouldn't want to make
> > existing running VMs not runnable on a stable update.
> 
> OK let's follow your approach and consider who would suffer more:
> 
> a) users who started a VM on a newer kernel and then migrated to an
>    older one, and got a guest crash on an unsupported MSR (I'm not even
>    sure they'd be able to see the warnings)
> 
> b) users who had a VM configuration with features which didn't actually
>    work (but that wasn't apparently a problem for them), and suddenly
>    couldn't start their VM after QEMU upgrade (the problem is not only
>    cold-restarting per se, but also live migration to the upgraded
>    version: dunno if the management layer would allow to adjust the VM
>    config to migrate successfully).
> 
> I don't have a strong opinion on this, will follow whatever direction
> you advise.

(a) is an existing bug (and rare, it seems: I didn't see it being
reported anywhere).  (b) would be a regression in a -stable
update.  I'd prefer to be conservative on -stable.

> 
> 
> > >  target/i386/kvm.c | 25 +++++++++++++++++++++----
> > >  1 file changed, 21 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > > index fb20ff18c2..c9c359241c 100644
> > > --- a/target/i386/kvm.c
> > > +++ b/target/i386/kvm.c
> > > @@ -658,17 +658,34 @@ 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 are not supported by kernel\n");
> > 
> > I would mention the corresponding "hv-..." -cpu option
> > explicitly, for clarity.
> 
> This sounds like a good idea, but I wonder if it should better be done
> uniformly for all hv_* options, together with transitioning to
> error_report(), and whether we want to do this at this point in the
> release cycle.

I don't think it will be a problem if we mention the property
names in the new messages, but not try to reword the existing
ones.
diff mbox series

Patch

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index fb20ff18c2..c9c359241c 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -658,17 +658,34 @@  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 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 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 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_INDEX is not supported by kernel\n");
+            return -ENOSYS;
+        }
         env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE;
     }
     if (cpu->hyperv_synic) {