Re: [PATCH 2/2] target/riscv: fence: reconcile with specification
On Thu, 8 Sept 2022 at 11:25, Alistair Francis wrote: > > On Fri, Aug 12, 2022 at 4:19 PM Philipp Tomsich > 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.
Re: [PATCH 2/2] target/riscv: fence: reconcile with specification
On Fri, Aug 12, 2022 at 4:19 PM Philipp Tomsich
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 wrote:
> >
> > On Fri, 12 Aug 2022 at 14:17, 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
> > >
> > > ---
> > >
> > > 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 000 .. 101 . 0110011 @r
> > > sra 010 .. 101 . 0110011 @r
> > > or 000 .. 110 . 0110011 @r
> > > and 000 .. 111 . 0110011 @r
> > > -fence pred:4 succ:4 - 000 - 000
> > > +fencefm:4 pred:4 succ:4 0 000 0 000
> > > fence_i 0 001 0 000
> > > 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 0b:
> > > + /* 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
>
Re: [PATCH 2/2] target/riscv: fence: reconcile with specification
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 wrote:
>
> On Fri, 12 Aug 2022 at 14:17, 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
> >
> > ---
> >
> > 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 000 .. 101 . 0110011 @r
> > sra 010 .. 101 . 0110011 @r
> > or 000 .. 110 . 0110011 @r
> > and 000 .. 111 . 0110011 @r
> > -fence pred:4 succ:4 - 000 - 000
> > +fencefm:4 pred:4 succ:4 0 000 0 000
> > fence_i 0 001 0 000
> > 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 0b:
> > + /* 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
Re: [PATCH 2/2] target/riscv: fence: reconcile with specification
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
>
> ---
>
> 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 000 .. 101 . 0110011 @r
> sra 010 .. 101 . 0110011 @r
> or 000 .. 110 . 0110011 @r
> and 000 .. 111 . 0110011 @r
> -fence pred:4 succ:4 - 000 - 000
> +fencefm:4 pred:4 succ:4 0 000 0 000
> fence_i 0 001 0 000
> 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 0b:
> + /* 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
>
>
Re: [PATCH 2/2] target/riscv: fence: reconcile with specification
On Fri, 12 Aug 2022 at 14:17, 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
>
> ---
>
> 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 000 .. 101 . 0110011 @r
> sra 010 .. 101 . 0110011 @r
> or 000 .. 110 . 0110011 @r
> and 000 .. 111 . 0110011 @r
> -fence pred:4 succ:4 - 000 - 000
> +fencefm:4 pred:4 succ:4 0 000 0 000
> fence_i 0 001 0 000
> 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 0b:
> + /* 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
