Re: [PATCH 1/2] fix cmpxchg instruction

[Wei Li]

>There is no branch in movcond -- this expands to cmov.

Thanks. I get it.

>Your branch invalidates the comment -- the store becomes conditional, and we no longer get
>a write fault on read-only pages when the comparison fails.  OTOH, we're already getting
>the incorrect SIGSEGV behaviour, since we get a read fault on an unmapped page instead of
>a write fault.

I cannot get the point. Here is my understanding.

when the comparison fails, program executes gen_op_st_v(s, ot, oldv, a0). Then we will get the

incorrect SIGSEGV behaviour that a read fault instead of a write fault. But why the SIGSEGV

behaviour is a read fault not a write fault? The memory operation is a write operation.

>Alternately, we can use the tcg_gen_atomic_cmpxchg_tl path whenever mod != 3.  While an

>unlocked cmpxchg need not be atomic, it is not required to be non-atomic either, and it

>would reduce code duplication.

A good advice to implement. I will update in patch v2.

Wei Li

On Monday, March 21, 2022, 03:07:50 AM GMT+8, Richard Henderson <richard.henderson@linaro.org> wrote:

On 3/19/22 09:06, Wei Li wrote:
> We need a branch to determine when the instruction can touch the
> accumulator. But there is a branch provided by movcond.

There is no branch in movcond -- this expands to cmov.

> -                /* store value = (old == cmp ? new : old);  */
> -                tcg_gen_movcond_tl(TCG_COND_EQ, newv, oldv, cmpv, newv, oldv);
> +                tcg_gen_brcond_tl(TCG_COND_EQ, oldv, cmpv, label1);
...
>                      /* Perform an unconditional store cycle like physical cpu;
>                          must be before changing accumulator to ensure
>                          idempotency if the store faults and the instruction
>                          is restarted */

Your branch invalidates the comment -- the store becomes conditional, and we no longer get
a write fault on read-only pages when the comparison fails.  OTOH, we're already getting
the incorrect SIGSEGV behaviour, since we get a read fault on an unmapped page instead of
a write fault.

The faulting behaviour could be addressed with a write_probe prior to the original load.
Alternately, we can use the tcg_gen_atomic_cmpxchg_tl path whenever mod != 3.  While an
unlocked cmpxchg need not be atomic, it is not required to be non-atomic either, and it
would reduce code duplication.

r~