On Thu, Jul 07, 2016 at 23:08:17 -0400, Emilio G. Cota wrote:
> On Fri, Jul 01, 2016 at 10:04:37 -0700, Richard Henderson wrote:
> > From: "Emilio G. Cota" <[email protected]>
> >
> > The diff here is uglier than necessary. All this does is to turn
> >
> > FOO
> >
> > into:
> >
> > if (s->prefix & PREFIX_LOCK) {
> > BAR
> > } else {
> > FOO
> > }
> >
> > where FOO is the original implementation of an unlocked cmpxchg.
> >
> > [rth: Adjust unlocked cmpxchg to use movcond instead of branches.
> > Adjust helpers to use atomic helpers.]
> >
> > Signed-off-by: Emilio G. Cota <[email protected]>
> > Message-Id: <[email protected]>
> > Signed-off-by: Richard Henderson <[email protected]>
> > ---
> > target-i386/mem_helper.c | 96
> > ++++++++++++++++++++++++++++++++++++++----------
> > target-i386/translate.c | 87 +++++++++++++++++++++----------------------
> > 2 files changed, 120 insertions(+), 63 deletions(-)
> >
> > diff --git a/target-i386/mem_helper.c b/target-i386/mem_helper.c
> > index c2f4769..5c0558f 100644
> > --- a/target-i386/mem_helper.c
> > +++ b/target-i386/mem_helper.c
> > @@ -22,6 +22,8 @@
> > #include "exec/helper-proto.h"
> > #include "exec/exec-all.h"
> > #include "exec/cpu_ldst.h"
> > +#include "qemu/int128.h"
> > +#include "tcg.h"
> >
> > /* broken thread support */
> >
> > @@ -58,20 +60,39 @@ void helper_lock_init(void)
> >
> > void helper_cmpxchg8b(CPUX86State *env, target_ulong a0)
> > {
> > - uint64_t d;
> > + uintptr_t ra = GETPC();
> > + uint64_t oldv, cmpv, newv;
> > int eflags;
> >
> > eflags = cpu_cc_compute_all(env, CC_OP);
> > - d = cpu_ldq_data_ra(env, a0, GETPC());
> > - if (d == (((uint64_t)env->regs[R_EDX] << 32) |
> > (uint32_t)env->regs[R_EAX])) {
> > - cpu_stq_data_ra(env, a0, ((uint64_t)env->regs[R_ECX] << 32)
> > - | (uint32_t)env->regs[R_EBX], GETPC());
> > - eflags |= CC_Z;
> > +
> > + cmpv = deposit64(env->regs[R_EAX], 32, 32, env->regs[R_EDX]);
> > + newv = deposit64(env->regs[R_EBX], 32, 32, env->regs[R_ECX]);
> > +
> > + if (parallel_cpus) {
>
> I think here we mean 'if (prefix_locked)', although prefix_locked isn't
> here. This is why in my original patch I defined two helpers ('locked'
> and 'unlocked'), otherwise we'll emulate cmpxchg atomically even when
> the LOCK prefix wasn't there. Not a big deal, but could hide bugs.
Correction to myself: we'd still need if (prefix_locked) for the whole
stop-the-world-and-do-non-atomically thing to work.
The concern that we might hide bugs by emulating non-locked cmpxchg
atomically persists. [ I wonder why one would ever use a non-locked
cmpxchg, though ]
E.
