* [PATCH 1/2] target/riscv: fence.i: update decode pattern @ 2022-08-12 13:13 Philipp Tomsich 2022-08-12 13:13 ` [PATCH 2/2] target/riscv: fence: reconcile with specification Philipp Tomsich 2022-08-12 14:01 ` [PATCH 1/2] target/riscv: fence.i: update decode pattern Andrew Jones 0 siblings, 2 replies; 10+ messages in thread From: Philipp Tomsich @ 2022-08-12 13:13 UTC (permalink / raw) To: qemu-devel Cc: Philipp Tomsich, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv The RISC-V specification specifies imm12, rs1 and rd to be all-zeros, so we can't ignore these bits when decoding into fence.i. Update the decode pattern to reflect the specification. Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> --- target/riscv/insn32.decode | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode index 014127d066..089128c3dc 100644 --- a/target/riscv/insn32.decode +++ b/target/riscv/insn32.decode @@ -151,7 +151,7 @@ sra 0100000 ..... ..... 101 ..... 0110011 @r or 0000000 ..... ..... 110 ..... 0110011 @r and 0000000 ..... ..... 111 ..... 0110011 @r fence ---- pred:4 succ:4 ----- 000 ----- 0001111 -fence_i ---- ---- ---- ----- 001 ----- 0001111 +fence_i 000000000000 00000 001 00000 0001111 csrrw ............ ..... 001 ..... 1110011 @csr csrrs ............ ..... 010 ..... 1110011 @csr csrrc ............ ..... 011 ..... 1110011 @csr -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-08-12 13:13 [PATCH 1/2] target/riscv: fence.i: update decode pattern Philipp Tomsich @ 2022-08-12 13:13 ` Philipp Tomsich 2022-08-12 13:21 ` Peter Maydell 2022-08-12 14:03 ` Andrew Jones 2022-08-12 14:01 ` [PATCH 1/2] target/riscv: fence.i: update decode pattern Andrew Jones 1 sibling, 2 replies; 10+ messages in thread From: Philipp Tomsich @ 2022-08-12 13:13 UTC (permalink / raw) To: qemu-devel Cc: Philipp Tomsich, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv 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. */ tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC); return true; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-08-12 13:13 ` [PATCH 2/2] target/riscv: fence: reconcile with specification Philipp Tomsich @ 2022-08-12 13:21 ` Peter Maydell 2022-08-12 14:17 ` Philipp Tomsich 2022-08-12 14:03 ` Andrew Jones 1 sibling, 1 reply; 10+ messages in thread From: Peter Maydell @ 2022-08-12 13:21 UTC (permalink / raw) To: Philipp Tomsich Cc: qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-08-12 13:21 ` Peter Maydell @ 2022-08-12 14:17 ` Philipp Tomsich 2022-09-08 9:24 ` Alistair Francis 0 siblings, 1 reply; 10+ messages in thread From: Philipp Tomsich @ 2022-08-12 14:17 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-08-12 14:17 ` Philipp Tomsich @ 2022-09-08 9:24 ` Alistair Francis 2022-09-08 9:28 ` Philipp Tomsich 0 siblings, 1 reply; 10+ messages in thread From: Alistair Francis @ 2022-09-08 9:24 UTC (permalink / raw) To: Philipp Tomsich Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng, Palmer Dabbelt, open list:RISC-V 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-09-08 9:24 ` Alistair Francis @ 2022-09-08 9:28 ` Philipp Tomsich 0 siblings, 0 replies; 10+ messages in thread From: Philipp Tomsich @ 2022-09-08 9:28 UTC (permalink / raw) To: Alistair Francis Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Alistair Francis, Bin Meng, Palmer Dabbelt, open list:RISC-V 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target/riscv: fence: reconcile with specification 2022-08-12 13:13 ` [PATCH 2/2] target/riscv: fence: reconcile with specification Philipp Tomsich 2022-08-12 13:21 ` Peter Maydell @ 2022-08-12 14:03 ` Andrew Jones 1 sibling, 0 replies; 10+ messages in thread From: Andrew Jones @ 2022-08-12 14:03 UTC (permalink / raw) To: Philipp Tomsich Cc: qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fence.i: update decode pattern 2022-08-12 13:13 [PATCH 1/2] target/riscv: fence.i: update decode pattern Philipp Tomsich 2022-08-12 13:13 ` [PATCH 2/2] target/riscv: fence: reconcile with specification Philipp Tomsich @ 2022-08-12 14:01 ` Andrew Jones 2022-08-12 14:09 ` Philipp Tomsich 1 sibling, 1 reply; 10+ messages in thread From: Andrew Jones @ 2022-08-12 14:01 UTC (permalink / raw) To: Philipp Tomsich Cc: qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv Please use a cover-letter for multi-patch patch series. On Fri, Aug 12, 2022 at 03:13:03PM +0200, Philipp Tomsich wrote: > The RISC-V specification specifies imm12, rs1 and rd to be all-zeros, > so we can't ignore these bits when decoding into fence.i. > > Update the decode pattern to reflect the specification. I got hung-up on this for a bit since there isn't any "must-be-0" fields, only ignored fields, but the next patch gives a clue which helped me make sense of this. The encoding of these instructions with ignored fields set to anything except zero gets into reserved instruction territory, and QEMU may legally raise an illegal-instruction in that case, which this patch will start doing. It'd be nice to have a bit more text in this commit message to make that clear. > > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu> > --- > > target/riscv/insn32.decode | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode > index 014127d066..089128c3dc 100644 > --- a/target/riscv/insn32.decode > +++ b/target/riscv/insn32.decode > @@ -151,7 +151,7 @@ sra 0100000 ..... ..... 101 ..... 0110011 @r > or 0000000 ..... ..... 110 ..... 0110011 @r > and 0000000 ..... ..... 111 ..... 0110011 @r > fence ---- pred:4 succ:4 ----- 000 ----- 0001111 > -fence_i ---- ---- ---- ----- 001 ----- 0001111 > +fence_i 000000000000 00000 001 00000 0001111 ^ need two more spaces here to line up with fence. > csrrw ............ ..... 001 ..... 1110011 @csr > csrrs ............ ..... 010 ..... 1110011 @csr > csrrc ............ ..... 011 ..... 1110011 @csr > -- > 2.34.1 > > Thanks, drew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fence.i: update decode pattern 2022-08-12 14:01 ` [PATCH 1/2] target/riscv: fence.i: update decode pattern Andrew Jones @ 2022-08-12 14:09 ` Philipp Tomsich 2022-08-12 14:18 ` Peter Maydell 0 siblings, 1 reply; 10+ messages in thread From: Philipp Tomsich @ 2022-08-12 14:09 UTC (permalink / raw) To: Andrew Jones Cc: qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv On Fri, 12 Aug 2022 at 16:01, Andrew Jones <ajones@ventanamicro.com> wrote: > > > Update the decode pattern to reflect the specification. > > I got hung-up on this for a bit since there isn't any "must-be-0" fields, Please refer to '“Zifencei” Instruction-Fetch Fence, Version 2.0' in the specification. The encoding diagram clearly states 0 for imm[11:0], 0 for rs1 and 0 for rd. However, there is an explanatory paragraph below (unfortunately, it is not clear whether this is normative or informative): > The unused fields in the FENCE.I instruction, imm[11:0], rs1, and rd, are reserved for finer-grain fences in future extensions. For forward compatibility, base implementations shall ignore these fields, and standard software shall zero these fields. Strictly speaking, this patch may be too restrictive (it violates the "for forward-compatibility" part — which I consider informative only, though). Thanks, Philipp. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] target/riscv: fence.i: update decode pattern 2022-08-12 14:09 ` Philipp Tomsich @ 2022-08-12 14:18 ` Peter Maydell 0 siblings, 0 replies; 10+ messages in thread From: Peter Maydell @ 2022-08-12 14:18 UTC (permalink / raw) To: Philipp Tomsich Cc: Andrew Jones, qemu-devel, Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv On Fri, 12 Aug 2022 at 15:11, Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > On Fri, 12 Aug 2022 at 16:01, Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > Update the decode pattern to reflect the specification. > > > > I got hung-up on this for a bit since there isn't any "must-be-0" fields, > > Please refer to '“Zifencei” Instruction-Fetch Fence, Version 2.0' in > the specification. > The encoding diagram clearly states 0 for imm[11:0], 0 for rs1 and 0 for rd. > > However, there is an explanatory paragraph below (unfortunately, it is > not clear whether this is normative or informative): > > The unused fields in the FENCE.I instruction, imm[11:0], rs1, and rd, are reserved for finer-grain fences in future extensions. For forward compatibility, base implementations shall ignore these fields, and standard software shall zero these fields. That's pretty clear that this patch is wrong, then -- QEMU is an implementation, and so we must ignore these fields. Otherwise when a future version of the spec defines a finer-grain fence instruction in this part of the encoding space, older QEMU will incorrectly make software that uses it crash. If you think the spec is insufficiently clear about whether that is normative then that would be something to raise with the spec authors, preferably before anybody builds hardware that enforces must-be-zeroes on these fields... thanks -- PMM ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-08 9:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-12 13:13 [PATCH 1/2] target/riscv: fence.i: update decode pattern Philipp Tomsich 2022-08-12 13:13 ` [PATCH 2/2] target/riscv: fence: reconcile with specification Philipp Tomsich 2022-08-12 13:21 ` Peter Maydell 2022-08-12 14:17 ` Philipp Tomsich 2022-09-08 9:24 ` Alistair Francis 2022-09-08 9:28 ` Philipp Tomsich 2022-08-12 14:03 ` Andrew Jones 2022-08-12 14:01 ` [PATCH 1/2] target/riscv: fence.i: update decode pattern Andrew Jones 2022-08-12 14:09 ` Philipp Tomsich 2022-08-12 14:18 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).