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

[Andrew Jones <ajones@ventanamicro.com>]

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
> 
>