* [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 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 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 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 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 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.