diff mbox series

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

Message ID 20220623152907.1606964-1-christoph.muellner@vrull.eu (mailing list archive)
State New, archived
Headers show
Series [RFC,v3] RISC-V: Add Zawrs ISA extension support | expand

Commit Message

Christoph Müllner June 23, 2022, 3:29 p.m. UTC
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

Comments

Heiko Stuebner June 23, 2022, 4:37 p.m. UTC | #1
Am Donnerstag, 23. Juni 2022, 17:29:07 CEST schrieb Christoph Muellner:
> 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>

on both rv64 and rv32
Tested-by: Heiko Stuebner <heiko@sntech.de>

> ---
>  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),
>      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
> +     * 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.
> +     */
> +
> +    /* 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"
>
Alistair Francis June 27, 2022, 5:20 a.m. UTC | #2
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?

>      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?

> +     * 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.

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
>
>
Christoph Müllner June 27, 2022, 8:16 a.m. UTC | #3
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?

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
> >
> >
>
Alistair Francis June 29, 2022, 11:35 p.m. UTC | #4
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
>> >
>> >
diff mbox series

Patch

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),
     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
+     * 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.
+     */
+
+    /* 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"