Re: [RFC PATCH v3] RISC-V: Add Zawrs ISA extension support

[Alistair Francis]

On Mon, Jun 27, 2022 at 6:16 PM Christoph Müllner
<christoph.muellner@vrull.eu> wrote:
>
>
>
> On Mon, Jun 27, 2022 at 7:20 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Jun 24, 2022 at 1:31 AM Christoph Muellner
>> <christoph.muellner@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.muellner@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.muellner@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
>> >
>> >