qemu-kvm: x86: Fix mismerge in cpu_post_load

Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in cpu_post_load. mp_state was readded later on, but tsc was missing, breaking the guest timing after resume. Also, reset of halt was dropped which is obviously required for in-kernel irqchip.

BTW, this patch just made me realize that the TSC MSR belongs to the list states that should not be written back unconditionally. Upstream does this currently, qemu-kvm not (at the price one more kvm-specific hook into generic code).

Unlike the other states we discussed, this one
is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in cpu_post_load. mp_state was readded later on, but tsc was missing, breaking the guest timing after resume. Also, reset of halt was dropped which is obviously required for in-kernel irqchip.

Signed-off-by Jan Kiszka <[email protected]>

---

This solves "Problem 1" on my list.

BTW, this patch just made me realize that the TSC MSR belongs to the list states that should not be written back unconditionally. Upstream does this currently, qemu-kvm not (at the price one more kvm-specific hook into generic code). Unlike the other states we discussed, this one is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

 target-i386/machine.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

target-i386/machine.c +0 -0

index 6bd447f..9ac477b 100644

Quoted 5 lines · Jan Kiszka

Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in cpu_post_load. mp_state was readded later on, but tsc was missing, breaking the guest timing after resume. Also, reset of halt was dropped which is obviously required for in-kernel irqchip.

Applied, thanks.

Quoted 4 lines

BTW, this patch just made me realize that the TSC MSR belongs to the list states that should not be written back unconditionally. Upstream does this currently, qemu-kvm not (at the price one more kvm-specific hook into generic code).

Yet another reminder why we don't rewrite, only refactor.

Quoted 6 lines

Unlike the other states we discussed, this one
is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

Yes. Gleb suggested treating mpstate/nmi as part of the APIC state (which makes sense), which would work, as APIC state is not automatically written back. But the tsc msr would need special treatment.

That said, I think we should hurry to add some mask to the new KVM_SET_VCPU_EVENTS (for 2.6.33) in order to skip nmi_pending and sipi_vector, giving us maximum flexibility how to address the issue. Will send a patch later.

Quoted 28 lines · Jan Kiszka

Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in cpu_post_load. mp_state was readded later on, but tsc was missing, breaking the guest timing after resume. Also, reset of halt was dropped which is obviously required for in-kernel irqchip.

Signed-off-by Jan Kiszka <[email protected]>

---

This solves "Problem 1" on my list.

BTW, this patch just made me realize that the TSC MSR belongs to the list states that should not be written back unconditionally. Upstream does this currently, qemu-kvm not (at the price one more kvm-specific hook into generic code). Unlike the other states we discussed, this one is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

 target-i386/machine.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

target-i386/machine.c +0 -0

index 6bd447f..9ac477b 100644
@@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
         hw_breakpoint_insert(env, i);
 

target-i386/machine.c static int cpu_post_load(void *opaque, int version_id) · line 374

Quoted 8 lines

target-i386/machine.c +6 -1

     tlb_flush(env, 1);
-    kvm_load_mpstate(env);
+
+    if (kvm_enabled()) {
+        /* when in-kernel irqchip is used, env->halted causes deadlock
+           because no userspace IRQs will ever clear this flag */
+        env->halted = 0;
+

That is strange. env->halted should be used only for "info cpus" output in case of in-kernel irqchip. Can you see where it hangs?

target-i386/machine.c static int cpu_post_load(void *opaque, int version_id) · line 380

Quoted 7 lines

target-i386/machine.c +3 -0

+        kvm_load_tsc(env);
+        kvm_load_mpstate(env);
+    }
 
     return 0;
 }

--
Gleb.

Quoted 29 lines · Gleb Natapov

Quoted 28 lines · Jan Kiszka

Merge 8e2c5ec2f6 forgot to restore some qemu-kvm-specific hooks in cpu_post_load. mp_state was readded later on, but tsc was missing, breaking the guest timing after resume. Also, reset of halt was dropped which is obviously required for in-kernel irqchip.

Signed-off-by Jan Kiszka <[email protected]>

---

This solves "Problem 1" on my list.

BTW, this patch just made me realize that the TSC MSR belongs to the list states that should not be written back unconditionally. Upstream does this currently, qemu-kvm not (at the price one more kvm-specific hook into generic code). Unlike the other states we discussed, this one is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

 target-i386/machine.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

target-i386/machine.c +0 -0

index 6bd447f..9ac477b 100644
@@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
         hw_breakpoint_insert(env, i);
 

Quoted 10 lines

target-i386/machine.c static int cpu_post_load(void *opaque, int version_id) · line 374

Quoted 8 lines

target-i386/machine.c +6 -1

     tlb_flush(env, 1);
-    kvm_load_mpstate(env);
+
+    if (kvm_enabled()) {
+        /* when in-kernel irqchip is used, env->halted causes deadlock
+           because no userspace IRQs will ever clear this flag */
+        env->halted = 0;
+

That is strange. env->halted should be used only for "info cpus" output in case of in-kernel irqchip. Can you see where it hangs?

This line was not directly involved in the regression I saw, it was just the next one (load_tsc). But I simply restored qemu-kvm to the state before the vmstate conversion merge, dropping only obviously unneeded bits (namely the full register write-back).

The above line may no longer be required, but I wanted to play safe. If you can explain which qemu-kvm change made this obsolete, please file a removal patch!

Quoted 5 lines

Quoted 5 lines

target-i386/machine.c +3 -0

+        kvm_load_tsc(env);
+        kvm_load_mpstate(env);
+    }
 

Quoted 5 lines

target-i386/machine.c static int cpu_post_load(void *opaque, int version_id) · line 380

Quoted 2 lines

target-i386/machine.c +0 -0

     return 0;
 }

--
Gleb.

Jan

Quoted 9 lines

Quoted 5 lines

Unlike the other states we discussed, this one
is not "fixable" in the kernel. So I tend to think there is a real need for my write-back scope abstraction - which would also be able to handle the other states cleanly, both in upstream and here.

Yes. Gleb suggested treating mpstate/nmi as part of the APIC state (which makes sense), which would work, as APIC state is not automatically written back. But the tsc msr would need special treatment.

(just realized that I forgot to answer him)

While this would make sense for mpstate, NMIs are not coupled to the APICs. The APIC just happens to be one source for them (though a common one). So if there is no in-kernel APIC state, there would never be a write-back of the NMI state, which is bad.