[Qemu-devel] [PATCH-for-4.2 0/1] Only enable the halt poll control MSR if it is supported by the host

[Qemu-devel] [PATCH-for-4.2 0/1] Only enable the halt poll control MSR if it is supported by the host

[Mark Kanda]

This patch addresses an issue with the 'queued, but not yet applied' halt
polling MSR patch. With this patch, halt polling is enabled 'by default';
this causes issues with hosts which don't support halt polling. The fix is
to only enable halt polling if it is supported by the host.

Mark Kanda (1):
  Only enable the halt poll control MSR if it is supported by the host

 target/i386/cpu.c     | 8 +++++++-
 target/i386/kvm.c     | 2 --
 target/i386/machine.c | 1 -
 3 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH-for-4.2 1/1] Only enable the halt poll control MSR if it is supported by the host

[Mark Kanda]

The halt poll control MSR should only be enabled on hosts which
support it.

Fixes: ("kvm: i386: halt poll control MSR support")

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 target/i386/cpu.c     | 8 +++++++-
 target/i386/kvm.c     | 2 --
 target/i386/machine.c | 1 -
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a8bafdb8b9..dacbf7a9fe 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2838,7 +2838,6 @@ static PropValue kvm_default_props[] = {
     { "kvm-asyncpf", "on" },
     { "kvm-steal-time", "on" },
     { "kvm-pv-eoi", "on" },
-    { "kvm-poll-control", "on" },
     { "kvmclock-stable-bit", "on" },
     { "x2apic", "on" },
     { "acpi", "off" },
@@ -5109,6 +5108,13 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
     }
 
+    /* Enable the halt poll control MSR if it is supported by the host */
+    if (x86_cpu_get_supported_feature_word(FEAT_KVM, cpu->migratable) &
+        (1 << KVM_FEATURE_POLL_CONTROL)) {
+        env->features[FEAT_KVM] |= 1 << KVM_FEATURE_POLL_CONTROL;
+        env->poll_control_msr = 1;
+    }
+
 out:
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index cb22684139..81dd5d2c1b 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1796,8 +1796,6 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
 
         hyperv_x86_synic_reset(cpu);
     }
-    /* enabled by default */
-    env->poll_control_msr = 1;
 }
 
 void kvm_arch_do_init_vcpu(X86CPU *cpu)
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 20077a8a5a..9d6095b264 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -394,7 +394,6 @@ static bool steal_time_msr_needed(void *opaque)
     return cpu->env.steal_time_msr != 0;
 }
 
-/* Poll control MSR enabled by default */
 static bool poll_control_msr_needed(void *opaque)
 {
     X86CPU *cpu = opaque;
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.2 1/1] Only enable the halt poll control MSR if it is supported by the host

[Paolo Bonzini]

On 16/07/19 23:44, Mark Kanda wrote:
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a8bafdb8b9..dacbf7a9fe 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2838,7 +2838,6 @@ static PropValue kvm_default_props[] = {
>      { "kvm-asyncpf", "on" },
>      { "kvm-steal-time", "on" },
>      { "kvm-pv-eoi", "on" },
> -    { "kvm-poll-control", "on" },
>      { "kvmclock-stable-bit", "on" },
>      { "x2apic", "on" },
>      { "acpi", "off" },
> @@ -5109,6 +5108,13 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>          env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>      }
>  
> +    /* Enable the halt poll control MSR if it is supported by the host */
> +    if (x86_cpu_get_supported_feature_word(FEAT_KVM, cpu->migratable) &
> +        (1 << KVM_FEATURE_POLL_CONTROL)) {
> +        env->features[FEAT_KVM] |= 1 << KVM_FEATURE_POLL_CONTROL;

This is already done by the "if (cpu->max_features)" block (and should
not be done if !cpu->max_features, or you break migration to older hosts).

> +        env->poll_control_msr = 1;
> +    }


> +
>  out:
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index cb22684139..81dd5d2c1b 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1796,8 +1796,6 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
>  
>          hyperv_x86_synic_reset(cpu);
>      }
> -    /* enabled by default */
> -    env->poll_control_msr = 1;
>  }
>  
>  void kvm_arch_do_init_vcpu(X86CPU *cpu)
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 20077a8a5a..9d6095b264 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -394,7 +394,6 @@ static bool steal_time_msr_needed(void *opaque)
>      return cpu->env.steal_time_msr != 0;
>  }
>  
> -/* Poll control MSR enabled by default */
>  static bool poll_control_msr_needed(void *opaque)
>  {
>      X86CPU *cpu = opaque;
> 

This will cause the migration subsection to be sent always on old hosts,
thus breaking migration to older QEMU.  I think leaving in the "/*
enabled by default */" hunk is fine, because indeed on hosts that lack
the MSR you have the same behavior as if it were 1.

So all in all only the first hunk is needed.

Paolo