On 11/09/2018 14:53, Richard Henderson wrote: >> >> -#ifndef CONFIG_ATOMIC64 >> seqlock_write_lock(&timers_state.vm_clock_seqlock, >> &timers_state.vm_clock_lock); >> -#endif >> atomic_set__nocheck(&timers_state.qemu_icount, >> timers_state.qemu_icount + executed); >> -#ifndef CONFIG_ATOMIC64 >> seqlock_write_unlock(&timers_state.vm_clock_seqlock, >> &timers_state.vm_clock_lock); >> -#endif > I don't understand this. Surely you either want atomic64_set, > or an actual atomic64_add, but no extra lock when CONFIG_ATOMIC64.
Even though writes of qemu_icount can safely race with reads in qemu_icount_raw, qemu_icount is also read by icount_adjust, which runs in the I/O thread. Therefore, writes do need protection of the vm_clock_lock; for simplicity Emilio's patch protects it with both seqlock+spinlock, which we already do for hosts that lack 64-bit atomics. The bug actually predated the introduction of vm_clock_lock; cpu_update_icount would have needed the BQL before the spinlock was introduced. The above could have been a commit message, by the way. :) Paolo
