[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64 |
Date: |
Tue, 24 Sep 2019 11:29:25 -0700 |
On Mon, Jun 24, 2019 at 11:21 AM Joel Sing <address@hidden> wrote:
>
> On 19-06-17 16:52:44, Richard Henderson wrote:
> > On 6/16/19 12:19 PM, Joel Sing wrote:
> > > + /*
> > > + * Clear the load reservation, since an SC must fail if there is
> > > + * an SC to any address, in between an LR and SC pair.
> > > + */
> > > + tcg_gen_movi_tl(load_res, 0);
> > > +
> > > gen_set_label(l2);
> >
> > This clear needs to be moved down below label l2.
> > Otherwise, with lr / sc / sc, the second sc could succeed in error.
>
> Indeed, thanks.
>
> > FWIW, other targets have used -1 as the "invalid" load reservation, since
> > the
> > architecture does not require address 0 to be unmapped. This should be
> > quite
> > visible in M-mode with paging disabled and ram at offset 0. Often, other
> > targets require alignment for the lr/sc address, though I don't see that
> > for riscv.
>
> I've switched to -1 as suggested. Regarding the alignment for reservations,
> the
> specification does require this, although I do not recall seeing any
> enforcement
> of this by qemu itself.
>
> New diff follows.
>
> From 8ef31a2ce8ef1cbeee92995a0b2994f480e9bb6d Mon Sep 17 00:00:00 2001
> From: Joel Sing <address@hidden>
> Date: Tue, 25 Jun 2019 02:44:24 +1000
> Subject: [PATCH] Clear load reservations on qemu riscv target
>
> This prevents a load reservation from being placed in one context/process,
> then being used in another, resulting in an SC succeeding incorrectly and
> breaking atomics.
>
> Signed-off-by: Joel Sing <address@hidden>
> ---
> target/riscv/cpu.c | 1 +
> target/riscv/cpu_helper.c | 9 +++++++++
> target/riscv/insn_trans/trans_rva.inc.c | 8 +++++++-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d61bce6d55..e7c8bf48fc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -281,6 +281,7 @@ static void riscv_cpu_reset(CPUState *cs)
> env->pc = env->resetvec;
> #endif
> cs->exception_index = EXCP_NONE;
> + env->load_res = -1;
> set_default_nan_mode(1, &env->fp_status);
> }
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index b17f169681..6a07b12e65 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -113,6 +113,15 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong
> newpriv)
> }
> /* tlb_flush is unnecessary as mode is contained in mmu_idx */
> env->priv = newpriv;
> +
> + /* Clear the load reservation - otherwise a reservation placed in one
> + * context/process can be used by another, resulting in an SC succeeding
> + * incorrectly. Version 2.2 of the ISA specification explicitly requires
> + * this behaviour, while later revisions say that the kernel "should" use
> + * an SC instruction to force the yielding of a load reservation on a
> + * preemptive context switch. As a result, do both.
> + */
> + env->load_res = -1;
> }
>
> /* get_physical_address - get the physical address for this virtual address
> diff --git a/target/riscv/insn_trans/trans_rva.inc.c
> b/target/riscv/insn_trans/trans_rva.inc.c
> index f6dbbc065e..fadd88849e 100644
> --- a/target/riscv/insn_trans/trans_rva.inc.c
> +++ b/target/riscv/insn_trans/trans_rva.inc.c
> @@ -61,7 +61,7 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic *a,
> TCGMemOp mop)
>
> gen_set_label(l1);
> /*
> - * Address comparion failure. However, we still need to
> + * Address comparison failure. However, we still need to
> * provide the memory barrier implied by AQ/RL.
> */
> tcg_gen_mb(TCG_MO_ALL + a->aq * TCG_BAR_LDAQ + a->rl * TCG_BAR_STRL);
> @@ -69,6 +69,12 @@ static inline bool gen_sc(DisasContext *ctx, arg_atomic
> *a, TCGMemOp mop)
> gen_set_gpr(a->rd, dat);
>
> gen_set_label(l2);
> + /*
> + * Clear the load reservation, since an SC must fail if there is
> + * an SC to any address, in between an LR and SC pair.
> + */
> + tcg_gen_movi_tl(load_res, -1);
> +
> tcg_temp_free(dat);
> tcg_temp_free(src1);
> tcg_temp_free(src2);
> --
This patch causes boot failures when booting systemd built with musl on RV64.
It could possibly be a musl bug, but I wanted to throw that out here
first to see what people think.
Alistair
> 2.21.0
>
>
- Re: [Qemu-devel] [PATCH] atomic failures on qemu-system-riscv64,
Alistair Francis <=