Re: [PATCH 2/2] target/riscv: fence: reconcile with specification

[Alistair Francis]

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
>