>
>
> [PATCH v2] KVM: x86: remove irq disablement around
> KVM_SET_CLOCK/KVM_GET_CLOCK
>
> The disablement of interrupts at KVM_SET_CLOCK/KVM_GET_CLOCK
> attempts to disable software suspend from causing "non atomic behaviour" of
> the operation:
>
> Add a helper function to compute the kernel time and convert nanoseconds
> back to CPU specific cycles. Note that these must not be called in
> preemptible
> context, as that would mean the kernel could enter software suspend
> state,
> which would cause non-atomic operation.
>
> However, assume the kernel can enter software suspend at the following 2
> points:
>
> ktime_get_ts(&ts);
> 1.
> hypothetical_ktime_get_ts(&ts)
> monotonic_to_bootbased(&ts);
> 2.
>
> monotonic_to_bootbased() should be correct relative to a ktime_get_ts(&ts)
> performed after point 1 (that is after resuming from software suspend),
> hypothetical_ktime_get_ts()
>
> Therefore it is also correct for the ktime_get_ts(&ts) before point 1,
> which is
>
> ktime_get_ts(&ts) = hypothetical_ktime_get_ts(&ts) +
> time-to-execute-suspend-code
>
> Note CLOCK_MONOTONIC does not count during suspension.
>
> So remove the irq disablement, which causes the following warning on
> -RT kernels:
>
> 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): [] c0
> [ 1064.668118] softirqs last enabled at (0): [] 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] [] dump_stack+0x19/0x1b
> [ 1064.668135] [] __might_sleep+0x12d/0x1f0
> [ 1064.668138] [] rt_spin_lock+0x24/0x60
> [ 1064.668155] [] __get_kvmclock_ns+0x36/0x110 [k]
> [ 1064.668159] [] ? futex_wait_queue_me+0x103/0x10
> [ 1064.668171] [] kvm_arch_vm_ioctl+0xa2/0xd70 [k]
> [ 1064.668173] [] ? futex_wait+0x1ac/0x2a0
>
> v2: notice get_kvmclock_ns with the same problem (Pankaj).
>
> Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1faf620..4901178 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1798,12 +1798,9 @@ static u64 __get_kvmclock_ns(struct kvm *kvm)
>
> u64 get_kvmclock_ns(struct kvm *kvm)
> {
> - unsigned long flags;
> s64 ns;
>
> - local_irq_save(flags);
> ns = __get_kvmclock_ns(kvm);
> - local_irq_restore(flags);
>
> return ns;
> }
> @@ -4196,10 +4193,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
> goto out;
>
> r = 0;
> - local_irq_disable();
> now_ns = __get_kvmclock_ns(kvm);
As we have wrapper 'get_kvmclock_ns' above '__get_kvmclock_ns(kvm)'
we can use that directly.
> kvm->arch.kvmclock_offset += user_ns.clock - now_ns;
> - local_irq_enable();
> kvm_gen_update_masterclock(kvm);
> break;
> }
> @@ -4207,11 +4202,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
> struct kvm_clock_data user_ns;
> u64 now_ns;
>
> - local_irq_disable();
> now_ns = __get_kvmclock_ns(kvm);
wrapper here as well.
> user_ns.clock = now_ns;
> user_ns.flags = kvm->arch.use_master_clock ? KVM_CLOCK_TSC_STABLE : 0;
> - local_irq_enable();
> memset(&user_ns.pad, 0, sizeof(user_ns.pad));
>
> r = -EFAULT;
This patch looks okay to me. Just a small suggestion above.
Thanks,
Pankaj
>
