On Mon, Aug 20, 2018 at 17:09:00 +0200, Paolo Bonzini wrote:
> Using the seqlock makes the atomic_read__nocheck safe, because it now
> happens always inside a seqlock and any torn reads will be retried.
Using a seqlock makes regular accesses safe as well, for the same
reason. It's undefined behaviour but I don't see how to avoid
it if the host might not have wide-enough atomics (see below).
> qemu_icount_bias and icount_time_shift also need to be accessed with
> atomics.
But qemu_icount_bias is always accessed through the seqlock, so regular
accesses should be fine there.
Moreover, seqlock + regular accesses allow us not to worry about
32-bit hosts without __atomic builtins, which might choke on
atomic accesses to u64's (regardless of __nocheck) like this one:
> -#ifdef CONFIG_ATOMIC64
> + /* The read is protected by the seqlock, so __nocheck is okay. */
> return atomic_read__nocheck(&timers_state.qemu_icount);
> -#else /* FIXME: we need 64bit atomics to do this safely */
> - return timers_state.qemu_icount;
> -#endif
So I think we should convert these to regular accesses. I just
wrote a patch to perform the conversion (after noticing the same
misuse of __nocheck + seqlock in qsp.c, which is my fault); however,
I have a question on patch 3, which I'd like to address first.
Thanks,
Emilio
