Re: [Qemu-devel] [PATCH] target-i386: clear guest TSC on reset

[Marcelo Tosatti]

On Fri, Dec 06, 2013 at 05:24:18PM +0900, Fernando Luis Vázquez Cao wrote:
> On 12/06/2013 01:38 AM, Paolo Bonzini wrote:
> >Il 05/12/2013 17:17, Marcelo Tosatti ha scritto:
> >>>>I agree it is a bit ugly, but in my testing QEMU seemed to loop over all
> >>>>the VCPUS fast enough for the kernel side kvm_write_tsc() to do a
> >>>>reasonable job of matching the offsets (the Linux guest did not mark
> >>>>the TSC unstable due to the TSCs being unsynchronized). Am I missing
> >>>>something?
> >>Right, modern kernels (see kvm_write_tsc) perform synchronization, so in
> >>theory the "KVM is yet unable to synchronize ..." code is not necessary
> >>anymore.
> >>
> >>I vote for dropping the thing entirely.
> 
> When I was writing the original patch I was tempted to do that,
> but I feared that it could break older kernels that do not have
> TSC synchronization code. Should we care about such uses
> (recent QEMU user space + old kernel)?

Unfortunately there is no clean way to detect kernels that support TSC
write synchronization (not directly at least).

However the combination of recent qemu, pre-2010 kernel, and no kvmclock
Linux guest can be ignored in my POV.

> I also wanted to make sure that the initialization that we do
> in kvm_arch_vcpu_postcreate on power up and the subsequent
> TSC writeback work well together, but I didn't have time to
> test it (reading the code, I would say that the TSC generation
> counter may end up being increased a few times but the TSCs
> would eventually converge).

A basic test should be fine, because the matching code is in use 
today.

> >If it can be dropped entirely, I certainly have no problem with starting
> >with a simple patch first.
> 
> Could we start with the patch that I already sent? It's been
> tested, it is conservative in the sense that it does the minimum
> necessary to fix an existing bug, and should be easy to
> backport. I will be replying to this email with an updated
> version that has a more appropriate and less scary patch
> description.
> 
> I will also be sending a patch that makes the TSC writeback
> unconditional, but this one should probably be kept on hold
> until it is properly tested.
> 
> As a follow-up effort we can work on Paolo's suggestions.
> 
> Is this an acceptable way forward?
> 
> Thanks,
> Fernando