Re: [Qemu-devel] [patch QEMU] kvm: i386: halt poll control MSR support

[Mark Kanda <mark.kanda@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 7488 bytes --]

Hi all,

If the host doesn't support halt polling, this patch seems to break 
libvirt save/restore:

"
virsh # save halt-poll-vm halt-poll-vm.sav --running --verbose
Save: [100 %]
Domain halt-poll-vm saved to halt-poll-vm.sav

virsh # restore halt-poll-vm.sav
error: Failed to restore domain from halt-poll-vm.sav
error: operation failed: guest CPU doesn't match specification
"

I believe this occurs because libvirt rejects the restore if there are 
filtered features, which is the case if halt polling is enabled on a 
host which doesn't support it (halt polling is enabled 'by default').

As such, I think we should only enable halt polling if it is supported 
on the host - see the attached patch.

...thoughts?

Thanks,
-Mark

On 7/15/2019 4:23 AM, Paolo Bonzini wrote:
> On 04/06/19 01:04, Marcelo Tosatti wrote:
>> (CC'ing qemu devel)
>>
>> Add support for halt poll control MSR: save/restore, migration
>> and new feature name.
>>
>> The purpose of this MSR is to allow the guest to disable
>> host halt poll.
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> diff --git a/include/standard-headers/asm-x86/kvm_para.h b/include/standard-headers/asm-x86/kvm_para.h
>> index 35cd8d6..e171514 100644
>> --- a/include/standard-headers/asm-x86/kvm_para.h
>> +++ b/include/standard-headers/asm-x86/kvm_para.h
>> @@ -29,6 +29,7 @@
>>   #define KVM_FEATURE_PV_TLB_FLUSH	9
>>   #define KVM_FEATURE_ASYNC_PF_VMEXIT	10
>>   #define KVM_FEATURE_PV_SEND_IPI	11
>> +#define KVM_FEATURE_POLL_CONTROL	12
>>   
>>   #define KVM_HINTS_REALTIME      0
>>   
>> @@ -47,6 +48,7 @@
>>   #define MSR_KVM_ASYNC_PF_EN 0x4b564d02
>>   #define MSR_KVM_STEAL_TIME  0x4b564d03
>>   #define MSR_KVM_PV_EOI_EN      0x4b564d04
>> +#define MSR_KVM_POLL_CONTROL	0x4b564d05
>>   
>>   struct kvm_steal_time {
>>   	uint64_t steal;
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index c1ab86d..1ca6944 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -903,7 +903,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>               "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
>>               "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
>>               NULL, "kvm-pv-tlb-flush", NULL, "kvm-pv-ipi",
>> -            NULL, NULL, NULL, NULL,
>> +            "kvm-poll-control", NULL, NULL, NULL,
>>               NULL, NULL, NULL, NULL,
>>               NULL, NULL, NULL, NULL,
>>               "kvmclock-stable-bit", NULL, NULL, NULL,
>> @@ -3001,6 +3001,7 @@ 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" },
>> @@ -5660,6 +5661,8 @@ static void x86_cpu_initfn(Object *obj)
>>       object_property_add_alias(obj, "kvm_steal_time", obj, "kvm-steal-time", &error_abort);
>>       object_property_add_alias(obj, "kvm_pv_eoi", obj, "kvm-pv-eoi", &error_abort);
>>       object_property_add_alias(obj, "kvm_pv_unhalt", obj, "kvm-pv-unhalt", &error_abort);
>> +    object_property_add_alias(obj, "kvm_poll_control", obj, "kvm-poll-control",
>> +                              &error_abort);
>>       object_property_add_alias(obj, "svm_lock", obj, "svm-lock", &error_abort);
>>       object_property_add_alias(obj, "nrip_save", obj, "nrip-save", &error_abort);
>>       object_property_add_alias(obj, "tsc_scale", obj, "tsc-scale", &error_abort);
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index bd06523..21ed2f8 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1241,6 +1241,7 @@ typedef struct CPUX86State {
>>       uint64_t steal_time_msr;
>>       uint64_t async_pf_en_msr;
>>       uint64_t pv_eoi_en_msr;
>> +    uint64_t poll_control_msr;
>>   
>>       /* Partition-wide HV MSRs, will be updated only on the first vcpu */
>>       uint64_t msr_hv_hypercall;
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 3b29ce5..a5e9cdf 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1369,6 +1369,8 @@ 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)
>> @@ -2059,6 +2061,11 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
>>           if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
>>               kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, env->steal_time_msr);
>>           }
>> +
>> +        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
>> +            kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, env->poll_control_msr);
>> +        }
>> +
>>           if (has_architectural_pmu_version > 0) {
>>               if (has_architectural_pmu_version > 1) {
>>                   /* Stop the counter.  */
>> @@ -2443,6 +2450,9 @@ static int kvm_get_msrs(X86CPU *cpu)
>>       if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
>>           kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, 0);
>>       }
>> +    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
>> +        kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, 1);
>> +    }
>>       if (has_architectural_pmu_version > 0) {
>>           if (has_architectural_pmu_version > 1) {
>>               kvm_msr_entry_add(cpu, MSR_CORE_PERF_FIXED_CTR_CTRL, 0);
>> @@ -2677,6 +2687,10 @@ static int kvm_get_msrs(X86CPU *cpu)
>>           case MSR_KVM_STEAL_TIME:
>>               env->steal_time_msr = msrs[i].data;
>>               break;
>> +        case MSR_KVM_POLL_CONTROL: {
>> +            env->poll_control_msr = msrs[i].data;
>> +            break;
>> +        }
>>           case MSR_CORE_PERF_FIXED_CTR_CTRL:
>>               env->msr_fixed_ctr_ctrl = msrs[i].data;
>>               break;
>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>> index 225b5d4..1c23e5e 100644
>> --- a/target/i386/machine.c
>> +++ b/target/i386/machine.c
>> @@ -323,6 +323,14 @@ 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;
>> +
>> +    return cpu->env.poll_control_msr != 1;
>> +}
>> +
>>   static const VMStateDescription vmstate_steal_time_msr = {
>>       .name = "cpu/steal_time_msr",
>>       .version_id = 1,
>> @@ -356,6 +364,17 @@ static const VMStateDescription vmstate_pv_eoi_msr = {
>>       }
>>   };
>>   
>> +static const VMStateDescription vmstate_poll_control_msr = {
>> +    .name = "cpu/poll_control_msr",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = poll_control_msr_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(env.poll_control_msr, X86CPU),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static bool fpop_ip_dp_needed(void *opaque)
>>   {
>>       X86CPU *cpu = opaque;
>> @@ -1062,6 +1081,7 @@ VMStateDescription vmstate_x86_cpu = {
>>           &vmstate_async_pf_msr,
>>           &vmstate_pv_eoi_msr,
>>           &vmstate_steal_time_msr,
>> +        &vmstate_poll_control_msr,
>>           &vmstate_fpop_ip_dp,
>>           &vmstate_msr_tsc_adjust,
>>           &vmstate_msr_tscdeadline,
>>
> 
> Queued, thanks.  Sorry for missing it until now.
> 
> Paolo
> 
> 

[-- Attachment #2: 0001-Only-enable-the-halt-poll-control-MSR-if-it-is-suppo.patch --]
[-- Type: text/plain, Size: 2269 bytes --]

From ac83cb52be3fbe226645e780500adb767f9bcfd2 Mon Sep 17 00:00:00 2001
From: Mark Kanda <mark.kanda@oracle.com>
Date: Tue, 16 Jul 2019 14:46:11 -0500
Subject: [PATCH QEMU] Only enable the halt poll control MSR if it is supported
 by the host

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