Re: [PATCH] KVM: x86: remove irq disablement around KVM_SET_CLOCK/KVM_GET_CLOCK

[Roman Kagan <rkagan@virtuozzo.com>]

On Wed, Apr 12, 2017 at 02:23:15PM -0300, Marcelo Tosatti wrote:
> 
> The disablement of interrupts at KVM_SET_CLOCK/KVM_GET_CLOCK 
> attempts to disable interrupts in that section to protect
> the values that are calculated in that section from interrupt interference. 
> 
> now_ns is calculated inside the irq protected region, 
> user_ns.clock is passed from userspace (therefore not susceptible
> to interrupt variation).
> 
> About the line
>                 now_ns = __get_kvmclock_ns(kvm);  (1)
> 
> Interrupts can happen afterwards local_irq_enable(), 
> rendering "now_ns" relative to its execution time PLUS 
> interrupt time.
> 
> Therefore the local_irq_disable() / local_irq_enable() protection is not
> necessary (that is: interrupts triggering after local_irq_enable cause
> the same problem that the protection is trying to avoid).
> 
> With this reasoning, and the -RT bug that the irq disablement causes
> (because spin_lock is now a sleeping lock), remove the IRQ protection as it causes: 
> 
> [ 1064.668109] in_atomic(): 0, irqs_disabled(): 1, pid: 15296, name:m
> [ 1064.668110] INFO: lockdep is turned off.
> [ 1064.668110] irq event stamp: 0
> [ 1064.668112] hardirqs last  enabled at (0): [<          (null)>]  )
> [ 1064.668116] hardirqs last disabled at (0): [<ffffffff9308184a>] c0
> [ 1064.668118] softirqs last  enabled at (0): [<ffffffff9308184a>] c0
> [ 1064.668118] softirqs last disabled at (0): [<          (null)>]  )
> [ 1064.668121] CPU: 13 PID: 15296 Comm: qemu-kvm Not tainted 3.10.0-1
> [ 1064.668121] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 5
> [ 1064.668123]  ffff8c1796b88000 00000000afe7344c ffff8c179abf3c68 f3
> [ 1064.668125]  ffff8c179abf3c90 ffffffff930ccb3d ffff8c1b992b3610 f0
> [ 1064.668126]  00007ffc1a26fbc0 ffff8c179abf3cb0 ffffffff9375f694 f0
> [ 1064.668126] Call Trace:
> [ 1064.668132]  [<ffffffff93757413>] dump_stack+0x19/0x1b
> [ 1064.668135]  [<ffffffff930ccb3d>] __might_sleep+0x12d/0x1f0
> [ 1064.668138]  [<ffffffff9375f694>] rt_spin_lock+0x24/0x60
> [ 1064.668155]  [<ffffffffc06ab996>] __get_kvmclock_ns+0x36/0x110 [k]
> [ 1064.668159]  [<ffffffff93112993>] ? futex_wait_queue_me+0x103/0x10
> [ 1064.668171]  [<ffffffffc06b8782>] kvm_arch_vm_ioctl+0xa2/0xd70 [k]
> [ 1064.668173]  [<ffffffff9311333c>] ? futex_wait+0x1ac/0x2a0
> 
> On -RT kernels.

Hmm, __get_kvmclock_ns used not to need a spinlock back when it was
added...  Why does it now?

Looking at its current state, I'm not sure I understand what it's
supposed to do: it uses the host tsc rate rather than the guest one,
which seems to just defeat the purpose of originally introducing it: to
have a way to obtain the clock value exactly the same as the guest would
see...

Am I missing anything obvious?

Thanks,
Roman.