[PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[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 <jan.kiszka@siemens.com>
---

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(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 6bd447f..9ac477b 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
         hw_breakpoint_insert(env, i);
 
     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;
+
+        kvm_load_tsc(env);
+        kvm_load_mpstate(env);
+    }
 
     return 0;
 }

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Avi Kivity]

On 12/03/2009 08:21 PM, Jan Kiszka wrote:
> 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.

> 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.

> 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.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Jan Kiszka]

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

Avi Kivity wrote:
> On 12/03/2009 08:21 PM, Jan Kiszka wrote:
>> 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.
> 
>> 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.

Which also has it's downsides like slow merging progress with quite a
few subtle merge regressions on qemu-kvm side...

> 
>> 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.

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.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Avi Kivity]

On 12/06/2009 12:39 PM, Jan Kiszka wrote:
> 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.
>    

Yes.  Thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Gleb Natapov]

On Sun, Dec 06, 2009 at 11:39:22AM +0100, Jan Kiszka wrote:
> >> 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.
> 
I agree about NMI. We never do RMW to NMI anyway. Mpstate/sipi_vector belong
to APIC state though.

--
			Gleb.

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Gleb Natapov]

On Thu, Dec 03, 2009 at 07:21:01PM +0100, Jan Kiszka wrote:
> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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(-)
> 
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 6bd447f..9ac477b 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
>          hw_breakpoint_insert(env, i);
>  
>      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?

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

--
			Gleb.

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Thu, Dec 03, 2009 at 07:21:01PM +0100, Jan Kiszka wrote:
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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(-)
>>
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index 6bd447f..9ac477b 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
>>          hw_breakpoint_insert(env, i);
>>  
>>      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!

> 
>> +        kvm_load_tsc(env);
>> +        kvm_load_mpstate(env);
>> +    }
>>  
>>      return 0;
>>  }
> 
> --
> 			Gleb.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] qemu-kvm: x86: Fix mismerge in cpu_post_load

[Gleb Natapov]

On Sun, Dec 06, 2009 at 11:44:40AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Thu, Dec 03, 2009 at 07:21:01PM +0100, Jan Kiszka wrote:
> >> 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 <jan.kiszka@siemens.com>
> >> ---
> >>
> >> 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(-)
> >>
> >> diff --git a/target-i386/machine.c b/target-i386/machine.c
> >> index 6bd447f..9ac477b 100644
> >> --- a/target-i386/machine.c
> >> +++ b/target-i386/machine.c
> >> @@ -366,7 +366,15 @@ static int cpu_post_load(void *opaque, int version_id)
> >>          hw_breakpoint_insert(env, i);
> >>  
> >>      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!
> 
Ah, OK then. There was no particular patch that made in kernel irqchip
to not use env->halted. AFAIR it was always this way.

> > 
> >> +        kvm_load_tsc(env);
> >> +        kvm_load_mpstate(env);
> >> +    }
> >>  
> >>      return 0;
> >>  }
> > 
> > --
> > 			Gleb.
> 
> Jan
> 



--
			Gleb.