From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38299) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPBr0-0002QZ-3A for qemu-devel@nongnu.org; Mon, 18 Jul 2016 12:57:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPBqt-0003CI-UN for qemu-devel@nongnu.org; Mon, 18 Jul 2016 12:57:13 -0400 Received: from mail-lf0-x244.google.com ([2a00:1450:4010:c07::244]:32965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPBqt-0003BU-Ho for qemu-devel@nongnu.org; Mon, 18 Jul 2016 12:57:07 -0400 Received: by mail-lf0-x244.google.com with SMTP id f93so11650292lfi.0 for ; Mon, 18 Jul 2016 09:57:07 -0700 (PDT) References: <1468851450-9863-1-git-send-email-pbonzini@redhat.com> <578D0938.2050004@gmail.com> From: Sergey Fedorov Message-ID: <578D0A60.4020608@gmail.com> Date: Mon, 18 Jul 2016 19:57:04 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] atomics: add volatile_read/volatile_set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: sergey.fedorov@linaro.org, alex.bennee@linaro.org On 18/07/16 19:53, Paolo Bonzini wrote: > > On 18/07/2016 18:52, Sergey Fedorov wrote: >> So how are we going to use them? > Instead of atomic_read/atomic_set when marking invalid TBs. But shouldn't they be atomic to avoid reading torn writes? Thanks, Sergey > > diff --git a/cpu-exec.c b/cpu-exec.c > index fd43de8..1275f3d 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -292,10 +292,10 @@ static inline TranslationBlock *tb_find(CPUState *cpu, > always be the same before a given translated block > is executed. */ > cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags); > - tb = atomic_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]); > - if (unlikely(!tb || atomic_read(&tb->pc) != pc || > - atomic_read(&tb->cs_base) != cs_base || > - atomic_read(&tb->flags) != flags)) { > + tb = atomic_rcu_read(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)]); > + if (unlikely(!tb || volatile_read(&tb->pc) != pc || > + volatile_read(&tb->cs_base) != cs_base || > + volatile_read(&tb->flags) != flags)) { > tb = tb_htable_lookup(cpu, pc, cs_base, flags); > if (!tb) { > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 8f0afcd..35e963b 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -262,9 +262,9 @@ static inline void tb_mark_invalid(TranslationBlock *tb) > uint32_t flags = 0; > > cpu_get_invalid_tb_cpu_state(&pc, &cs_base, &flags); > - atomic_set(&tb->pc, pc); > - atomic_set(&tb->cs_base, cs_base); > - atomic_set(&tb->flags, flags); > + volatile_set(&tb->pc, pc); > + volatile_set(&tb->cs_base, cs_base); > + volatile_set(&tb->flags, flags); > } > > static inline bool tb_is_invalid(TranslationBlock *tb) > > > Thanks, > > Paolo > >> Thanks, >> Sergey >> >> On 18/07/16 17:17, Paolo Bonzini wrote: >>> Signed-off-by: Paolo Bonzini >>> --- >>> docs/atomics.txt | 19 ++++++++++++++++--- >>> include/qemu/atomic.h | 17 +++++++++++++++++ >>> 2 files changed, 33 insertions(+), 3 deletions(-) >>> >>> diff --git a/docs/atomics.txt b/docs/atomics.txt >>> index c95950b..1f21d2e 100644 >>> --- a/docs/atomics.txt >>> +++ b/docs/atomics.txt >>> @@ -123,6 +123,14 @@ to do so, because it tells readers which variables are shared with >>> other threads, and which are local to the current thread or protected >>> by other, more mundane means. >>> >>> +atomic_read() and atomic_set() only support accesses as large as a >>> +pointer. If you need to access variables larger than a pointer you >>> +can use volatile_read() and volatile_set(), but be careful: these always >>> +use volatile accesses, and 64-bit volatile accesses are not atomic on >>> +several 32-bit processors such as ARMv7. In other words, volatile_read >>> +and volatile_set only provide "safe register" semantics when applied to >>> +64-bit variables. >>> + >>> Memory barriers control the order of references to shared memory. >>> They come in four kinds: >>> >>> @@ -335,11 +343,16 @@ and memory barriers, and the equivalents in QEMU: >>> Both semantics prevent the compiler from doing certain transformations; >>> the difference is that atomic accesses are guaranteed to be atomic, >>> while volatile accesses aren't. Thus, in the volatile case we just cross >>> - our fingers hoping that the compiler will generate atomic accesses, >>> - since we assume the variables passed are machine-word sized and >>> - properly aligned. >>> + our fingers hoping that the compiler and processor will provide atomic >>> + accesses, since we assume the variables passed are machine-word sized >>> + and properly aligned. >>> + >>> No barriers are implied by atomic_read/set in either Linux or QEMU. >>> >>> +- volatile_read and volatile_set are equivalent to ACCESS_ONCE in Linux. >>> + No barriers are implied by volatile_read/set in QEMU, nor by >>> + ACCESS_ONCE in Linux. >>> + >>> - atomic read-modify-write operations in Linux are of three kinds: >>> >>> atomic_OP returns void >>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >>> index 7e13fca..8409bdb 100644 >>> --- a/include/qemu/atomic.h >>> +++ b/include/qemu/atomic.h >>> @@ -18,6 +18,12 @@ >>> /* Compiler barrier */ >>> #define barrier() ({ asm volatile("" ::: "memory"); (void)0; }) >>> >>> +/* These will only be atomic if the processor does the fetch or store >>> + * in a single issue memory operation >>> + */ >>> +#define volatile_read(ptr) (*(__typeof__(*ptr) volatile*) (ptr)) >>> +#define volatile_set(ptr, i) ((*(__typeof__(*ptr) volatile*) (ptr)) = (i)) >>> + >>> #ifdef __ATOMIC_RELAXED >>> /* For C11 atomic ops */ >>> >>> @@ -260,6 +266,17 @@ >>> */ >>> #define atomic_read(ptr) (*(__typeof__(*ptr) volatile*) (ptr)) >>> #define atomic_set(ptr, i) ((*(__typeof__(*ptr) volatile*) (ptr)) = (i)) >>> +#define atomic_read(ptr) \ >>> + ({ \ >>> + QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ >>> + volatile_read(ptr); \ >>> + }) >>> + >>> +#define atomic_set(ptr, i) do { \ >>> + QEMU_BUILD_BUG_ON(sizeof(*ptr) > sizeof(void *)); \ >>> + volatile_set(ptr, i); \ >>> +} while(0) >>> + >>> >>> /** >>> * atomic_rcu_read - reads a RCU-protected pointer to a local variable