On Mon, Aug 20, 2018 at 17:09:02 +0200, Paolo Bonzini wrote:
> In the next patch, we will need to write cpu_ticks_offset from any
> thread, even outside the BQL. Currently, it is protected by the BQL
> just because cpu_enable_ticks and cpu_disable_ticks happen to hold it,
> but the critical sections are well delimited and it's easy to remove
> the BQL dependency.
>
> Add a spinlock that matches vm_clock_seqlock, and hold it when writing
> to the TimerState. This also lets us fix cpu_update_icount when 64-bit
> atomics are not available.
>
> Fields of TiemrState are reordered to avoid padding.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> cpus.c | 72 ++++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 680706aefa..63ddd4fd21 100644
> --- a/cpus.c
> +++ b/cpus.c
(snip)
> @@ -244,11 +250,15 @@ void cpu_update_icount(CPUState *cpu)
> int64_t executed = cpu_get_icount_executed(cpu);
> cpu->icount_budget -= executed;
>
> -#ifdef CONFIG_ATOMIC64
> +#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);
> -#else /* FIXME: we need 64bit atomics to do this safely */
> - timers_state.qemu_icount += executed;
> +#ifndef CONFIG_ATOMIC64
> + seqlock_write_unlock(&timers_state.vm_clock_seqlock,
> + &timers_state.vm_clock_lock);
> #endif
I'm puzzled by this hunk. Why are we only adding the seqlock_write
if !CONFIG_ATOMIC64, if we always read .qemu_icount with seqlock_read?
Thanks,
Emilio
