On Mon, Jun 27, 2022 at 6:16 PM Christoph Müllner <christoph.muell...@vrull.eu> wrote: > > > > On Mon, Jun 27, 2022 at 7:20 AM Alistair Francis <alistai...@gmail.com> wrote: >> >> On Fri, Jun 24, 2022 at 1:31 AM Christoph Muellner >> <christoph.muell...@vrull.eu> wrote: >> > >> > This patch adds support for the Zawrs ISA extension. >> > Given the current (incomplete) implementation of reservation sets >> > there seems to be no way to provide a full emulation of the WRS >> > instruction (wake on reservation set invalidation or timeout or >> > interrupt). Therefore, we just pretend that an interrupt occured, >> > exit the execution loop and finally continue execution. >> > >> > The specification can be found here: >> > https://github.com/riscv/riscv-zawrs/blob/main/zawrs.adoc >> > >> > Note, that the Zawrs extension is not frozen or ratified yet. >> > Therefore this patch is an RFC and not intended to get merged. >> > >> > Changes since v2: >> > * Adjustments according to a specification change >> > * Inline REQUIRE_ZAWRS() since it has only one user >> > >> > Changes since v1: >> > * Adding zawrs to the ISA string that is passed to the kernel >> > >> > Signed-off-by: Christoph Müllner <christoph.muell...@vrull.eu> >> > --- >> > target/riscv/cpu.c | 2 + >> > target/riscv/cpu.h | 1 + >> > target/riscv/insn32.decode | 4 ++ >> > target/riscv/insn_trans/trans_rvzawrs.c.inc | 54 +++++++++++++++++++++ >> > target/riscv/translate.c | 1 + >> > 5 files changed, 62 insertions(+) >> > create mode 100644 target/riscv/insn_trans/trans_rvzawrs.c.inc >> > >> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> > index 05e6521351..6cb00fadff 100644 >> > --- a/target/riscv/cpu.c >> > +++ b/target/riscv/cpu.c >> > @@ -882,6 +882,7 @@ static Property riscv_cpu_extensions[] = { >> > DEFINE_PROP_BOOL("Counters", RISCVCPU, cfg.ext_counters, true), >> > DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true), >> > DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true), >> > + DEFINE_PROP_BOOL("zawrs", RISCVCPU, cfg.ext_zawrs, true), >> >> Would this be enabled by default? > > > The "true" was a personal preference (I prefer to keep the argument list for > QEMU short) > and I did not see any conflicts with existing behavior (no code should break). > If you prefer otherwise or if I missed a policy I will change it. > >> >> >> > DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false), >> > DEFINE_PROP_BOOL("Zfhmin", RISCVCPU, cfg.ext_zfhmin, false), >> > DEFINE_PROP_BOOL("Zve32f", RISCVCPU, cfg.ext_zve32f, false), >> > @@ -1075,6 +1076,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char >> > **isa_str, int max_str_len) >> > ISA_EDATA_ENTRY(zicsr, ext_icsr), >> > ISA_EDATA_ENTRY(zifencei, ext_ifencei), >> > ISA_EDATA_ENTRY(zmmul, ext_zmmul), >> > + ISA_EDATA_ENTRY(zawrs, ext_zawrs), >> > ISA_EDATA_ENTRY(zfh, ext_zfh), >> > ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), >> > ISA_EDATA_ENTRY(zfinx, ext_zfinx), >> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> > index 7d6397acdf..a22bc0fa09 100644 >> > --- a/target/riscv/cpu.h >> > +++ b/target/riscv/cpu.h >> > @@ -380,6 +380,7 @@ struct RISCVCPUConfig { >> > bool ext_h; >> > bool ext_j; >> > bool ext_v; >> > + bool ext_zawrs; >> > bool ext_zba; >> > bool ext_zbb; >> > bool ext_zbc; >> > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode >> > index 4033565393..513ea227fe 100644 >> > --- a/target/riscv/insn32.decode >> > +++ b/target/riscv/insn32.decode >> > @@ -711,6 +711,10 @@ vsetvli 0 ........... ..... 111 ..... 1010111 >> > @r2_zimm11 >> > vsetivli 11 .......... ..... 111 ..... 1010111 @r2_zimm10 >> > vsetvl 1000000 ..... ..... 111 ..... 1010111 @r >> > >> > +# *** Zawrs Standard Extension *** >> > +wrs_nto 000000001101 00000 000 00000 1110011 >> > +wrs_sto 000000011101 00000 000 00000 1110011 >> > + >> > # *** RV32 Zba Standard Extension *** >> > sh1add 0010000 .......... 010 ..... 0110011 @r >> > sh2add 0010000 .......... 100 ..... 0110011 @r >> > diff --git a/target/riscv/insn_trans/trans_rvzawrs.c.inc >> > b/target/riscv/insn_trans/trans_rvzawrs.c.inc >> > new file mode 100644 >> > index 0000000000..d0df56378e >> > --- /dev/null >> > +++ b/target/riscv/insn_trans/trans_rvzawrs.c.inc >> > @@ -0,0 +1,54 @@ >> > +/* >> > + * RISC-V translation routines for the RISC-V Zawrs Extension. >> > + * >> > + * Copyright (c) 2022 Christoph Muellner, christoph.muell...@vrull.io >> > + * >> > + * This program is free software; you can redistribute it and/or modify it >> > + * under the terms and conditions of the GNU General Public License, >> > + * version 2 or later, as published by the Free Software Foundation. >> > + * >> > + * This program is distributed in the hope it will be useful, but WITHOUT >> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> > for >> > + * more details. >> > + * >> > + * You should have received a copy of the GNU General Public License >> > along with >> > + * this program. If not, see <http://www.gnu.org/licenses/>. >> > + */ >> > + >> > +static bool trans_wrs(DisasContext *ctx) >> > +{ >> > + if (!ctx->cfg_ptr->ext_zawrs) { >> > + return false; >> > + } >> > + >> > + /* >> > + * We may continue if one or more of the following conditions are met: >> > + * a) The reservation set is invalid >> >> Shouldn't this be valid? > > > The CPU is supposed to continue (stop waiting) when the reservation set > becomes invalid. > An earlier LR instruction registers a reservation set and the WRS.* > instructions wait until > this reservation set becomes invalided by a store from another hart to the > same reservation set. > So I think the description is correct. > >> >> >> > + * b) If WRS.STO, a short time since start of stall has elapsed >> > + * c) An interrupt is observed >> > + * >> > + * A reservation set can be invalidated by any store to a reserved >> > + * memory location. However, that's currently not implemented in QEMU. >> > + * So let's just exit the CPU loop and pretend that an interrupt >> > occured. >> >> We don't actually pretend an interrupt occurs though. It seems like >> it's valid to terminate the stall early, so we should just be able to >> do that. > > > The specification allows stopping the CPU stall if an interrupt occurs that > is disabled. > I think that would match the implemented behavior. > > The latest spec update introduced the following sentence: > "While stalled, an implementation is permitted to occasionally terminate the > stall and complete execution for any reason." > I did not want to use this justification for the implementation, because of > the word "occasionally" (the correct word would > be "always" in the implementation). Do you prefer to use this sentence > instead?
I think that is a better justification. When I first read your comment I thought you were going to generate a fake interrupt as well! Alistair > > Thanks, > Christoph > > > >> >> >> Alistair >> >> > + */ >> > + >> > + /* Clear the load reservation (if any). */ >> > + tcg_gen_movi_tl(load_res, -1); >> > + >> > + gen_set_pc_imm(ctx, ctx->pc_succ_insn); >> > + tcg_gen_exit_tb(NULL, 0); >> > + ctx->base.is_jmp = DISAS_NORETURN; >> > + >> > + return true; >> > +} >> > + >> > +#define GEN_TRANS_WRS(insn) \ >> > +static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn *a) \ >> > +{ \ >> > + (void)a; \ >> > + return trans_wrs(ctx); \ >> > +} >> > + >> > +GEN_TRANS_WRS(wrs_nto) >> > +GEN_TRANS_WRS(wrs_sto) >> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c >> > index b151c20674..a4f07d5166 100644 >> > --- a/target/riscv/translate.c >> > +++ b/target/riscv/translate.c >> > @@ -1007,6 +1007,7 @@ static uint32_t opcode_at(DisasContextBase *dcbase, >> > target_ulong pc) >> > #include "insn_trans/trans_rvh.c.inc" >> > #include "insn_trans/trans_rvv.c.inc" >> > #include "insn_trans/trans_rvb.c.inc" >> > +#include "insn_trans/trans_rvzawrs.c.inc" >> > #include "insn_trans/trans_rvzfh.c.inc" >> > #include "insn_trans/trans_rvk.c.inc" >> > #include "insn_trans/trans_privileged.c.inc" >> > -- >> > 2.35.3 >> > >> >