Re: [Qemu-devel] [PATCH v3 1/8] target/s390x: Implement CSST

[Richard Henderson]

On 07/14/2017 11:01 AM, Aurelien Jarno wrote:
> > +    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.

That's WITHOUT atomic128 support that mask = -16.
If we have atomic128, then mask = 0.

> > +            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.

Dunno.  Probably not too bad.  I have wondered about doing that, as these sorts 
of functions are quite ugly to write.


r~