Re: [Qemu-devel] [PATCH v3 14/34] tcg: Add atomic128 helpers

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <rth@twiddle.net> writes:

> Force the use of cmpxchg16b on x86_64.
>
> Wikipedia suggests that only very old AMD64 (circa 2004) did not have
> this instruction.  Further, it's required by Windows 8 so no new cpus
> will ever omit it.
>
> If we truely care about these, then we could check this at startup time
> and then avoid executing paths that use it.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  atomic_template.h     | 40 +++++++++++++++++++++++++++++++++++++++-
>  configure             | 29 ++++++++++++++++++++++++++++-
>  cputlb.c              |  5 +++++
>  include/qemu/int128.h |  6 ++++++
>  tcg-runtime.c         | 20 +++++++++++++++++++-
>  tcg/tcg.h             | 24 +++++++++++++++++++++++-
>  6 files changed, 120 insertions(+), 4 deletions(-)
>
<snip>
> diff --git a/tcg-runtime.c b/tcg-runtime.c
> index aa55d12..0c97cdf 100644
> --- a/tcg-runtime.c
> +++ b/tcg-runtime.c
> @@ -118,8 +118,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>  /* Macro to call the above, with local variables from the use context.  */
>  #define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
>
> -#define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
>  #define EXTRA_ARGS
> +#define ATOMIC_NAME(X)   HELPER(glue(glue(atomic_ ## X, SUFFIX), END))

Did I miss a subtly here? Should this change be squashed into the
original atomic helpers patch?


>  #define DATA_SIZE 1
>  #include "atomic_template.h"
> @@ -133,4 +133,22 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>  #define DATA_SIZE 8
>  #include "atomic_template.h"
>
> +/* The following is only callable from other helpers, and matches up
> +   with the softmmu version.  */
> +
> +#ifdef CONFIG_ATOMIC128
> +
> +#undef EXTRA_ARGS
> +#undef ATOMIC_NAME
> +#undef ATOMIC_MMU_LOOKUP
> +
> +#define EXTRA_ARGS     , TCGMemOpIdx oi, uintptr_t retaddr
> +#define ATOMIC_NAME(X) \
> +    HELPER(glue(glue(glue(atomic_ ## X, SUFFIX), END), _mmu))
> +#define ATOMIC_MMU_LOOKUP  atomic_mmu_lookup(env, addr, DATA_SIZE, retaddr)
> +
> +#define DATA_SIZE 16
> +#include "atomic_template.h"
> +#endif /* CONFIG_ATOMIC128 */
> +
>  #endif /* !CONFIG_SOFTMMU */
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index c91b8c6..5a94cec 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -1227,7 +1227,29 @@ GEN_ATOMIC_HELPER_ALL(xchg)
>
>  #undef GEN_ATOMIC_HELPER_ALL
>  #undef GEN_ATOMIC_HELPER
> -
>  #endif /* CONFIG_SOFTMMU */
>
> +#ifdef CONFIG_ATOMIC128
> +#include "qemu/int128.h"
> +
> +/* These aren't really a "proper" helpers because TCG cannot manage Int128.
> +   However, use the same format as the others, for use by the backends. */
> +Int128 helper_atomic_cmpxchgo_le_mmu(CPUArchState *env, target_ulong addr,
> +                                     Int128 cmpv, Int128 newv,
> +                                     TCGMemOpIdx oi, uintptr_t retaddr);
> +Int128 helper_atomic_cmpxchgo_be_mmu(CPUArchState *env, target_ulong addr,
> +                                     Int128 cmpv, Int128 newv,
> +                                     TCGMemOpIdx oi, uintptr_t retaddr);
> +
> +Int128 helper_atomic_ldo_le_mmu(CPUArchState *env, target_ulong addr,
> +                                TCGMemOpIdx oi, uintptr_t retaddr);
> +Int128 helper_atomic_ldo_be_mmu(CPUArchState *env, target_ulong addr,
> +                                TCGMemOpIdx oi, uintptr_t retaddr);
> +void helper_atomic_sto_le_mmu(CPUArchState *env, target_ulong addr, Int128 val,
> +                              TCGMemOpIdx oi, uintptr_t retaddr);
> +void helper_atomic_sto_be_mmu(CPUArchState *env, target_ulong addr, Int128 val,
> +                              TCGMemOpIdx oi, uintptr_t retaddr);
> +
> +#endif /* CONFIG_ATOMIC128 */
> +
>  #endif /* TCG_H */

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée