Re: [Qemu-riscv] [Qemu-devel] Fwd: address@hidden: atomic failures on qemu-system-riscv64]

[Palmer Dabbelt]

On Wed, 05 Jun 2019 13:59:53 PDT (-0700), address@hidden wrote:
> Joel is on vacation so here it is again.
>
> > Begin forwarded message:
> >
> > From: Alistair Francis <address@hidden>
> > Subject: Re: address@hidden: atomic failures on qemu-system-riscv64]
> > Date: June 5, 2019 at 7:19:53 PM GMT+1
> > To: "address@hidden" <address@hidden>, "address@hidden" <address@hidden>
> > Cc: "address@hidden" <address@hidden>, "address@hidden" <address@hidden>
> >
> > On Fri, 2019-05-31 at 03:21 +1000, Joel Sing wrote:
> > > I've just sent this to address@hidden - forwarding for
> > > visibility...
> >
> > Hello Joel,
> >
> > Can you please send this to the QEMU mailing list?
> > https://wiki.qemu.org/Contribute/MailingLists
> >
> > > ----- Forwarded message from Joel Sing <address@hidden> -----
> > >
> > > Date: Fri, 31 May 2019 03:20:03 +1000
> > > From: Joel Sing <address@hidden>
> > > To: address@hidden
> > > Subject: atomic failures on qemu-system-riscv64
> > > User-Agent: Mutt/1.10.1 (2018-07-13)
> > >
> > > While working on a Go (www.golang.org) port for riscv, I've run
> > > into issues with atomics (namely LR/SC) on qemu-system-riscv64.
> > > There are several reproducers for this problem including (using
> > > gcc builtin atomics):
> > >
> > >  https://gist.github.com/4a6f656c/8433032a3f70893a278259f8108aad90
> > >
> > > And a version using inline assembly:
> > >
> > >  https://gist.github.com/4a6f656c/d883091f5ca811822720213be343a75a
> > >
> > > Depending on the qemu configuration the number of threads may
> > > need increasing (to force context switching) and/or run in a
> > > loop. Go's sync/atomic tests also fail regularly.
> > >
> > > Having dug into the qemu code, what I believe is happening is
> > > along the lines of the following:
> > >
> > > 1. Thread 1 runs and executes an LR - this assigns an address
> > >   to load_res and a value to load_val (say 1).
> > >
> > > 2. A context switch occurs and thread 2 is now run - it runs
> > >   an LR and SC on the same address modifying the stored value.
> > >   Another LR is executed loading load_val with the current
> > >   value (say 3).
> > >
> > > 3. A context switch occurs and thread 1 is now run again, it
> > >   continues on its LR/SC sequence and now runs the SC instruction.
> > >   This is based on the assumption that it had a reservation
> > >   and the SC will fail if the memory has changed. The underlying
> > >   implementation of SC is a cmpxchg with the value in load_val
> > >   - this no longer has the original value and hence successfully
> > >   compares (as does the tcg_gen_setcond_tl() between the returned
> > >   value and load_val) thus the SC succeeds when it should not.
> > >
> > > The diff below clears load_res when the mode changes - with this
> > > applied the reproducers work correctly, as do Go's atomic tests.
> > > I'm not sure this "fix" is 100% correct, but it certainly verifies
> > > where the problem lies. It does also fall inline with the RISCV
> > > spec:
> > >
> > > "The SC must fail if there is an observable memory access from
> > > another hart to the address, or if there is an intervening context
> > > switch on this hart, or if in the meantime the hart executed a
> > > privileged exception-return instruction."

Sorry about this, but that wording is actually a bug in the ISA manual.  The
current version says

   An SC must fail if there is
   another SC (to any address) between the LR and the SC in program
   order.  The precise statement of the atomicity requirements for
   successful LR/SC sequences is defined by the Atomicity Axiom in
   Section~\ref{sec:rvwmo}.
   
   \begin{commentary}
   A store-conditional instruction to a scratch word of memory should be used
   during a preemptive context switch to forcibly yield any existing load
   reservation.
   \end{commentary}

The text in version 2.2 was changed because it doesn't really have meaning:
harts are a hardware construct, but context switches are a software context.
There were a few of these bugs floating around the ISA manual and we cleaned
them up more than a year ago, but the version on the RISC-V website hasn't
changed.  The ratified ISA manual will have the correct wording.

That said, the Linux user ABI allows programs to rely on reservations being
shot down on context switches -- if that wasn't the case then LR/SC would be
useless.  That's how the ambiguous wording snuck into the user ISA manual: we
confused the ISA behavior (ie, what the hardware does) with the user ABI
behavior (ie, what programs can rely on).

The reason you're only seeing this in QEMU is because the hardware has a
timeout on load reservations of something like 1000 cycles, and the Linux
context switching code takes longer than that timeout.  For some reason I
remember having fixed this bug in Linux, but I must have forgotten to actually
send out the patch.

Also, unless I'm misunderstanding something our implementation of LR/SC is
pretty broken.  We're just using a CAS to check if the value changed, which
suffers from the ABA problem that LR/SC is there to fix in the first place.  I
might be missing something here, though, as it looks like MIPS is doing
essentially the same thing.

> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index b17f169681..9875b8e5d3 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -113,6 +113,8 @@ void riscv_cpu_set_mode(CPURISCVState *env,
> > > target_ulong newpriv)
> > >     }
> > >     /* tlb_flush is unnecessary as mode is contained in mmu_idx */
> > >     env->priv = newpriv;
> > > +
> > > +    env->load_res = 0;
> >
> > This looks ok to me, I'll read the spec to double check though. Do you
> > mind adding a comment in the code as to why this is required?
> >
> > Alistair
> >
> > > }
> > >
> > > /* get_physical_address - get the physical address for this virtual
> > > address
> > >
> > > ----- End forwarded message -----