[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/8] target/s390x: Implement CSST
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/8] target/s390x: Implement CSST |
Date: |
Fri, 14 Jul 2017 23:01:05 +0200 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On 2017-07-10 10:45, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> target/s390x/helper.h | 1 +
> target/s390x/cpu_models.c | 2 +
> target/s390x/mem_helper.c | 189
> +++++++++++++++++++++++++++++++++++++++++++++
> target/s390x/translate.c | 13 +++-
> target/s390x/insn-data.def | 2 +
> 5 files changed, 206 insertions(+), 1 deletion(-)
>
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index ede8471..513b402 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1353,6 +1353,195 @@ void HELPER(cdsg)(CPUS390XState *env, uint64_t addr,
> env->regs[r1 + 1] = int128_getlo(oldv);
> }
>
> +uint32_t HELPER(csst)(CPUS390XState *env, uint32_t r3, uint64_t a1, uint64_t
> a2)
> +{
> +#if !defined(CONFIG_USER_ONLY) || defined(CONFIG_ATOMIC128)
> + uint32_t mem_idx = cpu_mmu_index(env, false);
> +#endif
> + uintptr_t ra = GETPC();
> + uint32_t fc = extract32(env->regs[0], 0, 8);
> + uint32_t sc = extract32(env->regs[0], 8, 8);
> + uint64_t pl = get_address(env, 1) & -16;
> + uint64_t svh, svl;
> + uint32_t cc;
> +
> + /* Sanity check the function code and storage characteristic. */
> + if (fc > 1 || sc > 3) {
> + if (!s390_has_feat(S390_FEAT_COMPARE_AND_SWAP_AND_STORE_2)) {
> + goto spec_exception;
> + }
> + if (fc > 2 || sc > 4 || (fc == 2 && (r3 & 1))) {
> + goto spec_exception;
> + }
> + }
> +
> + /* Sanity check the alignments. */
> + if (extract32(a1, 0, 4 << fc) || extract32(a2, 0, 1 << sc)) {
> + goto spec_exception;
> + }
> +
> + /* Sanity check writability of the store address. */
> +#ifndef CONFIG_USER_ONLY
> + probe_write(env, a2, mem_idx, ra);
> +#endif
> +
> + /* Note that the compare-and-swap is atomic, and the store is atomic, but
> + the complete operation is not. Therefore we do not need to assert
> serial
> + context in order to implement this. That said, restart early if we
> can't
> + support either operation that is supposed to be atomic. */
> + if (parallel_cpus) {
> + int mask = 0;
> +#if !defined(CONFIG_ATOMIC64)
> + mask = -8;
> +#elif !defined(CONFIG_ATOMIC128)
> + mask = -16;
> +#endif
> + if (((4 << fc) | (1 << sc)) & mask) {
> + cpu_loop_exit_atomic(ENV_GET_CPU(env), ra);
> + }
> + }
This doesn't look correct. For a 16-byte store, ie sc = 4, and with
ATOMIC128 support, ie mask = -16, the condition is true.
> + /* All loads happen before all stores. For simplicity, load the entire
> + store value area from the parameter list. */
> + svh = cpu_ldq_data_ra(env, pl + 16, ra);
> + svl = cpu_ldq_data_ra(env, pl + 24, ra);
> +
> + switch (fc) {
> + case 0:
> + {
> + uint32_t nv = cpu_ldl_data_ra(env, pl, ra);
> + uint32_t cv = env->regs[r3];
> + uint32_t ov;
> +
> + if (parallel_cpus) {
> +#ifdef CONFIG_USER_ONLY
> + uint32_t *haddr = g2h(a1);
> + ov = atomic_cmpxchg__nocheck(haddr, cv, nv);
> +#else
> + TCGMemOpIdx oi = make_memop_idx(MO_TEUL | MO_ALIGN, mem_idx);
> + ov = helper_atomic_cmpxchgl_be_mmu(env, a1, cv, nv, oi, ra);
> +#endif
Not a problem with the patch itself, but how complicated would it be to
make helper_atomic_cmpxchgl_be_mmu (just like the o version) available
also in user mode? That would make less #ifdef in the backends.
The remaining of the patch looks all good to me.
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
address@hidden http://www.aurel32.net