From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvtkp-0000FH-9l for qemu-devel@nongnu.org; Sun, 16 Oct 2016 18:18:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvtkk-0006qC-Ei for qemu-devel@nongnu.org; Sun, 16 Oct 2016 18:18:03 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:50421) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bvtkk-0006pw-8h for qemu-devel@nongnu.org; Sun, 16 Oct 2016 18:17:58 -0400 Date: Sun, 16 Oct 2016 18:17:55 -0400 From: "Emilio G. Cota" Message-ID: <20161016221755.GA5587@flamenco> References: <1476214861-31658-1-git-send-email-rth@twiddle.net> <1476214861-31658-14-git-send-email-rth@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1476214861-31658-14-git-send-email-rth@twiddle.net> Subject: Re: [Qemu-devel] [PATCH v6 13/35] tcg: Add atomic helpers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org, alex.bennee@linaro.org On Tue, Oct 11, 2016 at 14:40:39 -0500, Richard Henderson wrote: > Add all of cmpxchg, op_fetch, fetch_op, and xchg. > Handle both endian-ness, and sizes up to 8. > Handle expanding non-atomically, when emulating in serial. > > Signed-off-by: Richard Henderson > --- > Makefile.objs | 2 +- > Makefile.target | 1 + > atomic_template.h | 173 ++++++++++++++++++++++++++ > cputlb.c | 112 ++++++++++++++++- > include/qemu/atomic.h | 43 ++++--- I'd have the changes to atomic.h as a separate patch--this one is pretty big already. > tcg-runtime.c | 49 ++++++-- > tcg/tcg-op.c | 328 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tcg/tcg-op.h | 44 +++++++ > tcg/tcg-runtime.h | 75 ++++++++++++ > tcg/tcg.h | 53 ++++++++ > 10 files changed, 848 insertions(+), 32 deletions(-) > create mode 100644 atomic_template.h (snip) > diff --git a/atomic_template.h b/atomic_template.h > new file mode 100644 > index 0000000..d2c8a08 > --- /dev/null > +++ b/atomic_template.h (snip) > +#if DATA_SIZE == 1 > +# define END > +#elif defined(HOST_WORDS_BIGENDIAN) > +# define END _be > +#else > +# define END _le > +#endif It took me a while to figure out that ATOMIC_NAME needs END (ATOMIC_NAME is defined later in the patch). It might be clearer to pass it explicitly as a suffix in the macro, as in #define ATOMIC_NAME(name, suffix) to then have ATOMIC_NAME(cmpxchg, END). (snip) > + > +/* Note that for addition, we need to use a separate cmpxchg loop instead > + of bswaps for the reverse-host-endian helpers. */ > +ABI_TYPE ATOMIC_NAME(fetch_add)(CPUArchState *env, target_ulong addr, > + ABI_TYPE val EXTRA_ARGS) > +{ > + DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; > + DATA_TYPE ldo, ldn, ret, sto; > + > + ldo = *haddr; ldo = atomic_read(haddr) would be better here for C11 compliance (or tsan will complain). > + while (1) { > + ret = BSWAP(ldo); > + sto = BSWAP(ret + val); > + ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto); > + if (ldn == ldo) { > + return ret; > + } > + ldo = ldn; > + } > +} > + > +ABI_TYPE ATOMIC_NAME(add_fetch)(CPUArchState *env, target_ulong addr, > + ABI_TYPE val EXTRA_ARGS) > +{ > + DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; > + DATA_TYPE ldo, ldn, ret, sto; > + > + ldo = *haddr; ditto. No more comments. Have to say though that I really like how with the tables in tcg-op.c, code ends up looking neat! Emilio