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