| Message ID | 20220812131304.1674484-2-philipp.tomsich@vrull.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] target/riscv: fence.i: update decode pattern | expand |
On Fri, 12 Aug 2022 at 14:17, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > Our decoding of fence-instructions is problematic in respect to the > RISC-V ISA specification: > - rs and rd are ignored, but need to be 0 > - fm is ignored > > This change adjusts the decode pattern to enfore rs and rd being 0, > and validates the fm-field (together with pred/succ for FENCE.TSO) to > determine whether a reserved instruction is specified. > > While the specification allows UNSPECIFIED behaviour for reserved > instructions, we now always raise an illegal instruction exception. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > > --- > > target/riscv/insn32.decode | 2 +- > target/riscv/insn_trans/trans_rvi.c.inc | 19 ++++++++++++++++++- > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > index 089128c3dc..4e53df1b62 100644 > --- a/target/riscv/insn32.decode > +++ b/target/riscv/insn32.decode > @@ -150,7 +150,7 @@ srl 0000000 ..... ..... 101 ..... 0110011 @r > sra 0100000 ..... ..... 101 ..... 0110011 @r > or 0000000 ..... ..... 110 ..... 0110011 @r > and 0000000 ..... ..... 111 ..... 0110011 @r > -fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > +fence fm:4 pred:4 succ:4 00000 000 00000 0001111 > fence_i 000000000000 00000 001 00000 0001111 > csrrw ............ ..... 001 ..... 1110011 @csr > csrrs ............ ..... 010 ..... 1110011 @csr > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc > index ca8e3d1ea1..515bb3b22a 100644 > --- a/target/riscv/insn_trans/trans_rvi.c.inc > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > @@ -795,7 +795,24 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a) > > static bool trans_fence(DisasContext *ctx, arg_fence *a) > { > - /* FENCE is a full memory barrier. */ > + switch (a->fm) { > + case 0b0000: > + /* normal fence */ > + break; > + > + case 0b0001: > + /* FENCE.TSO requires PRED and SUCC to be RW */ > + if (a->pred != 0xb0011 || a->succ != 0b0011) { > + return false; > + } > + break; > + > + default: > + /* reserved for future use */ > + return false; > + } I think it would be neater to do this decode in the .decode file, rather than by hand in the trans function. thanks -- PMM
On Fri, Aug 12, 2022 at 03:13:04PM +0200, Philipp Tomsich wrote: > Our decoding of fence-instructions is problematic in respect to the > RISC-V ISA specification: > - rs and rd are ignored, but need to be 0 > - fm is ignored > > This change adjusts the decode pattern to enfore rs and rd being 0, > and validates the fm-field (together with pred/succ for FENCE.TSO) to > determine whether a reserved instruction is specified. > > While the specification allows UNSPECIFIED behaviour for reserved > instructions, we now always raise an illegal instruction exception. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > > --- > > target/riscv/insn32.decode | 2 +- > target/riscv/insn_trans/trans_rvi.c.inc | 19 ++++++++++++++++++- > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > index 089128c3dc..4e53df1b62 100644 > --- a/target/riscv/insn32.decode > +++ b/target/riscv/insn32.decode > @@ -150,7 +150,7 @@ srl 0000000 ..... ..... 101 ..... 0110011 @r > sra 0100000 ..... ..... 101 ..... 0110011 @r > or 0000000 ..... ..... 110 ..... 0110011 @r > and 0000000 ..... ..... 111 ..... 0110011 @r > -fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > +fence fm:4 pred:4 succ:4 00000 000 00000 0001111 > fence_i 000000000000 00000 001 00000 0001111 > csrrw ............ ..... 001 ..... 1110011 @csr > csrrs ............ ..... 010 ..... 1110011 @csr > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc > index ca8e3d1ea1..515bb3b22a 100644 > --- a/target/riscv/insn_trans/trans_rvi.c.inc > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > @@ -795,7 +795,24 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a) > > static bool trans_fence(DisasContext *ctx, arg_fence *a) > { > - /* FENCE is a full memory barrier. */ > + switch (a->fm) { > + case 0b0000: > + /* normal fence */ > + break; > + > + case 0b0001: > + /* FENCE.TSO requires PRED and SUCC to be RW */ > + if (a->pred != 0xb0011 || a->succ != 0b0011) { > + return false; > + } > + break; > + > + default: > + /* reserved for future use */ > + return false; > + } > + > + /* We implement FENCE(.TSO) is a full memory barrier. */ s/is/as/ Thanks, drew > tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC); > return true; > } > -- > 2.34.1 > >
Happy to lower it back into the decode file. However, I initially pulled it up into the trans-function to more closely match the ISA specification: there is only one FENCE instruction with 3 arguments (FM, PRED, and SUCC). One might argue that the decode table for "RV32I Base Instruction Set" in the specification lists FENCE.TSO as a separate instruction, but the normative text doesn't (and FENCE overlaps FENCE.TSO in the tabular representation) — so I would consider the table as informative. I'll wait until we see what consensus emerges from the discussion. Philipp. On Fri, 12 Aug 2022 at 15:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 12 Aug 2022 at 14:17, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > > > Our decoding of fence-instructions is problematic in respect to the > > RISC-V ISA specification: > > - rs and rd are ignored, but need to be 0 > > - fm is ignored > > > > This change adjusts the decode pattern to enfore rs and rd being 0, > > and validates the fm-field (together with pred/succ for FENCE.TSO) to > > determine whether a reserved instruction is specified. > > > > While the specification allows UNSPECIFIED behaviour for reserved > > instructions, we now always raise an illegal instruction exception. > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > > > > --- > > > > target/riscv/insn32.decode | 2 +- > > target/riscv/insn_trans/trans_rvi.c.inc | 19 ++++++++++++++++++- > > 2 files changed, 19 insertions(+), 2 deletions(-) > > > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > > index 089128c3dc..4e53df1b62 100644 > > --- a/target/riscv/insn32.decode > > +++ b/target/riscv/insn32.decode > > @@ -150,7 +150,7 @@ srl 0000000 ..... ..... 101 ..... 0110011 @r > > sra 0100000 ..... ..... 101 ..... 0110011 @r > > or 0000000 ..... ..... 110 ..... 0110011 @r > > and 0000000 ..... ..... 111 ..... 0110011 @r > > -fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > > +fence fm:4 pred:4 succ:4 00000 000 00000 0001111 > > fence_i 000000000000 00000 001 00000 0001111 > > csrrw ............ ..... 001 ..... 1110011 @csr > > csrrs ............ ..... 010 ..... 1110011 @csr > > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc > > index ca8e3d1ea1..515bb3b22a 100644 > > --- a/target/riscv/insn_trans/trans_rvi.c.inc > > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > > @@ -795,7 +795,24 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a) > > > > static bool trans_fence(DisasContext *ctx, arg_fence *a) > > { > > - /* FENCE is a full memory barrier. */ > > + switch (a->fm) { > > + case 0b0000: > > + /* normal fence */ > > + break; > > + > > + case 0b0001: > > + /* FENCE.TSO requires PRED and SUCC to be RW */ > > + if (a->pred != 0xb0011 || a->succ != 0b0011) { > > + return false; > > + } > > + break; > > + > > + default: > > + /* reserved for future use */ > > + return false; > > + } > > I think it would be neater to do this decode in the > .decode file, rather than by hand in the trans function. > > thanks > -- PMM
On Fri, Aug 12, 2022 at 4:19 PM Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > Happy to lower it back into the decode file. > However, I initially pulled it up into the trans-function to more > closely match the ISA specification: there is only one FENCE > instruction with 3 arguments (FM, PRED, and SUCC). > One might argue that the decode table for "RV32I Base Instruction Set" > in the specification lists FENCE.TSO as a separate instruction, but > the normative text doesn't (and FENCE overlaps FENCE.TSO in the > tabular representation) — so I would consider the table as > informative. > > I'll wait until we see what consensus emerges from the discussion. From the discussion on patch 1 it seems that QEMU ignoring these fields (current behaviour) is correct Alistair > > Philipp. > > On Fri, 12 Aug 2022 at 15:21, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Fri, 12 Aug 2022 at 14:17, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > > > > > Our decoding of fence-instructions is problematic in respect to the > > > RISC-V ISA specification: > > > - rs and rd are ignored, but need to be 0 > > > - fm is ignored > > > > > > This change adjusts the decode pattern to enfore rs and rd being 0, > > > and validates the fm-field (together with pred/succ for FENCE.TSO) to > > > determine whether a reserved instruction is specified. > > > > > > While the specification allows UNSPECIFIED behaviour for reserved > > > instructions, we now always raise an illegal instruction exception. > > > > > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > > > > > > --- > > > > > > target/riscv/insn32.decode | 2 +- > > > target/riscv/insn_trans/trans_rvi.c.inc | 19 ++++++++++++++++++- > > > 2 files changed, 19 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > > > index 089128c3dc..4e53df1b62 100644 > > > --- a/target/riscv/insn32.decode > > > +++ b/target/riscv/insn32.decode > > > @@ -150,7 +150,7 @@ srl 0000000 ..... ..... 101 ..... 0110011 @r > > > sra 0100000 ..... ..... 101 ..... 0110011 @r > > > or 0000000 ..... ..... 110 ..... 0110011 @r > > > and 0000000 ..... ..... 111 ..... 0110011 @r > > > -fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > > > +fence fm:4 pred:4 succ:4 00000 000 00000 0001111 > > > fence_i 000000000000 00000 001 00000 0001111 > > > csrrw ............ ..... 001 ..... 1110011 @csr > > > csrrs ............ ..... 010 ..... 1110011 @csr > > > diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc > > > index ca8e3d1ea1..515bb3b22a 100644 > > > --- a/target/riscv/insn_trans/trans_rvi.c.inc > > > +++ b/target/riscv/insn_trans/trans_rvi.c.inc > > > @@ -795,7 +795,24 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a) > > > > > > static bool trans_fence(DisasContext *ctx, arg_fence *a) > > > { > > > - /* FENCE is a full memory barrier. */ > > > + switch (a->fm) { > > > + case 0b0000: > > > + /* normal fence */ > > > + break; > > > + > > > + case 0b0001: > > > + /* FENCE.TSO requires PRED and SUCC to be RW */ > > > + if (a->pred != 0xb0011 || a->succ != 0b0011) { > > > + return false; > > > + } > > > + break; > > > + > > > + default: > > > + /* reserved for future use */ > > > + return false; > > > + } > > > > I think it would be neater to do this decode in the > > .decode file, rather than by hand in the trans function. > > > > thanks > > -- PMM >
On Thu, 8 Sept 2022 at 11:25, Alistair Francis <alistair23@gmail.com> wrote: > > On Fri, Aug 12, 2022 at 4:19 PM Philipp Tomsich > <philipp.tomsich@vrull.eu> wrote: > > > > Happy to lower it back into the decode file. > > However, I initially pulled it up into the trans-function to more > > closely match the ISA specification: there is only one FENCE > > instruction with 3 arguments (FM, PRED, and SUCC). > > One might argue that the decode table for "RV32I Base Instruction Set" > > in the specification lists FENCE.TSO as a separate instruction, but > > the normative text doesn't (and FENCE overlaps FENCE.TSO in the > > tabular representation) — so I would consider the table as > > informative. > > > > I'll wait until we see what consensus emerges from the discussion. > > From the discussion on patch 1 it seems that QEMU ignoring these > fields (current behaviour) is correct Yes, this is an accurate reading of the situation. Philipp.
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode index 089128c3dc..4e53df1b62 100644 --- a/target/riscv/insn32.decode +++ b/target/riscv/insn32.decode @@ -150,7 +150,7 @@ srl 0000000 ..... ..... 101 ..... 0110011 @r sra 0100000 ..... ..... 101 ..... 0110011 @r or 0000000 ..... ..... 110 ..... 0110011 @r and 0000000 ..... ..... 111 ..... 0110011 @r -fence ---- pred:4 succ:4 ----- 000 ----- 0001111 +fence fm:4 pred:4 succ:4 00000 000 00000 0001111 fence_i 000000000000 00000 001 00000 0001111 csrrw ............ ..... 001 ..... 1110011 @csr csrrs ............ ..... 010 ..... 1110011 @csr diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc index ca8e3d1ea1..515bb3b22a 100644 --- a/target/riscv/insn_trans/trans_rvi.c.inc +++ b/target/riscv/insn_trans/trans_rvi.c.inc @@ -795,7 +795,24 @@ static bool trans_srad(DisasContext *ctx, arg_srad *a) static bool trans_fence(DisasContext *ctx, arg_fence *a) { - /* FENCE is a full memory barrier. */ + switch (a->fm) { + case 0b0000: + /* normal fence */ + break; + + case 0b0001: + /* FENCE.TSO requires PRED and SUCC to be RW */ + if (a->pred != 0xb0011 || a->succ != 0b0011) { + return false; + } + break; + + default: + /* reserved for future use */ + return false; + } + + /* We implement FENCE(.TSO) is a full memory barrier. */ tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC); return true; }
Our decoding of fence-instructions is problematic in respect to the RISC-V ISA specification: - rs and rd are ignored, but need to be 0 - fm is ignored This change adjusts the decode pattern to enfore rs and rd being 0, and validates the fm-field (together with pred/succ for FENCE.TSO) to determine whether a reserved instruction is specified. While the specification allows UNSPECIFIED behaviour for reserved instructions, we now always raise an illegal instruction exception. Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> --- target/riscv/insn32.decode | 2 +- target/riscv/insn_trans/trans_rvi.c.inc | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-)