[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-devel
Subject:    Re: [Qemu-devel] [PATCH 1/4] cpus: protect all icount computation with seqlock
From:       "Emilio G. Cota" <cota () braap ! org>
Date:       2018-08-31 22:03:08
Message-ID: 20180831220308.GA18048 () flamenco
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]