On 2017-07-10 10:45, Richard Henderson wrote: > Signed-off-by: Richard Henderson <r...@twiddle.net> > --- > 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 aurel...@aurel32.net http://www.aurel32.net