Re: [Qemu-devel] [PATCH v2 11/27] target-i386: emulate LOCK'ed cmpxchg using cmpxchg helpers

[Emilio G. Cota]

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" <address@hidden>
> > 
> > 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 <address@hidden>
> > Message-Id: <address@hidden>
> > Signed-off-by: Richard Henderson <address@hidden>
> > ---
> >  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.