mbox series

[0/2] KVM: x86: do not mix raw and monotonic clocks in kvmclock

Message ID 1579702953-24184-1-git-send-email-pbonzini@redhat.com (mailing list archive)
Headers show
Series KVM: x86: do not mix raw and monotonic clocks in kvmclock | expand

Message

Paolo Bonzini Jan. 22, 2020, 2:22 p.m. UTC
Commit 53fafdbb8b21f ("KVM: x86: switch KVMCLOCK base to monotonic raw
clock") changed kvmclock to use tkr_raw instead of tkr_mono.  However,
the default kvmclock_offset for the VM was still based on the monotonic
clock and, if the raw clock drifted enough from the monotonic clock,
this could cause a negative system_time to be written to the guest's
struct pvclock.  RHEL5 does not like it and (if it boots fast enough to
observe a negative time value) it hangs.

This series fixes the issue by using the raw clock everywhere.

(And this, ladies and gentlemen, is why I was not applying patches to
the KVM tree.  I saw this before Christmas and could only reproduce it
today, since it requires almost 2 weeks of uptime to reproduce on my
machine.  Of course, once you have the reproducer the fix is relatively
easy to come up with).

Paolo

Paolo Bonzini (2):
  KVM: x86: reorganize pvclock_gtod_data members
  KVM: x86: use raw clock values consistently

 arch/x86/kvm/x86.c | 67 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

Comments

Marcelo Tosatti Jan. 24, 2020, 8:36 p.m. UTC | #1
On Wed, Jan 22, 2020 at 03:22:31PM +0100, Paolo Bonzini wrote:
> Commit 53fafdbb8b21f ("KVM: x86: switch KVMCLOCK base to monotonic raw
> clock") changed kvmclock to use tkr_raw instead of tkr_mono.  However,
> the default kvmclock_offset for the VM was still based on the monotonic
> clock and, if the raw clock drifted enough from the monotonic clock,
> this could cause a negative system_time to be written to the guest's
> struct pvclock.  RHEL5 does not like it and (if it boots fast enough to
> observe a negative time value) it hangs.
> 
> This series fixes the issue by using the raw clock everywhere.
> 
> (And this, ladies and gentlemen, is why I was not applying patches to
> the KVM tree.  I saw this before Christmas and could only reproduce it
> today, since it requires almost 2 weeks of uptime to reproduce on my
> machine.  Of course, once you have the reproducer the fix is relatively
> easy to come up with).
> 
> Paolo
> 
> Paolo Bonzini (2):
>   KVM: x86: reorganize pvclock_gtod_data members
>   KVM: x86: use raw clock values consistently
> 
>  arch/x86/kvm/x86.c | 67 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 35 insertions(+), 32 deletions(-)
> 
> -- 
> 1.8.3.1

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

BTW, should switch both masterclock and non-masterclock cases
to raw clock base. Do you see any problem with that? 

Using the same reasoning as raw clock for master, ntpd in 
the guest should correct the difference.

Could probably simplify things.
Paolo Bonzini Jan. 25, 2020, 9:42 a.m. UTC | #2
On 24/01/20 21:36, Marcelo Tosatti wrote:
> On Wed, Jan 22, 2020 at 03:22:31PM +0100, Paolo Bonzini wrote:
>> Commit 53fafdbb8b21f ("KVM: x86: switch KVMCLOCK base to monotonic raw
>> clock") changed kvmclock to use tkr_raw instead of tkr_mono.  However,
>> the default kvmclock_offset for the VM was still based on the monotonic
>> clock and, if the raw clock drifted enough from the monotonic clock,
>> this could cause a negative system_time to be written to the guest's
>> struct pvclock.  RHEL5 does not like it and (if it boots fast enough to
>> observe a negative time value) it hangs.
>>
>> This series fixes the issue by using the raw clock everywhere.
>>
>> (And this, ladies and gentlemen, is why I was not applying patches to
>> the KVM tree.  I saw this before Christmas and could only reproduce it
>> today, since it requires almost 2 weeks of uptime to reproduce on my
>> machine.  Of course, once you have the reproducer the fix is relatively
>> easy to come up with).
>>
>> Paolo
>>
>> Paolo Bonzini (2):
>>   KVM: x86: reorganize pvclock_gtod_data members
>>   KVM: x86: use raw clock values consistently
>>
>>  arch/x86/kvm/x86.c | 67 ++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 35 insertions(+), 32 deletions(-)
>>
>> -- 
>> 1.8.3.1
> 
> Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> BTW, should switch both masterclock and non-masterclock cases
> to raw clock base. Do you see any problem with that? 

Indeed, that makes sense as a kind of unification of the code.  Together
with adding pvclock_gtod support for 32-bit, it should be easy.

Paolo